[ovs-dev] [PATCH ovn v4 3/9] controller: Make use of Port_Binding:requested_chassis
Frode Nordahl
frode.nordahl at canonical.com
Thu Sep 16 17:54:10 UTC 2021
On Thu, Sep 16, 2021 at 7:50 PM Numan Siddique <numans at ovn.org> wrote:
>
> On Thu, Sep 16, 2021 at 1:33 PM Frode Nordahl
> <frode.nordahl at canonical.com> wrote:
> >
> > On Thu, Sep 16, 2021 at 6:12 PM Numan Siddique <numans at ovn.org> wrote:
> > >
> > > On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl
> > > <frode.nordahl at canonical.com> wrote:
> > > >
> > > > Improve the efficiency of the requested-chassis feature by using
> > > > the new Southbound Port_Binding:requested_chassis column instead
> > > > of each chassis performing option processing and string comparison.
> > > >
> > > > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> > >
> > > I've one comment which seems to be discussed in v3 of the first patch.
> > >
> > > Looks to me this patch would break the existing behavior (which the
> > > patch 2 addresses).
> > >
> > > Right now when CMS sets requested-chassis=foo for a logical port 'P1',
> > > then other ovn-controllers
> > > will not claim 'P1' if there is a VIF for this even if chassis 'foo'
> > > doesn't exist.
> > >
> > > With this patch series, other ovn-controller will claim 'P1' for the
> > > above scenario I mentioned.
> > >
> > > Is this going to be a problem ? Or are we going to break some existing
> > > deployments ?
> >
> > I have to admit that I would find it unusual for a CMS to refer to
> > non-existing chassis, but I do see that it could happen if the CMS
> > knows about a hypervisor prior to OVN knowing about it, so I see what
> > you mean.
>
> One usecase of this happening is when ovn-controller exits (without
> --restart) option.
> In this case, it deletes its own chassis record.
That would indeed happen quite often! Thank you for pointing that out.
> >
> > > I'd suggest not breaking this scenario. If the
> > > port_binding->options:requested-chassis is set,
> > > then ovn-controller should not claim that port binding if the
> > > port_binding->requested_chassis is
> > > set. In other words ovn-controller should claim only if
> > > port_binding->requested_chassis == this_chassis
> > > when the port_binding->options:requested-chassis is set.
> > >
> > > Does this make sense ?
> >
> > Yes. I guess it's not really possible to point at a non-existing
> > chassis at all with this approach, and having a phantom chassis for
> > this would be entering hacky territory? I can drop this patch, but I
> > would still need the column for the ovn-controller to be made aware of
> > ports it should consider prior to any interface existing locally.
>
> I'm not suggesting to drop this patch but to also check if
> port_binding->options:requested-chassis
> is set or not. If CMS sets logical_port->options:requested-chassis,
> this will be copied over
> to port_binding->options and only then would ovn-northd set
> port_binding->requested_chassis
> if the chassis row exists in SB DB.
Right, so use both to confirm, got it. That works for me!
> Something like :
>
> if (smap_get(&port_binding->options, "requested-chassis") &&
> !port_binding->requested_chassis)
> then don't claim the port binding if there is a VIF for this.
>
> If you do this way, then we would not need patch 2.
>
> Let me know if this is fine.
Sounds good, I'll work on that approach.
> >
> > Would you be ok with keeping the name, or should I find a different
> > name so that it does not conflict with the requested-chassis use case?
>
> I have no strong preference. If you can come up with a better name
> that would do too :)
If we're using it in conjunction with the requested-chassis option as
discussed above, I guess it's fine to keep the name too. Thx!
--
Frode Nordahl
> Thanks
> Numan
>
> >
> > --
> > Frode Nordahl
> >
> > > Numan
> > >
> > >
> > >
> > > > ---
> > > > controller/binding.c | 29 ++++++++++++-----------------
> > > > controller/ovn-controller.c | 3 +++
> > > > controller/physical.c | 7 ++-----
> > > > 3 files changed, 17 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/controller/binding.c b/controller/binding.c
> > > > index 34935bb9c..938e1d81d 100644
> > > > --- a/controller/binding.c
> > > > +++ b/controller/binding.c
> > > > @@ -1051,11 +1051,10 @@ is_binding_lport_this_chassis(struct binding_lport *b_lport,
> > > >
> > > > static bool
> > > > can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
> > > > - const char *requested_chassis)
> > > > + const struct sbrec_port_binding *pb)
> > > > {
> > > > - return !requested_chassis || !requested_chassis[0]
> > > > - || !strcmp(requested_chassis, chassis_rec->name)
> > > > - || !strcmp(requested_chassis, chassis_rec->hostname);
> > > > + return !pb->requested_chassis
> > > > + || chassis_rec == pb->requested_chassis;
> > > > }
> > > >
> > > > /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER,
> > > > @@ -1093,7 +1092,7 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec,
> > > >
> > > > static bool
> > > > consider_vif_lport_(const struct sbrec_port_binding *pb,
> > > > - bool can_bind, const char *vif_chassis,
> > > > + bool can_bind,
> > > > struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out,
> > > > struct binding_lport *b_lport,
> > > > @@ -1134,7 +1133,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
> > > > "requested-chassis %s",
> > > > pb->logical_port,
> > > > b_ctx_in->chassis_rec->name,
> > > > - vif_chassis);
> > > > + pb->requested_chassis ?
> > > > + pb->requested_chassis->name : "(none)");
> > > > }
> > > > }
> > > >
> > > > @@ -1157,9 +1157,7 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
> > > > struct local_binding *lbinding,
> > > > struct hmap *qos_map)
> > > > {
> > > > - const char *vif_chassis = smap_get(&pb->options, "requested-chassis");
> > > > - bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> > > > - vif_chassis);
> > > > + bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);
> > > >
> > > > if (!lbinding) {
> > > > lbinding = local_binding_find(&b_ctx_out->lbinding_data->bindings,
> > > > @@ -1189,8 +1187,8 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
> > > > }
> > > > }
> > > >
> > > > - return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> > > > - b_ctx_out, b_lport, qos_map);
> > > > + return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> > > > + b_lport, qos_map);
> > > > }
> > > >
> > > > static bool
> > > > @@ -1274,12 +1272,9 @@ consider_container_lport(const struct sbrec_port_binding *pb,
> > > > }
> > > >
> > > > ovs_assert(parent_b_lport && parent_b_lport->pb);
> > > > - const char *vif_chassis = smap_get(&parent_b_lport->pb->options,
> > > > - "requested-chassis");
> > > > - bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> > > > - vif_chassis);
> > > > + bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);
> > > >
> > > > - return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, b_ctx_out,
> > > > + return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> > > > container_b_lport, qos_map);
> > > > }
> > > >
> > > > @@ -1328,7 +1323,7 @@ consider_virtual_lport(const struct sbrec_port_binding *pb,
> > > > }
> > > > }
> > > >
> > > > - if (!consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out,
> > > > + if (!consider_vif_lport_(pb, true, b_ctx_in, b_ctx_out,
> > > > virtual_b_lport, qos_map)) {
> > > > return false;
> > > > }
> > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > > index 0031a1035..7387a177b 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -232,6 +232,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > > > sbrec_port_binding_add_clause_chassis(&pb, OVSDB_F_EQ,
> > > > &chassis->header_.uuid);
> > > >
> > > > + sbrec_port_binding_add_clause_requested_chassis(
> > > > + &pb, OVSDB_F_EQ, &chassis->header_.uuid);
> > > > +
> > > > /* Ensure that we find out about l2gateway and l3gateway ports that
> > > > * should be present on this chassis. Otherwise, we might never find
> > > > * out about those ports, if their datapaths don't otherwise have a VIF
> > > > diff --git a/controller/physical.c b/controller/physical.c
> > > > index 6f2c1cea0..2f6bd0e91 100644
> > > > --- a/controller/physical.c
> > > > +++ b/controller/physical.c
> > > > @@ -1066,11 +1066,8 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > > > } else {
> > > > ofport = local_binding_get_lport_ofport(local_bindings,
> > > > binding->logical_port);
> > > > - const char *requested_chassis = smap_get(&binding->options,
> > > > - "requested-chassis");
> > > > - if (ofport && requested_chassis && requested_chassis[0] &&
> > > > - strcmp(requested_chassis, chassis->name) &&
> > > > - strcmp(requested_chassis, chassis->hostname)) {
> > > > + if (ofport && binding->requested_chassis
> > > > + && binding->requested_chassis != chassis) {
> > > > /* Even though there is an ofport for this port_binding, it is
> > > > * requested on a different chassis. So ignore this ofport.
> > > > */
> > > > --
> > > > 2.32.0
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
More information about the dev
mailing list