Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

host-ctr: add label flag to host-ctr pull-image. #3757

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

gthao313
Copy link
Member

@gthao313 gthao313 commented Feb 5, 2024

Issue number:
#3745
Closes #

Description of changes:
Sandbox container image on k8s 1.29 isn't pinned and causea the Sandbox container image would be GC. we add label flag to host-ctr pull-image and pass "io.cri-containerd.pinned=pinned" label to all 1.29 varaints. Then the Sandbox contianer image will be pinned and avoid GC issue.

Testing done:

  • Launch the 1.29 nodes to 1.29 cluster and check if the pause image is pinned
602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause             3.1-eksbuild.1       106a8e54d5eb3       299kB
[root@admin]# /crictl --runtime-endpoint unix:///.bottlerocket/rootfs/run/containerd/containerd.sock inspecti 106a8e54d5eb3 | grep pinned
    "pinned": true
  • conformance test for 1.29 nodes
    AWS
[cargo-make][1] INFO - Running Task: testsys
 NAME                                           TYPE                STATE                         PASSED            FAILED            SKIPPED   BUILD ID            LAST UPDATE
 x86-64-aws-k8s-129-conformance-test            Test                passed                           392                 0               7019                       2024-02-05T03:18:41Z
 x86-64-aws-k8s-129-instances                   Resource            completed                                                                                       2024-02-05T01:25:21Z

VMWARE - a Known kubernetes flake on conformance test.

Plugin: e2e
Status: failed
Total: 7411
Passed: 391
Failed: 1
Skipped: 7019

Failed tests:
 [sig-network] Services should serve endpoints on same port and different protocols [Conformance]

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@yeazelm yeazelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You included a binary in the git commit that we should ensure we strip out.

I'd also like to sort out the lint failures to see if its the code or if its something else? I ran the lint manually and it did find something worth fixing while we are updating the code:

cmd/host-ctr/main.go:43:2: SA1019: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator. (staticcheck)
        rand.Seed(time.Now().UnixNano())

sources/host-ctr/cmd/host-ctr/main.go Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
packages/kubernetes-1.29/prestart-pull-pause-ctr.conf Outdated Show resolved Hide resolved
@@ -161,9 +162,15 @@ func App() *cli.App {
Destination: &useCachedImage,
Value: false,
},
&cli.StringFlag{
Name: "labels",
Usage: "add labels to the pulled image. Each label should be in the format of `key=value` and separated by a comma",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What characters are allowed in a label? If either comma or whitespace are allowed then this might not be the right interface.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
Comment on lines 1211 to 1233
labelKeyValue := strings.Split(label, "=")
labelsMap[labelKeyValue[0]] = labelKeyValue[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are labels required to have an = separate or can raw keys be used?

Linking to the label "spec" for containerd would help provide the right context for a thorough review.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containerd expects a map[string]string as labels input. Otherwise, all it cares about is that the key and value are under 4096 bytes combined. https://github.com/containerd/containerd/blob/f5e7fe0cb6286333a597a407029fe7c760626669/pkg/labels/validate.go#L32-L41

I'll add this validation and meanwhile I'll validate key=value format.

@gthao313
Copy link
Member Author

gthao313 commented Feb 5, 2024

^ addressed comments

  1. use StringSliceFlag which can allow you to pass multiple values for a single flag, and joins all inputs with ,. Therefore, we don't need validate if the input is seperated by ,
    Xxample:
... \
--label "io.cri-containerd.pinned=pinned \
--label "io.cri-containerd.example=example \
  1. add validation and unit-test for converLables functions. label rules
  • The single label must be None empty string
  • The single lable must be key=value format
  • The key and value are under 4096 bytes combined.
  1. set pinned label for all k8s versions

@ginglis13
Copy link
Contributor

I'd also like to sort out the lint failures to see if its the code or if its something else?

probably need a go mod tidy and/or bump the version in the workflow for golangci-lint to match host-ctr's (1.21):

Copy link
Contributor

@ginglis13 ginglis13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on green CI

@gthao313
Copy link
Member Author

gthao313 commented Feb 5, 2024

golangci-lint was broken on go 1.19 version. I'm going to bump bump the version in the workflow for golangci-lint to match 1.21.

@gthao313
Copy link
Member Author

gthao313 commented Feb 6, 2024

rebase to use go 1.21 version on golangci-lint

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
@gthao313
Copy link
Member Author

gthao313 commented Feb 6, 2024

^ addressed comments.

  1. remove Validate function and add label max size check to convertLabel.
  2. move 'covertLabel' before pullImageOnly. convert and validate the input first.
  3. add more unit tests.
  4. Use slice

Test

[root@admin]# /crictl --runtime-endpoint unix:///.bottlerocket/rootfs/run/containerd/containerd.sock inspecti 106a8e54d5eb3 | grep pinned
    "pinned": true
[root@admin]# sudo sheltie
bash-5.1# ctr -a /run/containerd/containerd.sock -n k8s.io images list | grep pause
602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause:3.1-eksbuild.1                                                                                application/vnd.docker.distribution.manifest.list.v2+json sha256:1cb4ab85a3480446f9243178395e6bee7350f0d71296daeb6a9fdd221e23aea6 292.4 KiB linux/amd64,linux/arm64 io.cri-containerd.image=managed,io.cri-containerd.pinned=pinned
ecr.aws/ar

@gthao313
Copy link
Member Author

gthao313 commented Feb 6, 2024

^addressed comments

tolerate input without an = and just assume it's a key where the value is the empty string.
If the provided label contains =, split them into key an value; otherwise, assume assume it's a key where the value is the empty string.

Copy link
Contributor

@yeazelm yeazelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me 🚢

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved
Sandbox container image on k8s 1.29 isn't pinned and causea the Sandbox container
image would be GC. we add label flag to host-ctr pull-image and pass
"io.cri-containerd.pinned=pinned" label to all 1.29 varaints. Then the
Sandbox contianer image will be pinned and avoid GC issue.
@gthao313 gthao313 merged commit acfea14 into bottlerocket-os:develop Feb 6, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants