On Mon, Jun 13, 2022 at 3:17 PM Han Zhou <hz...@ovn.org> wrote:
>
>
>
> On Mon, Jun 13, 2022 at 11:40 AM Ihar Hrachyshka <ihrac...@redhat.com> wrote:
> >
> > On Thu, Jun 9, 2022 at 3:01 AM Han Zhou <hz...@ovn.org> wrote:
> > >
> > >
> > >
> > > On Tue, Jun 7, 2022 at 7:06 PM Ihar Hrachyshka <ihrac...@redhat.com>
> > > wrote:
> > > >
> > > > When options:activation-strategy is set to "rarp" for LSP, when used in
> > > > combination with multiple chassis names listed in
> > > > options:requested-chassis, additional chassis will install special flows
> > > > that would block all ingress and egress traffic for the port until a
> > > > special activation event happens.
> > > >
> > > > For "rarp" strategy, an observation of a RARP packet sent from the port
> > > > on the additional chassis is such an event. When it occurs, a special
> > > > flow passes control to a controller() action handler that eventually
> > > > removes the installed blocking flows and also marks the port as
> > > > options:additional-chassis-activated in southbound db.
> > > >
> > > > This feature is useful in live migration scenarios where it's not
> > > > advisable to unlock the destination port location prematurily to avoid
> > > > duplicate packets originating from the port.
> > > >
> > > > Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> > >
> > > Thanks for the revision. Please see my comments below:
> > >
> > > > +
> > > > +static void
> > > > +en_activated_ports_run(struct engine_node *node, void *data_)
> > > > +{
> > > > + struct ed_type_activated_ports *data = data_;
> > > > + enum engine_node_state state = EN_UNCHANGED;
> > > > +
> > > > + if (!data->activated_ports) {
> > > > + get_activated_ports(&data->activated_ports);
> > > > + if (data->activated_ports) {
> > > > + state = EN_UPDATED;
> > > > + }
> > > > + } else {
> > >
> > > This is not possible because en_activated_ports_clear_tracked_data() will
> > > always be called before engine_run(). So this branch is not really needed.
> > >
> > > > + struct ovs_list *new_activated_ports;
> > > > + bool ap_changed = get_activated_ports(&new_activated_ports);
> > > > + if (ap_changed && new_activated_ports) {
> > > > + en_activated_ports_cleanup(data);
> > > > + data->activated_ports = new_activated_ports;
> > > > + state = EN_UPDATED;
> > > > + }
> > > > + }
> > > > + engine_set_node_state(node, state);
> > > > +}
> > > > +
> > > > +static bool
> > > > +runtime_data_activated_ports_handler(struct engine_node *node, void
> > > > *data)
> > >
> > > nit: better to put this function together with runtime_data_xxx
> > > functions, since this is how we organize the functions for each engine
> > > node.
> > >
> >
> > Hm. Is it not? Right after this function,
> > runtime_data_ovs_interface_shadow_handler goes.
> >
> That's because the new en_activated_ports related data structure and
> functions you added in the middle of the en_runtime_data functions :)
> But I think there is no need for another revision just for this. Anyone who
> applies the change can move the block of code to a proper place.
>
Ouch, sorry. :( I'll move if I need another revision.
> > > ...
> > > > +static void
> > > > +setup_rarp_activation_strategy(const struct sbrec_port_binding
> > > > *binding,
> > > > + ofp_port_t ofport, struct zone_ids
> > > > *zone_ids,
> > > > + struct ovn_desired_flow_table
> > > > *flow_table,
> > > > + struct ofpbuf *ofpacts_p)
> > > > +{
> > > > + struct match match = MATCH_CATCHALL_INITIALIZER;
> > > > +
> > > > + /* Unblock the port on ingress RARP. */
> > > > + match_set_dl_type(&match, htons(ETH_TYPE_RARP));
> > > > + match_set_in_port(&match, ofport);
> > > > + ofpbuf_clear(ofpacts_p);
> > > > +
> > > > + load_logical_ingress_metadata(binding, zone_ids, ofpacts_p);
> > > > +
> > > > + size_t ofs = ofpacts_p->size;
> > > > + struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p);
> > > > + oc->max_len = UINT16_MAX;
> > > > + oc->reason = OFPR_ACTION;
> > > > +
> > > > + struct action_header ah = {
> > > > + .opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP)
> > > > + };
> > > > + ofpbuf_put(ofpacts_p, &ah, sizeof ah);
> > > > +
> > > > + ofpacts_p->header = oc;
> > > > + oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc);
> > > > + ofpact_finish_CONTROLLER(ofpacts_p, &oc);
> > > > + put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
> > > > +
> > > > + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1010,
> > > > + binding->header_.uuid.parts[0],
> > > > + &match, ofpacts_p, &binding->header_.uuid);
> > > > + ofpbuf_clear(ofpacts_p);
> > > > +
> > > > + /* Block all non-RARP traffic for the port, both directions. */
> > > > + match_init_catchall(&match);
> > > > + match_set_in_port(&match, ofport);
> > > > +
> > > > + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 1000,
> > > > + binding->header_.uuid.parts[0],
> > > > + &match, ofpacts_p, &binding->header_.uuid);
> > > > +
> > > > + match_init_catchall(&match);
> > > > + uint32_t dp_key = binding->datapath->tunnel_key;
> > > > + uint32_t port_key = binding->tunnel_key;
> > > > + match_set_metadata(&match, htonll(dp_key));
> > > > + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
> > > > +
> > > > + ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 1000,
> > > > + binding->header_.uuid.parts[0],
> > > > + &match, ofpacts_p, &binding->header_.uuid);
> > >
> > > Recall my comment for v6:
> > > >> What if another LSP on the same chassis (an additional chassis of the
> > > >> migrating LSP) wants to send a packet to the migrating LSP on the main
> > > >> chassis? Would it be blocked, too?
> > >
> > > > Packets are cloned to all chassis, delivered to all of them, and each
> > > of them decides on its own if it's ok to deliver it (depending on
> > > whether activation flows are still present). I'll update the test case
> > > to cover the scenario you describe.
> > >
> > > What I meant was that: the flow is added to a datapath of LS, with the
> > > outport matching the LSP that is being migrated, consider the example
> > > below:
> > > LSP1 is migrating from HV1 to HV2. Now LSP2 on the same datapath and
> > > bound on HV2 is sending a packet destined to LSP1, it is expected that
> > > the packet is delivered to HV1 at this moment, right? However, because of
> > > the above flow, it would match (both dp_key and port_key), so the packet
> > > will be dropped. Did I misunderstand?
> > >
> >
> > There are two drop flows set up on HV2 related to activation strategy:
> >
> > cookie=0xc2723b3d, duration=2.150s, table=0, n_packets=0, n_bytes=0,
> > idle_age=2, priority=1000,in_port=3 actions=drop
> > cookie=0xc2723b3d, duration=2.151s, table=65, n_packets=3,
> > n_bytes=126, idle_age=0, priority=1000,reg15=0x1,metadata=0x1
> > actions=drop
> >
> > The first flow is for traffic that originate from the unactivated
> > port. The second flow is for traffic that is directed to the port.
> > Your scenario involves the second flow. In this case, first
> > table=37-39 flows are triggered that will clone packets to HV1, among
> > other things; then once we reach table=65, delivery to HV2 endpoint
> > will be blocked.
> >
> > I believe this scenario is already covered in
> > "options:activation-strategy for logical port", specifically see the
> > following check:
> >
> > # Packet from hv2:Second arrives to hv1:Migrator
> > # hv2:Second cannot reach hv2:Migrator because it is blocked by RARP
> > strategy
> > request=$(send_arp hv2 second 000000000002 000000000010 $second_spa
> > $migrator_spa)
> > echo $request >> hv1/migrator.expected
> >
>
> You are right. Thanks for explaining and sorry that I was misled by something
> wrong in my memory.
>
> > > ...
> > > >
> > > > +static bool activated_ports_changed = false;
> > > > +static struct ovs_list activated_ports = OVS_LIST_INITIALIZER(
> > > > + &activated_ports);
> > > > +
> > > > +bool
> > > > +get_activated_ports(struct ovs_list **ap)
> > > > +{
> > > > + ovs_mutex_lock(&pinctrl_mutex);
> > > > + if (ovs_list_is_empty(&activated_ports)) {
> > > > + activated_ports_changed = false;
> > > > + ovs_mutex_unlock(&pinctrl_mutex);
> > > > + *ap = NULL;
> > > > + return false;
> > > > + }
> > > > +
> > > > + struct activated_port *pp;
> > > > + *ap = xmalloc(sizeof **ap);
> > > > + ovs_list_init(*ap);
> > > > +
> > > > + LIST_FOR_EACH (pp, list, &activated_ports) {
> > > > + struct activated_port *pp_copy = xmalloc(sizeof *pp_copy);
> > > > + pp_copy->port_key = pp->port_key;
> > > > + pp_copy->dp_key = pp->dp_key;
> > > > + ovs_list_push_front(*ap, &pp_copy->list);
> > > > + }
> > > > + bool ap_changed = activated_ports_changed;
> > >
> > > Instead of a global flag, I guess we will need a per entry flag in the
> > > activated_ports list, to mark the entries as "consumed by I-P engine"
> > > once it is returned by this function. And this function should only
> > > return the entries that have not yet returned before. (alternatively,
> > > just use too different lists).
> > > Otherwise, the list may contain entries that are consumed by I-P engine
> > > but not yet made it to the SB DB, and also the entries that are not
> > > consumed by I-P engine yet, and may be reprocessed by I-P engine. Is that
> > > desired?
> >
> > Yes there's some redundancy in how this is being implemented in this
> > revision. I'll split it into two separate, initially duplicate, lists
> > for engine and db as you suggested.
> >
> Thanks,
> Han
>
> >
> > >
> > > > + ovs_mutex_unlock(&pinctrl_mutex);
> > > > + return ap_changed;
> > > > +}
> > > > +
> > >
> > > Thanks,
> > > Han
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
