-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid duplicate Node Results in Live Traceflow Status #4715
Avoid duplicate Node Results in Live Traceflow Status #4715
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -34,11 +34,16 @@ import ( | |||
binding "antrea.io/antrea/pkg/ovs/openflow" | |||
) | |||
|
|||
var skipTraceflowUpdateErr = errors.New("Skip Traceflow Update") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: errors.New("skip Traceflow update")
to follow the convention that error shouldn't be capitalized first letter unless proper noun, even though it's not appended to any log currently?
@@ -47,8 +52,7 @@ func (c *Controller) HandlePacketIn(pktIn *ofctrl.PacketIn) error { | |||
err = retry.RetryOnConflict(retry.DefaultRetry, func() error { | |||
tf, err := c.traceflowInformer.Lister().Get(oldTf.Name) | |||
if err != nil { | |||
klog.Warningf("Get traceflow failed: %+v", err) | |||
return err | |||
return fmt.Errorf("Get Traceflow failed: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("Get Traceflow failed: %w", err) | |
return fmt.Errorf("get Traceflow failed: %w", err) |
@@ -57,14 +61,13 @@ func (c *Controller) HandlePacketIn(pktIn *ofctrl.PacketIn) error { | |||
} | |||
_, err = c.traceflowClient.CrdV1alpha1().Traceflows().UpdateStatus(context.TODO(), update, v1.UpdateOptions{}) | |||
if err != nil { | |||
klog.Warningf("Update traceflow failed: %+v", err) | |||
return err | |||
return fmt.Errorf("Update Traceflow failed: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("Update Traceflow failed: %w", err) | |
return fmt.Errorf("update Traceflow failed: %w", err) |
After receiving a Packet In for a live Traceflow, the controller uninstalls the corresponding OVS flows to prevent additional Packet Ins (from different connections which also match the Live Traceflow filters). However, if 2 connections happen within a very short time window (< 1ms in my testbed) and both match the Live Traceflow filters, it is still possible for 2 Packet In messages to be received. To avoid duplicate Node Results in the Live Traceflow Status, we need to ignore all Packet In messages received after the first one. Fixes antrea-io#4714 Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
01b1843
to
66473ba
Compare
/test-all |
@tnqn I addressed your comments. Feel free to include it in Antrea v1.11 or postpone it to the next release, since this is not a serious / core bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think it's good to be included and there should be no risk. |
After receiving a Packet In for a live Traceflow, the controller uninstalls the corresponding OVS flows to prevent additional Packet Ins (from different connections which also match the Live Traceflow filters). However, if 2 connections happen within a very short time window (< 1ms in my testbed) and both match the Live Traceflow filters, it is still possible for 2 Packet In messages to be received. To avoid duplicate Node Results in the Live Traceflow Status, we need to ignore all Packet In messages received after the first one. Fixes antrea-io#4714
After receiving a Packet In for a live Traceflow, the controller uninstalls the corresponding OVS flows to prevent additional Packet Ins (from different connections which also match the Live Traceflow filters).
However, if 2 connections happen within a very short time window (< 1ms in my testbed) and both match the Live Traceflow filters, it is still possible for 2 Packet In messages to be received. To avoid duplicate Node Results in the Live Traceflow Status, we need to ignore all Packet In messages received after the first one.
Fixes #4714