cri-o · openshift-merge-bot · Jan 28, 2025 · Dec 19, 2024
@@ -423,6 +423,7 @@ func (c *Container) State() *ContainerState {
 }
 
 // StateNoLock returns the state of a container without using a lock.
+// It has been known to cause segfaults in the past so it really should be used sparingly.
 func (c *Container) StateNoLock() *ContainerState {
 	return c.state
 }
@@ -504,6 +505,8 @@ func (c *Container) exitFilePath() string {
 
 // Living checks if a container's init PID exists and it's running, without calling
 // a given runtime directly to check the state, which is expensive.
+// This can't be used for runtimeVM because it uses the VM's pid as a container pid,
+// and the VM doesn't necessarily stop when the container stops.
 func (c *Container) Living() error {
 	_, _, err := c.pid()
 	if err != nil {

@@ -80,6 +80,7 @@
 	ReopenContainerLog(context.Context, *Container) error
 	CheckpointContainer(context.Context, *Container, *rspec.Spec, bool) error
 	RestoreContainer(context.Context, *Container, string, string) error
+	IsContainerAlive(*Container) bool
 }
 
 // New creates a new Runtime with options provided.
@@ -504,3 +505,12 @@
 
 	return impl.RestoreContainer(ctx, c, cgroupParent, mountLabel)
 }
+
+func (r *Runtime) IsContainerAlive(c *Container) (bool, error) {
+	impl, err := r.RuntimeImpl(c)
+	if err != nil {
+		return false, err
+	}
+
+	return impl.IsContainerAlive(c), nil
+}
@@ -1576,3 +1576,7 @@
 	}
 	return nil
 }
+
+func (r *runtimeOCI) IsContainerAlive(c *Container) bool {
+	return c.Living() == nil
+}
@@ -294,3 +294,7 @@
 		ID: c.ID(),
 	})
 }
+
+func (r *runtimePod) IsContainerAlive(c *Container) bool {
+	return c.Living() == nil
+}
@@ -1275,3 +1275,7 @@
 	option := base64.StdEncoding.EncodeToString(validKataVirtualVolumeJSON)
 	return option, nil
 }
+
+func (r *runtimeVM) IsContainerAlive(c *Container) bool {
+	return r.kill(c.ID(), "", 0, false) == nil
+}
@@ -8,7 +8,6 @@
 	types "k8s.io/cri-api/pkg/apis/runtime/v1"
 
 	"github.com/cri-o/cri-o/internal/log"
-	"github.com/cri-o/cri-o/internal/oci"
 )
 
 // ReopenContainerLog reopens the containers log file.
@@ -20,13 +19,12 @@
 		return nil, fmt.Errorf("could not find container %s: %w", req.ContainerId, err)
 	}
 
-	if err := s.ContainerServer.Runtime().UpdateContainerStatus(ctx, c); err != nil {
+	isRunning, err := s.ContainerServer.Runtime().IsContainerAlive(c)
+	if err != nil {
 		return nil, err
 	}
-
-	cState := c.State()
-	if !(cState.Status == oci.ContainerStateRunning || cState.Status == oci.ContainerStateCreated) {
-		return nil, errors.New("container is not created or running")
+	if !isRunning {
+		return nil, errors.New("container is not running")
 	}
 
 	if err := s.ContainerServer.Runtime().ReopenContainerLog(ctx, c); err != nil {

@@ -0,0 +1,53 @@
+#!/usr/bin/env bats
+# vim:set ft=bash :
+
+load helpers
+
+function setup() {
+	setup_test
+}
+
+function teardown() {
+	cleanup_test
+}
+
+@test "ReopenContainerLog should succeed when the container is running" {
+	start_crio
+
+	jq '.metadata.name = "sleep"
+		| .command = ["/bin/sh", "-c", "sleep 600"]' \
+		"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
+
+	ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
+	crictl logs -r "$ctr_id"
+}
+
+@test "ReopenContainerLog should fail when the container is stopped" {
+	start_crio
+
+	jq '.metadata.name = "sleep"
+		| .command = ["/bin/sh", "-c", "sleep 600"]' \
+		"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
+
+	ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
+	crictl stop "$ctr_id"
+	run ! crictl logs -r "$ctr_id"
+}
+
+@test "ReopenContainerLog should not be blocked during deletion" {
+	start_crio
+
+	jq '.metadata.name = "trap"
+		| .command = ["/bin/sh", "-c", "trap \"sleep 600\" TERM && sleep 600"]' \
+		"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
+
+	ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
+	# Especially when using kata, it sometimes takes a few seconds to actually run container
+	sleep 5
+
+	crictl stop -t 10 "$ctr_id" &
+	wait_for_log "Request: &StopContainerRequest"
+	crictl logs -r "$ctr_id"
+	output=$(crictl inspect "$ctr_id" | jq -r ".status.state")
+	[[ "$output" == "CONTAINER_RUNNING" ]]
+}