Operating System: All
Platform: All

Changed to new feature, since from perspective of yangtools and binding generation, this is additional functionality, which was not in original scope and as you mentioned could not be derived from yang model.

It is new feature for Mac and v4 structures. For v6 is it is a bug.

Example as a result of the fact that the underlying representation is not normalized a v6 address coming from openflow southbound and BGPPCEP southbound will be different as BGPCEP uses java.net implementation while openflowjava uses a DIY one. I did not audit other projects, but I would not expect the situation to be any better - there the "primitive" type is simply not equivalent across projects. It cannot be as everyone is doing the conversions as they see fit and the "primitive" type is not sufficiently primitive.

IMHO, A good way forward will be to split this issue into ~ 3 to track the actual implementation of alternatives (and the bugfix for v6):

1. Mac
2. v4 - Ipv4address and friends
3. v6 - Ipv6address and friends

We can leave this one for a common replacement mechanism and keep it as an "improvement". We have to keep in mind that the v6 which depends on it is a bug and breaks overall controller v6 compliance.

I have an alternative backwards compatible MacAddress under test (as well as v4 and v6 pending after it). However, the replacement mechanism I have come up with is very ugly. We discuss what would be the appropriate way to override this (and other future cases) where yangtools has come up with the wrong answer for reasons outside our control (in this case ietf).

This issue seems like the proper home for that (open related issues for particular instances of broken results).

Second thought - this is bug for MAC too.

Lower case versus upper case. YANG model accepts both and there is no uniform check throughout.

The equal generated from the model, however is case sensitive.

In addition to the potential functional issues outlined by Anton, this is also a general performance bug/improvement to avoid converting to/from string/binary form. It just so happens that to address it, some changes/extension need to be added to yang tools that technically can be construed as new functionality. So do we want to split hairs over whether it's new functionality or not or do we want a highly performant controller?

I just saw this changed to feature again.

Generating code that declares these two Macs identical is a BUG:

01:23:DE:AD:BE:EF
01:23:de:ad:be:ef

What is the root cause for the bug and is it present in the yang model or not is not relevant as long as this code is generated is reused by the whole project.

Yangtools at present generates buggy code which is not fit for purpose these 3 structures:

Mac, v4 address/netmask and v6 address/netmask.

This is not a feature it is a bug.

@Tom,@Anton: I understand the issue you were having, but as you Tom pointed - providing functionality you are requiring - requires news feature across several
components in Yangtools, which I do not believe we could safely implement this late at point.

In order to have full potential (not going from string to binary and vise-versa)
affected is not only Binding generation, but whole yang-data-* series (which now
needs special casing for these types), data store (clustering) and restconf (since it has Instance Identifier parser).

I will note that automatic normalization is not good, as it prevents the specification of subnet+default gateway as a prefix: 192.168.1.1/24 (which I have seen in the field). OVS performs this sort of normalization and it wreaks havoc into OFP – just look at what the statistics manager had to do to compare flows.

If you want to propose a single set of normalization utilities, feel free to do so, but singling out three types and defining special handling at the data definition layer will probably not work as expected.

First of all.

I have MacAddress done, tested for Openflowforjava, Openflow and controller for a full matrix: pristine ofj, of and controller, fixed Mac, updated (to use binary) ofj, pristine of + controller and fixed Mac, ofj and of versus pristine controller. I can test it versus the rest of the controller too, its likely to pass with flying colours (this is for Helium-SR2).

The only reason it is not submitted as a patch is because I do not want to throw a major patch:

1. Without filing a bug (tick).

2. Without having a discussion (we are still not having it as we are not discussing on how to solve this, but how to avoid solving it).

3. Without coming to an agreement to a sensible design on how to do this.

4. My current approach is using a rather brutal override which throws out completely the yang generated one and replacing it by a human coded derivative which option A below. Proposing it without discussion on what is right (A, B or something else) is not the way to hold a proper discussion on something major in the community. At least as far as I am concerned.

