Disable podCIDR allocation from control-plane when using calico #10639

VannTen · 2023-11-22T21:18:53Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Calico does not use the .spec.podCIDR field for its IP address
management.
Furthermore, it can false positives from the kube controller manager if
kube_network_node_prefix and calico_pool_blocksize are unaligned, which
is the case with the default shipped by kubespray.

If the subnets obtained from using kube_network_node_prefix are bigger,
this would result at some point in the control plane thinking it does
not have subnets left for a new node, while calico will work without
problems.

some cleanup on variables (see second commit message for details).

Which issue(s) this PR fixes:

Not in this repo, but it avoids the issue described here projectcalico/calico#7722
Closes #9843
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kube-controller-manager will no longer assign pod CIDRs to cluster nodes when using calico (with its default IPAM, calico_ipam_host_local now has a default value of `false`) [⚠️ NOTE action required: users using a non-true value for calico_ipam_host_local will need to change it to `true`]

k8s-ci-robot · 2023-11-22T21:19:03Z

Hi @VannTen. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

yankay · 2023-11-23T01:42:00Z

/ok-to-test

yankay · 2023-11-23T01:59:24Z

Thanks @VannTen

The CI has been repaired, would you please repush it to trige the CI agiain :-)

VannTen · 2023-11-23T08:32:44Z

/retest
(Thanks for the CI :o/)
Look like that command does not work. :/

VannTen · 2023-11-23T10:27:48Z

Looks like the CI broke again. Could it be that too many PR pipelines at once tend to cause some overload and break the "docker+machine" executor ?

yankay · 2023-11-24T02:42:53Z

Looks like the CI broke again. Could it be that too many PR pipelines at once tend to cause some overload and break the "docker+machine" executor ?

HI @VannTen

The CI seems has been fixed: https://github.com/kubernetes-sigs/kubespray/pull/10639/checks?check_run_id=18979639785 :-)

Calico does not use the .spec.podCIDR field for its IP address management. Furthermore, it can false positives from the kube controller manager if kube_network_node_prefix and calico_pool_blocksize are unaligned, which is the case with the default shipped by kubespray. If the subnets obtained from using kube_network_node_prefix are bigger, this would result at some point in the control plane thinking it does not have subnets left for a new node, while calico will work without problems. Explicitely set a default value of false for calico_ipam_host_local to facilitate its use in templates.

They have different semantics: kube_network_node_prefix is intended to be the size of the subnet for all pods on a node, while there can be more than on calico block of the specified size (they are allocated on demand). Besides, this commit does not actually change anything, because the current code is buggy: we don't ever default to kube_network_node_prefix, since the variable is defined in the role defaults.

VannTen · 2023-11-25T14:33:02Z

Relevant : tigera/operator#2712

cyclinder · 2023-11-27T02:16:27Z

roles/network_plugin/calico/tasks/install.yml

