On 3/18/25 11:26 AM, Aleksandr Smirnov (K2 Cloud) wrote: > Hi guys, >
Hi Alexander, > 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. > That's a fair ask, a #define is OK from my perspective. > Thank you, > > Alexander > Regards, Dumitru > 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
