Hi guys, Could I kindly ask you not to bring to C-language comments any unnatural meaning? I mean let's keep comments as just comments. You can move 'cksum = NNN' away from comment to other entity like static variable with cost of few bytes or even #define that will cost you nothing.
Thank you, Alexander On 3/18/25 12:29 PM, Dumitru Ceara via dev wrote: > Hi Alexandra, Ilya, Vladislav, > > On 3/14/25 8:47 PM, Ilya Maximets wrote: >> On 3/14/25 18:10, Vladislav Odintsov wrote: >>> On 14.03.2025 15:51, Ilya Maximets wrote: >>>> On 3/13/25 11:15, Aleksandra Rukomoinikova wrote: >>>>> Hi Ilya! Thanks for the review, I wanted to ask, don't you think it's a >>>>> good >>>>> idea to add a version for the pipeline, just like it was added for the >>>>> database >>>>> schemes, and take it into account in the version? And not just rely on the >>>>> fact that a person will notice the version when changing the checksum and >>>>> decide to change it. Or will this change the versioning a lot now? In >>>>> general, >>>>> what do you think, Ilya, Dumitru? >>>> Hi. People forget to change the database schema frequently, even though >>>> the version is right above the checksum that they are forced to update. >>>> So, regardless of having or not having the version, it's still mostly on >>>> reviewers to catch the cases where the patch author didn't update it. >>>> With that, I'm not sure if there is a big value in having a pipeline >>>> version >>>> number. >>> Hi Ilya, >>> >>> I just want to make sure, that Alexandra's idea about the version was >>> correctly understood. >>> >>> She proposed to make pipeline version a part the ovn-northd internal >>> version. This can eliminate situation, where: developer changed >>> pipeline -> saw check error, that checksum was not updated -> fix >>> checksum. And that is all. Compilation succeeds. But he/she should >>> manually bump internal ovn version so recomute to occur. But with this >>> proposal developer shouldn't manually bump internal ovn northd version - >>> it got updated automatically with new pipeline version as a part of >>> internal version like ovsdb sb/nb schemas etc. >>> >>> While I was writing this mail I guess I get your point ;) Did you mean >>> that people will just update checksum without updating pipeline >>> version? >> Yes, that's what I meant. :) >> >>> So, maybe just make a checksum a part of ovn-northd internal >>> version? >> I thought about this, we can do that, but here is a couple of considerations >> on why it may be not a desirable solution: >> >> 1. We do not check that anywhere, but it's nice when the internal version >> is not a random number and has some sort of semantic, i.e. it's nice to >> be able to compare two versions. Having a hash within the version will >> not allow to do that. >> > Good point! > >> 2. It's possible that some changes in the pipeline do not actually require >> the version bump. At least cosmetic changes do not require that. The >> internal version change leads to extra work done by OVN components on >> update and the extra database traffic that may not be necessary. It will >> also make ovn-controllers pause receiving new flow updates if they are >> configured for the wrong upgrade order. >> So, it might be better to only bump the internal version is there is an >> actual pipeline change. >> > I think I'd prefer manually bumping the internal version too. As Ilya > mentioned it does put responsibility on reviewers/maintainers to check > that the bump happened when it should have but that's the case for DB > schema changes already. So, in my opinion, it is acceptable. It was > already the case until now every time we had to change the stages in the > pipeline but it was easier to miss. > > This change will make it easier for reviewers to spot the cases when the > bump didn't happen. > > Can we enhance checkpatch.py to warn if the patch diff contains a > checksum value change but doesn't contain an OVN_INTERNAL_MINOR_VER > change? It will generate false positives when pipeline changes are > cosmetic/minor but it will be the responsibility of the committers to > waive/ignore those. > >> Best regards, Ilya Maximets. >> >>> WDYT? >>> >>>> Best regards, Ilya Maximets. >>>> >>>>>> On 13 Mar 2025, at 02:52, Ilya Maximets <[email protected]> wrote: >>>>>> >>>>>> Hi, Alexandra. Thanks for the patch! >>>>>> >>>>>> See a few comments below. >>>>>> >>>>>> Best regards, Ilya Maximets. >>>>>> >>>>>> On 3/12/25 11:51, Alexandra Rukomoinikova wrote: >>>>>>> Added checksum which is calculated for stages in northd. >>>>>>> Also, the tool calculate-northd-cksum is introduced. This tool >>>>>>> calculates the cksum of a northd stages. >>>>>> nit: For some reason the commit message above is indented by 1 space. >>>>>> >>>>>> In general, I'd not call them "northd stages", but use "pipeline stages" >>>>>> or >>>>>> "OVN pipeline stages" instead everywhere in this patch. They are >>>>>> integral >>>>>> parts of OVN in general, not only northd, even if northd is the module >>>>>> that >>>>>> defines them. >>>>>> >>>>>>> Signed-off-by: Alexandra Rukomoinikova <[email protected]> >>>>>>> --- >>>>>>> Makefile.am | 8 ++++++++ >>>>>>> build-aux/automake.mk | 2 ++ >>>>>>> build-aux/calculate-northd-cksum | 3 +++ >>>>>>> build-aux/cksum-northd-check | 11 +++++++++++ >>>>>>> lib/ovn-util.c | 6 +++++- >>>>>>> 5 files changed, 29 insertions(+), 1 deletion(-) >>>>>>> create mode 100755 build-aux/calculate-northd-cksum >>>>>>> create mode 100755 build-aux/cksum-northd-check >>>>>>> >>>>>>> diff --git a/Makefile.am b/Makefile.am >>>>>>> index 6b0f1913a..5580584be 100644 >>>>>>> --- a/Makefile.am >>>>>>> +++ b/Makefile.am >>>>>>> @@ -379,6 +379,14 @@ check-ifconfig: >>>>>>> fi >>>>>>> .PHONY: check-ifconfig >>>>>>> >>>>>>> +# Check if northd stages cksum valid. >>>>>>> +ALL_LOCAL += check-northd >>>>>>> + >>>>>>> +.PHONY: check-northd >>>>>>> + >>>>>>> +check-northd: >>>>>>> +@build-aux/cksum-northd-check || exit 1 >>>>>> nit: Empty lines above don't seem to be needed. >>>>>> >>>>>> For the logic, this fails to build when the build directory is not the >>>>>> source directory. You need to prefix the build-aux patch with the >>>>>> $(srcdir) to be able to consistently find the script. >>>>>> >>>>>> Also, the script itself will not be able to find the northd.h and >>>>>> odp-util.h if they are not in the build directory. One way to fix that >>>>>> is to add both northd/northd.h and lib/ovn-util.c as dependencies for >>>>>> this build target and then pass them to the script as arguments via $?. >>>>>> Make will make sure to use proper paths for them pointing to the source >>>>>> directory. The scripts will need to use those arguments. See how it >>>>>> is done for the schema checksum checks for example. >>>>>> >>>>>> You may test this by running 'make distcheck' locally. This is also >>>>>> one of the reasons CI failed. >>>>>> >>>>>>> + >>>>>>> if HAVE_GROFF >>>>>>> ALL_LOCAL += manpage-check >>>>>>> manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS) >>>>>>> diff --git a/build-aux/automake.mk b/build-aux/automake.mk >>>>>>> index 255f573d7..a626c8aed 100644 >>>>>>> --- a/build-aux/automake.mk >>>>>>> +++ b/build-aux/automake.mk >>>>>>> @@ -1,7 +1,9 @@ >>>>>>> EXTRA_DIST += \ >>>>>>> build-aux/calculate-schema-cksum \ >>>>>>> +build-aux/calculate-northd-cksum \ >>>>>>> build-aux/cccl \ >>>>>>> build-aux/cksum-schema-check \ >>>>>>> +build-aux/cksum-northd-check \ >>>>>> Lexicographical order, please. >>>>>> >>>>>>> build-aux/dist-docs \ >>>>>>> build-aux/dpdkstrip.py \ >>>>>>> build-aux/initial-tab-whitelist \ >>>>>>> diff --git a/build-aux/calculate-northd-cksum >>>>>>> b/build-aux/calculate-northd-cksum >>>>>>> new file mode 100755 >>>>>>> index 000000000..41e1f3ca8 >>>>>>> --- /dev/null >>>>>>> +++ b/build-aux/calculate-northd-cksum >>>>>>> @@ -0,0 +1,3 @@ >>>>>>> +#!/bin/sh >>>>>>> + >>>>>>> +grep '^\s*PIPELINE_STAGE(' northd/northd.h | cksum >>>>>> Here the file name will need to be passed from the command line. >>>>>> >>>>>>> diff --git a/build-aux/cksum-northd-check b/build-aux/cksum-northd-check >>>>>>> new file mode 100755 >>>>>>> index 000000000..36a3e5046 >>>>>>> --- /dev/null >>>>>>> +++ b/build-aux/cksum-northd-check >>>>>>> @@ -0,0 +1,11 @@ >>>>>>> +#!/bin/sh >>>>>>> + >>>>>> And here. Two files. >>>>>> >>>>>>> +cksumcheckpath=`dirname $0` >>>>>>> +sum=`$cksumcheckpath/calculate-northd-cksum` >>>>>>> +checksum=$(sed -n '/\* cksum *=/{s/.*cksum *= *\([0-9]*\) >>>>>>> \([0-9]*\).*/\1 \2/p;q}' lib/ovn-util.c) >>>>>> This sed syntax is not portable. This is why osx build failed: >>>>>> >>>>>> sed: 1: "/\* cksum *=/{s/.*cksum ...": extra characters at the end of >>>>>> q command >>>>>> >>>>>> We should not use GNU extensions. In this case, actually, a simple >>>>>> grep may be sufficient. E.g.: >>>>>> >>>>>> grep -o 'cksum *= [0-9 ]*' -m1 lib/ovn-util.c | cut -d' ' -f3-4 >>>>>> >>>>>> One more thing is that this file is using both `` and $() for subshell >>>>>> execution. Choose one. >>>>>> >>>>>>> + >>>>>>> +if [ "$sum" != "$checksum" ]; then >>>>>>> + ln=`sed -n '/cksum =/{=;q}' lib/ovn-util.c` >>>>>> This also fails on osx: >>>>>> >>>>>> sed: 1: "/cksum =/{=;q}": extra characters at the end of q command >>>>>> >>>>>> grep can be used here instead as well: >>>>>> >>>>>> grep -on 'cksum *= [0-9 ]*' -m1 lib/ovn-util.c | cut -d':' -f1 >>>>>> >>>>>> >>>>>>> + echo >&2 "lib/ovn-util.c:$ln: The checksum \"$sum\" was calculated >>>>>>> from the northd stages and does not match cksum field file - you should >>>>>>> probably update the ovn version number and the checksum in the file." >>>>>> Please, split this line into multiple to avoid the line length warning. >>>>>> I know it's written this way for the schema, but there is no reason to >>>>>> repeat bad patterns. :) It can be cut into multiple strings and split >>>>>> across multiple lines with line continuations. >>>>>> >>>>>> Also, "cksum field file" ? A strange combination of words. May be >>>>>> better to use the actual file name. >>>>>> >>>>>>> + exit 1 >>>>>>> +fi >>>>>>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >>>>>>> index c65b36bb5..18841c2fd 100644 >>>>>>> --- a/lib/ovn-util.c >>>>>>> +++ b/lib/ovn-util.c >>>>>>> @@ -892,7 +892,11 @@ ip_address_and_port_from_lb_key(const char *key, >>>>>>> char **ip_address, >>>>>>> return true; >>>>>>> } >>>>>>> >>>>>>> -/* Increment this for any logical flow changes, if an existing OVN >>>>>>> action is >>>>>>> +/* >>>>>>> + * Checksum for stages in northd: >>>>>>> + * cksum = 1317960254 5844 >>>>>>> + * > We need to increment OVN_INTERNAL_MINOR_VER also whenever any existing > OVNACT is updated (see the comment in actions.h). Should we compute the > checksum of OVNACT rows too? That might be a bit trickier though.. > > * These OVNACTS are used to generate the OVN internal version. See > * ovn_get_internal_version() in lib/ovn-util.c. If any OVNACT is updated, > * increment the OVN_INTERNAL_MINOR_VER macro in lib/ovn-util.c. > > Any thoughts on this, Alexandra, Ilya, Vladislav? > >>>>>>> + * Increment this for any logical flow changes, if an existing OVN >>>>>>> action is >>>>>>> * modified or a stage is added to a logical pipeline. > I think we should update this comment too and mention that this should > happen for changes in the pipeline definition in northd.h and updates to > actions in actions.h. > >>>>>>> * >>>>>>> * This value is also used to handle some backward compatibility >>>>>>> during >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > Regards, > Dumitru > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
