sandbox: use created/stopped instead of infra container for readiness

haircommander · haircommander · commit cd5e6b2bbd81 · 2025-06-05T16:38:25.000-04:00
As we hit cases of deadlock when a sandbox is stuck stopping

Signed-off-by: Peter Hunt &lt;pehunt@redhat.com&gt;
diff --git a/internal/lib/container_server.go b/internal/lib/container_server.go
@@ -375,6 +375,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
@@ -59,15 +59,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.
@@ -326,6 +329,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
 	}
@@ -340,16 +347,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
 }
 
@@ -363,6 +378,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
 	}
@@ -394,6 +413,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
@@ -419,6 +439,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
 	}
@@ -441,11 +464,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
@@ -456,22 +482,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
@@ -136,6 +136,7 @@ var _ = t.Describe("Sandbox", func() {
 
 			// Then
 			Expect(testSandbox.Stopped()).To(BeTrue())
+			Expect(testSandbox.Ready()).To(BeFalse())
 		})
 	})
 
@@ -181,6 +182,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, po
 		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
@@ -792,6 +792,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
`@@ -375,6 +375,11 @@ func (c ContainerServer) LoadSandbox(ctx context.Context, id string) (sb sandb`
`375`	`375`	`}`
`376`	`376`
`377`	`377`	`sb.SetCreated()`
	`378`	`+`
	`379`	`+ if scontainer.State().Status == oci.ContainerStateStopped {`
	`380`	`+ sb.SetStopped(ctx, true)`
	`381`	`+ }`
	`382`	`+`
`378`	`383`	`if err := label.ReserveLabel(processLabel); err != nil {`
`379`	`384`	`return sb, err`
`380`	`385`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ func (s *StreamService) PortForward(ctx context.Context, podSandboxID string, po`
`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
`@@ -792,6 +792,12 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) {`
`792`	`792`	`}`
`793`	`793`	`c = sb.InfraContainer()`
`794`	`794`	`resource = "sandbox infra"`
	`795`	`+ // We discovered the infra container stopped (potentially unexpectedly).`
	`796`	`+ // Since sandboxes status is now being judged by the sb.stopped boolean,`
	`797`	`+ // rather than the infra container's status, we have to manually set stopped here.`
	`798`	`+ // It's likely we're doing double the work here, but that's better than missing it`
	`799`	`+ // if the infra container crashed.`
	`800`	`+ sb.SetStopped(ctx, true)`
`795`	`801`	`} else {`
`796`	`802`	`sb = s.GetSandbox(c.Sandbox())`
`797`	`803`	`}`