Use calico_pool_blocksize from cluster when existing #10516

VannTen · 2023-10-11T14:01:54Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

The blockSize attribute from Calico IPPool resources cannot be changed
once set 1. Consequently, we use the one currently defined when
configuring the existing IPPool, avoiding upgrade errors by trying to
change it.

In particular, this can be useful when calico_pool_blocksize default
changes in kubespray, which would otherwise force users to add an
explicit setting to their inventories.

Special notes for your reviewer:

We faced that problem while upgrading our cluster (to 2.19.1). We have some clusters which have different blockSize (for reasons unclear to me), and while setting the correct value (instead of the default) is not that long, it's still a bit annoying.

Does this PR introduce a user-facing change?:

[calico] Use calico_pool_blocksize from cluster when existing

k8s-ci-robot · 2023-10-11T14:02:05Z

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.

The blockSize attribute from Calico IPPool resources cannot be changed once set [1]. Consequently, we use the one currently defined when configuring the existing IPPool, avoiding upgrade errors by trying to change it. In particular, this can be useful when calico_pool_blocksize default changes in kubespray, which would otherwise force users to add an explicit setting to their inventories. [1]: https://docs.tigera.io/calico/latest/reference/resources/ippool#spec

yankay · 2023-10-15T02:02:16Z

/ok-to-test

yankay · 2023-10-19T02:58:30Z

HI @cyclinder
would you please help to review it :-)

cyclinder

thanks, I think it's a good idea.

cyclinder · 2023-10-19T06:09:00Z

roles/network_plugin/calico/tasks/install.yml

+            _calico_blocksize: >
+              {
+                "spec": {
+                  "blockSize": {{ (_calico_pool_cmd.stdout | from_json).spec.blockSize }}


Do we need also to merge the CIDR? update the CIDR of the existing ippool is dangerous.

I'd say it depends on whether we consider changing the cidr as part of the playbook a valid use-case, and what's the "default history" of the cidr attribute is (I came across this little inconvenience because the blockSize default changed apparently, because those clusters never had an explicit setting in their inventory for that).

Changing the blockSize is definitely out of scope, because it's simply impossible, since it's immutable in the IPPool. The cidr, I don't know.

Overall, I would err on the side on keeping it that way, for minimal changes (which might be breaking someone workflow => can't be the case for blockSize).

I would err on the side on keeping it that way, for minimal changes

Agree, It's ok for me.

In your case, the default blocksize was not used when the cluster was installed, but the default value was still used when upgrading the cluster, right? It makes sense that blocksize is immutable so we don't make it be updated.

Yeah, I think that's how it went. The clusters were installed some years ago (not by me), and I think for some it was the calico default value for blocksize (26) and for others... well I don't really know ^^.

I used to change the blockSize of calico from 24 to 26(#9055) because it was consistent with the upstream of calico. maybe your cluster was created before this PR merged.

cyclinder · 2023-10-19T09:11:40Z

/lgtm

floryut

@VannTen Thank you for the PR 👍

k8s-ci-robot · 2023-10-30T16:29:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, 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~~ [floryut]

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

…#10516) The blockSize attribute from Calico IPPool resources cannot be changed once set [1]. Consequently, we use the one currently defined when configuring the existing IPPool, avoiding upgrade errors by trying to change it. In particular, this can be useful when calico_pool_blocksize default changes in kubespray, which would otherwise force users to add an explicit setting to their inventories. [1]: https://docs.tigera.io/calico/latest/reference/resources/ippool#spec

The blockSize attribute from Calico IPPool resources cannot be changed once set [1]. Consequently, we use the one currently defined when configuring the existing IPPool, avoiding upgrade errors by trying to change it. In particular, this can be useful when calico_pool_blocksize default changes in kubespray, which would otherwise force users to add an explicit setting to their inventories. [1]: https://docs.tigera.io/calico/latest/reference/resources/ippool#spec

…#10516) The blockSize attribute from Calico IPPool resources cannot be changed once set [1]. Consequently, we use the one currently defined when configuring the existing IPPool, avoiding upgrade errors by trying to change it. In particular, this can be useful when calico_pool_blocksize default changes in kubespray, which would otherwise force users to add an explicit setting to their inventories. [1]: https://docs.tigera.io/calico/latest/reference/resources/ippool#spec

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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 Oct 11, 2023

k8s-ci-robot requested review from ErikJiang and yankay October 11, 2023 14:02

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

VannTen force-pushed the fix/calico_ippool_blocksize branch from b72b6c2 to b3e84a6 Compare October 11, 2023 14:53

VannTen force-pushed the fix/calico_ippool_blocksize branch from b3e84a6 to 6439d0c Compare October 13, 2023 07:17

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 Oct 15, 2023

yankay requested a review from cyclinder October 19, 2023 02:58

cyclinder reviewed Oct 19, 2023

View reviewed changes

k8s-ci-robot assigned cyclinder Oct 19, 2023

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

floryut approved these changes Oct 30, 2023

View reviewed changes

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

floryut added kind/network Network section in the release note and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 30, 2023

k8s-ci-robot merged commit 34754cc into kubernetes-sigs:master Oct 30, 2023
59 checks passed

VannTen mentioned this pull request Nov 28, 2023

REQUEST: New membership for VannTen kubernetes/org#4607

Closed

9 tasks

yankay mentioned this pull request Dec 15, 2023

Release Proposal v2.24 #10720

Closed

rptaylor mentioned this pull request Jan 4, 2024

feat: change default blockSize for calico #9055

Merged

cyclinder mentioned this pull request Jan 5, 2024

[Cherry-Pick] Use calico_pool_blocksize from cluster when existing (#10516) #10770

Closed

cyclinder mentioned this pull request Jan 5, 2024

[Cherry-Pick] Use calico_pool_blocksize from cluster when existing (#10516) #10771

Merged

cyclinder mentioned this pull request Jan 5, 2024

[Cherry-Pick] Use calico_pool_blocksize from cluster when existing (#10516) #10772

Merged

This was referenced Feb 5, 2024

Release proposal 2.22.2 #10880

Closed

Release Proposal 2.23.3 #10881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use calico_pool_blocksize from cluster when existing #10516

Use calico_pool_blocksize from cluster when existing #10516

VannTen commented Oct 11, 2023 •

edited by floryut

Loading

k8s-ci-robot commented Oct 11, 2023

yankay commented Oct 15, 2023

yankay commented Oct 19, 2023

cyclinder left a comment

cyclinder Oct 19, 2023

VannTen Oct 19, 2023 •

edited

Loading

cyclinder Oct 19, 2023

VannTen Oct 19, 2023

cyclinder Oct 19, 2023

cyclinder commented Oct 19, 2023

floryut left a comment

k8s-ci-robot commented Oct 30, 2023

Use calico_pool_blocksize from cluster when existing #10516

Use calico_pool_blocksize from cluster when existing #10516

Conversation

VannTen commented Oct 11, 2023 • edited by floryut Loading

k8s-ci-robot commented Oct 11, 2023

yankay commented Oct 15, 2023

yankay commented Oct 19, 2023

cyclinder left a comment

Choose a reason for hiding this comment

cyclinder Oct 19, 2023

Choose a reason for hiding this comment

VannTen Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

cyclinder Oct 19, 2023

Choose a reason for hiding this comment

VannTen Oct 19, 2023

Choose a reason for hiding this comment

cyclinder Oct 19, 2023

Choose a reason for hiding this comment

cyclinder commented Oct 19, 2023

floryut left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 30, 2023

VannTen commented Oct 11, 2023 •

edited by floryut

Loading

VannTen Oct 19, 2023 •

edited

Loading