Add regression tests for sandbox pods when no CNI plugins are initialized #10771

nabeelmohamed · 2024-10-04T15:51:16Z

What is the problem which i am trying to solve?

As mentioned in PR #10744, when no CNI plugins are initialized, the pod network teardown within sandbox_run.go gets deferred and fails, causing NetNS removal to get blocked. This results in a failed pod creation that cannot be cleaned up.

Describing the solution

To address issue #10768, I've added a regression test to sandbox_run_rollback_test.go to ensure that the sandbox creation fails with the expected error message when no CNI plugins are initialized, as identified in PR #10744. @sameersaeed @mikebrow

k8s-ci-robot · 2024-10-04T15:51:26Z

Hi @nabeelmohamed. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sameersaeed · 2024-10-04T21:28:24Z

integration/sandbox_run_rollback_test.go

+			sbConfig := PodSandboxConfig(t.Name(), "nocni", WithPodLabels(labels))
+
+			t.Log("Inject CNI plugin initialization failpoint")
+			injectCNIFailpoint(t, sbConfig, map[string]string{


can make the following change to resolve the syntax error:

conf := &failpointConf{ Add: "1*error(no CNI plugins initialized))", } injectCNIFailpoint(t, sbConfig, conf)

could you also please run gofmt -w -s <containerd-path>/integration/sandbox_run_rollback_test.go after making the necessary changes to resolve the linter error?

sameersaeed · 2024-10-04T21:30:30Z

please also note that PR #10770 has been opened to resolve the issue of pod network teardown failures blocking cleanup

nabeelmohamed · 2024-10-05T04:24:56Z

i think that error can be fixed by
t.Log("Inject CNI plugin initialization failpoint")
conf := &failpointConf{
Add: "1*error(no CNI plugins initialized)",
}
injectCNIFailpoint(t, sbConfig, conf)
adding these 'failpointconf' struct to this. since passing a map[string]string to the injectCNIFailpoint function, but it's expecting a pointer to a failpointConf struct

mikebrow · 2024-10-16T19:13:20Z

Please rebase now that #10839 has been merged.

mikebrow · 2024-10-22T14:11:04Z

@nabeelmohamed can you rebase your test and make it work correctly with the update?

sameersaeed · 2024-11-08T17:03:24Z

Hi @nabeelmohamed, I left some suggestions on your most recent commit - please review and let me know if you have any questions!

sameersaeed · 2024-11-08T17:08:58Z

integration/sandbox_run_rollback_test.go

+			t.Log("ListPodSandbox with the specific label")
+			l, err := runtimeService.ListPodSandbox(&criapiv1.PodSandboxFilter{LabelSelector: labels})
+			require.NoError(t, err)
+			require.Len(t, l, 1)


We want to make sure that no pods are up if the 'no CNI plugins initialized' error is thrown, so the number of pods returned by ListPodSandbox should be 0, not 1.

nabeelmohamed · 2024-11-09T09:40:50Z

Hi @sameersaeed

I’ve made the changes as per your suggestions. Specifically, I’ve:

Removed the detailed PodSandbox state checks.

Updated the test to assert that zero pods are up when the 'no CNI plugins initialized' error is thrown.

Added cleanup logic to remove the pod used for this check.

Included assertions to ensure no pods are up before and after the containerd restart.

Thanks for your guidance. Please let me know if there are any further adjustments needed.

sameersaeed · 2024-11-09T16:49:13Z

Hi @nabeelmohamed, thanks for making the requested changes. There are still some minor nitpicks I have with the current code based on my previous suggestions.

I've made some additional changes on my own branch. Could you please merge these changes onto your branch?

Please also make sure to rebase and run gofmt if needed, so that the tests pass. Thank you!

sameersaeed · 2024-11-11T14:08:08Z

Thank you @nabeelmohamed! Just seems like the gofmt you ran in commit 0cbcd3a modified some unexpected files. We would only need to run it on the file we are changing for this test, sandbox_run_rollback_test.go

To fix this, could you please first revert to commit 7526ea1, (i.e. git reset --hard 7526ea1), and then squash the extra 2 commits that you have on this branch (i.e. git reset --soft HEAD~2)?

After doing the above, you can check if you need to run gofmt on sandbox_run_rollback_test.go by running gofmt -l integration from within your containerd directory. If you see the test file being outputted, you can run gofmt -s -w ./integration/sandbox_run_rollback_test.go to format the file before pushing. Otherwise, it should be fine to push without any further changes (i.e., git add . + git commit -s --amend + git push [...]).

One other minor thing - when running git commit -s --amend as mentioned above, in the commit message for 7526ea1, can you please remove any extra text after the Signed-off-by / Co-authored-by lines?

i.e., in the following, you can remove the line modified sandbox_run_roollback_test.go by merging and rebasing:

Add regression tests for sandbox pods when no CNI plugins are initialized

Co-authored-by: Mohamed Nabeel <137497525+nabeelmohamed@users.noreply.github.com>
Signed-off-by: Sameer <sameer.saeed@live.ca>

modified sandbox_run_roollback_test.go by merging and rebasing

sameersaeed · 2024-11-12T14:25:54Z

/retest

k8s-ci-robot · 2024-11-12T14:26:10Z

@sameersaeed: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sameersaeed · 2024-11-08T17:10:28Z

integration/sandbox_run_rollback_test.go

+			require.NoError(t, err)
+			require.Len(t, l, 1)
+			sb := l[0]
+			require.Equal(t, criapiv1.PodSandboxState_SANDBOX_NOTREADY, sb.State)


Can leave out the extra checks. We should just have an assert to pass the test if we see that 0 pods are up after checking the output of ListPodSandbox, and fail it if that isn't the case

Lines 328 - 329, 354 - 381 and 386 - 390 can be removed. We just need to make sure no pods come up and, in the case that this test fails, remove any pods that may get created.

Calling runtimeService.RemovePodSandbox(sb.Id) may also throw an error if no pods are up. It would instead be better to only make the call if the number of pods outputted by ListPodSandbox is greater than 0.

i.e.,

l, err := runtimeService.ListPodSandbox(&criapiv1.PodFilter{LabelSelector: labels}) require.Len(t, l, 0) if len(l) > 0 { t.Log("Cleanup leaky pod(s)") cleanupPods(t, runtimeService) }

sameersaeed · 2024-11-08T20:37:00Z

integration/sandbox_run_rollback_test.go

+			injectCNIFailpoint(t, sbConfig, conf)
+
+			// Continue with the rest of your test...
+			t.Log("Create a sandbox")


I would change the log message to something like: "Create a sandbox pod while having no CNI plugins initialized"

Can also remove unnecessary comments on lines 337 and 343

sameersaeed · 2024-11-08T20:37:59Z

integration/sandbox_run_rollback_test.go

+			require.Error(t, err)
+			require.ErrorContains(t, err, "no CNI plugins initialized")
+
+			t.Log("ListPodSandbox with the specific label")


I would also change this log message to something like: "No pods should be up, as CNI failure is expected clean up the pod that was attempted to be created"

sameersaeed · 2024-11-12T14:54:47Z

integration/sandbox_run_rollback_test.go

+	t.Log("No pods should be up, as CNI failure is expected to clean up the pod that was attempted to be created")
+	l, err := runtimeService.ListPodSandbox(&criapiv1.PodSandboxFilter{LabelSelector: labels})
+	require.NoError(t, err)
+	require.Len(t, l, 0)


From looking over again, it would probably be better to change this require on L349 to an assert, i.e.:
assert.Len(t, l, 0)

With this change, the test will not immediately exit in a failing case, so we can properly cleanup the pod(s) if needed before finishing the test

Please also rebase if necessary and make sure to use git commit -s --amend for the commit message

mikebrow · 2024-11-12T15:29:27Z

/ok-to-test

sameersaeed · 2024-11-19T22:06:18Z

Hi @nabeelmohamed, thanks for making the requested changes - seems like all the tests are passing as expected now.

Could you please rebase once again? Please also make sure to set the commit message using git -s --amend once again, as was mentioned in my previous post

nabeelmohamed · 2024-11-22T11:03:42Z

hey @sameersaeed ,i dont know why all of a sudden there is 5 failing test which is not in the previos check. all i done is rebase with upstream and force pushed with commit amend message.can you guide me....

sameersaeed · 2024-11-22T15:57:13Z

Hi @nabeelmohamed, it seems that there were failures in the latest CI run that are not related to the changes from this PR's test code. You can rerun the tests with the /retest command if needed.

Before you do so, could you please use git commit -s --amend to set modify the commit message to the following?

Add regression tests for sandbox pods when no CNI plugins are initialized

Co-authored-by: Mohamed Nabeel <137497525+nabeelmohamed@users.noreply.github.com>
Signed-off-by: Sameer <sameer.saeed@live.ca>

sameersaeed · 2024-11-25T02:32:15Z

Hi @nabeelmohamed, from checking your most recent commit (5ac091f), it seems that the commit message is still set as the following:

Add regression tests for sandbox pods when no CNI plugins are initialized
Co-authored-by: Mohamed Nabeel <137497525+nabeelmohamed@users.noreply.github.com>

Signed-off-by: nabeelmohamed <mohamednabeel321@gmail.com>

As mentioned, could you please change it to the following by using git commit -s --amend and editing the commit message?

Add regression tests for sandbox pods when no CNI plugins are initialized

Co-authored-by: Sameer <sameer.saeed@live.ca>
Signed-off-by: nabeelmohamed <mohamednabeel321@gmail.com>

sameersaeed · 2024-12-02T22:04:14Z

Hi @nabeelmohamed - sorry for the delay, I didn't receive a notification on your most recent rebase

Could you please rebase once again and tag me or @mikebrow after doing so? Thank you

…ized Co-authored-by: Sameer <sameer.saeed@live.ca> Signed-off-by: nabeelmohamed <mohamednabeel321@gmail.com>

nabeelmohamed · 2024-12-04T05:03:37Z

Hi @nabeelmohamed - sorry for the delay, I didn't receive a notification on your most recent rebase

Could you please rebase once again and tag me or @mikebrow after doing so? Thank you

hey @sameersaeed just rebased and force pushed. also I didn't forget to use git commit --amend command . could you please review this

sameersaeed · 2024-12-04T16:30:25Z

/retest

mikebrow

LGTM thanks!

k8s-ci-robot added needs-ok-to-test size/M labels Oct 4, 2024

dosubot bot added the kind/test label Oct 4, 2024

mikebrow self-assigned this Oct 4, 2024

sameersaeed reviewed Oct 4, 2024

View reviewed changes

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from d587e55 to 71bf817 Compare October 23, 2024 04:41

k8s-ci-robot added the do-not-merge/contains-merge-commits label Oct 23, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 6cfd2a3 to 37e9587 Compare October 23, 2024 04:53

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Oct 23, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch 4 times, most recently from 92e2b4a to bffc458 Compare October 23, 2024 06:30

sameersaeed reviewed Nov 8, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/contains-merge-commits label Nov 9, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 56db713 to b53fb1b Compare November 9, 2024 10:22

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Nov 9, 2024

k8s-ci-robot added size/XXL and removed size/M labels Nov 11, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 0cbcd3a to a29f1f0 Compare November 12, 2024 04:55

k8s-ci-robot removed the size/XXL label Nov 12, 2024

sameersaeed reviewed Nov 12, 2024

View reviewed changes

k8s-ci-robot added ok-to-test do-not-merge/contains-merge-commits and removed needs-ok-to-test labels Nov 12, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch 2 times, most recently from 3b25598 to 4cd3211 Compare November 13, 2024 05:41

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Nov 13, 2024

k8s-ci-robot added the do-not-merge/contains-merge-commits label Nov 21, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from f2adeb2 to f562d30 Compare November 21, 2024 04:48

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Nov 21, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from f562d30 to 4f647b3 Compare November 21, 2024 05:39

k8s-ci-robot added the do-not-merge/contains-merge-commits label Nov 23, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 117ca1c to 5ac091f Compare November 23, 2024 10:24

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Nov 23, 2024

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 5ac091f to 7ace914 Compare November 25, 2024 04:52

k8s-ci-robot added the do-not-merge/contains-merge-commits label Dec 4, 2024

Add regression tests for sandbox pods when no CNI plugins are initial…

fb33422

…ized Co-authored-by: Sameer <sameer.saeed@live.ca> Signed-off-by: nabeelmohamed <mohamednabeel321@gmail.com>

nabeelmohamed force-pushed the Adding-regression-tests-for-sandbox-pods-when-no-CNI-plugins-are-initialized branch from 2bf4346 to fb33422 Compare December 4, 2024 05:00

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Dec 4, 2024

mikebrow approved these changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regression tests for sandbox pods when no CNI plugins are initialized #10771

Add regression tests for sandbox pods when no CNI plugins are initialized #10771

nabeelmohamed commented Oct 4, 2024

k8s-ci-robot commented Oct 4, 2024

sameersaeed Oct 4, 2024

sameersaeed Oct 4, 2024

sameersaeed commented Oct 4, 2024

nabeelmohamed commented Oct 5, 2024

mikebrow commented Oct 16, 2024

mikebrow commented Oct 22, 2024

sameersaeed commented Nov 8, 2024

sameersaeed Nov 8, 2024 •

edited

Loading

nabeelmohamed commented Nov 9, 2024

sameersaeed commented Nov 9, 2024 •

edited

Loading

sameersaeed commented Nov 11, 2024 •

edited

Loading

sameersaeed commented Nov 12, 2024

k8s-ci-robot commented Nov 12, 2024

sameersaeed Nov 8, 2024

sameersaeed Nov 8, 2024

sameersaeed Nov 8, 2024

sameersaeed Nov 8, 2024

sameersaeed Nov 8, 2024

sameersaeed Nov 12, 2024

mikebrow commented Nov 12, 2024

sameersaeed commented Nov 19, 2024

nabeelmohamed commented Nov 22, 2024

sameersaeed commented Nov 22, 2024 •

edited

Loading

sameersaeed commented Nov 25, 2024

sameersaeed commented Dec 2, 2024

nabeelmohamed commented Dec 4, 2024

sameersaeed commented Dec 4, 2024

mikebrow left a comment

Add regression tests for sandbox pods when no CNI plugins are initialized #10771

Are you sure you want to change the base?

Add regression tests for sandbox pods when no CNI plugins are initialized #10771

Conversation

nabeelmohamed commented Oct 4, 2024

k8s-ci-robot commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameersaeed commented Oct 4, 2024

nabeelmohamed commented Oct 5, 2024

mikebrow commented Oct 16, 2024

mikebrow commented Oct 22, 2024

sameersaeed commented Nov 8, 2024

sameersaeed Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

nabeelmohamed commented Nov 9, 2024

sameersaeed commented Nov 9, 2024 • edited Loading

sameersaeed commented Nov 11, 2024 • edited Loading

sameersaeed commented Nov 12, 2024

k8s-ci-robot commented Nov 12, 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

mikebrow commented Nov 12, 2024

sameersaeed commented Nov 19, 2024

nabeelmohamed commented Nov 22, 2024

sameersaeed commented Nov 22, 2024 • edited Loading

sameersaeed commented Nov 25, 2024

sameersaeed commented Dec 2, 2024

nabeelmohamed commented Dec 4, 2024

sameersaeed commented Dec 4, 2024

mikebrow left a comment

Choose a reason for hiding this comment

sameersaeed Nov 8, 2024 •

edited

Loading

sameersaeed commented Nov 9, 2024 •

edited

Loading

sameersaeed commented Nov 11, 2024 •

edited

Loading

sameersaeed commented Nov 22, 2024 •

edited

Loading