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.

Sign up for GitHub

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

Jump to bottom

Write e2e test for PodEphemeralcontainers endpoints + 2 Endpoints #117895

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from ii:create-ephemeralcontainer-test

May 18, 2023

Contributor

riaankleinhans commented May 9, 2023 •

edited

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR adds a test to test the following untested endpoints:

readCoreV1NamespacedPodEphemeralcontainers
replaceCoreV1NamespacedPodEphemeralcontainers

Which issue(s) this PR fixes:
Fixes #117894

Testgrid Link:
Testgrid

Special notes for your reviewer:
Adds +2 endpoint test coverage (good for conformance)

Does this PR introduce a user-facing change?:

NONE

Release note:

NONE

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

NONE

/sig testing
/sig architecture
/area conformance

k8s-ci-robot added release-note-none size/M kind/cleanup sig/testing sig/architecture cncf-cla: yes area/conformance needs-triage needs-priority area/test sig/node labels

riaankleinhans mentioned this pull request

Write e2e test for PodEphemeralcontainers endpoints + 2 Endpoints #117894

Closed

6 tasks

k8s-ci-robot requested review from Random-Liu and yujuhong

May 9, 2023 22:46

Contributor Author

riaankleinhans commented May 9, 2023

/triage accepted
/cc @heyste

k8s-ci-robot requested a review from heyste

May 9, 2023 22:47

k8s-ci-robot added triage/accepted and removed needs-triage labels

riaankleinhans added this to In Progress /Active Issues in conformance-definition

riaankleinhans moved this from In Progress /Active Issues to PRs Needing Review in conformance-definition

heyste force-pushed the create-ephemeralcontainer-test branch 2 times, most recently from 4d17cf0 to 297808e Compare

May 10, 2023 01:22

Member

heyste commented May 10, 2023

/test pull-kubernetes-node-e2e-containerd
unrelated flake: e2e.go: Node Tests

I0510 01:47:44.252] Master not detected. Is the cluster up?
I0510 01:47:47.377] Dumping logs from nodes locally to '/workspace/_artifacts'
I0510 01:47:47.377] Detecting nodes in the cluster
I0510 01:47:47.377] No nodes found!

Member

heyste commented May 10, 2023

/test pull-kubernetes-e2e-kind-ipv6
unrelated flake: [sig-node] Pods [It] should run through the lifecycle of Pods and PodStatus [Conformance]

  May 10 01:48:08.104: INFO: observed event type MODIFIED
  May 10 01:48:38.933: INFO: failed to see DELETED event: timed out waiting for the condition
  [FAILED] in [It] - test/e2e/common/node/pods.go:1070 @ 05/10/23 01:48:38.937
  May 10 01:48:38.937: INFO: Waiting up to 7m0s for all (but 0) nodes to be ready

bart0sh added this to Triage in SIG Node PR Triage

Contributor

bart0sh commented May 10, 2023

/priority important-longterm
/cc @pohly @dims

k8s-ci-robot requested review from dims and pohly

May 10, 2023 09:43

k8s-ci-robot added priority/important-longterm and removed needs-priority labels

Member

dims commented May 10, 2023 •

edited

/approve

leaving /lgtm for @heyste

k8s-ci-robot added the approved label

Member

dims commented May 10, 2023 •

edited

cc @verb @mrunalp @SergeyKanzhelev

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board

Member

SergeyKanzhelev commented May 10, 2023

/assign @rphillips

k8s-ci-robot assigned rphillips

SergeyKanzhelev moved this from Triage to PRs - Needs Reviewer in SIG Node CI/Test Board

Contributor Author

riaankleinhans commented May 10, 2023

Thank you for the reviews and the /approve.
We would appriciate a /lgtm from SIG Node folks.

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go Outdated

    
              		ginkgo.By(fmt.Sprintf("checking pod %q has only one ephemeralcontainer", pod.Name))

              		podResource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}

              		unstruct, err := f.DynamicClient.Resource(podResource).Namespace(f.Namespace.Name).Get(ctx, "ephemeral-containers-target-pod", metav1.GetOptions{}, "ephemeralcontainers")

              		framework.ExpectNoError(err, "can't get ephermalcontainers: %#v", err)

Member

SergeyKanzhelev May 12, 2023

why %#v?

Member

heyste May 14, 2023

Personal preference. I find the "Go-syntax representation" helps me confirm the format/types being used. Based on the feedback below I've removed it.

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go Outdated

    
              			_, err = podClient.UpdateEphemeralContainers(context.TODO(), pod.Name, podToUpdate, metav1.UpdateOptions{})

              			return err

              		})

              		framework.ExpectNoError(err, "Failed to update status. %v", err)

Member

SergeyKanzhelev May 12, 2023

no need to have err in both - first parameter and string substitute. It will be written twice.

Also typically context message is about something was hepenning, not starting with "Failed". In this case it will be "updating ephemeral container" or sometjing like this

Member

heyste May 14, 2023

