Skip to content

Commit 3dab91d

Browse files
bitokubitoku
committed
Avoid using UpdateContainerStatus for ReopenContainerLog and add logs tests
Signed-off-by: Ayato Tokubi <[email protected]> (cherry picked from commit 0529655) Signed-off-by: Ayato Tokubi <[email protected]>
1 parent bbf9018 commit 3dab91d

File tree

8 files changed

+100
-9
lines changed

8 files changed

+100
-9
lines changed

‎internal/oci/container.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,7 @@ func (c *Container) State() *ContainerState {
421421
}
422422

423423
// StateNoLock returns the state of a container without using a lock.
424+
// It has been known to cause segfaults in the past so it really should be used sparingly.
424425
func (c *Container) StateNoLock() *ContainerState {
425426
return c.state
426427
}
@@ -492,6 +493,8 @@ func (c *Container) exitFilePath() string {
492493

493494
// Living checks if a container's init PID exists and it's running, without calling
494495
// a given runtime directly to check the state, which is expensive.
496+
// This can't be used for runtimeVM because it uses the VM's pid as a container pid,
497+
// and the VM doesn't necessarily stop when the container stops.
495498
func (c *Container) Living() error {
496499
_, _, err := c.pid()
497500
if err != nil {

‎internal/oci/oci.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ type RuntimeImpl interface {
7979
ReopenContainerLog(context.Context, *Container) error
8080
CheckpointContainer(context.Context, *Container, *rspec.Spec, bool) error
8181
RestoreContainer(context.Context, *Container, string, string) error
82+
IsContainerAlive(*Container) bool
8283
}
8384

8485
// New creates a new Runtime with options provided
@@ -493,3 +494,12 @@ func (r *Runtime) RestoreContainer(ctx context.Context, c *Container, cgroupPare
493494

494495
return impl.RestoreContainer(ctx, c, cgroupParent, mountLabel)
495496
}
497+
498+
func (r *Runtime) IsContainerAlive(c *Container) (bool, error) {
499+
impl, err := r.RuntimeImpl(c)
500+
if err != nil {
501+
return false, err
502+
}
503+
504+
return impl.IsContainerAlive(c), nil
505+
}

‎internal/oci/runtime_oci.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1569,3 +1569,7 @@ func (r *runtimeOCI) checkpointRestoreSupported(runtimePath string) error {
15691569
}
15701570
return nil
15711571
}
1572+
1573+
func (r *runtimeOCI) IsContainerAlive(c *Container) bool {
1574+
return c.Living() == nil
1575+
}

‎internal/oci/runtime_pod.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,3 +296,7 @@ func (r *runtimePod) ReopenContainerLog(ctx context.Context, c *Container) error
296296
ID: c.ID(),
297297
})
298298
}
299+
300+
func (r *runtimePod) IsContainerAlive(c *Container) bool {
301+
return c.Living() == nil
302+
}

‎internal/oci/runtime_vm.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,3 +1274,7 @@ func EncodeKataVirtualVolumeToBase64(ctx context.Context, volume *katavolume.Kat
12741274
option := base64.StdEncoding.EncodeToString(validKataVirtualVolumeJSON)
12751275
return option, nil
12761276
}
1277+
1278+
func (r *runtimeVM) IsContainerAlive(c *Container) bool {
1279+
return r.kill(c.ID(), "", 0, false) == nil
1280+
}

‎server/container_reopen_log.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
package server
22

33
import (
4+
"context"
45
"errors"
56
"fmt"
67

7-
"github.com/cri-o/cri-o/internal/log"
8-
"github.com/cri-o/cri-o/internal/oci"
9-
"golang.org/x/net/context"
108
types "k8s.io/cri-api/pkg/apis/runtime/v1"
9+
10+
"github.com/cri-o/cri-o/internal/log"
1111
)
1212

13-
// ReopenContainerLog reopens the containers log file
13+
// ReopenContainerLog reopens the containers log file.
1414
func (s *Server) ReopenContainerLog(ctx context.Context, req *types.ReopenContainerLogRequest) (*types.ReopenContainerLogResponse, error) {
1515
ctx, span := log.StartSpan(ctx)
1616
defer span.End()
@@ -19,13 +19,12 @@ func (s *Server) ReopenContainerLog(ctx context.Context, req *types.ReopenContai
1919
return nil, fmt.Errorf("could not find container %s: %w", req.ContainerId, err)
2020
}
2121

22-
if err := s.ContainerServer.Runtime().UpdateContainerStatus(ctx, c); err != nil {
22+
isRunning, err := s.ContainerServer.Runtime().IsContainerAlive(c)
23+
if err != nil {
2324
return nil, err
2425
}
25-
26-
cState := c.State()
27-
if !(cState.Status == oci.ContainerStateRunning || cState.Status == oci.ContainerStateCreated) {
28-
return nil, errors.New("container is not created or running")
26+
if !isRunning {
27+
return nil, errors.New("container is not running")
2928
}
3029

3130
if err := s.ContainerServer.Runtime().ReopenContainerLog(ctx, c); err != nil {

‎test/logs.bats‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
#!/usr/bin/env bats
2+
# vim:set ft=bash :
3+
4+
load helpers
5+
6+
function setup() {
7+
setup_test
8+
}
9+
10+
function teardown() {
11+
cleanup_test
12+
}
13+
14+
@test "ReopenContainerLog should succeed when the container is running" {
15+
start_crio
16+
17+
jq '.metadata.name = "sleep"
18+
| .command = ["/bin/sh", "-c", "sleep 600"]' \
19+
"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
20+
21+
ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
22+
crictl logs -r "$ctr_id"
23+
}
24+
25+
@test "ReopenContainerLog should fail when the container is stopped" {
26+
start_crio
27+
28+
jq '.metadata.name = "sleep"
29+
| .command = ["/bin/sh", "-c", "sleep 600"]' \
30+
"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
31+
32+
ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
33+
crictl stop "$ctr_id"
34+
run ! crictl logs -r "$ctr_id"
35+
}
36+
37+
@test "ReopenContainerLog should not be blocked during deletion" {
38+
start_crio
39+
40+
jq '.metadata.name = "trap"
41+
| .command = ["/bin/sh", "-c", "trap \"sleep 600\" TERM && sleep 600"]' \
42+
"$TESTDATA"/container_config.json > "$TESTDIR"/trap.json
43+
44+
ctr_id=$(crictl run "$TESTDIR"/trap.json "$TESTDATA"/sandbox_config.json)
45+
# Especially when using kata, it sometimes takes a few seconds to actually run container
46+
sleep 5
47+
48+
crictl stop -t 10 "$ctr_id" &
49+
wait_for_log "Request: &StopContainerRequest"
50+
crictl logs -r "$ctr_id"
51+
output=$(crictl inspect "$ctr_id" | jq -r ".status.state")
52+
[[ "$output" == "CONTAINER_RUNNING" ]]
53+
}

‎test/mocks/oci/oci.go‎

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)