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

upgrade IPv6DualStack feature to beta and turn on by default #98969

Merged
merged 2 commits into from Feb 11, 2021
Merged

upgrade IPv6DualStack feature to beta and turn on by default #98969

merged 2 commits into from Feb 11, 2021

Conversation

khenidak
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Upgrades IPv6Dualstack to Beta and turns it on by default.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

Upgrades `IPv6Dualstack` to `Beta` and turns it on by default. Clusters new and existing will not be affected until user starting adding secondary pod and service cidrs cli flags as described here: https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/563-dual-stack

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

- [KEP]:  https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/563-dual-stack

@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/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 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. 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 Feb 10, 2021
@aojea
Copy link
Member

aojea commented Feb 10, 2021

/assign @thockin @liggitt
/lgtm

Beta graudation requirements are met:

Kubernetes types finalized
CRI types finalized
Pods to support multi-IPs
Nodes to support multi-CIDRs
Service resource supports pods with multi-IP
Kubenet to support multi-IPs

In addition, there are several optional presubmits and periodic jobs testing the feature:

We are still adding more automated test and working in stabilizing some of the tests and jobs in some platforms, but kind jobs are stable.

Having it enabled by default will allow us ot get a better signal and detect possible problems earlier.

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

aojea commented Feb 10, 2021

/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 Feb 10, 2021
@aojea
Copy link
Member

aojea commented Feb 10, 2021

/lgtm cancel

heh, it seems we have some real work to do before enabling.

@khenidak lot of tests and places fail because of the following

feature gate "IPv6DualStack" with different spec already exists: {true false BETA}

Ironically, it also seems to break KIND, the controller manager panics

2021-02-10T16:50:40.493921602Z stderr F panic: feature gate "IPv6DualStack" with different spec already exists: {true false BETA}
2021-02-10T16:50:40.494042788Z stderr F 
2021-02-10T16:50:40.49406094Z stderr F goroutine 1 [running]:
2021-02-10T16:50:40.494070199Z stderr F k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.Must(...)
2021-02-10T16:50:40.494078717Z stderr F 	staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go:171
2021-02-10T16:50:40.494085943Z stderr F k8s.io/kubernetes/vendor/k8s.io/controller-manager/pkg/features/register.init.0()
2021-02-10T16:50:40.494092494Z stderr F 	staging/src/k8s.io/controller-manager/pkg/features/register/register.go:26 +0x6f

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2021
@aramase
Copy link
Member

aramase commented Feb 10, 2021

We need to update the feature flag here too: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/controller-manager/pkg/features/kube_features.go#L64

After this the tests should pass

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 10, 2021
@aojea
Copy link
Member

aojea commented Feb 10, 2021

/test pull-kubernetes-bazel-test

@khenidak the failure is legit

k8s.io/kubernetes/pkg/kubelet/dockershim/network/cni.TestCNIPlugin(0xc0003b5b00)
pkg/kubelet/dockershim/network/cni/cni_test.go:323 +0x2934

thockin
thockin reviewed Feb 10, 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.

Scary scary! Glad to see this. Just a small sting nit.

cmd/kube-apiserver/app/options/options.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 10, 2021
@khenidak
Copy link
Contributor Author

/test pull-kubernetes-bazel-test

@aojea
Copy link
Member

aojea commented Feb 11, 2021

/lgtm
Go go go

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021
thockin
thockin reviewed Feb 11, 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.

w00t!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2021
@khenidak
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@ehashman ehashman added this to Triage in SIG Node PR Triage Feb 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2d153fe into kubernetes:master Feb 11, 2021
SIG Node PR Triage automation moved this from Triage to Done Feb 11, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 11, 2021
@fedebongio
Copy link
Contributor

/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 Feb 11, 2021
@khenidak khenidak deleted the beta-feature branch February 11, 2021 21:20
@josenobile josenobile mentioned this pull request Feb 18, 2021
Open
@pacoxu pacoxu mentioned this pull request Feb 22, 2021
Merged
This was referenced Mar 3, 2021
Closed
Merged
This was referenced Mar 15, 2021
Closed
Closed
@aojea aojea mentioned this pull request Apr 1, 2021
Merged
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

@aramase aramase aramase left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@liggitt liggitt

@thockin thockin

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. 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
SIG Node PR Triage
Done
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@khenidak @aojea @aramase @k8s-ci-robot @fedebongio @thockin @liggitt