[meta] Add check for enum const values

[kcudnik]

This check will make sure that enum values are constant from this point
on all future commits.

[kcudnik]

@lguohan there will be problem with SAI_ACL_TABLE_ATTR_FIELD_END, since if new FIELD will be added to TABLE, SAI_ACL_TABLE_ATTR_ENTRY_LIST will be shifted, look here: https://github.com/opencomputeproject/SAI/blob/master/inc/saiacl.h#L1391

Solution here: #1266

[shri-khare]

Thanks Kamil.

Few questions:

• [meta] Add check for enum const values #1256 enforces "enum values are constant from this point on all future commits". How do we solve the original issue we reported that breaks warmboot from SAI spec 1.6.5 to 1.8.1 due to "in between" insertions to saihash.h and saiacl.h?

• During the Thursday weekly SAI meeting (June 24, 2021), we agreed to add explicit numbering for each enum on top of the tree and then also port those changes to 1.8.1 to release 1.8.2. This PR is towards that. Has that decision changed? If yes, could you please share more details on why? Was there any discussion around this that I missed?

• During last Thursday meeting @lguohan observed that mandating that new enums must be inserted at the end may mean loss of readability as the fields to add may belong together with existing fields for readability. With [meta] Add check for enum const values #1256 , if enum values are to be constant on future commits, how do we handle insertions without loss of readability? Again, I think I am missing some context here. Could you please share?

Thanks,
Shri

[kcudnik]

• [meta] Add check for enum const values #1256 enforces "enum values are constant from this point on all future commits". How do we solve the original issue we reported that breaks warmboot from SAI spec 1.6.5 to 1.8.1 due to "in between" insertions to saihash.h and saiacl.h?

merge them before this merge happens

• During the Thursday weekly SAI meeting (June 24, 2021), we agreed to add explicit numbering for each enum on top of the tree and then also port those changes to 1.8.1 to release 1.8.2. This PR is towards that. Has that decision changed? If yes, could you please share more details on why? Was there any discussion around this that I missed?

yea, explicit numbers may be provided, but currently this is not required as i made it shis way its doesnt mind

• During last Thursday meeting @lguohan observed that mandating that new enums must be inserted at the end may mean loss of readability as the fields to add may belong together with existing fields for readability. With [meta] Add check for enum const values #1256 , if enum values are to be constant on future commits, how do we handle insertions without loss of readability? Again, I think I am missing some context here. Could you please share?

we have to sacrifice something, either metadata of readability, there is no win-win situation here, and from my perspective i dont want to weaken metadata from already make sanity checks. Put item at the end of list and forget about ut would be the choice

[kcudnik]

Required for this PR:
#1264
#1266

[shri-khare]

• [meta] Add check for enum const values #1256 enforces "enum values are constant from this point on all future commits". How do we solve the original issue we reported that breaks warmboot from SAI spec 1.6.5 to 1.8.1 due to "in between" insertions to saihash.h and saiacl.h?

merge them before this merge happens

Does that refer to moving the enum fields inserted "in between" SAI spec 1.6.5 to 1.8.1 to the end.
I already have a PR open for that. How do I further request to merge this:

#1250

[kcudnik]

Does that refer to moving the enum fields inserted "in between" SAI spec 1.6.5 to 1.8.1 to the end.
I already have a PR open for that. How do I further request to merge this:

#1250

you need to discuss this on weekly SAI meeting, this seems like a lot of shuffling

[rlhui]

@lguohan - please help review

[rlhui]

#1259 should be merged in before this one.

[kcudnik]

@lguohan @shri-khare
I came up with this scenario 1:

• our starting backward compatibility commit is A, (enum type X_* already exists)
• at next commit B we add enum X_A = 5 (last in list)
• at next commit C we remove enum X_A
• at next commit D we add enum X_B =5 and X_A = 6

there can be any other number of other commits in between ABCD

Here is the problem: from backward compatibility each separate branch B C D is compatible with branch A, but branch B is not compatible with branch D, since in branch C we removed an enum. Solution would be to not allow to remove any existing enums and just deprecate them, but SAI repo history shows that enums many times was removed completely or renamed or changed attr type many times.

We can't compare every branch permutation since it's not possible in let say 100 commits, and banning remove enums also don't seem like solution, this happed to segment route: https://github.com/opencomputeproject/SAI/pull/1231/files, entire api disappeared and was renamed to different one. In that case should we keep segment route for eternity? seems also not best way.

I came up with this scenario 2:

• our starting backward compatibility commit is A (enum type X_ don't exists yet)
• at next commit B we introduce enum X_A = 1, X_B = 2
• at next commit C we remove entire enum type X_*
• at next commit D we introduce enum X_A = 1, X_C =2 and rename X_B = 3

there can be any other number of other commits in between ABCD.

Branch BCD are backward compatible with branch A, but B and D is not compatible.

At first i though that we can compare every new commit to previous branch and then to base branch A, but this is not good enough ether since if above example there will be additional commit between branch B and D then this detection will not work. So we would need check current commit with every commit down to base commit A, which would be insane. Again solution to this would be to ban removing enums, but same problem arises with like with segment route that shows entire enums can disappear.

[kcudnik]

@lguohan @shri-khare
I also noticed other issue that can happen. Let say we have 3 PRs

1. is adding attribute in the middle of PORT and assign explicit value to it, and this is the latest attribute. last attribute in the enum is not actually last. Validation was successful, all checks passed
2. is adding attribute at the end of PORT Validation is successful, all checks passed
3. is adding PORT attribute in the middle but at different point than 1st PR Validation is successful, all checks passed

now, 1st PR is merged. Then second PR is merged since all checks passed, but this is invalid, since 2nd PR the lass attribute should have explicit value assigned, since 1st PR last attribute was explicitly assigned in the middle. So after merging 2nd PR current SAI status in PORT is invalid, wrong values. Then we we merge 3rd PR, and this also causes invalid enum values and possibly repetitions.

So there will be a inconsistent enum values for some time, until 4th PR will be created and it will then show all errors, and 4th person will need to fix all those previous enum values. And this will be constant problem, since once checks passed, they are valid all the time until next PR comes and causes to run validation check. Currently there is no solution for this, workaround for that would be to manually run az pipeline to run validator before merge each PR.

[kcudnik]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).