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.

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

Dual stack follow-ups #2173

Merged
merged 3 commits into from
Apr 1, 2021
Merged

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Apr 1, 2021

Fix documentation and workaround 1.20 versions to enable the feature gate

This needs an image bump for having the kindnet dualstack versions, so pods can get dual-stack addresses

Fixes: #2172

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 1, 2021
@k8s-ci-robot k8s-ci-robot requested review from krzyzacy and munnerz April 1, 2021 12:50
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2021
@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

/hold
it needs images bumps

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2021
IPv6DualStack: true
`
if len(cfg.KubeadmConfigPatches) > 0 {
cfg.KubeadmConfigPatches = append(cfg.KubeadmConfigPatches, dualPatch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this ok, or should we use Json6902?

Copy link
Member

Choose a reason for hiding this comment

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

There's a much easier way to default this when we generate the base kind config which also respects the user patch applied on top. See how we handled it for the PV feature gatr

}

// for 1.20 we have to explicitly enable the feature gate
// we can patch kubeadm here so we avoid to plumb this hack further
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubeadm adds the corresponding feature gates flags to the other components, controller-manager, apiserver, kube-proxy ... and validates that podsubnet and serviceSubnet are correct ... so we don't need to set the feature flags

@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

/assign @BenTheElder
/cc @aspenmesh @howardjohn

@k8s-ci-robot
Copy link
Contributor

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

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

In response to this:

/assign @BenTheElder
/cc @aspenmesh @howardjohn

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.

@aojea aojea force-pushed the dual_followup branch 2 times, most recently from b24f7c0 to fc07f05 Compare April 1, 2021 13:46
@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

after updated the kindnet image

kubectl set image ds/kindnet kindnet-cni=kindest/kindnetd:v20210326-1e038dc5 -n kube-system

and delete the old kindnet pods that should be crashlooping because it can't handle dualstack correctly

I0401 13:49:37.797835       1 main.go:185] handling current node
panic: Can't synchronize rules after 3 attempts: running [/usr/sbin/iptables -t nat -C KIND-MASQ-AGENT -d 10.244.0.0/16,fd00:10:244::/56 -j RETURN -m comment --comment kind-masq-agent: local traffic is not subject to MASQUERADE --wait]: exit status 2: iptables v1.8.5 (legacy): invalid mask `56' specified
Try `iptables -h' or 'iptables --help' for more information.

I have some problems because the apiserver fails to validate everything after this error

E0401 12:44:27.426813       1 controller.go:152] Unable to remove old endpoints from kubernetes service: StorageError: key not found, Code: 1, Key: /registry/masterleases/172.18.0.2, ResourceVersion: 0, AdditionalErrorMsg: 
E0401 12:44:27.435678       1 fieldmanager.go:186] [SHOULD NOT HAPPEN] failed to update managedFields for /, Kind=: failed to convert new object (/v1, Kind=Node) to smd typed: .status.addresses: duplicate entries for key [type="InternalIP"]

@liggitt can you give me some hints to debug this further?

EDIT

this is with 1.20.2

@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

/retest

@liggitt
Copy link
Contributor

liggitt commented Apr 1, 2021

E0401 12:44:27.435678       1 fieldmanager.go:186] [SHOULD NOT HAPPEN] failed to update managedFields for /, Kind=: failed to convert new object (/v1, Kind=Node) to smd typed: .status.addresses: duplicate entries for key [type="InternalIP"]

@liggitt can you give me some hints to debug this further?

sounds like kubernetes/kubernetes#88182

cc @apelisse

@liggitt
Copy link
Contributor

liggitt commented Apr 1, 2021

duplicate entries for key [type="InternalIP"]

Looks like a retread of kubernetes/kubernetes#79391 (comment), but now with server-side apply.

@liggitt
Copy link
Contributor

liggitt commented Apr 1, 2021

If we can have existing objects with duplicate items here (and it sounds like we can), the managedFields update cannot break update of those objects. Complaining on Apply requests is in-bounds (though it sounds like the merge key selected for the field may be wrong in the first place if we expect to have multiple entries of the same address type with different IP families)

@apelisse
Copy link

apelisse commented Apr 1, 2021

Yeah, I'd be very surprised if it does, I think that's a red-herring.

{{< /codeFromInline >}}

##### Dual Stack clusters
You can run dual stack clusters using `kind`, on kubernetes versions 1.20+.
Copy link
Member

Choose a reason for hiding this comment

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

Please note that this requires 0.11.0+

@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

@liggitt @apelisse indeed it was a redherring, thanks for your quick response, I can't explain why by it was caused by a wrong kubeadm configuration, there was also rbac errors on the scheduler and the controller manager.

@BenTheElder PTAL, it needs the new kindnet image in order to work

kubectl set image ds/kindnet kindnet-cni=kindest/kindnetd:v20210326-1e038dc5 -n kube-system
$ kubectl apply -f https://gist.githubusercontent.com/aojea/90768935ab71cb31950b6a13078a7e92/raw/99ceac308f2b2658c7313198a39fbe24b155ae68/dual-stack.yaml
$ kubectl get endpointslices -o wide
NAME                            ADDRESSTYPE   PORTS   ENDPOINTS                       AGE
kubernetes                      IPv4          6443    172.18.0.2                      4m24s
my-service-prefer-dual-jswjr    IPv4          80      10.244.0.6,10.244.0.5           45s
my-service-prefer-dual-vvkdt    IPv6          80      fd00:10:244::3,fd00:10:244::2   45s
my-service-require-dual-4hqq2   IPv6          80      fd00:10:244::3,fd00:10:244::2   45s
my-service-require-dual-75hk8   IPv4          80      10.244.0.6,10.244.0.5           45s
my-service-v4-429wd             IPv4          80      10.244.0.6,10.244.0.5           45s
my-service-v6-crtqs             IPv6          80      fd00:10:244::3,fd00:10:244::2   45s

@aojea
Copy link
Contributor Author

aojea commented Apr 1, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2021
// dual-stack is only supported in 1.20+
// TODO: remove this when 1.20 is EOL or we no longer support
// dual-stack for 1.20 in KIND
if ver.LessThan(version.MustParseSemantic("v1.21.0")) &&
Copy link
Member

Choose a reason for hiding this comment

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

we should really use the version for the PR that enabled it by default so it works with arbitrary commits 🙃

if you checkout kubernetes master then the commit you can grab the version.

Copy link
Member

Choose a reason for hiding this comment

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

can be a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the kubeadm one kubernetes/kubernetes#99294

and this the kubernetes one kubernetes/kubernetes#98969

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the must parse semantic doesn't accept versions with letters IIRC

@BenTheElder
Copy link
Member

@aojea upgrade the kindnetd image / manifest? you have access 🙃

@BenTheElder
Copy link
Member

/lgtm
/approve
can be handled in another PR, along with refining the versions

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dualstack should automatically enable feature gate for < 1.21
5 participants