Thanks for catching the issue with the message context. It was from another test I've done for status endpoints. Fixed.

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go

    
              						Name:    "test-container-1",

              						Image:   imageutils.GetE2EImage(imageutils.BusyBox),

              						Command: []string{"/bin/sleep"},

              						Args:    []string{"10000"},

Member

SergeyKanzhelev May 12, 2023

is it intended to be an infinity? Maybe use infinity

Member

heyste May 14, 2023

This part of the test was taken from the e2e test "will start an ephemeral container in an existing pod" in the same file. I'm not sure why that figure was used. As this test now covers all of the ephemeral container endpoints the current thought is the above test could be removed after this test has been promoted to conformance.

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go Outdated

    
              		unstruct, err = f.DynamicClient.Resource(podResource).Namespace(f.Namespace.Name).Get(ctx, "ephemeral-containers-target-pod", metav1.GetOptions{}, "ephemeralcontainers")

              		framework.ExpectNoError(err, "can't get ephermalcontainers: %#v", err)

              		verifyPod, err = unstructuredToPod(unstruct)

              		framework.ExpectNoError(err, "Getting the pod %q ephemeralcontainers", verifyPod.Name)

Member

SergeyKanzhelev May 12, 2023

Suggested change

      
            		framework.ExpectNoError(err, "Getting the pod %q ephemeralcontainers", verifyPod.Name)
          
            		framework.ExpectNoError(err, "Getting the pod's %q ephemeralcontainers", verifyPod.Name)

Member

heyste May 14, 2023

Updated this as well line 133

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go

    
              		ginkgo.By(fmt.Sprintf("checking pod %q has only one ephemeralcontainer", pod.Name))

              		podResource := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}

              		unstruct, err := f.DynamicClient.Resource(podResource).Namespace(f.Namespace.Name).Get(ctx, "ephemeral-containers-target-pod", metav1.GetOptions{}, "ephemeralcontainers")

Member

SergeyKanzhelev May 12, 2023

for my education, why dynamic client is used here?

Member

heyste May 14, 2023

AFAIK, there isn't way to use the standard client-go to hit the readCoreV1NamespacedPodEphemeralcontainers endpoint which is why I've used the dynamic client. There was a GetEphemeralContainers method but that was removed from kubernetes/typed/core/v1/pod.go

heyste force-pushed the create-ephemeralcontainer-test branch from 297808e to e347f42 Compare

May 14, 2023 22:22

SergeyKanzhelev reviewed

View reviewed changes

test/e2e/common/node/ephemeral_containers.go Outdated

    
              		ginkgo.By(fmt.Sprintf("checking pod %q has only two ephemeralcontainers", pod.Name))

              		unstruct, err = f.DynamicClient.Resource(podResource).Namespace(f.Namespace.Name).Get(ctx, "ephemeral-containers-target-pod", metav1.GetOptions{}, "ephemeralcontainers")

              		framework.ExpectNoError(err, "can't get ephermalcontainers: %#v", err)

Member

SergeyKanzhelev May 16, 2023

no need to have err as a first AND third argument here. It will be part of error by having it a first argument

Member

heyste May 16, 2023

Fixed


          Create e2e test for PodEphemeralcontainers endpoints

45603ef

e2e test validates the following 2 endpoints
- readCoreV1NamespacedPodEphemeralcontainers
- replaceCoreV1NamespacedPodEphemeralcontainers

heyste force-pushed the create-ephemeralcontainer-test branch from e347f42 to 45603ef Compare

May 16, 2023 18:31

SergeyKanzhelev approved these changes

View reviewed changes

Member

SergeyKanzhelev left a comment

/lgtm

k8s-ci-robot assigned SergeyKanzhelev

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented May 18, 2023

LGTM label has been added.

Git tree hash: 0afef0d865798c63283c81f3a6bc6317e2b5847a

Contributor

k8s-ci-robot commented May 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, Riaankl, SergeyKanzhelev

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/common/OWNERS~~ [SergeyKanzhelev,dims]

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 merged commit e3db923 into kubernetes:master

13 checks passed

conformance-definition automation moved this from PRs Needing Review to Done

SIG Node CI/Test Board automation moved this from PRs - Needs Reviewer to Done

SIG Node PR Triage automation moved this from Needs Reviewer to Done

k8s-ci-robot added this to the v1.28 milestone

riaankleinhans moved this from Done to Promotion PRs Needing Two Weeks (flake free) in conformance-definition

riaankleinhans mentioned this pull request

Remove Ephemeral Containers form pending_eligible_endpoints.yaml #118328

Merged

riaankleinhans moved this from Promotion PRs Needing Two Weeks (flake free) to Done in conformance-definition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

SergeyKanzhelev SergeyKanzhelev approved these changes

Random-Liu Awaiting requested review from Random-Liu

yujuhong Awaiting requested review from yujuhong

heyste Awaiting requested review from heyste

dims Awaiting requested review from dims

pohly Awaiting requested review from pohly

Labels

approved area/conformance area/test cncf-cla: yes kind/cleanup lgtm priority/important-longterm release-note-none sig/architecture sig/node sig/testing size/M triage/accepted