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

haircommander · haircommander · commit bfa7401ea21b · 2025-05-08T17:05:42.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/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.
@@ -329,6 +332,8 @@ 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
 	}
@@ -349,11 +354,15 @@ func (s *Sandbox) Stopped() bool {
 
 // 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.Lock()
+	defer s.stateMutex.Unlock()
 	return s.networkStopped
 }
 
@@ -368,6 +377,8 @@ 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
 	}
@@ -404,6 +415,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
@@ -459,11 +471,13 @@ func (s *Sandbox) fileExistsInInfraDir(filename string) bool {
 
 // Created returns the created status of sandbox.
 func (s *Sandbox) Created() bool {
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
 	return s.created
 }
 
 func (s *Sandbox) State() types.PodSandboxState {
-	if s.Ready(false) {
+	if s.Ready() {
 		return types.PodSandboxState_SANDBOX_READY
 	}
 
@@ -475,23 +489,8 @@ 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()
-	}
-
-	return cState.Status == oci.ContainerStateRunning
+func (s *Sandbox) Ready() bool {
+	s.stateMutex.Lock()
+	defer s.stateMutex.Unlock()
+	return s.created && !s.stopped
 }
diff --git a/internal/lib/sandbox/sandbox_test.go b/internal/lib/sandbox/sandbox_test.go
@@ -139,6 +139,7 @@ var _ = t.Describe("Sandbox", func() {
 
 			// Then
 			Expect(testSandbox.Stopped()).To(BeTrue())
+			Expect(testSandbox.Ready()).To(BeFalse())
 		})
 	})
 
@@ -184,6 +185,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
@@ -51,7 +51,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_status.go b/server/sandbox_status.go
@@ -25,10 +25,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 {

Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ func (s *StreamService) PortForward(ctx context.Context, podSandboxID string, po`
`51`	`51`	`return fmt.Errorf("could not find sandbox %s", podSandboxID)`
`52`	`52`	`}`
`53`	`53`
`54`		`- if !sb.Ready(true) {`
	`54`	`+ if !sb.Ready() {`
`55`	`55`	`return fmt.Errorf("sandbox %s is not running", podSandboxID)`
`56`	`56`	`}`
`57`	`57`