Handle update network status failed after add and del attachments #237

Longchuanzheng · 2024-07-23T11:02:10Z

What this PR does / why we need it:
This pr delete all attachments added in this processNextWorkItem when set NetworkStatus failed in handleResult. Because DEL is Idempotence we can always return to the state before dynamic handle.

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 #235

Special notes for your reviewer (optional):

Longchuanzheng · 2024-07-24T02:00:48Z

This pr will cancel(delete) the added attachment in two cases:

When update network status failed in handleResult, we delete all attachment added in this reconcile.
When add attachments failed in reconcile, we try to delete add failed and all subsequent attachments that need to be added.

LionelJouin

I think this feature makes sense. I don't know if it would be possible to add unit tests for it?

pkg/controller/pod.go

Longchuanzheng · 2024-07-25T01:47:43Z

I think this feature makes sense. I don't know if it would be possible to add unit tests for it?

ok, I will try.

Longchuanzheng · 2024-07-27T06:00:59Z

Right now, it will return an err if we want to delet net1 in ut. If we change this, some previous ut will failed. So my ut expect FailedRemovingInterface for remove the net1.
Or do we need do more in this pr to improve ut (I think it will change more codes in ut.)?

LionelJouin · 2024-08-28T08:48:59Z

pkg/controller/pod.go

+	if shouldDeleteattachmentsAdded {
+		attachmentsToCancel = attachmentsToAdd
+	} else if addfailedIndex != -1 {
+		attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[addfailedIndex])


Suggested change

attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[addfailedIndex])

attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[:addfailedIndex])

Why having reverted this changes? All non failing attachment added must be reverted no?

this is a good point ...

I agree with @LionelJouin; shouldn't you delete all the attachments you've just added ? i.e. setting the pod back to its original state ?

otherwise, the pod's network-status and the pod's actual interfaces will not be consistent.

LionelJouin · 2024-08-28T09:12:00Z

pkg/controller/pod_test.go

