failed callbacks on create calls

Author: SergeyKanzhelev

pkg/validate/nri_linux.go

@@ -18,6 +18,7 @@ package validate
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -781,6 +782,81 @@ var _ = framework.KubeDescribe("NRI", func() {
 				framework.Logf("RemoveContainer(%s) failed: %v", containerID, err)
 			}
 		})
+
+		It("should fail RunPodSandbox and clean up when the NRI hook errors, then allow retry", func(ctx SpecContext) {
+			var err error
+
+			testStub, err = StartNRITestStub("cri-test-nri-run-error", "00")
+			Expect(err).NotTo(HaveOccurred(), "failed to start NRI test stub")
+
+			// Fail only the first RunPodSandbox invocation so the retry can pass.
+			var failOnce sync.Once
+
+			testStub.Plugin.OnRunPodSandbox = func(_ context.Context, _ *nri.PodSandbox) error {
+				shouldFail := false
+
+				failOnce.Do(func() { shouldFail = true })
+
+				if shouldFail {
+					return errors.New("induced NRI RunPodSandbox failure")
+				}
+
+				return nil
+			}
+
+			By("building the pod sandbox config")
+
+			podSandboxName := "nri-test-run-error-" + framework.NewUUID()
+			uid := framework.DefaultUIDPrefix + framework.NewUUID()
+			namespace := framework.DefaultNamespacePrefix + framework.NewUUID()
+			podConfig = &runtimeapi.PodSandboxConfig{
+				Metadata: framework.BuildPodSandboxMetadata(podSandboxName, uid, namespace, framework.DefaultAttempt),
+				Linux: &runtimeapi.LinuxPodSandboxConfig{
+					CgroupParent: common.GetCgroupParent(ctx, rc),
+				},
+				Labels: framework.DefaultPodLabels,
+			}
+
+			By("attempting RunPodSandbox while the NRI hook is failing")
+
+			failedPodID, runErr := rc.RunPodSandbox(ctx, podConfig, framework.TestContext.RuntimeHandler)
+			Expect(runErr).To(HaveOccurred(),
+				"RunPodSandbox MUST fail when the NRI RunPodSandbox hook returns an error")
+			Expect(failedPodID).To(BeEmpty(),
+				"No pod sandbox ID should be returned when RunPodSandbox fails")
+
+			By("verifying the NRI RunPodSandbox hook actually fired")
+			// The hook records its event before returning the error, so the
+			// attempted sandbox ID is available for the cleanup/leak check.
+			attemptedID := testStub.Plugin.LastRunPodSandboxID()
+			Expect(attemptedID).NotTo(BeEmpty(),
+				"NRI RunPodSandbox hook should have fired before the failure")
+
+			By("verifying the failed sandbox is not left behind")
+
+			pods, listErr := rc.ListPodSandbox(ctx, nil)
+			Expect(listErr).NotTo(HaveOccurred())
+
+			for _, pod := range pods {
+				matches := pod.GetId() == attemptedID ||
+					(pod.GetMetadata() != nil && pod.GetMetadata().GetName() == podSandboxName)
+				Expect(matches).To(BeFalse(),
+					"sandbox %s MUST be cleaned up after a failed RunPodSandbox", pod.GetId())
+			}
+
+			By("retrying RunPodSandbox after the NRI hook stops failing")
+
+			podID = framework.RunPodSandbox(ctx, rc, podConfig)
+			Expect(podID).NotTo(BeEmpty(),
+				"RunPodSandbox retry should succeed after the NRI hook stops failing")
+
+			By("verifying the retried sandbox becomes Ready")
+
+			statusResp, err := rc.PodSandboxStatus(ctx, podID, false)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(statusResp.GetStatus().GetState()).To(Equal(runtimeapi.PodSandboxState_SANDBOX_READY),
+				"sandbox should become Ready after a successful RunPodSandbox retry")
+		})
 	})
 
 	Context("StopPodSandbox contract", Serial, func() {
@@ -1061,4 +1137,271 @@ var _ = framework.KubeDescribe("NRI", func() {
 				"Failed CreateContainer on a stopped sandbox MUST NOT generate an NRI CreateContainer event")
 		})
 	})
