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 <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
>>>>>> + *
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
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
