Add e2e test for Regular Container image change #126794

dshebib · 2024-08-19T20:23:10Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

See #126525 for context.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Demonstrates current behaviour before this use case is implemented (if it is) #122926

k8s-ci-robot · 2024-08-19T20:23:20Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-08-19T20:23:21Z

Hi @dshebib. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

BenTheElder · 2024-08-19T21:19:34Z

test/e2e_node/container_lifecycle_test.go

+				Containers: []v1.Container{
+					{
+						Name:  regular1,
+						Image: busyboxImage,


nit: prefer agnhost to other images:
https://github.com/kubernetes/kubernetes/blob/master/test/images/agnhost/README.md

@aojea @pohly @dims we could probably do more to highlight this?

Do what? Communicating this to devs (like a mail to dev) or something technical (like a linter?)?

speaking as a new contributor, if most of the tests use agnhost then I'd continue to use it for most tests without thinking twice. I'm working on a PR for this test file to reuse the Context and will also update the images to agnhost.

The problem is that we need to inform new and existing contributors about such best practices. That's hard, in particular when existing code and tests don't follow them for historic reasons 😢

The problem is that we need to inform new and existing contributors about such best practices. That's hard, in particular when existing code and tests don't follow them for historic reasons 😢

Right. I think just an email would be a good start. We have #122751 but that doesn't handle additional usage of other currently permitted images from historic reasons.

aojea · 2024-08-26T09:17:16Z

/ok-to-test

dshebib · 2024-08-27T21:24:04Z

/retest

toVersus · 2024-08-28T00:54:25Z

test/e2e_node/container_lifecycle_test.go

+			},
+			Spec: v1.PodSpec{
+				RestartPolicy: v1.RestartPolicyAlways,
+				Containers: []v1.Container{


Could you update the PodSpec to have two or more regular containers and then check to see if only the one with the updated image restarts?

updated test

toVersus · 2024-08-28T00:55:58Z

test/e2e_node/container_lifecycle_test.go

+
+			ginkgo.By("updating the image", func() {
+				client.Update(ctx, podSpec.Name, func(pod *v1.Pod) {
+					pod.Spec.Containers[0].Image = "fakeimage"


We also want to add a test case for when the container image is updated to one that starts correctly.

@BenTheElder @pohly @aojea
What would be the preferred approach from the SIG-Testing perspective?

Update to a different image tag with agnhost

Update to busybox or another image

We have a curated list of images which are okay to use in tests: https://github.com/kubernetes/kubernetes/blob/master/test/images/.permitted-images

We don't have different tags for the same image. So for this test, I think switching to registry.k8s.io/pause is fine. There's also registry.k8s.io/e2e-test-images/busybox, but it might get (be?) superseded by agnhost and I wouldn't add more usages of it.

Instead of fakeimage, use invalid.registry.k8s.io/invalid/alpine?

Pause is probably suitable for this, the main difference in recent versions is windows support.

However we want to keep it minimal. So maybe the one pause image and the one agnhost image, instead of multiple tags for those images. Which would also mean we are at the latest windows support for both.

Sorry I lost this thread in all the other github notifications.

test/e2e_node/container_lifecycle_test.go

hshiina · 2024-09-30T13:14:22Z

test/e2e_node/container_lifecycle_test.go

+
+		preparePod(originalPodSpec)
+
+		ginkgo.It("should not restart when the image is updated", func(ctx context.Context) {


This looks inconsistent with the actual code:

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager.go

Lines 991 to 995 in 4aeaf1e

if _, _, changed := containerChanged(&container, containerStatus); changed {

message = fmt.Sprintf("Container %s definition changed", container.Name)

// Restart regardless of the restart policy because the container

// spec changed.

restart = true

hshiina · 2024-09-30T13:15:55Z

test/e2e_node/container_lifecycle_test.go

+
+			ginkgo.By("updating the image", func() {
+				client.Update(ctx, podSpec.Name, func(pod *v1.Pod) {
+					podSpec.Spec.Containers[0].Image = imageutils.GetE2EImage(imageutils.InvalidRegistryImage)


This should be:

Suggested change

podSpec.Spec.Containers[0].Image = imageutils.GetE2EImage(imageutils.InvalidRegistryImage)

pod.Spec.Containers[0].Image = imageutils.GetE2EImage(imageutils.InvalidRegistryImage)

Then, the pod will be restarted.

hshiina · 2024-09-30T13:58:17Z

test/e2e_node/container_lifecycle_test.go

+				podSpec, err = client.Get(ctx, podSpec.Name, metav1.GetOptions{})
+				framework.ExpectNoError(err)
+				results := parseOutput(ctx, f, podSpec)
+				framework.ExpectNoError(results.HasNotRestarted(regular1))


By the way, we cannot use HasNotRestarted() with an invalid image because this function depends on log messages emitted by valid containers.

hshiina · 2024-09-30T14:40:05Z

I'm afraid a previous comment (#126794 (comment)) was based on a behavior with an invalid image. If a new image is valid, the pod will be running after the container is restarted because the newer container will be observed.

hshiina · 2024-10-03T08:49:54Z

test/e2e_node/container_lifecycle_test.go

+				err := e2epod.WaitForPodCondition(ctx, f.ClientSet, podSpec.Namespace, podSpec.Name, "wait for container to fail due to image",
+					time.Duration(bufferSeconds)*time.Second, func(pod *v1.Pod) (bool, error) {
+						containerStatus := pod.Status.ContainerStatuses[0]
+						if containerStatus.State.Waiting != nil && containerStatus.State.Waiting.Reason == "ErrImagePull" {


The container that hit ErrImagePull at restarting doesn't get into Waiting because ShouldContainerBeRestarted() returns false based on RestartPolicy:

kubernetes/pkg/kubelet/kubelet_pods.go

Lines 2330 to 2332 in 7dd03c1

if !kubecontainer.ShouldContainerBeRestarted(&container, pod, podStatus) {

continue

}

In addition, this container will not restarted:

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager.go

Line 985 in 7dd03c1

continue

If there is no other container in the pod, the behavior is a little different. The pod will be terminated as mentioned in #126794 (comment).

Anyway, a container whose image was updated to an invalid image will never restart. This may be a bug. It looks a corner case, though.

It seems like a very specific bug, because it only occurs when restartPolicy=onFailure but not when restartPolicy=Never. So far I could not figure out the reason. For now I just put a fixme but the test is still failing.

@SergeyKanzhelev

hshiina · 2024-10-03T08:59:49Z

test/e2e_node/container_lifecycle_test.go

 		framework.ExpectNoError(results.ExitsBefore(prefixedName(PostStartPrefix, regular1), regular2))
 	})

+	ginkgo.When("A pod is running a regular container with restartPolicy=Never", func() {


A test case with restartPolicy=OnFailure is rendered like:
[sig-node] [NodeConformance] Containers Lifecycle when A pod is running a regular container with restartPolicy=Never [It] should restart when the image is updated with a bad image and restartPolicy=OnFailure
There are restartPolicy=Never and restartPolicy=OnFailure. It would be better to move restartPolicy to each It.

hshiina · 2024-10-03T09:02:49Z

test/e2e_node/container_lifecycle_test.go

+		})
+	})
+
+	ginkgo.When("A pod is running a regular container with restartPolicy=Always", func() {


The initialization here looks almost the same as one above. Could we put all test cases under a single When?

SergeyKanzhelev · 2024-10-17T06:34:39Z

@dshebib do you think you will continue with this PR?

hshiina · 2024-11-17T16:23:33Z

test/e2e_node/container_lifecycle_test.go

+				regularContainerInvalidImgUpdateTest(ctx)
+			})
+
+			ginkgo.It("should successfully restart when the image is updated and restartPolicy=OnFailure", func(ctx context.Context) {


Suggested change

ginkgo.It("should successfully restart when the image is updated and restartPolicy=OnFailure", func(ctx context.Context) {

ginkgo.It("should successfully restart when the image is updated and restartPolicy=Never", func(ctx context.Context) {

hshiina · 2024-11-17T16:23:52Z

test/e2e_node/container_lifecycle_test.go

+				regularContainerImgUpdateTest(ctx)
+			})
+
+			ginkgo.It("should restart when the image is updated with a bad image and restartPolicy=OnFailure", func(ctx context.Context) {


Suggested change

ginkgo.It("should restart when the image is updated with a bad image and restartPolicy=OnFailure", func(ctx context.Context) {

ginkgo.It("should restart when the image is updated with a bad image and restartPolicy=Never", func(ctx context.Context) {

dshebib · 2024-11-22T15:27:01Z

/retest

SergeyKanzhelev · 2025-01-15T18:26:20Z

/assign @haircommander

haircommander · 2025-01-15T19:56:57Z

test/e2e_node/container_lifecycle_test.go

+				})
+			}
+
+			regularContainerInvalidImgUpdateTest := func(ctx context.Context) {


it's not immediately clear to me the difference between these two funcitons, but can you possibly compress them? I think it'll be easier to reason about

haircommander · 2025-01-15T20:04:53Z

this is a great update! I have one nit to make it a bit easier to read but otherwise LGTM

dshebib · 2025-01-28T00:55:04Z

/retest

dshebib · 2025-03-20T17:08:04Z

/retest

SergeyKanzhelev

/lgtm
/approve

k8s-ci-robot · 2025-03-20T19:47:44Z

LGTM label has been added.

Details

Git tree hash: 36a1a3ad7c9e686076058c979454bb2990156f7b

k8s-ci-robot · 2025-03-20T19:47:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dshebib, SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/e2e_node/OWNERS~~ [SergeyKanzhelev]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 19, 2024

k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 19, 2024

k8s-ci-robot requested review from dchen1107 and feiskyer August 19, 2024 20:23

BenTheElder reviewed Aug 19, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2024

toVersus reviewed Aug 28, 2024

View reviewed changes

dshebib commented Aug 30, 2024

View reviewed changes

test/e2e_node/container_lifecycle_test.go Outdated Show resolved Hide resolved

hshiina reviewed Sep 30, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

hshiina reviewed Sep 30, 2024

View reviewed changes

hshiina reviewed Oct 3, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2024

dshebib force-pushed the addRegularContainerImgChangeE2E branch from e3c81ac to e91689e Compare October 14, 2024 00:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 14, 2024

dshebib force-pushed the addRegularContainerImgChangeE2E branch from e91689e to e9e46f2 Compare October 22, 2024 05:07

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2024

hshiina reviewed Nov 17, 2024

View reviewed changes

k8s-ci-robot assigned haircommander Jan 15, 2025

haircommander reviewed Jan 15, 2025

View reviewed changes

dshebib force-pushed the addRegularContainerImgChangeE2E branch from 3553293 to 21649fd Compare January 20, 2025 03:26

WIP add tests for regular container image updates

1ee7d94

dshebib force-pushed the addRegularContainerImgChangeE2E branch from 21649fd to 1ee7d94 Compare March 20, 2025 00:38

SergeyKanzhelev approved these changes Mar 20, 2025

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Mar 20, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2025

k8s-ci-robot merged commit a9d0f39 into kubernetes:master Mar 20, 2025
14 checks passed

k8s-ci-robot added this to the v1.33 milestone Mar 20, 2025

github-project-automation bot moved this from PRs - Needs Reviewer to Done in SIG Node CI/Test Board Mar 20, 2025

github-project-automation bot moved this from Needs Reviewer to Done in SIG Node: code and documentation PRs Mar 20, 2025

pacoxu mentioned this pull request Mar 21, 2025

Revert "Add e2e test for Regular Container image change" #130965

Merged


		preparePod(originalPodSpec)

		ginkgo.It("should not restart when the image is updated", func(ctx context.Context) {

	if _, _, changed := containerChanged(&container, containerStatus); changed {
	message = fmt.Sprintf("Container %s definition changed", container.Name)
	// Restart regardless of the restart policy because the container
	// spec changed.
	restart = true

	podSpec.Spec.Containers[0].Image = imageutils.GetE2EImage(imageutils.InvalidRegistryImage)
	pod.Spec.Containers[0].Image = imageutils.GetE2EImage(imageutils.InvalidRegistryImage)

	if !kubecontainer.ShouldContainerBeRestarted(&container, pod, podStatus) {
	continue
	}

	ginkgo.It("should successfully restart when the image is updated and restartPolicy=OnFailure", func(ctx context.Context) {
	ginkgo.It("should successfully restart when the image is updated and restartPolicy=Never", func(ctx context.Context) {

	ginkgo.It("should restart when the image is updated with a bad image and restartPolicy=OnFailure", func(ctx context.Context) {
	ginkgo.It("should restart when the image is updated with a bad image and restartPolicy=Never", func(ctx context.Context) {

Add e2e test for Regular Container image change #126794

Add e2e test for Regular Container image change #126794

Uh oh!

Conversation

dshebib commented Aug 19, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Aug 19, 2024

Uh oh!

k8s-ci-robot commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Aug 26, 2024

Uh oh!

dshebib commented Aug 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

hshiina commented Sep 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dshebib commented Nov 22, 2024

Uh oh!

SergeyKanzhelev commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jan 15, 2025

Uh oh!

dshebib commented Jan 28, 2025

Uh oh!

dshebib commented Mar 20, 2025