batch of changes related to bagpipe

[tmmorin]

some changes are not required for ExaBGP but only for BaGPipe BGP
some changes are also required for ExaBGP to properly work (e.g. fixes in RTC class)
some others changes are cosmetic

(please tell me if you prefer multiple pull requests)

[thomas-mangin]

Could you please be explain to me why you are using repr and not of str in your own code ?

[landscape-bot]

Repository health decreased by 0.27% when pulling c56e1fb on tmmorin:master into c185219 on Exa-Networks:master.

• 39 new problems were found (including 4 errors and 5 code smells).
• 3 problems were fixed (including 0 errors and 0 code smells).

[landscape-bot]

Repository health decreased by 0.27% when pulling c56e1fb on tmmorin:master into c185219 on Exa-Networks:master.

• 39 new problems were found (including 4 errors and 5 code smells).
• 3 problems were fixed (including 0 errors and 0 code smells).

[thomas-mangin]

Hello @tmmorin - merged everything but the repr changes.

I propose to change ExaBGP to:

• not use str (find a new name)
• cause every str to raise a Deprecated Message while calling the new function.
• update my code and your to the new API

What do you think ? Using str was a bad idea in the first place but it took ExaBGP to become a large(r) program for me to realise it. As I am working on 3.5, this is the ideal time for fix this issue.

[thomas-mangin]

https://www.codacy.com/app/thomasmangin/exabgp_2/issues?&filters=W3siaWQiOiJDYXRlZ29yeSIsInZhbHVlcyI6WyJFcnJvciBQcm9uZSJdfV0=

The third issue is in the new code

[tmmorin]

Thanks for merging these pieces!

There are two reasons why I ended up changing ExaBGP behavior wrt. repr and str:

• some __hash__, __eq__, __ne__ or __cmp__ implementations actually rely on repr or str to expose the right things to behave properly wrt. to how bagpipe uses nlri, attributes and RTs in sets and dicts
• bagpipe logs use repr on some of these objects

The reason why I proposed to use repr instead of str is the following: str(iterable) calls repr on the internal objects, thus str(multivalue attribute) ends up calling repr on each attribute (e.g. for extended communities). By replacing all the __str__ definitions by __repr__ definitions, I get the wanted behavior, and __str__ still works because if str is undefined but repr is Python calls repr (Python does not do it the reverse way: if repr is undefined, repr(object) displays the object address, which is useless).

I would still like __repr__ or __str__ to return something useful, if you end up deciding that str should be used, this would be also fine for bagpipe, but for extended communities I would need a tweak on bagpipe side so that str is called on members of attributes[Attribute.CODE.EXTENDED_COMMUNITY].communities.

Another thing is, if you drop str and repr, bagpipe would need that __hash__, __eq__, __ne__ and __cmp__ behavior is made consistent to allow storing objects in sets and/or using them in hashes. We may need a discussion focused on this particular issue because, the above pull request still does not get the right behavior for bagpipe:

• example 1: hash and cmp defined in MPLSVPN are good, but cmp is in fact not called when MPLSVPN objects are used in sets because eq/neq defined in MPLS are defined and used preferably. I can override eq/neq in MPLSVPN
• example 2: using RTs as dict keys or in sets would not work because cmp defined in extended.Community is not called, because eq/neq defined in Attribute is preferred ; I can override eq/neq in ExtendedCommunity or in RouteTarget, but I fear that the behavior would not be consistent with the behavior of eq of Attribute (which tests only attribute id, but not the value); my question is: do the Attribute and Attributes classes depend on this behavior of Attribute.eq or could we change this ?

[landscape-bot]

Repository health decreased by 0.13% when pulling 34116bb on tmmorin:master into b31eec7 on Exa-Networks:master.

• 18 new problems were found (including 1 error and 4 code smells).
• 5 problems were fixed (including 0 errors and 1 code smell).

[thomas-mangin]

Are you saying that repr should return a object which can be used by cmp ?
If so we should do this and have an repr object which is not the same as what is printed.

I have a wish to undefine all str object and make the printing explicit. I believe using the built-in lead to some issue when it comes to provide backward compatibility as it can not be passed a version number ...

Your opinion ? If I have to perform such intrusive change 3.5.0 - which is where we are - is going to be the right moment for it !

[thomas-mangin]

my question is: do the Attribute and Attributes classes depend on this behavior of Attribute.eq or could we change this ?

I am pretty sure I rely on eq to check if two routes are equals. I would need to dig the code. I have only define comparison when it was required - many object have them as I ported your code as it and did not want to remove it.

[tmmorin]

Are you saying that repr should return a object which can be used by cmp ? If so we should do this and have an repr object which is not the same as what is printed.

No, my idea would be:

• have a nice repr usable for logs and not implement str unless we want str to output something else than repr
• cover all classes with hash/cmp implementations that allow proper testing of equality of objects and using them in sets or as dict keys (implementing cmp alone allows to not implement eq and neq, Python will derive eq and neq from the result of cmp).
• multivalued attributes (such as ext coms) will also need to be comparable (see Attributes.sameValuesAs)

The last item I hope we can achieve, but we might fail if exabgp have different needs when comparing objects. The one typical case that I have in mind is the following: bagpipe stores nlris advertised by a peer in a set, a relies on set matching to identify that an already advertised route is withdrawn ; for this to work for MPLS labelled routes, eq and cmp have to ignore the label field. If this is fine for ExaBGP then we are all good, but if exabgp needs to have two MPLS NLRIs differing by their label to be inequal, then the solution will not be trivial.

I have a wish to undefine all str object and make the printing explicit. I believe using the built-in lead to some issue when it comes to provide backward compatibility as it can not be passed a version number ...

I don't see such a need right now for bagpipe.
You know better for ExaBGP, of course.
Maybe you could have a new method, versioned for exabgp use, that would default to calling str or repr ?

I am pretty sure I rely on eq to check if two routes are equals. I would need to dig the code. I have only define comparison when it was required - many object have them as I ported your code as it and did not want to remove it.

Attribute._eq only compare the attribute id, but does currently not look at the attribute value.
If used to compare route equality, it will not give the expected result.

[thomas-mangin]

Would this be acceptable ?
(1) I first need to make sure ALL classes LOOSE the str, make it raise a RuntimeError
(2) replace it with a def text(version) - this is for ExaBGP
(3) then we can add the repr you need which does not need to conform to the text api interface used by exabgp.
Ideally any format which need to not change for bagpipe should be another function name so it does not rely on any python "magic".
(5) perhaps remove all the str functions ...

For the comparison, I need to look at how your use case but I am sure we can find a way which works for both implementation. I do little comparison of objects and it to know if two attributes are the same or if a route need to be withdrawn/announced so our constraint must be similar.

[tmmorin]

For str/repr and text(),yes, that looks good.

And for the comparison, it's great if you are able to conclude that our needs are the same. I can push to you the changes I would have for eq/neq/cmp/hash and you will see if you think this will work well for ExaBGP.

[thomas-mangin]

I am closing the pull request as nothing more can be pulled here.