So, I would actually like to have sensible discussion on how we introduce one or more of the following:

A) Human override provided that it is 100% feature compatible with yang generated types. These are the obvious examples, they are not the only one. This is trivial, it is a 6 lines addition ot a POM file and an extra java file. It changes NOTHING elsewhere on its own as everyone upstream still sees the same class interface.

From there on individual replacements, their features, normalization, representation, etc - all become individual bugs and are dealt with on their own merit.

B) Additional features for generation to account for the fact that yang types with respect to legacy pre-yang IETF work are nowhere near the actual data and are incapable on their own to produce an adequate data representation.

C) Something else beside A or B.

Second, Ipv4Address (or 6) is one object, netmask is another and an address with netmask yet another. Can we not mix these in here please.

An address sans netmask needs to be normalized (especially if it is v6). Otherwise you get the current situation where BGCEP codes it one way and ofj/of code it in another way. So can we stick to the subject at hand - low level data type representation for objects that come from both network and norhtbound and have a performance constraint regarding their implementation.

Could you propose draft patch to be more clear how you are trying to achieve this?

From your description it
seems that you manually reimplemented Binding classes for types in question.

Correct.

I replace the generated MacAddress.java one with the attached one using maven exec plugin.

It brings back some of the functionality we (unfortunately) lost when moving away from AD-SAL and changes the back-end to binary.

As I said - the actual replacement mechanism is ugly and is for POC only. I do not want to propose it. We should have a better way integrated with the generation (metadata, whatever).

The patches for openflow and openflowjava which show how to leverage it will follow shortly. This works both with and without them because the binary form constructor and accessor are in addition to the interfaces one would get from yangtools.

Attachment MacAddress.java has been added with description: Replacement MacAddress.java

I reviewed the proposed change. Aside from security and performance issues, this has, as you already noted, the problem of solving a symptom at the edge, not taking into account what the system in which this data is exchanged actually looks like.

On the surface, this may look like a bug, but the behavior is actually a consequence of how the system is designed and what assumptions are made – from that perspective this issue is an enhancement.

The assumptions are are as follows:
1) each type has precisely one native representation
2) that representation is derived from the YANG type hierarchy, as defined by RFC6020
3) when used in keys, the representation conforms to overall YANG expectations, as set out by RFC6020
4) any implicit data normalization is forbidden, the client must observe the same data she has written into the system (e.g. if I PUT something through RESTCONF, what I GET back must be the same thing in the String.equals() sense)

All the types you mention are derived from a YANG string by applying a restriction on format. Note that the definition of MAC allows both upper- and lower-case values, which when treated as keys are distinct, as far as I can tell. Same goes for IPv6, where I think any valid representation is allows and none is favored in any way.

What you are proposing is severing assumption 1) and maybe relaxing 2) for a select set of types. Since these types are not part of language definition, they are in no way special (I could ask the same treatment for ISO system IDs, for example), it seems to me that the solution:
A) must work across the entire system
B) must be extensible to work on any type
C) must not violate rules 3) and 4)
D) must not rely on altering the model (ietf-inet-types is an upstream model)

I guess I have to ask some questions.

Quote: "Aside from security and performance issues, this has, as you already noted, the problem of solving a symptom at the edge, not taking into account what the system in which this data is exchanged actually looks like."

1. That is a verbatim description of the current design. It fails to take into account any and every performance consideration and how data should be exchanged towards the network side. Are you referring to it?

2. AFAIK re-engineering the form of the OO representation of key datatypes used by all of the controller does not fall in the category of "solving at the egde". What is being done today - the constant upcase/to-hex/to-bin and from-bin-to-hex before being feds as string is solving a problem at the edge indeed. It is a deficiency in the design of the data representation and it is being solved by every subproject as they see fit.

Quote: "On the surface, this may look like a bug, but the behavior is actually a consequence of how the system is designed and what assumptions are made – from that perspective this issue is an enhancement."

Bugs are not necessarily a sole function of implementation. A buggy design is a bug.

Described assumptions are nice for implementing standalone yangtools which live on their own and are never used for something useful. yangTOOLS are tools to generate an OO model for the rest of the controller. That is requirement 0 which comes before 1, 2, 3, 4.

Further to this on your reqs:

1. We are not changing req 1 - we are replacing the representation.
2. We are not changing req 2 - we are just either adjusting it by hand or if generation can be improved we will be improving that. A human generator is still a generator
3. The representation still conforms - we did not change the exposed accessor so whoever get the value to use as key will use the yang defined. Issues?
4. That assumption is bogus. To put it bluntly, you normalize when you write out to network. Following strictly this assumption you are not allowed to ever write anything to network. This assumption also contradicts to key principles of stable and scalable software design - that all data should be normalized.

A) It works across the entire system.

B) It works on every type - if you have support for "human" override. You just use the "human" generator.

C) It conforms to rule 3, rule 4 - deliberately keeping your data not normalized is bogus. To say the least. It is self-serving the fact that yang has no data to represent the normalization to the detriment of the rest of the controller. Also, as noted and if followed strictly, write-out to network is a form of normalization. Are we prohibiting it?

D) We are not alterting the model, we are only altering the implementation.

In any case, as I said before, can we stop discussing why an obvious fundamental design fault which leads to a massive performance FAIL as well as interop issues across the whole of the controller is not a bug and actually work to solve it.

OpenFlow Java changes

Attachment openflowjava.diff has been added with description: OpenFlow Java differences

I am not submitting a gerrit because we continue to fail to progress the discusision on how we override the default generation.

These are the cascaded (tested) changes.

I would like to use one of them - the openflowplugin nicira extension to illustrate my point.

Let's look at extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/match/ArpShaCodec.java as an eccelent illustration.

In its current form it has to (I am going to quote Robert here as what he described is 100% fitting to the code, thanks - it is a perfect description of the current yangtool result):

"the problem of solving a symptom at the edge, not taking into account what the system in which this data is exchanged actually looks like."

The data walks like 6 bytes, talks like 6 bytes, it is 6 bytes. Because it is not presented as 6 bytes in the core, the plugin has to "solve the problem at the edge" and this is exactly what it does at line 30 or thereabouts - it converts the data to Hex (wasting 100+ instructions in the proces), pushes it as a string across the regexp (wasting 1000+ instructions in a worst case scenario).

All of this is done only in order to, once the data is digested and is being pushed out (line 40 or thereabouts), to take the hex string, waste a few 100 instrusctions on parsing it to push it out.

If that is not solving a problem at the "edge" due to a design fault in the core, dunno what is.

Every signle one of the changes is nearly identical to this one - it is all trivial, decreases processing CPU costs by 1000s of instructions per packet and most importantly - optional. Any code which does not use the new interface can still work without any changes.

Attachment openflowplugin.diff has been added with description: OpenFlow Plugin differences

Affects APIs and there is no way we can make this work by Lithium API freeze.

No. It does not.

The old API is unchanged.

The new one is optional - it is an accelerator. Want to use it, be my guest.

Want to have the slowest controller known to man, sure use the old one. It will STILL work.

In any case, while I would rather have this fixed for Lithium, if we cannot, we cannot. Berillium is still good.

Though IMHO, as the API is fully backwards compatible it is fixable for Lithium or even as a backport for Helium-SR2.

Hi All,

It's sad to see that we are still arguing about whether it's bug or feature. If i look at the end goal, i don't think it matters. Robert, based on your response it looks like the solution that anton proposed is not going to work, and as per the assumption you mentioned in your response yangtool is working as expected and we should not do any change? is my understanding correct? If that's the case i don't see any point in arguing whether it's bug or feature , or whether it can be done in Li or Be. isn't it ?

