Operating System: All
Platform: All

Hi Claudio, could you please give us the steps to reproduce this issue?

thanks in advance,

Issue is seen in the test, the link is attached.
Therefore you will need to investigate which part of the test is triggering.
Check if the error if from our side or from the test, etc..

Regards,

I merged this CSIT patch:

https://git.opendaylight.org/gerrit/#/c/64424/

still, seems like we could handle hte BGPDocumentedException a little
cleaner than throwing the whole trace. Can we get a more clear/concise
log.error() message instead?

Hi Jamo, Which was the point of the patch merged? I don't see that it fixed anything[0],
neither I see any attachment/prove that patch was tested against sandbox or any other place.
Commit message is unclear, and it doesn't say what is fixing and how.

Regards,
Claudio

[0] https://logs.opendaylight.org/releng/jenkins092/bgpcep-csit-1node-gate-userfeatures-all-oxygen/19/odl1_karaf.log.gz

I just saw this bug was updated, and when I looked I realized I never responded. sorry.

Hi Jamo, Which was the point of the patch merged? I don't see that it fixed anything[0],
neither I see any attachment/prove that patch was tested against sandbox or any other place.
Commit message is unclear, and it doesn't say what is fixing and how.

Regards,
Claudio

[0]https://logs.opendaylight.org/releng/jenkins092/bgpcep-csit-1node-gate-userfeatures-all-oxygen/19/odl1_karaf.log.gz

I don't know/remember the point of the patch. Maybe Yrineu can comment? I think I merged it
assuming it was resolve the Exception, but it has not, now that I have checked a recent job

I do suspect the patch was actually helpful though, as you can see the output of the robot
step using the string that was updated in the patch does not have funky characters being returned
like this one does

Hey Jamo Luhrsen, when I sent that patch I saw that we was requesting to an incomplete URL, once I changed it, I ran the tests locally and has worked fine. The same when I sent the patch to review.

Hey Jamo Luhrsen, when I sent that patch I saw that we was requesting to an incomplete URL, once I changed it, I ran the tests locally and has worked fine. The same when I sent the patch to review.

ok, Yrineu Rodrigues Like I said, I think your patch was helpful. But the actual bug this is filed for:

BGPDocumentedException: Well known mandatory attribute missing: ORIGIN

is still happening.

I do suspect the patch was actually helpful though, as you can see the output of the robot
step using the string that was updated in the patch does not have funky characters being returned
like this one does

Regarding patch https://git.opendaylight.org/gerrit/#/c/64424/ it has been reverted since. And if you check the scritp_uri_opt in both of those logs, they are the same, so it has not been fixed by this. I don't think it does anything useful.

This ERROR actually comes from completely different test, bgpcep/bgpuser/ibgp_peer_lsp.robot, where we actually send WRONG routes, and expect BGP to reject them.

from documentation

TC1_Connect_BGP_Peer
[Documentation] Connect BGP peer with advertising the routes without mandatory params like LOC_PREF.

TC1_Check_Example_Bgp_Rib
[Documentation] Check RIB for not containig linkstate-route(s), because update messages were not good.

Indeed if we search by topic. I see 3 patches merged, one adding, second reverting what it was added few hours ago, and then a third one 

which is updating other different test and different feature, and with a conversation that was still going on. 

jluhrsen would it be possible to create some kind of rule, that some of the formal committers from the project which is been tested, has to review and approve it before tests are update. I can propose it on next TSC meeting if you agree or required.

Basically my point is that those test represent the stability of the project, and PTLs cannot be checking everyday if someone changed the test, before 

validate the results of them. I'm really thankful Yrineu Rodrigues for your contribution, but we need to be careful what changes are introduced to test.

Otherwise we will ending thinking that we are testing one thing when indeed we are doing something different.

Let me know what both of you think about this, and I'll close the bug as resolved later.

Regards,, 

I think we have two separate conversations here.

1. This bug
   This bug was filed by cdgasparini and deals with the ugly Exception

   Error = WELL_KNOWN_ATTR_MISSING
   org.opendaylight.protocol.bgp.parser.BGPDocumentedException: Well known mandatory attribute missing: ORIGIN

   That has not been fixed, according to a recent job
   If the Error is expected (negative test case) then my suggestion (you can take it or leave it) is to catch that Exception
   and print something more meaningful to a non-expert parsing the logs. If it's not expected, then I guess we still have the
   bug to fix. I was just chiming in on this bug in good faith. This project and features are totally out of my realm of expertise.

2. CSIT changes
   If the problem we ended up with here is happening a lot, or even just every now and again, we can discuss
   a better way to ensure it happens less. We can discuss creating some formal rules, but let's make sure the
   problem is worth the overhead. However, if BGPCEP would like to have additional review coverage from the
   project's committers, we can try to implement that by culture. I can ask all int/test committers to make sure
   those patches are vetted by a bgpcep committer. In some of the other active projects we have (e.g. netvirt
   and genius) there are usually committers paying attention (reviewing and writing) CSIT patches. Even then
   we get issues creeping in that we have to clean up.
   Shall I request the int/test committers to ensure we get a bgpcep committer sign-off on those project's
   related CSIT patches?
   BTW, here is one

Thanks

If the Error is expected (negative test case) then my suggestion (you can take it or leave it) is to catch that Exception
and print something more meaningful to a non-expert parsing the logs.

It is expected, I will have a look at possible solutions to clarify logs.

jluhrsen

1. tomas.markovic could you take care of this, thanks
2. I agree with you that this will end creating more overhead, mostly on project with not many active committers. There is no need to create an specific rule only for BGPCEP, it was just a suggestion that in my IMHO could be useful  for all projects. I leave it to your expertise. I'm not fully familiar with merge process under int/test. We use to had to provide some bug where keep track, and sandbox link with run,  when Vratko was reviewing our patches. But I guess its not mandatory.

Thank you to both of you for your time and contribution.

Regards, 

https://git.opendaylight.org/gerrit/#/c/70085/