Skip to content

Commit

Permalink
Make ImageVolume garbage collection work
Browse files Browse the repository at this point in the history
We need to return the image ID for the localhost mount, otherwise the
kubelet garbage collection is not able to handle them.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
  • Loading branch information
saschagrunert committed Jul 23, 2024
1 parent 56e6b84 commit 402ffbe
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 5 deletions.
1 change: 1 addition & 0 deletions internal/oci/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type ContainerVolume struct {
RecursiveReadOnly bool `json:"recursive_read_only"`
Propagation types.MountPropagation `json:"propagation"`
SelinuxRelabel bool `json:"selinux_relabel"`
Image *types.ImageSpec `json:"image,omitempty"` // A possible image for OCI volume mounts
}

// ContainerState represents the status of a container.
Expand Down
13 changes: 8 additions & 5 deletions server/container_create_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -1075,10 +1075,12 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container,
return nil, nil, errors.New("mount.ContainerPath is empty")
}
if m.Image != nil && m.Image.Image != "" {
m.HostPath, err = s.mountImage(ctx, m.Image.Image, mountLabel)
mountPoint, imageID, err := s.mountImage(ctx, m.Image.Image, mountLabel)

Check warning on line 1078 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1078

Added line #L1078 was not covered by tests
if err != nil {
return nil, nil, fmt.Errorf("mount image: %w", err)
}
m.Image.Image = imageID // Set the ID for container status later on
m.HostPath = mountPoint // Adjust the host path to use the mount point

Check warning on line 1083 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1082-L1083

Added lines #L1082 - L1083 were not covered by tests
}
if m.HostPath == "" {
return nil, nil, errors.New("mount.HostPath is empty")
Expand Down Expand Up @@ -1199,6 +1201,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container,
RecursiveReadOnly: m.RecursiveReadOnly,
Propagation: m.Propagation,
SelinuxRelabel: m.SelinuxRelabel,
Image: m.Image,
})

uidMappings := getOCIMappings(m.UidMappings)
Expand Down Expand Up @@ -1235,11 +1238,11 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container,
}

// mountImage mounts the provided imageRef using the mountLabel and returns the hostPath on success.
func (s *Server) mountImage(ctx context.Context, imageRef, mountLabel string) (hostPath string, err error) {
func (s *Server) mountImage(ctx context.Context, imageRef, mountLabel string) (hostPath, imageID string, err error) {

Check warning on line 1241 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1241

Added line #L1241 was not covered by tests
log.Debugf(ctx, "Image ref to mount: %s", imageRef)
status, err := s.storageImageStatus(ctx, types.ImageSpec{Image: imageRef})
if err != nil {
return "", fmt.Errorf("get storage image status: %w", err)
return "", "", fmt.Errorf("get storage image status: %w", err)

Check warning on line 1245 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1245

Added line #L1245 was not covered by tests
}

id := status.ID.IDStringForOutOfProcessConsumptionOnly()
Expand All @@ -1248,11 +1251,11 @@ func (s *Server) mountImage(ctx context.Context, imageRef, mountLabel string) (h
options := []string{"ro", "noexec", "nosuid", "nodev"}
mountPoint, err := s.Store().MountImage(id, options, mountLabel)
if err != nil {
return "", fmt.Errorf("mount storage: %w", err)
return "", "", fmt.Errorf("mount storage: %w", err)

Check warning on line 1254 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1254

Added line #L1254 was not covered by tests
}

log.Infof(ctx, "Image mounted to: %s", mountPoint)
return mountPoint, nil
return mountPoint, id, nil

Check warning on line 1258 in server/container_create_linux.go

View check run for this annotation

Codecov / codecov/patch

server/container_create_linux.go#L1258

Added line #L1258 was not covered by tests
}

func getOCIMappings(m []*types.IDMapping) []rspec.LinuxIDMapping {
Expand Down
1 change: 1 addition & 0 deletions server/container_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ func (s *Server) ContainerStatus(ctx context.Context, req *types.ContainerStatus
RecursiveReadOnly: cv.RecursiveReadOnly,
Propagation: cv.Propagation,
SelinuxRelabel: cv.SelinuxRelabel,
Image: cv.Image,
})
}
resp.Status.Mounts = mounts
Expand Down
5 changes: 5 additions & 0 deletions test/oci_volumes.bats
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ function teardown() {
# Image removal should be blocked
run ! crictl rmi $IMAGE

# The kubelet garbage collection expects the image ID set in the container status mount
IMAGE_ID=$(crictl inspecti quay.io/crio/artifact:v1 | jq -e .status.id)
IMAGE_MOUNT_ID=$(crictl inspect "$CTR_ID" | jq -e '.status.mounts[0].image.image')
[[ "$IMAGE_ID" == "$IMAGE_MOUNT_ID" ]]

# Remove the container
crictl rm -f "$CTR_ID"

Expand Down

0 comments on commit 402ffbe

Please sign in to comment.