Merge pull request #9291 from openshift-cherrypick-robot/cherry-pick-9224-to-release-1.31

openshift-merge-bot[bot] · web-flow · commit abfadc8a71e5 · 2025-06-30T18:39:35.000Z
OCPBUGS-58201: [release-1.31] sandbox: use created/stopped instead of infra container for readiness
diff --git a/internal/lib/container_server.go b/internal/lib/container_server.go
@@ -353,6 +353,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
@@ -57,15 +57,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.
@@ -353,6 +356,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
 	}
@@ -367,16 +374,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
 }
 
@@ -390,6 +405,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
 	}
@@ -421,6 +440,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
@@ -446,6 +466,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
 	}
@@ -468,11 +491,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
@@ -483,22 +509,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
@@ -115,6 +115,7 @@ var _ = t.Describe("Sandbox", func() {
 
 			// Then
 			Expect(testSandbox.Stopped()).To(BeTrue())
+			Expect(testSandbox.Ready()).To(BeFalse())
 		})
 	})
 
@@ -160,6 +161,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
@@ -50,7 +50,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
@@ -96,8 +96,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
@@ -24,10 +24,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
@@ -808,6 +808,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
`@@ -353,6 +353,11 @@ func (c ContainerServer) LoadSandbox(ctx context.Context, id string) (sb sandb`
`353`	`353`	`}`
`354`	`354`
`355`	`355`	`sb.SetCreated()`
	`356`	`+`
	`357`	`+ if scontainer.State().Status == oci.ContainerStateStopped {`
	`358`	`+ sb.SetStopped(ctx, true)`
	`359`	`+ }`
	`360`	`+`
`356`	`361`	`if err := label.ReserveLabel(processLabel); err != nil {`
`357`	`362`	`return sb, err`
`358`	`363`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ func (s StreamService) PortForward(ctx context.Context, podSandboxID string, por`
`50`	`50`	`return fmt.Errorf("could not find sandbox %s", podSandboxID)`
`51`	`51`	`}`
`52`	`52`
`53`		`- if !sb.Ready(true) {`
	`53`	`+ if !sb.Ready() {`
`54`	`54`	`return fmt.Errorf("sandbox %s is not running", podSandboxID)`
`55`	`55`	`}`
`56`	`56`
Original file line number	Diff line number	Diff line change
`@@ -808,6 +808,12 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) {`
`808`	`808`	`}`
`809`	`809`	`c = sb.InfraContainer()`
`810`	`810`	`resource = "sandbox infra"`
	`811`	`+ // We discovered the infra container stopped (potentially unexpectedly).`
	`812`	`+ // Since sandboxes status is now being judged by the sb.stopped boolean,`
	`813`	`+ // rather than the infra container's status, we have to manually set stopped here.`
	`814`	`+ // It's likely we're doing double the work here, but that's better than missing it`
	`815`	`+ // if the infra container crashed.`
	`816`	`+ sb.SetStopped(ctx, true)`
`811`	`817`	`} else {`
`812`	`818`	`sb = s.GetSandbox(c.Sandbox())`
`813`	`819`	`}`