Merge pull request #9310 from openshift-cherrypick-robot/cherry-pick-9291-to-release-1.30

openshift-merge-bot[bot] · web-flow · commit b6af3c84d136 · 2025-07-07T16:27:02.000Z
OCPBUGS-58286: [release-1.30] : sandbox: use created/stopped instead of infra container for readiness
diff --git a/internal/lib/container_server.go b/internal/lib/container_server.go
@@ -358,6 +358,11 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb
 	}
 
 	sb.SetCreated()
+
+	if scontainer.State().Status == oci.ContainerStateStopped {
+		sb.SetStopped(ctx, true)
+	}
+
 	if err := label.ReserveLabel(processLabel); err != nil {
 		return sb, err
 	}
diff --git a/internal/lib/sandbox/sandbox.go b/internal/lib/sandbox/sandbox.go
@@ -56,15 +56,18 @@ type Sandbox struct {
 	nsOpts             *types.NamespaceOption
 	dnsConfig          *types.DNSConfig
 	stopMutex          sync.RWMutex
-	created            bool
-	stopped            bool
-	networkStopped     bool
-	privileged         bool
-	hostNetwork        bool
-	usernsMode         string
-	containerEnvPath   string
-	podLinuxOverhead   *types.LinuxContainerResources
-	podLinuxResources  *types.LinuxContainerResources
+	// stateMutex protects the use of created, stopped and networkStopped bools
+	// which are all fields that can change at runtime
+	stateMutex        sync.RWMutex
+	created           bool
+	stopped           bool
+	networkStopped    bool
+	privileged        bool
+	hostNetwork       bool
+	usernsMode        string
+	containerEnvPath  string
+	podLinuxOverhead  *types.LinuxContainerResources
+	podLinuxResources *types.LinuxContainerResources
 }
 
 // DefaultShmSize is the default shm size
