oci: remove redundant ShouldBeStopped check for stopping containers

Fixes cri-o#8030
This commit removes the ShouldBeStopped function to address the
issue where stopping a container blocks all further stop attempts
for the same container. That function acquired the opLock
mutex to check the container's state, which could lead to contention and
potential deadlocks if other goroutines were waiting to acquire the same lock.

Additionally, the container's state is checked later in the StopContainer
function by calling the Living method, which checks if the container's
process is still running. Therefore, the ShouldBeStopped function is
redundant and can be removed.

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>

internal/oci/container.go

@@ -586,20 +586,6 @@ func (c *Container) verifyPid() (string, error) {
 	return state, nil
 }
 
-// ShouldBeStopped checks whether the container state is in a place
-// where attempting to stop it makes sense
-// a container is not stoppable if it's paused or stopped
-// if it's paused, that's an error, and is reported as such
-func (c *Container) ShouldBeStopped() error {
-	switch c.State().Status {
-	case ContainerStateStopped: // no-op
-		return ErrContainerStopped
-	case ContainerStatePaused:
-		return errors.New("cannot stop paused container")
-	}
-	return nil
-}
-
 // Spoofed returns whether this container is spoofed.
 // A container should be spoofed when it doesn't have to exist in the container runtime,
 // but does need to exist in the storage. The main use of this is when an infra container

internal/oci/container_test.go

@@ -427,42 +427,6 @@ var _ = t.Describe("Container", func() {
 			Expect(err).To(HaveOccurred())
 		})
 	})
-	t.Describe("ShouldBeStopped", func() {
-		It("should fail to stop if already stopped", func() {
-			// Given
-			state := &oci.ContainerState{}
-			state.Status = oci.ContainerStateStopped
-			sut.SetState(state)
-			// When
-			err := sut.ShouldBeStopped()
-
-			// Then
-			Expect(err).To(Equal(oci.ErrContainerStopped))
-		})
-		It("should fail to stop if paused", func() {
-			// Given
-			state := &oci.ContainerState{}
-			state.Status = oci.ContainerStatePaused
-			sut.SetState(state)
-			// When
-			err := sut.ShouldBeStopped()
-
-			// Then
-			Expect(err).NotTo(Equal(oci.ErrContainerStopped))
-			Expect(err).To(HaveOccurred())
-		})
-		It("should succeed to stop if started", func() {
-			// Given
-			state := &oci.ContainerState{}
-			state.Status = oci.ContainerStateRunning
-			sut.SetState(state)
-			// When
-			err := sut.ShouldBeStopped()
-
-			// Then
-			Expect(err).ToNot(HaveOccurred())
-		})
-	})
 	t.Describe("Living", func() {
 		It("should be false if pid uninitialized", func() {
 			// Given

internal/oci/runtime_oci.go

@@ -830,13 +830,6 @@ func (r *runtimeOCI) StopContainer(ctx context.Context, c *Container, timeout in
 		return nil
 	}
 
-	if err := c.ShouldBeStopped(); err != nil {
-		if errors.Is(err, ErrContainerStopped) {
-			err = nil
-		}
-		return err
-	}
-
 	// The initial container process either doesn't exist, or isn't ours.
 	if err := c.Living(); err != nil {
 		c.state.Finished = time.Now()

internal/oci/runtime_oci_test.go

@@ -73,20 +73,6 @@ var _ = t.Describe("Oci", func() {
 			cmdrunner.ResetPrependedCmd()
 		})
 
-		It("should fail to stop if container paused", func() {
-			state := &oci.ContainerState{}
-			state.Status = oci.ContainerStatePaused
-			sut.SetState(state)
-
-			Expect(sut.ShouldBeStopped()).NotTo(Succeed())
-		})
-		It("should fail to stop if container stopped", func() {
-			state := &oci.ContainerState{}
-			state.Status = oci.ContainerStateStopped
-			sut.SetState(state)
-
-			Expect(sut.ShouldBeStopped()).To(Equal(oci.ErrContainerStopped))
-		})
 		It("should return early if runtime command fails and process stopped", func() {
 			// Given
 			gomock.InOrder(

internal/oci/runtime_vm.go

@@ -608,7 +608,7 @@ func (r *runtimeVM) StopContainer(ctx context.Context, c *Container, timeout int
 	log.Debugf(ctx, "RuntimeVM.StopContainer() start")
 	defer log.Debugf(ctx, "RuntimeVM.StopContainer() end")
 
-	if err := c.ShouldBeStopped(); err != nil {
+	if err := shouldBeStopped(c); err != nil {
 		if errors.Is(err, ErrContainerStopped) {
 			err = nil
 		}
@@ -669,6 +669,21 @@ func (r *runtimeVM) StopContainer(ctx context.Context, c *Container, timeout int
 	return nil
 }
 
+// shouldBeStopped checks whether the container's state permits
+// stopping. It determines if stopping the container makes sense
+// based on its current state. A container cannot be stopped if
+// it is already stopped or paused. If the container is paused,
+// the function attempts to unpause it and update its status.
+func shouldBeStopped(c *Container) error {
+	switch c.State().Status {
+	case ContainerStateStopped: // no-op
+		return ErrContainerStopped
+	case ContainerStatePaused:
+		return errors.New("cannot stop paused container")
+	}
+	return nil
+}
+
 func (r *runtimeVM) waitCtrTerminate(sig syscall.Signal, stopCh chan error, timeout time.Duration) error {
 	select {
 	case err := <-stopCh: