On Fri, Sep 18, 2020 at 11:54 AM Han Zhou <[email protected]> wrote: > > > > On Fri, Sep 18, 2020 at 7:22 AM Ihar Hrachyshka <[email protected]> wrote: > > > > On Fri, Sep 18, 2020 at 3:19 AM Han Zhou <[email protected]> wrote: > > > > > > > > > > > > On Thu, Sep 17, 2020 at 7:15 PM Ihar Hrachyshka <[email protected]> > > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
