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.
Best regards, Ilya Maximets.
>
>> On 13 Mar 2025, at 02:52, Ilya Maximets <i.maxim...@ovn.org> 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 <arukomoinikova@k2.cloud>
>>> ---
>>> 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
>>> + *
>>> + * Increment this for any logical flow changes, if an existing OVN action
>>> is
>>> * modified or a stage is added to a logical pipeline.
>>> *
>>> * This value is also used to handle some backward compatibility during
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