@@ -223,7 +223,7 @@
              "name": "{{ calico_pool_name }}",
            },
            "spec": {
-              "blockSize": {{ calico_pool_blocksize | default(kube_network_node_prefix) }},
+              "blockSize": {{ calico_pool_blocksize }},


default(26) ?

That's already in defaults/main.yml, I'd rather avoid duplicating.

cyclinder · 2023-11-27T02:24:22Z

roles/kubernetes/control-plane/templates/kubeadm-config.v1beta3.yaml.j2

@@ -292,11 +292,15 @@ controllerManager:
    cluster-cidr: "{{ kube_pods_subnet }}{{ ',' + kube_pods_subnet_ipv6 if enable_dual_stack_networks else '' }}"
 {% endif %}
    service-cluster-ip-range: "{{ kube_service_addresses }}{{ ',' + kube_service_addresses_ipv6 if enable_dual_stack_networks else '' }}"
+{% if kube_network_plugin is defined and kube_network_plugin == "calico" and not calico_ipam_host_local %}
+    allocate-node-cidrs: "false"


Do we really need this? The default value is true. I believe it won't affect the operation of calico-ipam, as calico-ipam does not allocate addresses from node.PodCIDR; it allocates from blocks it creates on its own. These two are not correlated.

It does not affect calico, it affects kube-controller-manager.
The current kubespray defaults are kube_network_node_prefix = 24 and calico_pool_blocksize = 26.
We faced the following situation during upgrade, ( described in projectcalico/calico#7722).
While calico is fine, the controller-manager thinks it has no more cidrs to allocate, and give the node the CIDRNotAvailable condition.

VannTen · 2023-12-07T10:27:09Z

/cc @EppO @cyclinder @mzaian
Pinging reviewers as this is pretty important for big calico clusters

cyclinder · 2023-12-07T13:43:15Z

sorry for delay, it looks lgtm for now, need ack for other reviewers.

mzaian

Thanks @VannTen

/lgtm

k8s-ci-robot · 2023-12-12T13:28:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mzaian, VannTen

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:

~~OWNERS~~ [mzaian]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…rnetes-sigs#10639) * Disable control plane allocating podCIDR for nodes when using calico Calico does not use the .spec.podCIDR field for its IP address management. Furthermore, it can false positives from the kube controller manager if kube_network_node_prefix and calico_pool_blocksize are unaligned, which is the case with the default shipped by kubespray. If the subnets obtained from using kube_network_node_prefix are bigger, this would result at some point in the control plane thinking it does not have subnets left for a new node, while calico will work without problems. Explicitely set a default value of false for calico_ipam_host_local to facilitate its use in templates. * Don't default to kube_network_node_prefix for calico_pool_blocksize They have different semantics: kube_network_node_prefix is intended to be the size of the subnet for all pods on a node, while there can be more than on calico block of the specified size (they are allocated on demand). Besides, this commit does not actually change anything, because the current code is buggy: we don't ever default to kube_network_node_prefix, since the variable is defined in the role defaults.

…) (#10715) * Disable control plane allocating podCIDR for nodes when using calico Calico does not use the .spec.podCIDR field for its IP address management. Furthermore, it can false positives from the kube controller manager if kube_network_node_prefix and calico_pool_blocksize are unaligned, which is the case with the default shipped by kubespray. If the subnets obtained from using kube_network_node_prefix are bigger, this would result at some point in the control plane thinking it does not have subnets left for a new node, while calico will work without problems. Explicitely set a default value of false for calico_ipam_host_local to facilitate its use in templates. * Don't default to kube_network_node_prefix for calico_pool_blocksize They have different semantics: kube_network_node_prefix is intended to be the size of the subnet for all pods on a node, while there can be more than on calico block of the specified size (they are allocated on demand). Besides, this commit does not actually change anything, because the current code is buggy: we don't ever default to kube_network_node_prefix, since the variable is defined in the role defaults.

…rnetes-sigs#10639) * Disable control plane allocating podCIDR for nodes when using calico Calico does not use the .spec.podCIDR field for its IP address management. Furthermore, it can false positives from the kube controller manager if kube_network_node_prefix and calico_pool_blocksize are unaligned, which is the case with the default shipped by kubespray. If the subnets obtained from using kube_network_node_prefix are bigger, this would result at some point in the control plane thinking it does not have subnets left for a new node, while calico will work without problems. Explicitely set a default value of false for calico_ipam_host_local to facilitate its use in templates. * Don't default to kube_network_node_prefix for calico_pool_blocksize They have different semantics: kube_network_node_prefix is intended to be the size of the subnet for all pods on a node, while there can be more than on calico block of the specified size (they are allocated on demand). Besides, this commit does not actually change anything, because the current code is buggy: we don't ever default to kube_network_node_prefix, since the variable is defined in the role defaults.

rptaylor · 2025-01-28T23:59:06Z

The phrasing of the release note didn't really make sense to me (our current value is undefined, aka non-true, so why would we need to change it to true?). Then I had a closer look at the previous behaviour of calico_ipam_host_local:

./roles/network_plugin/calico/templates/cni-calico.conflist.j2:{% if calico_ipam_host_local is defined %}

It had the surprising and unusual result that if calico_ipam_host_local was defined as any value, even false (!!), it would enable the host local IPAM mode.

With that in mind it makes sense to interpret the release note as "users who were relying on enabling host local mode by using a non-true value for calico_ipam_host_local will need to explicitly change it to true to keep it enabled`

Anyway thanks for fixing this!

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 22, 2023

k8s-ci-robot requested review from EppO and mzaian November 22, 2023 21:18

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 22, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 22, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2023

VannTen closed this Nov 23, 2023

VannTen reopened this Nov 23, 2023

VannTen force-pushed the fix/calica/dont_allocate_cidr branch 2 times, most recently from 96fe1b6 to daee34d Compare November 23, 2023 10:12

VannTen force-pushed the fix/calica/dont_allocate_cidr branch from daee34d to 259755f Compare November 23, 2023 20:53

VannTen force-pushed the fix/calica/dont_allocate_cidr branch 5 times, most recently from 00086d4 to c323567 Compare November 25, 2023 13:05

VannTen added 2 commits November 25, 2023 15:29

VannTen force-pushed the fix/calica/dont_allocate_cidr branch from c323567 to be555fd Compare November 25, 2023 14:29

cyclinder reviewed Nov 27, 2023

View reviewed changes

VannTen requested a review from cyclinder November 28, 2023 13:24

mzaian approved these changes Dec 12, 2023

View reviewed changes

k8s-ci-robot assigned mzaian Dec 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2023

k8s-ci-robot merged commit 8abf49a into kubernetes-sigs:master Dec 12, 2023
59 checks passed

VannTen mentioned this pull request Dec 12, 2023

[2.23] backport: disable podCIDR allocation from control-plane when using calico #10715

Merged

yankay mentioned this pull request Dec 15, 2023

Release Proposal v2.24 #10720

Closed

VannTen mentioned this pull request Jan 12, 2024

Release Proposal v2.23.2 #10792

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable podCIDR allocation from control-plane when using calico #10639

Disable podCIDR allocation from control-plane when using calico #10639

VannTen commented Nov 22, 2023 •

edited by floryut

Loading

k8s-ci-robot commented Nov 22, 2023

yankay commented Nov 23, 2023

yankay commented Nov 23, 2023

VannTen commented Nov 23, 2023 •

edited

Loading

VannTen commented Nov 23, 2023

yankay commented Nov 24, 2023

VannTen commented Nov 25, 2023

cyclinder Nov 27, 2023

VannTen Nov 27, 2023

cyclinder Nov 27, 2023

VannTen Nov 27, 2023

VannTen commented Dec 7, 2023

cyclinder commented Dec 7, 2023

mzaian left a comment

k8s-ci-robot commented Dec 12, 2023

rptaylor commented Jan 28, 2025

Disable podCIDR allocation from control-plane when using calico #10639

Disable podCIDR allocation from control-plane when using calico #10639

Conversation

VannTen commented Nov 22, 2023 • edited by floryut Loading

k8s-ci-robot commented Nov 22, 2023

yankay commented Nov 23, 2023

yankay commented Nov 23, 2023

VannTen commented Nov 23, 2023 • edited Loading

VannTen commented Nov 23, 2023

yankay commented Nov 24, 2023

VannTen commented Nov 25, 2023

cyclinder Nov 27, 2023

Choose a reason for hiding this comment

VannTen Nov 27, 2023

Choose a reason for hiding this comment

cyclinder Nov 27, 2023

Choose a reason for hiding this comment

VannTen Nov 27, 2023

Choose a reason for hiding this comment

VannTen commented Dec 7, 2023

cyclinder commented Dec 7, 2023

mzaian left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 12, 2023

rptaylor commented Jan 28, 2025

VannTen commented Nov 22, 2023 •

edited by floryut

Loading

VannTen commented Nov 23, 2023 •

edited

Loading