Skip to content

dual stack services#91824

Merged
k8s-ci-robot merged 22 commits intokubernetes:masterfrom
khenidak:dualstack-phase-3
Oct 26, 2020
Merged

dual stack services#91824
k8s-ci-robot merged 22 commits intokubernetes:masterfrom
khenidak:dualstack-phase-3

Conversation

@khenidak
Copy link
Copy Markdown
Contributor

@khenidak khenidak commented Jun 5, 2020 •
edited by thockin
Loading

implements kubernetes/enhancements#1679 in its eventual form.

status of work

  • api structure change
  • api work
    • helpers, utils etc..
    • defaulting
    • defaulting for existing services (headful + headless + headless without selectors)
    • validation
    • conversion
  • tie dualstack services to EndPointSlice feature - startup flag validation
    • api server
    • controller manager
  • EndpointSlice
    • ensure that EndpointSlice handles multi family services
  • service ip alloc
    • default kuberentes service
    • force kuberenetes default service into dual stack
    • user services
    • strategy: clear IPFamilies, IPFamilyPolicy, ClusterIPs when converting ExternalName type.
    • api-server repair loop
  • pod/pod net
    • env vars handle dual stack services
    • remove kubenet v4,v6 enforcement
  • kube proxy
    • flag validation, tie dual stack to EndPointSlice
    • reserve NodePort for both ipv6/ipv4 by default
    • modify metaproxier to work only with EndPointSlice
    • modify metaproxier to expect EndPointSlice with both families endpoints
    • force ipvs proxier to lookup ClusterIP by family (and correctly select ranges and related fields accordingly)
    • force iptables to lookup ClusterIP by family (and correctly select ranges and related fields accordingly)
    • FIX ipvs mishandling of families.
    • validate that topology keys does not conflict with dualstack
  • kubectl
    • modify kubectl describe to work with multi ClusterIPs
  • e2e
    • fix existing e2e tests that will break due to api change
    • add new e2e tests for upgrade and downgrade scenarios
Add dual-stack Services (alpha).  This is a BREAKING CHANGE to an alpha API.
It changes the dual-stack API wrt Service from a single ipFamily field to 3
fields: ipFamilyPolicy (SingleStack, PreferDualStack, RequireDualStack),
ipFamilies (a list of families assigned), and clusterIPs (inclusive of
clusterIP).  Most users do not need to set anything at all, defaulting will
handle it for them.  Services are single-stack unless the user asks for
dual-stack.  This is all gated by the "IPv6DualStack" feature gate.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 5, 2020
@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 5, 2020
@aramase
Copy link
Copy Markdown
Member

aramase commented Jun 5, 2020

/cc

@k8s-ci-robot k8s-ci-robot requested a review from aramase June 5, 2020 16:10
@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Jun 5, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/types.go Outdated
aojea
aojea reviewed Jun 5, 2020
View reviewed changes
Comment thread pkg/apis/core/v1/conversion_test.go Outdated
@khenidak khenidak force-pushed the dualstack-phase-3 branch from b6a8f50 to 0329e38 Compare June 11, 2020 17:37
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 11, 2020
@khenidak khenidak force-pushed the dualstack-phase-3 branch 2 times, most recently from 6f17970 to 376014e Compare June 11, 2020 22:53
khenidak and others added 9 commits October 26, 2020 18:05
@khenidak
[FIX] kubeproxy: call both proxier at the same time
61eef7a
@khenidak
kubenet: remove forced pod IP sorting
8b78122
@khenidak
kubectl: modify describe to include ClusterIPs, IPFamilies, and IPFam…
7b7afdd 
…ilyPolicy
@khenidak
e2e: fix tests that depends on IPFamily field AND add dual stack tests
59569ab
@khenidak
e2e: fix expected error message for ClusterIP immutability
675362d
@khenidak
add integration tests for dualstack
07f6752 
the third phase of dual stack is a very complex change in the API,
basically it introduces Dual Stack services. Main changes are:

- It pluralizes the Service IPFamily field to IPFamilies,
and removes the singular field.
- It introduces a new field IPFamilyPolicyType that can take
3 values to express the "dual-stack(mad)ness" of the cluster:
SingleStack, PreferDualStack and RequireDualStack
- It pluralizes ClusterIP to ClusterIPs.

The goal is to add coverage to the services API operations,
taking into account the 6 different modes a cluster can have:

- single stack: IP4 or IPv6 (as of today)
- dual stack: IPv4 only, IPv6 only, IPv4 - IPv6, IPv6 - IPv4
@khenidak
[FIX] add integration tests for dualstack
96e26b5
@khenidak
generated data
52a1925
@khenidak
generated files
14b3451
@bridgetkromhout bridgetkromhout mentioned this pull request Oct 26, 2020
Merged
@thockin
Copy link
Copy Markdown
Member

thockin commented Oct 26, 2020 •
edited
Loading

joker_here_we_go

/lgtm
/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 26, 2020

[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

Details 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

@aojea
Copy link
Copy Markdown
Member

aojea commented Oct 26, 2020

Fantastic job 🥇

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 26, 2020 •
edited
Loading

@khenidak: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-iptables-azure-dualstack 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-iptables-azure-dualstack
pull-kubernetes-e2e-azure-dualstack 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-azure-dualstack
pull-kubernetes-e2e-kind-dual-canary 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-kind-dual-canary
pull-kubernetes-e2e-kind-ipvs-dual-canary 11cfd5e0499d8a65b1e7fba0bc454c0ae930baf9 link /test pull-kubernetes-e2e-kind-ipvs-dual-canary
pull-kubernetes-e2e-gci-gce-ipvs 14b3451 link /test pull-kubernetes-e2e-gci-gce-ipvs

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@aojea
Copy link
Copy Markdown
Member

aojea commented Oct 26, 2020

/retest
I saw this failure before, I don' t think is related

Kubernetes e2e suite: [sig-network] Services should be able to change the type from ClusterIP to ExternalName [Conformance] expand_more

@dcbw
Copy link
Copy Markdown
Contributor

dcbw commented Oct 26, 2020

@khenidak fantastic. Now part 2 of X is complete!

@khenidak
Copy link
Copy Markdown
Contributor Author

khenidak commented Oct 26, 2020

@dcbw true. We are in a much better shape than before. Second time a charm ;-)

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Oct 27, 2020

congrats!

I noticed the API testdata changed since the alpha ipFamily field was dropped. Would it make sense to leave a tombstone in place in the v1 API type to leave a hint to future civilizations not to reuse the ipFamily field or 15 protobuf tag? e.g.

diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go
index bd67349d957..1e21f17328c 100644
--- a/staging/src/k8s.io/api/core/v1/types.go
+++ b/staging/src/k8s.io/api/core/v1/types.go
@@ -4142,6 +4142,9 @@ type ServiceSpec struct {
 	// +optional
 	SessionAffinityConfig *SessionAffinityConfig `json:"sessionAffinityConfig,omitempty" protobuf:"bytes,14,opt,name=sessionAffinityConfig"`
 
+	// IPFamily is tombstoned to show why 15 is a reserved protobuf tag.
+	// IPFamily *IPFamily `json:"ipFamily,omitempty" protobuf:"bytes,15,opt,name=ipFamily,Configcasttype=IPFamily"`
+
 	// IPFamilies identifies all the IPFamilies assigned for this Service. If a value
 	// was not provided for IPFamilies it will be defaulted based on the cluster
 	// configuration and the value of service.spec.ipFamilyPolicy. A maximum of two

@aojea aojea mentioned this pull request Oct 27, 2020
Merged
@khenidak khenidak mentioned this pull request Oct 27, 2020
Merged
@khenidak
Copy link
Copy Markdown
Contributor Author

khenidak commented Oct 27, 2020

@liggitt #95924

@github-actions github-actions Bot mentioned this pull request Nov 3, 2020
Open
@wilsonehusin wilsonehusin mentioned this pull request Dec 3, 2020
Merged
@Shashankft9 Shashankft9 mentioned this pull request Dec 10, 2020
Closed
@xmudrii xmudrii mentioned this pull request Jan 26, 2021
Closed
This was referenced Mar 18, 2021
Merged
Closed
@xliuxu xliuxu mentioned this pull request May 25, 2021
Merged
This was referenced Sep 29, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@thockin thockin thockin requested changes

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@lavalamp lavalamp Awaiting requested review from lavalamp

@aramase aramase Awaiting requested review from aramase

@dcbw dcbw Awaiting requested review from dcbw

+8 more reviewers

@danwinship danwinship danwinship left review comments

@anfernee anfernee anfernee left review comments

@robscott robscott robscott left review comments

@jayunit100 jayunit100 jayunit100 left review comments

@bowei bowei bowei left review comments

@aojea aojea aojea left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@sftim sftim sftim left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@thockin thockin

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/ipvs area/kubectl area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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/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/cli Categorizes an issue or PR as relevant to SIG CLI. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

v1.20

Development

Successfully merging this pull request may close these issues.