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? So, maybe just make a checksum a part of ovn-northd internal
version?
WDYT?
>
> 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
--
Regards,
Vladislav Odintsov
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
