| Message ID | 20221219161814.55422-5-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drop sampling: Fixes and optimizations | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
For the code change itself, Acked-by: Mark Michelson <mmichels@redhat.com> I have a note about the commit message. On 12/19/22 11:18, Adrian Moreno wrote: > The default zero value can lead to sampling errors if the pipeline sets > an the input port to OFP_NONE during flow processing. The commit message seems to contradict the code change. I'm guessing it should be something more like: "The default zero value can lead to sampling errors. To avoid this, the pipeline sets the input port to OFP_NONE during flow processing." The original version makes it sound like using OFP_NONE would cause sampling errors. Does my updated language make more sense? > > Fixes: a42c808f30b4 ("northd: add drop sampling") > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > controller/physical.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/controller/physical.c b/controller/physical.c > index 2444d3ebd..d715c2bfa 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -843,6 +843,7 @@ put_drop(const struct physical_debug *debug, uint8_t table_id, > os->collector_set_id = debug->collector_set_id; > os->obs_domain_id = (debug->obs_domain_id << 24); > os->obs_point_id = table_id; > + os->sampling_port = OFPP_NONE; > } > } >
On 1/6/23 20:15, Mark Michelson wrote: > For the code change itself, > > Acked-by: Mark Michelson <mmichels@redhat.com> > > I have a note about the commit message. > > On 12/19/22 11:18, Adrian Moreno wrote: >> The default zero value can lead to sampling errors if the pipeline sets >> an the input port to OFP_NONE during flow processing. > > The commit message seems to contradict the code change. I'm guessing it should > be something more like: > > "The default zero value can lead to sampling errors. To avoid this, the pipeline > sets the input port to OFP_NONE during flow processing." > > The original version makes it sound like using OFP_NONE would cause sampling > errors. Does my updated language make more sense? > Sorry for the confusion. I think the double use of OFP_NONE makes this sentence unreadable. What I meant by "if the pipeline sets an the input port to OFP_NONE during flow processing" is that, sometimes OVN will decide to add an action that sets the MFF_IN_PORT field to OFP_NONE (e.g: Table 64 in physical.c). If a sample action with the default zero value is executed after OVN has set the MFF_IN_PORT to OFP_NONE, the sampling fails. But, to be completely fair, the default zero value is wrong in itself (as in, a sample will be emitted with misleading data). So I could just rephrase it as: "The default zero value would likely match an existing openflow port and end up generating a sample with wrong output interface information. Since in this case we're sampling in the middle of the pipeline, the correct value for sampling port is OFP_NONE." What do you think? >> >> Fixes: a42c808f30b4 ("northd: add drop sampling") >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> controller/physical.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/controller/physical.c b/controller/physical.c >> index 2444d3ebd..d715c2bfa 100644 >> --- a/controller/physical.c >> +++ b/controller/physical.c >> @@ -843,6 +843,7 @@ put_drop(const struct physical_debug *debug, uint8_t >> table_id, >> os->collector_set_id = debug->collector_set_id; >> os->obs_domain_id = (debug->obs_domain_id << 24); >> os->obs_point_id = table_id; >> + os->sampling_port = OFPP_NONE; >> } >> } >
diff --git a/controller/physical.c b/controller/physical.c index 2444d3ebd..d715c2bfa 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -843,6 +843,7 @@ put_drop(const struct physical_debug *debug, uint8_t table_id, os->collector_set_id = debug->collector_set_id; os->obs_domain_id = (debug->obs_domain_id << 24); os->obs_point_id = table_id; + os->sampling_port = OFPP_NONE; } }
The default zero value can lead to sampling errors if the pipeline sets an the input port to OFP_NONE during flow processing. Fixes: a42c808f30b4 ("northd: add drop sampling") Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- controller/physical.c | 1 + 1 file changed, 1 insertion(+)