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

slice mirroring controller should mirror annotations (except endpoints.kubernetes.io/last-change-trigger-time annotation) and labels #98116

Merged
merged 1 commit into from Mar 9, 2021
Merged

slice mirroring controller should mirror annotations (except endpoints.kubernetes.io/last-change-trigger-time annotation) and labels #98116

merged 1 commit into from Mar 9, 2021

Conversation

aojea
Copy link
Member

@aojea aojea commented Jan 16, 2021 •
edited

Add support to the endpoint slice mirroring controller to mirror
annotations, in addition to labels.

Also, fix a bug in the endpointslice mirroring controller, that
wasn't updating the mirrored slice with the new labels, in case
that only the endpoint labels were modified.

Fixes #98026

The endpointslice mirroring controller mirrors endpoints annotations and labels to the generated endpoint slices, it also ensures that updates on any of these fields are mirrored. 
The well-known annotation endpoints.kubernetes.io/last-change-trigger-time is skipped and not mirrored.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. 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 Jan 16, 2021
@aojea
Copy link
Member Author

aojea commented Jan 16, 2021

/assign @robscott
/cc @frobware @sgreene570
/sig network
/kind bug
/kind feature

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 16, 2021
@k8s-ci-robot
Copy link
Contributor

@aojea: GitHub didn't allow me to request PR reviews from the following users: sgreene570.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @robscott
/cc @frobware @sgreene570
/sig network
/kind bug
/kind feature

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jan 16, 2021
@aojea aojea force-pushed the mirror_dual branch 2 times, most recently from 6ff56be to 759ce41 Compare January 16, 2021 22:14
@aojea
Copy link
Member Author

aojea commented Jan 16, 2021

/retest
ReplicaSet should surface a failure condition on a common issue like exceeded quota

@frobware frobware mentioned this pull request Jan 18, 2021
Merged
frobware
frobware reviewed Jan 18, 2021
View reviewed changes
Copy link
Contributor

@frobware frobware left a comment

Choose a reason for hiding this comment

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

Overall LGTM but I think the tests should really assert on the actual annotation values that were set, not relying on just the update to indicate success.

test/integration/endpointslice/endpointslicemirroring_test.go Show resolved Hide resolved
@robscott
Copy link
Member

Thanks for taking this on!

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 18, 2021
@aojea
Copy link
Member Author

aojea commented Mar 4, 2021

Sorry I lost track of this @aojea! Happy to try to get this into 1.21 if you have time.

Let's do it

@robscott
Copy link
Member

robscott commented Mar 8, 2021

@aojea This LGTM, looks like it just needs a rebase.

@robscott
Copy link
Member

robscott commented Mar 8, 2021

@MrHohn can you approve this for integration test change?

/assign @MrHohn

MrHohn
MrHohn approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

Thanks, approval for integration test change.
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2021
@aojea
slice mirroring controller mirror annotations
c421e22 
Add support to the endpoint slice mirroring controller to mirror
annotations, in addition to labels, but don´t mirror endpoint
triggertime annotation.

Also, fix a bug in the endpointslice mirroring controller, that
wasn't updating the mirrored slice with the new labels, in case
that only the endpoint labels were modified.
@aojea
Copy link
Member Author

aojea commented Mar 8, 2021

seems CI start to get overloaded? 🤔

W0308 22:37:31.810] ERROR: failed to create cluster: failed to copy kubeadm config to node: failed to create directory /kind: command "docker exec --privileged kind-worker2 mkdir -p /kind" failed with error: exit status 126
W0308 22:37:31.811] 
W0308 22:37:31.811] Command Output: unexpected EOF: unknown

and


k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy: TestConnectionPings expand_less | 0s
-- | --
=== RUN   TestConnectionPings     connection_test.go:240: server: failed to send any pings (check logs) --- FAIL: TestConnectionPings (0.10s)

/retest

@robscott
Copy link
Member

robscott commented Mar 8, 2021

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, MrHohn, robscott

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

@robscott
Copy link
Member

robscott commented Mar 9, 2021

/retest

1 similar comment
@robscott
Copy link
Member

robscott commented Mar 9, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7eb9919 into kubernetes:master Mar 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021
@liggitt
Copy link
Member

liggitt commented Mar 9, 2021

looks like something around this is flaking badly (not sure if it is just the test or also the impl) - https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&test=TestEndpointSliceMirroringUpdates

@aojea
Copy link
Member Author

aojea commented Mar 9, 2021

yep

@aojea
Copy link
Member Author

aojea commented Mar 9, 2021

checking it right now

@liggitt
Copy link
Member

liggitt commented Mar 9, 2021 •
edited

it's currently affecting batch merges as well, so I would recommend opening a revert PR to have passing CI in parallel with trying to resolve the flake

@aojea
Copy link
Member Author

aojea commented Mar 9, 2021

doing it righ tnow

@liggitt liggitt mentioned this pull request Mar 9, 2021
Merged
@aojea aojea changed the title slice mirroring controller should mirror annotations (but endpoints.kubernetes.io/last-change-trigger-time annotation) and labels slice mirroring controller should mirror annotations (except endpoints.kubernetes.io/last-change-trigger-time annotation) and labels Mar 18, 2021
This was referenced Mar 18, 2021
Closed
Merged
k8s-ci-robot added a commit that referenced this pull request Mar 29, 2021
@k8s-ci-robot
Merge pull request #100443 from aojea/automated-cherry-pick-of-#98116…
7849986 
…-upstream-release-1.20

Automated cherry pick of #98116: slice mirroring controller mirror annotations
@liggitt
Copy link
Member

liggitt commented May 6, 2021 •
edited

This was picked to release-1.19 (#100504) and release-1.20 (#100443), but the added integration test is flaky

In the 1.21 timeframe, there were several follow-up fixes:

Should #100027 or #100103 also be picked to 1.19 and 1.20?

This was referenced May 6, 2021
Merged
Merged
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

@frobware frobware frobware left review comments

@sgreene570 sgreene570 sgreene570 left review comments

@MrHohn MrHohn MrHohn approved these changes

@m1093782566 m1093782566 Awaiting requested review from m1093782566

@SataQiu SataQiu Awaiting requested review from SataQiu

Assignees

@robscott robscott

@MrHohn MrHohn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/feature Categorizes issue or PR as related to a new feature. 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/apps Categorizes an issue or PR as relevant to SIG Apps. 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. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

EndpointSliceMirroring controller does not mirror annotations from endpoints
7 participants
@aojea @k8s-ci-robot @robscott @liggitt @frobware @MrHohn @sgreene570