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 auto upgraded preferDual-Stack services (in cluster upgrade) #102898

Merged
merged 2 commits into from
Jun 25, 2021
Merged

fix auto upgraded preferDual-Stack services (in cluster upgrade) #102898

merged 2 commits into from
Jun 25, 2021

Conversation

khenidak
Copy link
Contributor

@khenidak khenidak commented Jun 15, 2021 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

fixes #101681 replaces #101914

PreferDualstack services were automatically upgraded to dual-stack if:

  1. They were created before the cluster was upgraded AND
  2. user performed any update on them (e.g., updating labels).

This PR ensures that preferDualstack services are not upgraded automatically (nor downgraded)

Which issue(s) this PR fixes:

Fixes #101681

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

The current behavior for Services that `IPFamilyPolicy` set as `PreferDualstack`. The current behavior when the cluster is upgraded to dual-stack is:
- Services that have been set to IPFamilyPolicy = PreferDualstack will be upgraded when the service object is updated. e.g., when a user change a label.

This behavior will change to:
- Services that have been set  IPFamilyPolicy = PreferDualstack will not be upgraded when the service object is updated. User can still change policy, type etc and existing behaviors remain the same.

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

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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. labels Jun 15, 2021
@k8s-ci-robot
Copy link
Contributor

@khenidak: 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.

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 added 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 Jun 15, 2021
@khenidak khenidak changed the title fix auto upgraded preferDualStack services (in cluster upgrade) fix auto upgraded preferDual-Stack services (in cluster upgrade) Jun 15, 2021
@khenidak khenidak mentioned this pull request Jun 15, 2021
Closed
@khenidak
Copy link
Contributor Author

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 16, 2021
@aojea
Copy link
Member

aojea commented Jun 17, 2021

are the integration tests or at least some of them from 4b484a2
still valid? I'll be more confident with them 😄

/assign @thockin

@aojea
Copy link
Member

aojea commented Jun 17, 2021

/test pull-kubernetes-e2e-kind

@aojea
Copy link
Member

aojea commented Jun 17, 2021

/test pull-kubernetes-e2e-kind-dual-canary
/tests pull-kubernetes-e2e-iptables-azure-dualstack

@khenidak
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind-dual-canary
/tests pull-kubernetes-e2e-iptables-azure-dualstack
/test pull-kubernetes-e2e-kind

@khenidak
Copy link
Contributor Author

khenidak commented Jun 18, 2021 •
edited
Loading

are the integration tests or at least some of them from 4b484a2
still valid? I'll be more confident with them 😄

/assign @thockin

The thing about re-running api-server from integration test framework (with a different config) is that it is a hit or a miss. Sometimes it works, sometimes it just hangs. I am yet to look into it.

thockin
thockin reviewed Jun 18, 2021
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

This is surprisingly straight-forward. LGTM except one bit of test clarity

pkg/registry/core/service/storage/rest_test.go Show resolved Hide resolved
pkg/registry/core/service/storage/rest_test.go Show resolved Hide resolved
pkg/registry/core/service/storage/rest_test.go Show resolved Hide resolved
@aojea
Copy link
Member

aojea commented Jun 22, 2021

this hangs when i run it. if it is just my setup then I will try to push it and see.

@khenidak I've adapted the test from your previous PR, do you think you can include it?

diff --git a/test/integration/dualstack/dualstack_test.go b/test/integration/dualstack/dualstack_test.go
index eeeba0219e6..4feacacf7fe 100644
...

have you rebased over master, there were some changes on the integration framework , that is one of the reasons I had to adapt it :)

@khenidak
Copy link
Contributor Author

@aojea added your tests.. let us see how it goes.

@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 22, 2021
@bridgetkromhout
Copy link
Member

/retest

aojea
aojea reviewed Jun 22, 2021
View reviewed changes
test/integration/dualstack/dualstack_test.go
if err != nil {
t.Fatalf("Unexpected error to get the service %s %v", svc.Name, err)
}
// service should be single stack
Copy link
Member

Choose a reason for hiding this comment

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

this is my fault, the comment is wrong,

Suggested change
// service should be single stack
// service should remain dual stack

Copy link
Member

Choose a reason for hiding this comment

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

yep

@aojea
Copy link
Member

aojea commented Jun 22, 2021

/lgtm

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

/test pull-kubernetes-e2e-kind-dual-canary
/tests pull-kubernetes-e2e-iptables-azure-dualstack
/test pull-kubernetes-e2e-kind

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 23, 2021
@thockin
Copy link
Member

thockin commented Jun 25, 2021

Other than Antonio's 1 comment nit, I am good with this

/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 Jun 25, 2021
@dcbw
Copy link
Member

dcbw commented Jun 25, 2021 •
edited
Loading

/lgtm
Same as Tim, some nits but good to go. I'll register my approval too :)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, khenidak, thockin

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 e19dc07 into kubernetes:master Jun 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 25, 2021
@thockin
Copy link
Member

thockin commented Jun 25, 2021

woops, I assumed we were waiting for the comment fix - @khenidak or @aojea can you please followup?

@aojea aojea mentioned this pull request Jun 25, 2021
Merged
@aojea
Copy link
Member

aojea commented Jun 25, 2021

woops, I assumed we were waiting for the comment fix - @khenidak or @aojea can you please followup?

done #103220

k8s-ci-robot added a commit that referenced this pull request Jun 25, 2021
@k8s-ci-robot
Merge pull request #103220 from aojea/nit_dual
7498c28 
Follow up on fix auto upgraded preferDual-Stack services (in cluster upgrade) #102898
@bridgetkromhout bridgetkromhout mentioned this pull request Jun 29, 2021
Closed
This was referenced Jun 30, 2021
Merged
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@aojea aojea aojea left review comments

@dcbw dcbw dcbw left review comments

@hongchaodeng hongchaodeng Awaiting requested review from hongchaodeng

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@thockin thockin

@aojea aojea

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. 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

Dual-stack updates w/ PreferDualStack changes IP assignments
6 participants
@khenidak @k8s-ci-robot @aojea @bridgetkromhout @thockin @dcbw