+					Eventually(<-eventRecorder.Events).Should(Equal(expectedEventPayload))
+				})
+
+				It("the pod network-status dosen't change", func() {


Suggested change

It("the pod network-status dosen't change", func() {

It("the pod network-status doesn't change", func() {

LionelJouin · 2024-08-28T09:29:10Z

pkg/controller/pod_test.go

+					Eventually(<-eventRecorder.Events).Should(Equal(expectedEventPayload))
+
+					expectedEventPayload = fmt.Sprintf(
+						"Warning FailedRemovingInterface pod [%s]: failed removing interface %s from network: %s",


you can add networkConfig(multuscni.CmdDel, "net1", macAddr), to the fake multus client line 126 to test the simple case if you want (the simple case I mean: everything works except status update).

LionelJouin · 2024-08-28T09:31:06Z

pkg/controller/pod_test.go

+					Eventually(<-eventRecorder.Events).Should(Equal(expectedEventPayload))
+				})
+
+				It("the pod network-status dosen't change", func() {


I am not sure if this is required as the status update has been prevented, so this might test Kubernetes.

LionelJouin · 2024-08-28T09:31:27Z

pkg/controller/pod_test.go

+					expectedError := errors.New("someerror")
+					k8sClient.PrependReactor("update", "pods", func(_ k8stesting.Action) (bool, runtime.Object, error) {
+						return true, nil, expectedError
+					})


To be moved to the JustBeforeEach

Longchuanzheng · 2024-09-20T07:58:32Z

@maiqueb Hi, can you review this pr, thank you in advance. I think this #235 will cause pod add/del attachnet stuck.
It is not very friendly if we stuck when try to hotplugin nic for vm with kubevirt.

maiqueb

Thank you for this PR. I agree we should keep the pod's interfaces consistent with the reported network-status at all times. Extra thanks for complying and adding the unit test @LionelJouin requested.

I'm requesting some changes to improve the overall code quality, and I also wonder if your approach (only deleting one interface) wouldn't break the pod interface / pod network status consistency.

maiqueb · 2024-07-24T13:48:20Z

pkg/controller/pod.go

+	defer func() {
+		pnc.handleResult(err, podNamespacedName, pod, results, attachmentsToAdd, netnsPath, podSandboxID, addFailedIndex)
+	}()


I don't like how this looks.

How about we define a Rollback interface and pass that to the handle result func ?

It would be something like

type interface Rollback { Rollback() error } type asd struct { netnsPath string podSandboxID string attachmentsToRollback []nadv1.NetworkSelectionElement } func (a *asd) Rollback() error { // ... invoke DEL for attachments on asd.attachments }

Then, on handleResult you just invoke rollback if err != nil

pkg/controller/pod_test.go

maiqueb · 2024-09-20T08:50:41Z

pkg/controller/pod.go

+	if shouldDeleteattachmentsAdded {
+		attachmentsToCancel = attachmentsToAdd
+	} else if addfailedIndex != -1 {
+		attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[addfailedIndex])


this is a good point ...

I agree with @LionelJouin; shouldn't you delete all the attachments you've just added ? i.e. setting the pod back to its original state ?

otherwise, the pod's network-status and the pod's actual interfaces will not be consistent.

Signed-off-by: zhuanlan <[email protected]>

Longchuanzheng requested a review from maiqueb as a code owner July 23, 2024 11:02

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch from 3d1610d to 0320270 Compare July 23, 2024 11:16

Longchuanzheng mentioned this pull request Jul 23, 2024

Handle Interface already exists due to SetNetworkStatus failed #236

Closed

Longchuanzheng changed the title ~~handle Interface already exists due to SetNetworkStatus failed~~ Handle update network status failed after add and del attachments Jul 23, 2024

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch from 0320270 to c1a00ec Compare July 24, 2024 01:48

LionelJouin reviewed Jul 24, 2024

View reviewed changes

pkg/controller/pod.go Outdated Show resolved Hide resolved

pkg/controller/pod.go Outdated Show resolved Hide resolved

pkg/controller/pod.go Outdated Show resolved Hide resolved

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch from 61ec648 to c2829d1 Compare July 27, 2024 05:55

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch 2 times, most recently from 79be62f to 1c54179 Compare July 30, 2024 09:11

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch from 1c54179 to 24c5dbe Compare August 7, 2024 02:27

Longchuanzheng requested a review from LionelJouin August 20, 2024 01:06

LionelJouin reviewed Aug 28, 2024

View reviewed changes

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch 2 times, most recently from 13eaa7b to 2cd1a8d Compare August 30, 2024 06:31

maiqueb requested changes Sep 20, 2024

View reviewed changes

Longchuanzheng added 2 commits September 27, 2024 20:31

handle SetNetworkStatus failed

db9a292

Signed-off-by: zhuanlan <[email protected]>

add UT for handle SetNetworkStatus failed

2aa70ec

Signed-off-by: zhuanlan <[email protected]>

Longchuanzheng force-pushed the handleSetNetworkStatusFailed1 branch 2 times, most recently from 13eaa7b to 2aa70ec Compare September 27, 2024 12:33

Longchuanzheng requested review from LionelJouin and maiqueb September 27, 2024 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle update network status failed after add and del attachments #237

Handle update network status failed after add and del attachments #237

Longchuanzheng commented Jul 23, 2024 •

edited

Loading

Longchuanzheng commented Jul 24, 2024

LionelJouin left a comment

Longchuanzheng commented Jul 25, 2024

Longchuanzheng commented Jul 27, 2024

LionelJouin Aug 28, 2024

maiqueb Sep 20, 2024

LionelJouin Aug 28, 2024

LionelJouin Aug 28, 2024

LionelJouin Aug 28, 2024

LionelJouin Aug 28, 2024

Longchuanzheng commented Sep 20, 2024

maiqueb left a comment

maiqueb Jul 24, 2024

maiqueb Sep 20, 2024

	attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[addfailedIndex])
	attachmentsToCancel = append(attachmentsToCancel, attachmentsToAdd[:addfailedIndex])

	It("the pod network-status dosen't change", func() {
	It("the pod network-status doesn't change", func() {

Handle update network status failed after add and del attachments #237

Are you sure you want to change the base?

Handle update network status failed after add and del attachments #237

Conversation

Longchuanzheng commented Jul 23, 2024 • edited Loading

Longchuanzheng commented Jul 24, 2024

LionelJouin left a comment

Choose a reason for hiding this comment

Longchuanzheng commented Jul 25, 2024

Longchuanzheng commented Jul 27, 2024

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

Longchuanzheng commented Sep 20, 2024

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

Longchuanzheng commented Jul 23, 2024 •

edited

Loading