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.

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

Update and improve Endpoints resource lifecycle test #92891

Merged
merged 1 commit into from Jul 23, 2020
Merged

Update and improve Endpoints resource lifecycle test #92891

merged 1 commit into from Jul 23, 2020

Conversation

riaankleinhans
Copy link
Contributor

@riaankleinhans riaankleinhans commented Jul 8, 2020 •
edited

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Improves naming, adds more checks throughout the test:

  • created
  • modified
  • deleted

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

Special notes for your reviewer:
Relate to PR #90939 improvement

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 k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. area/conformance Issues or PRs related to kubernetes conformance tests cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/network Categorizes an issue or PR as relevant to SIG Network. labels Jul 8, 2020
This was referenced Jul 8, 2020
Merged
Closed
@riaankleinhans
Copy link
Contributor Author

/assign @spiffxp
Created to address comments in #90939

@riaankleinhans riaankleinhans added this to In Progress PRs in conformance-definition Jul 8, 2020
@spiffxp
Copy link
Member

spiffxp commented Jul 8, 2020

/release-note-none
/retest

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 8, 2020
spiffxp
spiffxp suggested changes Jul 8, 2020
View reviewed changes
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

I feel like this is about halfway toward where I was suggesting we take this

test/e2e/network/service.go Outdated
break
}
}
framework.ExpectEqual(eventFound, true, "failed to find Endpoints %v event", watch.Deleted)
Copy link
Member

Choose a reason for hiding this comment

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

So we're verifying that a deleted event has occurred. Is this the same thing as verifying that the Endpoints object has been deleted? I would suggest trying to retrieve the Endpoints to verify it's no longer there as demonstrated in 225e7c7#diff-cfb4fba63c5d60607f76e0c2e1266d77R225-R231

Copy link
Member

Choose a reason for hiding this comment

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

Updated in 95e41663d7ef330874fad713e664b11e1ca320cb

test/e2e/network/service.go Outdated

// set up a watch for the Endpoint
// this timeout was chosen as there was timeout failure from the CI
endpointWatchTimeoutSeconds := int64(180)
endpointWatch, err := f.ClientSet.CoreV1().Endpoints(ns).Watch(context.TODO(), metav1.ListOptions{LabelSelector: "testendpoint-static=true", TimeoutSeconds: &endpointWatchTimeoutSeconds})
endpointWatch, err := f.ClientSet.CoreV1().Endpoints(testNamespaceName).Watch(context.TODO(), metav1.ListOptions{LabelSelector: "test-endpoint-static=true", TimeoutSeconds: &endpointWatchTimeoutSeconds})
Copy link
Member

Choose a reason for hiding this comment

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

Using a bare watch like this is susceptible to disconnects. Please consider using k8s.io/client-go/tools/watch.Until as demonstrated in 4582e26 instead of a for event := range chan loop

Copy link
Member

Choose a reason for hiding this comment

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

Updated in e65a1cc661e104a61ef5abe1feac1cbdbc94f100

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-integration

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2020
@BobyMCbobs
Copy link
Member

/retest

1 similar comment
@BobyMCbobs
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 20, 2020
@hh hh force-pushed the update-and-improve-endpoint-resource-lifecycle-test branch from 849ee94 to b0b4ca7 Compare July 20, 2020 18:16
@hh
Copy link
Member

hh commented Jul 20, 2020

/retest

1 similar comment
@hh
Copy link
Member

hh commented Jul 20, 2020

/retest

@hh hh force-pushed the update-and-improve-endpoint-resource-lifecycle-test branch from b0b4ca7 to 93c0f19 Compare July 20, 2020 18:31
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 20, 2020
@hh
Copy link
Member

hh commented Jul 20, 2020

/retest

3 similar comments
@riaankleinhans
Copy link
Contributor Author

/retest

@BobyMCbobs
Copy link
Member

/retest

@BobyMCbobs
Copy link
Member

/retest

@nikhita
Copy link
Member

nikhita commented Jul 22, 2020

@Riaankl 👋 Can you please squash the commits into a single commit? Looks ready to go otherwise!

@nikhita
Copy link
Member

nikhita commented Jul 22, 2020

@spiffxp @Riaankl Also, does this need to be in the 1.19 milestone? Given that master will be open for 1.20 soon, only blocker bugs are being considered for 1.19.

@hh
Copy link
Member

hh commented Jul 22, 2020

Hi Nikhita!

Test Freeze is Thu, August 6. My understanding is that if we merge this week (today?), we would have the required two weeks flake free we need for this test to be further modified for the 1.19 release.

Thank you for your help and leadership in the community!

@BobyMCbobs BobyMCbobs force-pushed the update-and-improve-endpoint-resource-lifecycle-test branch from 948c102 to c9acca7 Compare July 22, 2020 20:53
@nikhita
Copy link
Member

nikhita commented Jul 22, 2020 via email

@riaankleinhans
Copy link
Contributor Author

@nikhita Thank you for your support. @BobyMCbobs squashed the commits.
As soon as the checks is done, we will ask for a /LGTM

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@hh
Copy link
Member

hh commented Jul 22, 2020

@spiffxp already approved, and I've looked through this and feel that @BobyMCbobs addressed @spiffxp comments.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2020
@riaankleinhans
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-100-performance

@hh
Copy link
Member

hh commented Jul 23, 2020

/retest

1 similar comment
@hh
Copy link
Member

hh commented Jul 23, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 09e2230 into kubernetes:master Jul 23, 2020
conformance-definition automation moved this from In Progress PRs to Done Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BobyMCbobs BobyMCbobs BobyMCbobs left review comments

@spiffxp spiffxp spiffxp approved these changes

@danwinship danwinship Awaiting requested review from danwinship

@nicksardo nicksardo Awaiting requested review from nicksardo

Assignees

@hh hh

@spiffxp spiffxp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
conformance-definition
  
Done
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

Write CoreV1Endpoints test - +4 coverage
6 participants
@riaankleinhans @spiffxp @BobyMCbobs @k8s-ci-robot @hh @nikhita