[ovs-dev] [PATCH v6 ovn] Allow to run multiple controllers on the same machine

Han Zhou hzhou at ovn.org
Fri Sep 18 15:54:30 UTC 2020


On Fri, Sep 18, 2020 at 7:22 AM Ihar Hrachyshka <ihrachys at redhat.com> wrote:
>
> On Fri, Sep 18, 2020 at 3:19 AM Han Zhou <hzhou at ovn.org> wrote:
> >
> >
> >
> > On Thu, Sep 17, 2020 at 7:15 PM Ihar Hrachyshka <ihrachys at redhat.com>
wrote:
> > >
> > > Hi Han,
> > >
> > > thanks a lot for the review! I addressed most comments below, listing
> > > here the questions not immediately addressed in the next version I am
> > > about to push:
> > >
> > > 1. why is file_system_id array static? there are drawbacks in
> > > switching it to local: we would need to do a lot more xstrdup / free
> > > work in multiple places. But it's doable.
> >
> > Thanks for your explanation. Static makes sense.
> >
> > >
> > > 2. the confusion between system-id (OVN) and system-id.conf (OVS) file
> > > names. Should we rename the former into e.g.
> > > $ovsconfdir/ovn-chassis-name-override or something along these lines?
> > >
> >
> > ovn-chassis-name-override sounds good to me. Since it is for OVN only,
I think it is better to put in OVN's config dir.
>
> Ack. You mean $ovn_sysconfdir/ovn? (/etc/ovn/)
>
Yes.

> >
> > However, I just realized another problem related to the use of the
file. Since it is a new input to the I-P engine, we will have to add a node
to the engine with the only data being the ID read from the file. We will
also track the old ID, and in its run() function we will check if the ID is
changed, and then trigger engine compute for all the nodes that depend on
it. Otherwise, changing the ID in the file won't get enforced by the I-P
engine. Sorry that I didn't thought about this in my previous review.
>
> I appreciate the request, just tested and indeed it doesn't (always)
> work as one could expect assuming the file can be changed in runtime.
> I am happy to fix this. For that though, I may need some more time
> than a couple of hours (need to understand the I-P machinery first,
> which I haven't touched yet). Is it possible that we e.g. mark the
> limitation in documentation and in release notes and follow up with a
> backportable bug fix for that? Let me know.
>
If changing it at runtime is not required, we don't have to implement the
I-P node now, as long as it is documented. And we can add the support
changing it at runtime later (but I hope it doesn't have to be backported
if it is not a bug fix).

However, I guess the current behavior is not consistent, because when
recompute is triggered on en_runtime_data, the new file content would take
effect, which may confuse user because they can't predict when the change
will take effect. It would be better to make it either taking effect
immediately, or never. If we choose the latter, we can simply perform the
reading of the file only once at initialization, and don't re-read it in
get_ovs_chassis_id().

Thanks,
Han


More information about the dev mailing list