+
+	Context("CreateContainer contract", Serial, func() {
+		var (
+			testStub  *NRITestStub
+			podID     string
+			podConfig *runtimeapi.PodSandboxConfig
+			// containerID holds the successfully created retry container so
+			// AfterEach can remove it even if an inline assertion fails.
+			containerID string
+		)
+
+		BeforeEach(func(ctx SpecContext) {
+			var err error
+
+			testStub, err = StartNRITestStub("cri-test-nri-create", "00")
+			Expect(err).NotTo(HaveOccurred(), "failed to start NRI test stub")
+
+			// Ensure the test image is available before creating containers.
+			framework.PullPublicImage(ctx, ic, framework.TestContext.TestImageList.DefaultTestContainerImage, nil)
+
+			By("creating a pod sandbox")
+
+			podSandboxName := "nri-test-create-" + framework.NewUUID()
+			uid := framework.DefaultUIDPrefix + framework.NewUUID()
+			namespace := framework.DefaultNamespacePrefix + framework.NewUUID()
+			podConfig = &runtimeapi.PodSandboxConfig{
+				Metadata: framework.BuildPodSandboxMetadata(podSandboxName, uid, namespace, framework.DefaultAttempt),
+				Linux: &runtimeapi.LinuxPodSandboxConfig{
+					CgroupParent: common.GetCgroupParent(ctx, rc),
+				},
+				Labels: framework.DefaultPodLabels,
+			}
+			podID = framework.RunPodSandbox(ctx, rc, podConfig)
+			Expect(podID).NotTo(BeEmpty())
+		})
+
+		AfterEach(func(ctx SpecContext) {
+			// Stop the stub first so a still-failing hook cannot interfere with
+			// teardown of the container or sandbox.
+			if testStub != nil {
+				testStub.Cleanup()
+			}
+
+			if containerID != "" {
+				if err := rc.StopContainer(ctx, containerID, 0); err != nil {
+					framework.Logf("AfterEach: StopContainer(%s) failed: %v", containerID, err)
+				}
+
+				if err := rc.RemoveContainer(ctx, containerID); err != nil {
+					framework.Logf("AfterEach: RemoveContainer(%s) failed: %v", containerID, err)
+				}
+			}
+
+			if podID != "" {
+				if err := rc.StopPodSandbox(ctx, podID); err != nil {
+					framework.Logf("AfterEach: StopPodSandbox(%s) failed: %v", podID, err)
+				}
+
+				if err := rc.RemovePodSandbox(ctx, podID); err != nil {
+					framework.Logf("AfterEach: RemovePodSandbox(%s) failed: %v", podID, err)
+				}
+			}
+		})
+
+		It("should not expose container while CreateContainer hook is in progress", func(ctx SpecContext) {
+			// This test validates the spec contract: during CreateContainer hook
+			// execution, the container MUST NOT be visible via ListContainers or
+			// ContainerStatus.
+			hookBlocking := make(chan struct{})
+			hookReached := make(chan struct{})
+
+			var hookContainerID string
+
+			testStub.Plugin.OnCreateContainer = func(hookCtx context.Context, _ *nri.PodSandbox, container *nri.Container) error {
+				hookContainerID = container.GetId()
+
+				close(hookReached)
+
+				select {
+				case <-hookBlocking:
+				case <-hookCtx.Done():
+				}
+
+				return nil
+			}
+
+			By("triggering CreateContainer in a goroutine")
+
+			containerName := "nri-test-block-create-" + framework.NewUUID()
+			containerConfig := &runtimeapi.ContainerConfig{
+				Metadata: framework.BuildContainerMetadata(containerName, framework.DefaultAttempt),
+				Image: &runtimeapi.ImageSpec{
+					Image:              framework.TestContext.TestImageList.DefaultTestContainerImage,
+					UserSpecifiedImage: framework.TestContext.TestImageList.DefaultTestContainerImage,
+				},
+				Command: framework.DefaultPauseCommand,
+				Linux:   &runtimeapi.LinuxContainerConfig{},
+			}
+
+			var (
+				createErr error
+				createdID string
+				createWg  sync.WaitGroup
+			)
+
+			createWg.Go(func() {
+				createdID, createErr = rc.CreateContainer(ctx, podID, containerConfig, podConfig)
+			})
+
+			By("waiting for CreateContainer hook to be reached")
+
+			select {
+			case <-hookReached:
+				// Hook is now blocking
+			case <-time.After(30 * time.Second):
+				close(hookBlocking) // unblock to avoid goroutine leak
+				Fail("Timed out waiting for CreateContainer NRI hook to fire")
+			}
+
+			By("verifying container is NOT listed while hook is blocking")
+
+			containers, listErr := rc.ListContainers(ctx, &runtimeapi.ContainerFilter{
+				PodSandboxId: podID,
+			})
+			Expect(listErr).NotTo(HaveOccurred())
+
+			for _, c := range containers {
+				Expect(c.GetId()).NotTo(Equal(hookContainerID),
+					"Container %s MUST NOT be listed while CreateContainer hook is blocking", hookContainerID)
+			}
+
+			By("verifying ContainerStatus is not accessible while hook is blocking")
+
+			if hookContainerID != "" {
+				statusResp, statusErr := rc.ContainerStatus(ctx, hookContainerID, false)
+				if statusErr == nil && statusResp != nil && statusResp.GetStatus() != nil {
+					Expect(statusResp.GetStatus().GetState()).NotTo(Equal(runtimeapi.ContainerState_CONTAINER_CREATED),
+						"Container MUST NOT report CREATED state while CreateContainer hook is in progress")
+				}
+			}
+
+			By("releasing the hook and verifying container is created")
+			close(hookBlocking)
+			createWg.Wait()
+			Expect(createErr).NotTo(HaveOccurred(), "CreateContainer should succeed after hook returns")
+			Expect(createdID).NotTo(BeEmpty())
+			containerID = createdID
+
+			// After hook completes, container should be in CREATED state
+			statusResp, err := rc.ContainerStatus(ctx, containerID, false)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(statusResp.GetStatus().GetState()).To(Equal(runtimeapi.ContainerState_CONTAINER_CREATED),
+				"Container should be in CREATED state after CreateContainer hook completes")
+		})
+
+		It("should fail CreateContainer when the NRI hook errors, leak nothing, and allow retry", func(ctx SpecContext) {
+			// Fail only the first CreateContainer invocation so the retry passes.
+			var failOnce sync.Once
+
+			testStub.Plugin.OnCreateContainer = func(_ context.Context, _ *nri.PodSandbox, _ *nri.Container) error {
+				shouldFail := false
+
+				failOnce.Do(func() { shouldFail = true })
+
+				if shouldFail {
+					return errors.New("induced NRI CreateContainer failure")
+				}
+
+				return nil
+			}
+
+			// Reset events so we only observe container events from this point.
+			testStub.Plugin.Reset()
+
+			By("attempting CreateContainer while the NRI hook is failing")
+
+			containerName := "nri-test-create-error-ctr-" + framework.NewUUID()
+			containerConfig := &runtimeapi.ContainerConfig{
+				Metadata: framework.BuildContainerMetadata(containerName, framework.DefaultAttempt),
+				Image:    &runtimeapi.ImageSpec{Image: framework.TestContext.TestImageList.DefaultTestContainerImage},
+				Command:  framework.DefaultPauseCommand,
+				Linux:    &runtimeapi.LinuxContainerConfig{},
+			}
+
+			failedContainerID, createErr := framework.CreateContainerWithError(ctx, rc, ic, containerConfig, podID, podConfig)
+			Expect(createErr).To(HaveOccurred(),
+				"CreateContainer MUST fail when the NRI CreateContainer hook returns an error")
+			Expect(failedContainerID).To(BeEmpty(),
+				"No container ID should be returned when CreateContainer fails")
+
+			By("verifying the NRI CreateContainer hook actually fired")
+			// The hook records its event before returning the error, confirming
+			// the failure was induced on the creation path as intended.
+			Eventually(func() int {
+				count := 0
+
+				for _, e := range testStub.Plugin.Events() {
+					if e.Type == EventCreateContainer {
+						count++
+					}
+				}
+
+				return count
+			}, 10*time.Second, 50*time.Millisecond).Should(BeNumerically(">=", 1),
+				"NRI CreateContainer hook should have fired before the failure")
+
+			By("verifying the failed CreateContainer leaked no container")
+
+			containers, listErr := rc.ListContainers(ctx, &runtimeapi.ContainerFilter{
+				PodSandboxId: podID,
+			})
+			Expect(listErr).NotTo(HaveOccurred(), "ListContainers after failed CreateContainer")
+
+			for _, c := range containers {
+				if c.GetMetadata() != nil && c.GetMetadata().GetName() == containerName {
+					containerID = c.GetId() // hand to AfterEach for cleanup
+					Fail(fmt.Sprintf("container %s was leaked after a failed CreateContainer", c.GetId()))
+				}
+			}
+
+			By("verifying the failed CreateContainer did not start a container")
+			// A failed CreateContainer MUST NOT result in a started container.
+			// The runtime may emit Stop/Remove events as part of internal cleanup
+			// of the partially created container, but a StartContainer event must
+			// never appear. Use Consistently so events delivered slightly after the
+			// failure are still caught.
+			Consistently(func() int {
+				count := 0
+
+				for _, e := range testStub.Plugin.Events() {
+					if e.Type == EventStartContainer {
+						count++
+					}
+				}
+
+				return count
+			}, 2*time.Second, 200*time.Millisecond).Should(BeZero(),
+				"a failed CreateContainer MUST NOT result in a started container")
+
+			By("retrying CreateContainer after the NRI hook stops failing")
+
+			retryName := "nri-test-create-retry-ctr-" + framework.NewUUID()
+			retryConfig := &runtimeapi.ContainerConfig{
+				Metadata: framework.BuildContainerMetadata(retryName, framework.DefaultAttempt),
+				Image:    &runtimeapi.ImageSpec{Image: framework.TestContext.TestImageList.DefaultTestContainerImage},
+				Command:  framework.DefaultPauseCommand,
+				Linux:    &runtimeapi.LinuxContainerConfig{},
+			}
+
+			retryID := framework.CreateContainer(ctx, rc, ic, retryConfig, podID, podConfig)
+			Expect(retryID).NotTo(BeEmpty(),
+				"CreateContainer retry should succeed after the NRI hook stops failing")
+			// Hand the retry container to AfterEach immediately so a failure in
+			// the start/stop/remove assertions below cannot leak it.
+			containerID = retryID
+
+			By("verifying the retried container can be started, stopped, and removed")
+			Expect(rc.StartContainer(ctx, retryID)).NotTo(HaveOccurred(),
+				"the retried container should start successfully")
+			Expect(rc.StopContainer(ctx, retryID, 0)).NotTo(HaveOccurred(),
+				"the retried container should stop successfully")
+			Expect(rc.RemoveContainer(ctx, retryID)).NotTo(HaveOccurred(),
+				"the retried container should be removable")
+			// Clear containerID so AfterEach does not attempt to stop/remove it again.
+			containerID = ""
+		})
+	})
 })