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

using configmap to configure calico cni config #10177

Merged
merged 1 commit into from
Jun 30, 2023
Merged

using configmap to configure calico cni config #10177

merged 1 commit into from
Jun 30, 2023

Conversation

cyclinder
Copy link
Contributor

@cyclinder cyclinder commented Jun 1, 2023 •
edited by floryut
Loading

Signed-off-by: cyclinder qifeng.guo@daocloud.io

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

using configmap to configure calico cni config

Which issue(s) this PR fixes:

Fixes #10093

Special notes for your reviewer:

https://github.com/projectcalico/calico/blob/aff70be8ba03e1d8b2ea3b35372afa3910717ea4/manifests/calico.yaml#L59

Does this PR introduce a user-facing change?:

[Calico] Use configmap to configure calico cni config

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 1, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 1, 2023
@cyclinder
Copy link
Contributor Author

cyclinder commented Jun 2, 2023 •
edited
Loading

packet_debian11-calico-upgrade test failed, It looks like this job used tag v2.22.0 to create the cluster and used this PR's branch to update calico, causing the test to fail. Should we all be creating clusters and updating calico based on the current branch?

@cyclinder cyclinder force-pushed the calico_configmap branch 3 times, most recently from 25d401d to 997648f Compare June 9, 2023 06:59
@cyclinder
Copy link
Contributor Author

Could you please take a look? Thanks :)

/cc @floryut @oomichi

floryut
floryut reviewed Jun 22, 2023
View reviewed changes
roles/network_plugin/calico/templates/calico-node.yml.j2 Outdated
@@ -223,6 +226,8 @@ spec:
# # Configure the IP Pool from which Pod IPs will be chosen.
# - name: CALICO_IPV4POOL_CIDR
# value: "{{ calico_pool_cidr | default(kube_pods_subnet) }}"
- name: NO_DEFAULT_POOLS
Copy link
Member

@floryut floryut Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this one a side change or is it normal ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calico extracts the pod_subnet from the configmap: kubeadm-config and creates a default IP pool (provided the variable CALICO_IPV4POOL_CIDR is not empty, it is empty in kubespray). see https://github.com/projectcalico/calico/blob/aff70be8ba03e1d8b2ea3b35372afa3910717ea4/node/pkg/lifecycle/startup/startup.go#L745

But kubespray does not create a default IP pool in this way, by creating it manually. see

kubespray/roles/network_plugin/calico/tasks/install.yml

Line 244 in a1521dc

- name: Calico | Configure calico network pool
. So if so, should we make sure that calico doesn't create a default IP pool?

I'm not sure, unless I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, the names of the default calico's IPPools created by kubespray are default-pool and default-pool-v6. It would be nice if we are aligned with the calico community.

https://github.com/projectcalico/calico/blob/aff70be8ba03e1d8b2ea3b35372afa3910717ea4/node/pkg/lifecycle/startup/startup.go#L58)

MrFreezeex
MrFreezeex approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@MrFreezeex MrFreezeex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I checked with upstream yaml seems very close to what they have indeed 👍.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@cyclinder
Copy link
Contributor Author

thanks @MrFreezeex

I removed NO_DEFAULT_POOLS in the latest changes, I think the change should be discussed in another PR.

@MrFreezeex
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@cyclinder
using configmap to configure calico cni config
62f30a3 
Signed-off-by: cyclinder qifeng.guo@daocloud.io
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2023
@MrFreezeex
Copy link
Member

Thanks for the rebase :p 🙏
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2023
@cyclinder
Copy link
Contributor Author

/cc @floryut

Could you please take a look?

floryut
floryut approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

@cyclinder Looks good indeed! Thank you

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyclinder, floryut, MrFreezeex

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2023
@floryut floryut added kind/network Network section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jun 30, 2023
@k8s-ci-robot k8s-ci-robot merged commit 4f85b75 into kubernetes-sigs:master Jun 30, 2023
@yankay yankay mentioned this pull request Aug 24, 2023
Closed
This was referenced Sep 14, 2023
Closed
Merged
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
@cyclinder @pedromcpedro
using configmap to configure calico cni config (kubernetes-sigs#10177)
b389728 
Signed-off-by: cyclinder qifeng.guo@daocloud.io

Signed-off-by: cyclinder qifeng.guo@daocloud.io
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrFreezeex MrFreezeex MrFreezeex approved these changes

@floryut floryut floryut approved these changes

@holmsten holmsten Awaiting requested review from holmsten

@yankay yankay Awaiting requested review from yankay

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@MrFreezeex MrFreezeex

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. kind/network Network section in the release note 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using configmap config calico config without calico.conflist.template
4 participants
@cyclinder @MrFreezeex @k8s-ci-robot @floryut