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

handle pod-infra-container-image on older versions #1596

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

ndbaker1
Copy link
Member

@ndbaker1 ndbaker1 commented Jan 29, 2024

Issue #, if available:

Description of changes:

some k8s versions will require the --pod-infra-container-image flag to avoid the sandbox image getting GC'd. Kubelet options state this will be removed in 1.27.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

e2e test cases

See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.

@ndbaker1 ndbaker1 marked this pull request as ready for review January 29, 2024 20:54
@cartermckinnon
Copy link
Member

cartermckinnon commented Jan 31, 2024

@ndbaker1 do you intend to add the if statement back here to exclude the flag on 1.29? If it's a no-op, I think it's confusing to leave it there...

@ndbaker1 ndbaker1 force-pushed the gc-pod-infra-image branch 7 times, most recently from 2f8bcaa to 9aaa063 Compare February 1, 2024 00:47
@ndbaker1
Copy link
Member Author

ndbaker1 commented Feb 1, 2024

@ndbaker1 do you intend to add the if statement back here to exclude the flag on 1.29? If it's a no-op, I think it's confusing to leave it there...

@cartermckinnon sorry bout that, updated the if + e2e test (since this reads IMDS) + comments with more context

// runtime is moved to containerd 2.0
func (ksc *kubeletConfig) withPodInfraContainerImage(cfg *api.NodeConfig, kubeletVersion string, flags map[string]string) error {
// the flag is a noop on 1.29+, since the behavior was changed to use the
// CRI image pinning behavior and no longer considers the flag value.
Copy link
Member

@Issacwww Issacwww Feb 1, 2024

Choose a reason for hiding this comment

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

IIUC, we also need to add #1605 and #1601 into al2023 to help with 1.29 right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, it looks like we will need the crictl for sure in some equivalent fashion. Am i missing any go libraries to pull the image through the CRI versus crictl?

Copy link
Member Author

@ndbaker1 ndbaker1 Feb 1, 2024

Choose a reason for hiding this comment

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

i could use the same upstream resources to pull this to remove the cri-tools install,
it would look something like:

import (
	v1 "k8s.io/cri-api/pkg/apis/runtime/v1"
	"k8s.io/kubernetes/pkg/kubelet/cri/remote"
)

imageManager, err := remote.NewRemoteImageService("unix:///run/containerd/containerd.sock", 2*time.Second, nil)
imageSpec := v1.ImageSpec{Image: sandboxImage}
authConfig := v1.AuthConfig{Username: "AWS", Password: ecrUserToken}
imageRef, err := imageManager.PullImage(context.TODO(), &imageSpec, &authConfig, nil)

Copy link
Member

Choose a reason for hiding this comment

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

Chatted offline, I think it's a good idea for nodeadm to bake in this functionality but we need to find the right CRI client library to use (pulling in kubelet internals should be a last resort)

@ndbaker1 ndbaker1 merged commit 84e38b7 into awslabs:al2023 Feb 1, 2024
9 checks passed
@ndbaker1 ndbaker1 deleted the gc-pod-infra-image branch February 1, 2024 23:54
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.

3 participants