Robert/Tony, irrespective of whether it can be done in Li or Be or whether it's bug or feature, do you agree that it's a valid performance problem and causing significant performance impact (given that it's per packet penalty)and fixing this issue can gain significant performance improvement for controller?

Project milestone is valid concern and i agree with that. But rather then out rightly saying it can't be done, can we list high level work item that is required to implement this solution, like yangtool changes, level of impact on downstream projects and take a calculated estimate and decide whether it can be done or not in lithium? If we can determine that it can be done in lithium by slipping the milestone by a week or so, i think we can take exception from tsc, and tsc might consider it given that performance is critical requirement for the controller. If not Be it is.

So let's list out task and determine the size of work and go from there. Let me know your thoughts.

Thanks
Anil

Anil - I agree, we should have a constructive discussion on how we solve this.

Otherwise I will re-iterate:

1. This is a defficiency in the core.

2. It is exactly as Robert has described - every project solves it at the egde, because it is a defect in the core.

3. The example code submitted does not introduce a fundamental difference in the behaviour - it folds what every component is forced to do today due to the defective design into a centralized solution.

It is difficult to get an exact count for the full numbers because every project and in some cases subproject has bespoke one-off and different solve-at-the-edge solution to fix this. The overall count across the entire codebase for instances where people have worked around this defect runs into thousands.

4. All the code does is provides a centralized solution to replace the thousands of workarounds for this defect sprinkled across the code base.

5. For the case of Mac and IPv6 it is a definitive bug as this means that the same Mac or v6 originating from different components is non-identical

All I want to know from the project maintainers what are we doing here:

A. Leaving controller deliberately crippled performance-wise, non-v6 compliant and buggy for Ethernet processing across the board. If so why, and let's have an answer in the public record.

B. Solving this by human override - option to yangtools to override generation for selected classes. I currently do this using maven-exec, I would prefer something cleaner, but if there is no other choice, this will do.

C. Solving this by generation improvements and at the very least an outline what these improvements need to be. IMHO this is non-trivial and we need something (f.e. B) as an interim workaround. Also, I do not think that anything besides 4-6 base types needs to be touched this way at present. So in my opinion, investing half a man year into improving the generator in this case is like shooting pigeons with a SAM.

D. Technical proposal for an alternative to A, B or C.

Just to be clear with regards to work required:

Based on my experiments so far:

~ assuming maven-exec - a few hours to implement option B

~ 2-3 hours per project to update a project to use newer APIs which is 100% optional too as things still work with the old ones. This has to be done by hand because people have implemented all kinds of different workarounds for the design defect so a search and replace does not cut it.

We spend more time here so far than is required to fix this. In the entire controller.

It sounds like we need to bring this to the TWS or MD-SAL call to help clarify the understanding among all parties. I will reach out to Anton and see if he is able to join the MD-SAL call on Tuesday.

Sounds like the agenda should be:

1) Confirm and agree that there is a problem to be solved
2) Anton can share his suggested approach and findings
3) Come to a decision on how to proceed

I was asked if I am considering the BI/BA boundary.

The short answer is "yes, this does not make it any worse than it already is and in most use cases it makes it better".

The long answer:

Use case A: Data goes in BI, stays in BI, leaves via one of BI interfaces - no change. Sorry, this cannot be helped - there is no way to fix this because having a non-normalized BI design for something that gets anywhere near the IPv6 human/machine representation madness is a rather optimistic idea.

Use case B: Data goes in BI, moves BI->BA is used in BA. No performance penalty or performance improvement. Normalization related stability and bugfix (as expected) - it does not matter how you enter the data, where it will be used it will be normalized.

Breakdown:

Current design: BI->BA cost, pause, BA->network cost, use. The second cost is applied at the edge by everyone using it as they have to work around an inherently broken design. Example - openflow, bgpcep, l2switch - you name it. Everyone.

Proposed redesign: BI->BA cost, BA->Network cost, pause, use. That at face value sums to the same amount. In reality it is less - you do the second step CPU cache hot instead of crippling performance by waiting for data to go back to memory before use.

Use case C:

Data enters BA from network, is processed in BA, leaves BA to network without anything significant going up to BI interface.

This is the use case were this really matters. All plugins presently do this to some extent (how much depends on plugin). For example in openflow or l2switch you have internal lists and tables formed, data is matched versus them and something is spit out.

So the data flow here is N->BA->N

Current design:

1. Data enters from network, is deserialized/crippled by conversion to a string - 100 of instrusctions
2. String is regexped to make sure that it looks like network data (pointless excercise) - 1000s
3. String is converted temporarily to binary for nearly every operation related to it (examples in bgpcep, of, etc). Prefix matches, mask matches, bit ops, etc. 100s of instructions at a time
4. New string is created and passed through the regexp for fun and profit: 1000s of instructions
5. String is serialized to network - conversion penalty to parse it - 100s of instructions.

In this case the cost using the revised proposal is:
1. Deserialization - simple copy, a few instructions
2. No conversion
3. Comparisons, matches, etc done in binary - no penalty besides inherent cost of comparison
4. No penalty to create object to be serialized (we use the binary interface as this is the natural result of prefix/mask applications).
5. Binary is serialized to network - simple copy, few instructions.

Use case D:

This is the REALLY interesting use case going forwards as this the use case for most modern applications which may use the controller as infrastructure.

Data is originated in network - N->BA, BA->BI, Application consumes BI, Spits out something derived from the data onto BI, BI->BA in a DIFFERENT module, BA->N

Current:

1. All penalties as described in the prior use case going up. So the app will be getting the data very slow and at a constrained rate.
2. App uses data, spits out something derived from itvia let's say restconf it goes into another plugin. BOOM. That plugin today uses a DIFFERENT Do It Yourself normalization and serialization scheme. It will not match correctly. This is a BUG for MAC and V6 (v4 due to good weather on the Olympus and Athena & Zeus being in a good mood got lucky here).

Proposed:

1. No penalties going up - only string representation. Data IS normalized.
2. App derives responce from normalized data.
3. Other plugin uses the same data normalization mechanism.
4. Match/Action is done based on normalized representation.

This will actually work. Present design will not. It will break regularly in a very erratic and unpredictable manner.

Overall:

1. BI internally cannot be fixed. All we can do is to SDK-ise the base classes to be used by any of our potential app users and offer key other language ports. I am happy to do the honors for the python and perl libs too and maybe C.

Unless the design changes, this cannot be fixed so this is the workaround.

2. The proposal fixes everything else

I believe this answers remaining questions regarding the proposal, so can we finally stop discussing is it bug or not. It is a bug and as the controller is used by 3rd parties (scenario D) it will start showing its ugly head on a regular basis. So I suggest that we get on with "how do we fix it".

P.S. I will update the patches to SR3 next week. I cannot do master as OF/OFJ is too much in-flux at the moment. It is trivial, once it settles, there will be a patch for that too.

I would prefer such long posts be moved to mailing lists, as we cannot have a structured conversation here.

I disagree that the cost of going BI->BA does not change. Your code puts string parsing (which you move out the user-facing Java API) into that boundary. In your implementation that includes compiling a Pattern, which will have a very visible impact.

Same thing happens at BA->BI, you are forcing a string construction, eventhough the user may have provided the appropriate representation. That code in your implementation, is again non-optimal, as it forces parsing of the format string. There are much better implementations available (in openflowjava, I think).

Crossing the BA/BI boundary is costly and increasing that cost will actually make the system worse, for example in the use case D you describe. Your appraisal is correct, that that is an important use case. It is not important for future, though, as we have that sort of an application already shipping in Helium. It does not suffer from the normalization problems you describe, as the data at hand is kept encapsulated and unmodified. For performance impact evaluation, may I suggest you modify Ipv4Address, load it up, along with BGP, and try to pump something like 100K routes into the system and see how quickly this gets processed.

Your solution also applies only to verbatim IPv4 and IPv6 classes, not types derived from those, like BGP Identifier. Any such subclasses must be hand-coded and maintained manually, which essentially means we are going back to hand-maintained interfaces. Getting the workaround in will require a major version bump on the artifact (note our infra cannot handle semantic versioning well yet), as it the original/new classes are not compatible.

The cause of the disconnect so far comes from the point 1) of your appraisal of the overall situation.

While you dismiss the possibility of solving the problem, we think a solution is feasible. Furthermore we believe that IPv4/IPv6 are just a special case and a generalized and reusable solution is required.

As an initial estimate, the SDK approach you hint at will at some point require some sort of mechanism by which it hook into the system to provide an alternative access interface without sacrificing data integrity. Without that, the placement of string/binary translation boundaries will be unpredictable and will vary wildly between deployments, I fear.

So this is a larger effort. Can you start a design document (of which parts have already been stated in the comments above) as a subpage of https://wiki.opendaylight.org/view/YANG_Tools:Beryllium:Design_items, please?

Robert - cost of passing does change. Cost of passing is irrelevant because the interesting metric is cost of passing + cost of use as this is the metric to affect the overall controller.

Cost of passing + USE which is the cost which matters decreases.

Presently, you, going down (BI->BA):

1. Apply patterns, etc and store as string.

2. Go and do something else, in the meantime the data is evicted from CPU cache.

3. Parse the data. you parse there using a DIY parser which is also DIFFERENT for every plugin so it is never cache-hot as different code is loaded.

4. Use the data.

The proposal is:

1. Apply patterns, etc - same as before.

2. Parse there and then.

3. Go and do something else.

4. Use the data

This is the SAME e2e cost at face value, in reality it is lower as there is less cache thrashing of the CPU even if you use it only ONCE.

Also, if you use the data more than once (f.e. if you are building a matcher), current you are likely to incur the "parse" penalty on every match (depends on type of matching). Proposed you do not for most types of matchers.

Going UP the e2e full cost is less by 10x+ times, because you no longer include the convert-to-string and then parse for network originated data.

Otherwise I agree - we should move this to the mailing list.

Discussion moved to mailing lists, let's use the bug solely for actual work on a solution from now onwards.

https://wiki.opendaylight.org/view/String_data_normalization_and_alternative_storage lists the requirements and preliminary breakdown.

New gerrit:

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

Corresponding openflowjava java gerrits on Helium-SR3 (the yangtools one cleanly cherry-picks on helium-sr2 or sr3)

https://git.opendaylight.org/gerrit/18451

Openflowplugin

https://git.opendaylight.org/gerrit/18452

L2Switch

https://git.opendaylight.org/gerrit/18453

OF and OFJ are helium based because the master is still in-flux.

(In reply to Robert Varga from comment #26)
> https://wiki.opendaylight.org/view/
> String_data_normalization_and_alternative_storage lists the requirements and
> preliminary breakdown.

One comment on the last paragraph.

You cannot use InetAddress as it will automatically convert v4 in v6 to v4. I was also having some JMX issues when I tried to add it to the API.

So I would suggest that any optimization stop at byte[] and integer derived types.

A.

Some food for thought around the solution domain: http://conferences.idealliance.org/extreme/html/2006/Tennison01/EML2006Tennison01.html

I think we can solve this via a set of tricks, which do require some changes in how yang-model-api and yang-data-api contracts work around string types.

What we need to do is semantically bind a derived type definition with a data type representation other than string. This needs to happen at model definition time because it affects default statement mapping, as known to the system. To fulfill that requirement, we can support this feature only types marked with an annotation, of whose base type is either a built-in type or an unannotated type.

The annotation needs to a FQCN (or similarly unique) pointer towards a codec-type support class which defines native representation class and toString/forString factory methods, which provide

These annotations are expected to be packaged along with the base model and the support class, so that is difficult to encounter a situation when we are seeing the model without the classes being present - that particular scenario being an error case, where we fall back to string representation and java binding suffers, as it needs to recover from that.

We need to be able to inject the annotation externally so things like ietf-inet-types do not have to be edited.