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

fix e2e test apiserver endpoint and endpointslices #104664

Merged
merged 1 commit into from Sep 3, 2021
Merged

fix e2e test apiserver endpoint and endpointslices #104664

merged 1 commit into from Sep 3, 2021

Conversation

aojea
Copy link
Member

@aojea aojea commented Aug 30, 2021 •
edited

The e2e test "should have Endpoints and EndpointSlices pointing to
the API Server Service" was veryfing the current endpoints
reconciler implementation on the apiservers, however, users may
disable the endpoint reconciler and create their own.

This e2e test is also a conformance test, so we should test the
behaviour and not the implementation details, so we just verify
that a kubernetes.default service exist, an endpoint and endpoint
slices object referencing that service exist with the same endpoints.

/kind bug
/kind cleanup
/kind documentation
/kind failing-test

Fixes #104612

conformance: the test "[sig-network] EndpointSlice should have Endpoints and EndpointSlices pointing to API Server [Conformance]" only requires that there is an EndpointSlice that references the "kubernetes.default" service, it no longer requires that its named "kubernetes".

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 30, 2021
@aojea
Copy link
Member Author

aojea commented Aug 30, 2021

/assign @liggitt @robscott

@k8s-ci-robot k8s-ci-robot added area/test sig/network Categorizes an issue or PR as relevant to SIG Network. labels Aug 30, 2021
@aojea
Copy link
Member Author

aojea commented Aug 30, 2021

/sig network
/area conformance

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. area/conformance Issues or PRs related to kubernetes conformance tests approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 30, 2021
@aojea aojea force-pushed the apiserver_endpoints_conformance branch from 185237e to 5771052 Compare August 30, 2021 17:55
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2021
@aojea aojea force-pushed the apiserver_endpoints_conformance branch from 5771052 to d2a6d57 Compare August 30, 2021 22:42
robscott
robscott reviewed Aug 31, 2021
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @aojea! You covered a lot of great edge cases with this one and significantly improved the test.

test/e2e/network/endpointslice.go Outdated Show resolved Hide resolved
@aojea aojea force-pushed the apiserver_endpoints_conformance branch from d2a6d57 to 18ce25d Compare August 31, 2021 08:03
e2e test apiserver endpoint and endpointslices
2a5ad65 
The e2e test "should have Endpoints and EndpointSlices pointing to
the API Server Service" was veryfing the current endpoints
reconciler implementation on the apiservers, however, users may
disable the endpoint reconciler and create their own.

This e2e test is also a conformance test, so we should test the
behaviour and not the implementation details. The test verifies
that a kubernetes.default service exist, an endpoint and endpoint
slices object referencing that service exist and are equivalent.
@aojea aojea force-pushed the apiserver_endpoints_conformance branch from 18ce25d to 2a5ad65 Compare August 31, 2021 09:29
@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 31, 2021
@aojea
Copy link
Member Author

aojea commented Aug 31, 2021

/retest

Kubernetes e2e suite: [sig-node] Probing container should have monotonically increasing restart count [NodeConformance] [Conformance]

@robscott
Copy link
Member

Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2021
@robscott
Copy link
Member

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 31, 2021
@robscott
Copy link
Member

/assign @johnbelamaric

@oomichi
Copy link
Member

oomichi commented Sep 2, 2021

/lgtm

@aojea
Copy link
Member Author

aojea commented Sep 2, 2021

/assign @spiffxp @smarterclayton

spiffxp
spiffxp approved these changes Sep 3, 2021
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.

/approve
/lgtm
Thank you for doing this, much cleaner

test/e2e/network/endpointslice.go
slicePorts := sets.NewInt32()
for _, slice := range endpointSliceList.Items {
if slice.AddressType != addrType {
framework.Logf("Skipping slice %s: wanted %s family, got %s", slice.Name, addrType, slice.AddressType)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be skipping or failing because the family is unexpected?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I guess it's more ignoring or filtering down to what we're looking for

If the server didn't return any addresses of our expected family we'd error on not found

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a kep to dualstack the Apiserver , in theory it should report for each family but is not clear defined, and failing means is not allowed, that is not completely correct

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2021
@khenidak
Copy link
Contributor

khenidak commented Sep 3, 2021

/retest
/lgtm
/approve

Thanks

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, khenidak, spiffxp

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:

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 k8s-ci-robot merged commit 81e41b7 into kubernetes:master Sep 3, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@liggitt liggitt liggitt left review comments

@spiffxp spiffxp spiffxp approved these changes

@bowei bowei Awaiting requested review from bowei

@freehan freehan Awaiting requested review from freehan

Assignees

@spiffxp spiffxp

@robscott robscott

@liggitt liggitt

@smarterclayton smarterclayton

@oomichi oomichi

@khenidak khenidak

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/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
8 participants
@aojea @robscott @oomichi @khenidak @k8s-ci-robot @spiffxp @liggitt @smarterclayton