@@ -352,6 +355,10 @@ func (s *Sandbox) RemoveInfraContainer() {
 func (s *Sandbox) SetStopped(ctx context.Context, createFile bool) {
 	ctx, span := log.StartSpan(ctx)
 	defer span.End()
+
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
+
 	if s.stopped {
 		return
 	}
@@ -366,16 +373,24 @@ func (s *Sandbox) SetStopped(ctx context.Context, createFile bool) {
 // Stopped returns whether the sandbox state has been
 // set to stopped.
 func (s *Sandbox) Stopped() bool {
+	s.stateMutex.RLock()
+	defer s.stateMutex.RUnlock()
+
 	return s.stopped
 }
 
 // SetCreated sets the created status of sandbox to true
 func (s *Sandbox) SetCreated() {
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
 	s.created = true
 }
 
 // NetworkStopped returns whether the network has been stopped
 func (s *Sandbox) NetworkStopped() bool {
+	s.stateMutex.RLock()
+	defer s.stateMutex.RUnlock()
+
 	return s.networkStopped
 }
 
@@ -389,6 +404,10 @@ func (s *Sandbox) NetworkStopped() bool {
 func (s *Sandbox) SetNetworkStopped(ctx context.Context, createFile bool) error {
 	ctx, span := log.StartSpan(ctx)
 	defer span.End()
+
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
+
 	if s.networkStopped {
 		return nil
 	}
@@ -420,6 +439,7 @@ func (s *Sandbox) SetContainerEnvFile(ctx context.Context) error {
 	return nil
 }
 
+// This function assumes the state lock has been taken for this sandbox.
 func (s *Sandbox) createFileInInfraDir(ctx context.Context, filename string) error {
 	// If the sandbox is not yet created,
 	// this function is being called when
@@ -445,6 +465,9 @@ func (s *Sandbox) createFileInInfraDir(ctx context.Context, filename string) err
 }
 
 func (s *Sandbox) RestoreStopped() {
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
+
 	if s.fileExistsInInfraDir(sbStoppedFilename) {
 		s.stopped = true
 	}
@@ -467,11 +490,14 @@ func (s *Sandbox) fileExistsInInfraDir(filename string) bool {
 
 // Created returns the created status of sandbox
 func (s *Sandbox) Created() bool {
+	s.stateMutex.RLock()
+	defer s.stateMutex.RUnlock()
+
 	return s.created
 }
 
 func (s *Sandbox) State() types.PodSandboxState {
-	if s.Ready(false) {
+	if s.Ready() {
 		return types.PodSandboxState_SANDBOX_READY
 	}
 	return types.PodSandboxState_SANDBOX_NOTREADY
@@ -482,22 +508,9 @@ func (s *Sandbox) State() types.PodSandboxState {
 // `takeLock` should be set if we need to take the lock to get the infra container's state.
 // If there is no infra container, it is never considered ready.
 // If the infra container is spoofed, the pod is considered ready when it has been created, but not stopped.
-func (s *Sandbox) Ready(takeLock bool) bool {
-	podInfraContainer := s.InfraContainer()
-	if podInfraContainer == nil {
-		return false
-	}
-	if podInfraContainer.Spoofed() {
-		return s.created && !s.stopped
-	}
-	// Assume the sandbox is ready, unless it has an infra container that
-	// isn't running
-	var cState *oci.ContainerState
-	if takeLock {
-		cState = podInfraContainer.State()
-	} else {
-		cState = podInfraContainer.StateNoLock()
-	}
+func (s *Sandbox) Ready() bool {
+	s.stateMutex.RLock()
+	defer s.stateMutex.RUnlock()
 
-	return cState.Status == oci.ContainerStateRunning
+	return s.created && !s.stopped
 }
diff --git a/internal/lib/sandbox/sandbox_test.go b/internal/lib/sandbox/sandbox_test.go
@@ -114,6 +114,7 @@ var _ = t.Describe("Sandbox", func() {
 
 			// Then
 			Expect(testSandbox.Stopped()).To(BeTrue())
+			Expect(testSandbox.Ready()).To(BeFalse())
 		})
 	})
 
@@ -159,6 +160,7 @@ var _ = t.Describe("Sandbox", func() {
 
 			// Then
 			Expect(testSandbox.Created()).To(BeTrue())
+			Expect(testSandbox.Ready()).To(BeTrue())
 		})
 	})
 
diff --git a/server/container_portforward.go b/server/container_portforward.go
@@ -49,7 +49,7 @@ func (s StreamService) PortForward(ctx context.Context, podSandboxID string, por
 		return fmt.Errorf("could not find sandbox %s", podSandboxID)
 	}
 
-	if !sb.Ready(true) {
+	if !sb.Ready() {
 		return fmt.Errorf("sandbox %s is not running", podSandboxID)
 	}
 
diff --git a/server/sandbox_list_test.go b/server/sandbox_list_test.go
@@ -95,8 +95,9 @@ var _ = t.Describe("ListPodSandbox", func() {
 			// Given
 			mockDirs(testManifest)
 			createDummyState()
-			_, err := sut.LoadSandbox(context.Background(), sandboxID)
+			sb, err := sut.LoadSandbox(context.Background(), sandboxID)
 			Expect(err).ToNot(HaveOccurred())
+			sb.SetStopped(context.Background(), false)
 
 			// When
 			response, err := sut.ListPodSandbox(context.Background(),
diff --git a/server/sandbox_status.go b/server/sandbox_status.go
@@ -23,10 +23,7 @@ func (s *Server) PodSandboxStatus(ctx context.Context, req *types.PodSandboxStat
 		return nil, status.Errorf(codes.NotFound, "could not find pod %q: %v", req.PodSandboxId, err)
 	}
 
-	rStatus := types.PodSandboxState_SANDBOX_NOTREADY
-	if sb.Ready(true) {
-		rStatus = types.PodSandboxState_SANDBOX_READY
-	}
+	rStatus := sb.State()
 
 	var linux *types.LinuxPodSandboxStatus
 	if sb.NamespaceOptions() != nil {
diff --git a/server/server.go b/server/server.go
@@ -802,6 +802,12 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) {
 		}
 		c = sb.InfraContainer()
 		resource = "sandbox infra"
+		// We discovered the infra container stopped (potentially unexpectedly).
+		// Since sandboxes status is now being judged by the sb.stopped boolean,
+		// rather than the infra container's status, we have to manually set stopped here.
+		// It's likely we're doing double the work here, but that's better than missing it
+		// if the infra container crashed.
+		sb.SetStopped(ctx, true)
 	} else {
 		sb = s.GetSandbox(c.Sandbox())
 	}

Original file line number	Diff line number	Diff line change
`@@ -358,6 +358,11 @@ func (c ContainerServer) LoadSandbox(ctx context.Context, id string) (sb sandb`
`358`	`358`	`}`
`359`	`359`
`360`	`360`	`sb.SetCreated()`
	`361`	`+`
	`362`	`+ if scontainer.State().Status == oci.ContainerStateStopped {`
	`363`	`+ sb.SetStopped(ctx, true)`
	`364`	`+ }`
	`365`	`+`
`361`	`366`	`if err := label.ReserveLabel(processLabel); err != nil {`
`362`	`367`	`return sb, err`
`363`	`368`	`}`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ func (s StreamService) PortForward(ctx context.Context, podSandboxID string, por`
`49`	`49`	`return fmt.Errorf("could not find sandbox %s", podSandboxID)`
`50`	`50`	`}`
`51`	`51`
`52`		`- if !sb.Ready(true) {`
	`52`	`+ if !sb.Ready() {`
`53`	`53`	`return fmt.Errorf("sandbox %s is not running", podSandboxID)`
`54`	`54`	`}`
`55`	`55`
Original file line number	Diff line number	Diff line change
`@@ -802,6 +802,12 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) {`
`802`	`802`	`}`
`803`	`803`	`c = sb.InfraContainer()`
`804`	`804`	`resource = "sandbox infra"`
	`805`	`+ // We discovered the infra container stopped (potentially unexpectedly).`
	`806`	`+ // Since sandboxes status is now being judged by the sb.stopped boolean,`
	`807`	`+ // rather than the infra container's status, we have to manually set stopped here.`
	`808`	`+ // It's likely we're doing double the work here, but that's better than missing it`
	`809`	`+ // if the infra container crashed.`
	`810`	`+ sb.SetStopped(ctx, true)`
`805`	`811`	`} else {`
`806`	`812`	`sb = s.GetSandbox(c.Sandbox())`
`807`	`813`	`}`