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

Ihar Hrachyshka ihrachys at redhat.com
Fri Sep 18 17:14:18 UTC 2020


On Fri, Sep 18, 2020 at 11:54 AM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> 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().

That's fair. I've done just that since I-P mechanism is not obvious
since when chassis is changed, a lot of cleanup steps should happen,
like changing ownership for patch ports, creating new chassis records,
removing old, etc. I've sent a patch with the approach above.

>
> Thanks,
> Han
>



More information about the dev mailing list