-
Notifications
You must be signed in to change notification settings - Fork 12
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
Controller: handle add/remove in the same reconciliation #158
Conversation
16b862a
to
c9a6b50
Compare
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.
Some questions.
pkg/controller/pod_test.go
Outdated
@@ -119,7 +119,9 @@ var _ = Describe("Dynamic Attachment controller", func() { | |||
fakecri.NewFakeRuntime(*pod), | |||
fakemultusclient.NewFakeClient( | |||
networkConfig(multuscni.CmdAdd, "net1", macAddr), | |||
networkConfig(multuscni.CmdDel, "net0", "")), | |||
networkConfig(multuscni.CmdDel, "net0", ""), | |||
networkConfig(multuscni.CmdAdd, "net0", macAddr), |
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.
why do you need to mock adding net0 ?
IIUC your test removed net0 and adds net1 ?... Both of which are already mocked.
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.
That's correct sorry, I was probably trying something and I forgot to remove this part.
@@ -64,6 +66,31 @@ func PodDynamicNetworkStatus(currentPod *corev1.Pod) ([]nettypes.NetworkStatus, | |||
return currentIfaceStatus, nil | |||
} | |||
|
|||
// SetNetworkStatus updates the Pod status | |||
func SetNetworkStatus(pod *corev1.Pod, statuses []v1.NetworkStatus) error { |
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.
hm, maybe I'm missing something, but why are you implementing this function instead of re-using the one provided in the network attachment definition client ?
We're already importing those utils.
If there's a reason for that, please also refer it in the commit message.
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.
This function in the nad-client will keep the pod object passed in parameter intact. It fetches the pod via the name from the Kubernetes API, updates its annotation and updates it via the Kubenretes API.
https://github.com/k8snetworkplumbingwg/network-attachment-definition-client/blob/v1.4.0/pkg/utils/net-attach-def.go#L87
The function I created is different. It updates the annotation of the pod object passed as parameter without updating it in Kubernetes.
So, when the add happens, the pod object is modified with the new annotations (not yet updated in Kubernetes). The del will then happen with the pod object modified with the added annotation (here is the difference with the previous behavior), and then the del will modify the annotations and then the Kubernetes API is called to update the pod itself.
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.
I see, thanks for the explanation.
err = annotations.SetNetworkStatus(pod, newIfaceStatus) | ||
if err != nil { |
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.
what's the purpose of this change ?
Please keep the old format. It's OK to update the error msg.
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.
This updates the annotation in the pod object passed in parameter. The update of the pod via the Kubernetes API is no longer done here
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.
You're right, I had missed you had changed the package !!! My bad.
I do think the function's name is misleading though. And this is not as explicit as I would like it to be.
|
||
err = annotations.SetNetworkStatus(pod, newIfaceStatus) | ||
if err != nil { | ||
return fmt.Errorf("failed to set the network status annotation: %v", 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.
ditto
// apply the network status using the Kubernetes API. | ||
if err := nadutils.SetNetworkStatus(pnc.k8sClientSet, pod, newIfaceStatus); err != nil { | ||
klog.Errorf("failed while setting network status annotation: %v", err) | ||
return true | ||
} |
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.
The commit message says:
The controller is now setting the pod network status annotation only
once in the reconcile loop to avoid conflicts when a network attachment
is added and another one removed at the same time.
But I don't think that's true; IIUC, you're setting it 3 times - once in addNetworks
, once in remove
networks, and a final one (this one) at the end of processNextWorkItem
.
Right ?
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.
Add and remove is only updating the pod object, it doesn't update the pod in Kubernetes
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.
I see, thanks for explaining.
The controller is now setting the pod network status annotation only once in the reconcile loop to avoid conflicts when a network attachment is added and another one removed at the same time. Fixes: k8snetworkplumbingwg#157 Signed-off-by: Lionel Jouin <[email protected]>
c9a6b50
to
9a2536a
Compare
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.
OK !
First of: thanks for reporting the bug, and thanks for providing a fix for it. I appreciate both your effort for being a contributing member of the community, and the way you're doing so.
Secondly, as stated in the issue, I agree we can improve the user flow.
Lastly, while code wise this could work, I don't like how it looks like. It mislead me a couple of times.
I think we can (and should!) do what your PR proposes - i.e. only set the status once, at the end of the flow. I do disagree on the "how" you're doing it though.
What do you think about removing the "update state" functions from the addNetworks
/ removeNetworks
functions, and update the PodDynamicNetworkStatus to receive the list of "added" attachments and "removed" attachments ? Then just compute it all in one go, and write that to the API once ?
It is more code, but I do feel it is more explicit. The update status block should happen in a defer, so it gets done even if there's an error adding/removing an attachment to the pod (but some others succeeded).
@LionelJouin do you see any issues with this ?
I also welcome other options.
err = annotations.SetNetworkStatus(pod, newIfaceStatus) | ||
if err != nil { |
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.
You're right, I had missed you had changed the package !!! My bad.
I do think the function's name is misleading though. And this is not as explicit as I would like it to be.
// apply the network status using the Kubernetes API. | ||
if err := nadutils.SetNetworkStatus(pnc.k8sClientSet, pod, newIfaceStatus); err != nil { | ||
klog.Errorf("failed while setting network status annotation: %v", err) | ||
return true | ||
} |
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.
I see, thanks for explaining.
@@ -64,6 +66,31 @@ func PodDynamicNetworkStatus(currentPod *corev1.Pod) ([]nettypes.NetworkStatus, | |||
return currentIfaceStatus, nil | |||
} | |||
|
|||
// SetNetworkStatus updates the Pod status | |||
func SetNetworkStatus(pod *corev1.Pod, statuses []v1.NetworkStatus) error { |
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.
I see, thanks for the explanation.
Thoughts @kmabda ? |
Thanks for reporting this bug and providing a fix for it @LionelJouin IMHO, the approach used to fix this issue as provided in this PR looks okay to me, the virtue is keeping it simple. |
OK, fair enough. Please approve it, and let's merge. @kmabda I can refactor this later on, and we can discuss if it that's preferable. The unit tests seem good enough for me to work on top of. |
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.
Looks good to me. Please address the 2 format related changes (around lines 363 and 422 of pkg/controller/pod.go) requested by @maiqueb
Agreed to move forward with these changes, and refactor later.
What this PR does / why we need it:
The controller is now setting the pod network status annotation only once in the reconcile loop to avoid conflicts when a network attachment is added and another one removed at the same time.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes: #157
Special notes for your reviewer (optional):