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

Use calico_pool_blocksize from cluster when existing #10516

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Use calico_pool_blocksize from cluster when existing #10516

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions roles/network_plugin/calico/tasks/install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,20 @@
}

- name: Calico | Process calico network pool
set_fact:
_calico_pool: "{{ _calico_pool_cmd.stdout | from_json | combine(_calico_pool, recursive=True) }}"
when:
- _calico_pool_cmd is success
block:
- name: Calico | Get current calico network pool blocksize
set_fact:
_calico_blocksize: >
{
"spec": {
"blockSize": {{ (_calico_pool_cmd.stdout | from_json).spec.blockSize }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@VannTen VannTen Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ^^.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}
- name: Calico | Merge calico network pool
set_fact:
_calico_pool: "{{ _calico_pool_cmd.stdout | from_json | combine(_calico_pool, _calico_blocksize, recursive=True) }}"

- name: Calico | Configure calico network pool
command:
Expand Down Expand Up @@ -273,10 +283,20 @@
}

- name: Calico | Process calico ipv6 network pool
set_fact:
_calico_pool_ipv6: "{{ _calico_pool_ipv6_cmd.stdout | from_json | combine(_calico_pool_ipv6, recursive=True) }}"
when:
- _calico_pool_ipv6_cmd is success
block:
- name: Calico | Get current calico ipv6 network pool blocksize
set_fact:
_calico_blocksize_ipv6: >
{
"spec": {
"blockSize": {{ (_calico_pool_ipv6_cmd.stdout | from_json).spec.blockSize }}
}
}
- name: Calico | Merge calico ipv6 network pool
set_fact:
_calico_pool_ipv6: "{{ _calico_pool_ipv6_cmd.stdout | from_json | combine(_calico_pool_ipv6, _calico_blocksize_ipv6, recursive=True) }}"

- name: Calico | Configure calico ipv6 network pool
command:
Expand Down