Controller: handle add/remove in the same reconciliation #158

LionelJouin · 2023-09-14T09:46:40Z

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):

maiqueb

Some questions.

maiqueb · 2023-09-21T08:28:23Z

pkg/controller/pod_test.go

@@ -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),


why do you need to mock adding net0 ?

IIUC your test removed net0 and adds net1 ?... Both of which are already mocked.

That's correct sorry, I was probably trying something and I forgot to remove this part.

maiqueb · 2023-09-21T08:30:59Z

pkg/annotations/dynamic-network-status.go

@@ -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 {


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.

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.

I see, thanks for the explanation.

maiqueb · 2023-09-21T08:32:06Z

pkg/controller/pod.go

+	err = annotations.SetNetworkStatus(pod, newIfaceStatus)
+	if err != nil {


what's the purpose of this change ?

Please keep the old format. It's OK to update the error msg.

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

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.

maiqueb · 2023-09-21T08:32:59Z

pkg/controller/pod.go

+
+	err = annotations.SetNetworkStatus(pod, newIfaceStatus)
+	if err != nil {
+		return fmt.Errorf("failed to set the network status annotation: %v", err)


maiqueb · 2023-09-21T08:37:29Z

pkg/controller/pod.go

+	// 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
+	}


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 ?

Add and remove is only updating the pod object, it doesn't update the pod in Kubernetes

I see, thanks for explaining.

pkg/controller/pod_test.go

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]>

maiqueb

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.

maiqueb · 2023-09-22T11:51:28Z

pkg/controller/pod.go

+	err = annotations.SetNetworkStatus(pod, newIfaceStatus)
+	if err != nil {


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.

maiqueb · 2023-09-22T11:51:43Z

pkg/controller/pod.go

+	// 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
+	}


I see, thanks for explaining.

maiqueb · 2023-09-22T11:52:14Z

pkg/annotations/dynamic-network-status.go

@@ -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 {


I see, thanks for the explanation.

maiqueb · 2023-10-02T13:32:02Z

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.

Thoughts @kmabda ?

kmabda · 2023-10-02T19:11:44Z

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.

maiqueb · 2023-10-03T12:53:49Z

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.

kmabda

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.

LionelJouin requested a review from maiqueb as a code owner September 14, 2023 09:46

LionelJouin mentioned this pull request Sep 14, 2023

Controller: Set CNI args on the delegate request #156

Merged

LionelJouin force-pushed the issue-157 branch from 16b862a to c9a6b50 Compare September 14, 2023 14:46

maiqueb reviewed Sep 21, 2023

View reviewed changes

LionelJouin force-pushed the issue-157 branch from c9a6b50 to 9a2536a Compare September 21, 2023 17:09

maiqueb previously requested changes Oct 2, 2023

View reviewed changes

kmabda approved these changes Oct 3, 2023

View reviewed changes

maiqueb self-requested a review October 4, 2023 15:10

maiqueb approved these changes Oct 4, 2023

View reviewed changes

maiqueb merged commit bae1bb0 into k8snetworkplumbingwg:main Oct 4, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller: handle add/remove in the same reconciliation #158

Controller: handle add/remove in the same reconciliation #158

LionelJouin commented Sep 14, 2023

maiqueb left a comment

maiqueb Sep 21, 2023

LionelJouin Sep 21, 2023

maiqueb Sep 21, 2023

LionelJouin Sep 21, 2023

maiqueb Sep 22, 2023

maiqueb Sep 21, 2023

LionelJouin Sep 21, 2023

maiqueb Sep 22, 2023

maiqueb Sep 21, 2023

maiqueb Sep 21, 2023

LionelJouin Sep 21, 2023

maiqueb Sep 22, 2023

maiqueb left a comment

maiqueb Sep 22, 2023

maiqueb Sep 22, 2023

maiqueb Sep 22, 2023

maiqueb commented Oct 2, 2023

kmabda commented Oct 2, 2023

maiqueb commented Oct 3, 2023

kmabda left a comment

		err = annotations.SetNetworkStatus(pod, newIfaceStatus)
		if err != nil {

Controller: handle add/remove in the same reconciliation #158

Controller: handle add/remove in the same reconciliation #158

Conversation

LionelJouin commented Sep 14, 2023

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb commented Oct 2, 2023

kmabda commented Oct 2, 2023

maiqueb commented Oct 3, 2023

kmabda left a comment

Choose a reason for hiding this comment