pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes #23048

mvisonneau · 2023-01-11T17:48:59Z

Currently, when using Cilium with IPAM in AWS ENI, Azure & AlibabaCloud methods. A random podCIDR block is being generated when the CiliumNode is created although is not being leveraged.

[...]
spec:
  ipam:
    podCIDRs:
    - 10.194.0.0/16 # << this one
    pool:
      172.16.5.192:
        resource: eni-0f2cda9c5fe02382b
      172.16.5.193:
        resource: eni-0f2cda9c5fe02382b
      172.16.5.194:
        resource: eni-0f2cda9c5fe02382b

To avoid operators confusion and make it easier to troubleshoot, I felt it could be interesting propose this change which omits the generation of this CIDR block.

[...]
spec:
  ipam:
    pool:
      172.16.5.192:
        resource: eni-0f2cda9c5fe02382b
      172.16.5.193:
        resource: eni-0f2cda9c5fe02382b
      172.16.5.194:
        resource: eni-0f2cda9c5fe02382b

Fixes #9409

pkg/node/address.go

thorn3r · 2023-01-11T19:26:16Z

thanks for the PR @mvisonneau! looks like the checkpatch test is failing because the commit title is too long. Mind amending that to shorten it a bit? I'll kick off the rest of the test suite

thorn3r · 2023-01-11T19:26:23Z

/test

mvisonneau · 2023-01-12T08:06:11Z

Thanks for the review @thorn3r! I updated the commit message.

thorn3r · 2023-01-13T16:07:17Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #22019 (94.82% similarity)

thorn3r

LGTM! seems like we hit an already tracked flake

jaffcheng

Thank you! I think this should close #9409

pkg/node/address.go

maintainer-s-little-helper · 2023-01-20T09:13:41Z

Commit 608dbb8acfd7c6ebeac4204589ccd98beaa459e3 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

jaffcheng · 2023-01-20T13:22:07Z

Code changes LGTM! I'm not familiar with cilium CI workflows, looks like the checkpatch test fails with the current commit title, and maybe we should remove the second commit.

Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 81)

Currently, when using Cilium with AWS ENI, Azure & AlibabaCloud methods. A random podCIDR block is being generated when the CiliumNode is created although is not being leveraged. ```yaml [...] spec: ipam: podCIDRs: - 10.194.0.0/16 # << this one pool: 172.16.5.192: resource: eni-0f2cda9c5fe02382b 172.16.5.193: resource: eni-0f2cda9c5fe02382b 172.16.5.194: resource: eni-0f2cda9c5fe02382b ``` To avoid operators confusion and make it easier to troubleshoot, I felt it could be interesting propose this change which omits the generation of this CIDR block. ```yaml [...] spec: ipam: pool: 172.16.5.192: resource: eni-0f2cda9c5fe02382b 172.16.5.193: resource: eni-0f2cda9c5fe02382b 172.16.5.194: resource: eni-0f2cda9c5fe02382b ``` Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>

mvisonneau · 2023-01-23T10:35:25Z

👋 thanks for the review @jaffcheng. Not sure how this merge commit ended up there, I removed it and also shorten the message of the relevant one!

jaffcheng · 2023-01-30T10:16:21Z

pkg/node/address.go

@@ -423,6 +425,12 @@ func AutoComplete() error {

 	InitDefaultPrefix(option.Config.DirectRoutingDevice)


@mvisonneau I'm a bit lost about how this work, could you share more on that? I assume the cidr is generated in InitDefaultPrefix, but it's not skipped.
https://github.com/cilium/cilium/pull/23048/files#diff-9143180298a1f5ba5dc10fd98d87945d1381ef45b7347a639f85e13e84285ca2L424

absolutely, I did not find it useful to skip the InitDefaultPrefix function call during my tests. I assumed that the allocation was happening later (L633 &/or L644).

As we return early (L431) it seems to be sufficient. Not sure about the impact of moving the return logic before the InitDefaultPrefix call but happy to try it out if you want/prefer.

Thanks for clarifying.
Looks like there are two places calling InitDefaultPrefix:

cilium/pkg/node/address.go

Line 426 in 5ff3d72

InitDefaultPrefix(option.Config.DirectRoutingDevice)

cilium/daemon/cmd/daemon_main.go

Line 1404 in 5ff3d72

node.InitDefaultPrefix(option.Config.DirectRoutingDevice)

I tried the patch in Alibabacloud mode and found that I have to skip both InitDefaultPrefix calls to omit the podCIDR. Not sure if there are some differences between ENI mode.

christarazi

Thanks for the PR. I don't quite understand why any of these changes are needed. Shouldn't it be as simple as detecting which IPAM mode we need to skip updating the podCIDR field for during the CiliumNode creation? I'd expect such a change to be here.

christarazi · 2023-03-09T01:22:43Z

@mvisonneau Do you plan to continue on this PR?

mvisonneau · 2023-03-09T10:59:49Z

I was yes, I apologize as I haven't been able to prioritize it yet. Feel free to update or make progress onto if necessary though!

github-actions · 2023-04-09T01:53:49Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

aanm · 2023-04-27T14:22:00Z

@mvisonneau ping? 🙂

joestringer · 2023-05-05T17:22:16Z

Thanks for the contribution. I've marked the PR as draft while the feedback is addressed. When the PR is ready for review, you can indicate this by clicking the "Ready for review" button at the bottom of the page.

mvisonneau · 2023-05-31T06:57:44Z

My apologies, I am going to put my head back onto it and will keep you posted!

github-actions · 2023-07-01T02:09:32Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

gandro · 2023-07-06T09:11:48Z

I have pushed a PR that implements Chris' suggestion to not announce the podCIDR for now: #26663

Long term however, I think we do want to fully get rid of the code that auto-generates the podCIDR for all IPAM modes. It's not used by any IPAM mode, so let's just remove that.

github-actions · 2023-08-06T01:46:56Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

christarazi · 2023-08-07T21:14:11Z

@mvisonneau Any update?

github-actions · 2023-09-07T01:45:03Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

jaffcheng · 2023-09-07T03:56:21Z

Looks like the related issue is fixed by #26663 ?

gandro · 2023-09-07T08:16:22Z

Looks like the related issue is fixed by #26663 ?

Yes, that PR fixed the issue for ENI. There is still more work around the use of pod CIDRs in IPAM modes that don't have them (c.f. #27018), but the issue addressed by this PR should be fixed now.

@mvisonneau I'm closing this PR as "obsolete". Please feel free to re-open or ping me if you feel otherwise.

mvisonneau · 2023-09-07T12:53:05Z

Apologies, I had messed up with my email notifications! 🙈 Amazing, thanks, I'll try it out but at first sight it does indeed seem to supersedes it 👍

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 11, 2023

mvisonneau marked this pull request as ready for review January 11, 2023 17:49

mvisonneau requested a review from a team as a code owner January 11, 2023 17:49

mvisonneau requested a review from thorn3r January 11, 2023 17:49

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 11, 2023

thorn3r reviewed Jan 11, 2023

View reviewed changes

pkg/node/address.go Outdated Show resolved Hide resolved

mvisonneau force-pushed the pkg_node_addr_eni_ignore branch from 2b801b2 to 49371a5 Compare January 12, 2023 08:04

mvisonneau changed the title ~~pkg/node: omit the generated podCIDRs from the CiliumNode when running in ENI mode~~ pkg/node: omit the generated spec.ipam.podCIDRs when running in ENI mode Jan 12, 2023

mvisonneau force-pushed the pkg_node_addr_eni_ignore branch from 49371a5 to 99f841b Compare January 12, 2023 08:18

thorn3r added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jan 13, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 13, 2023

maintainer-s-little-helper bot mentioned this pull request Jan 13, 2023

CI: K8sDatapathConfig Iptables Skip conntrack for pod traffic #22019

Closed

thorn3r approved these changes Jan 17, 2023

View reviewed changes

thorn3r requested a review from a team January 17, 2023 18:26

jaffcheng reviewed Jan 18, 2023

View reviewed changes

pkg/node/address.go Outdated Show resolved Hide resolved

thorn3r removed the request for review from a team January 19, 2023 19:22

mvisonneau force-pushed the pkg_node_addr_eni_ignore branch from 99f841b to 14898a0 Compare January 20, 2023 09:11

mvisonneau changed the title ~~pkg/node: omit the generated spec.ipam.podCIDRs when running in ENI mode~~ pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes Jan 20, 2023

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 20, 2023

mvisonneau requested a review from jaffcheng January 20, 2023 09:14

mvisonneau force-pushed the pkg_node_addr_eni_ignore branch from 608dbb8 to 5ff3d72 Compare January 23, 2023 10:33

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 23, 2023

jaffcheng reviewed Jan 30, 2023

View reviewed changes

christarazi requested changes Feb 1, 2023

View reviewed changes

christarazi added kind/enhancement This would improve or streamline existing functionality. sig/ipam IP address management, including cloud IPAM labels Feb 1, 2023

aanm removed their assignment Feb 1, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 9, 2023

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 19, 2023

joestringer marked this pull request as draft May 5, 2023 17:21

gandro mentioned this pull request Jun 28, 2023

IPAM Multi-Pool Follow-ups #25470

Closed

29 tasks

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 1, 2023

christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 3, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 6, 2023

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 8, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 7, 2023

gandro closed this Sep 7, 2023

mvisonneau deleted the pkg_node_addr_eni_ignore branch September 7, 2023 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes #23048

pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes #23048

mvisonneau commented Jan 11, 2023 •

edited

thorn3r commented Jan 11, 2023

thorn3r commented Jan 11, 2023

mvisonneau commented Jan 12, 2023

thorn3r commented Jan 13, 2023 •

edited by maintainer-s-little-helper bot

thorn3r left a comment

jaffcheng left a comment

maintainer-s-little-helper bot commented Jan 20, 2023

jaffcheng commented Jan 20, 2023

mvisonneau commented Jan 23, 2023

jaffcheng Jan 30, 2023

mvisonneau Jan 30, 2023

jaffcheng Jan 31, 2023

christarazi left a comment •

edited

christarazi commented Mar 9, 2023

mvisonneau commented Mar 9, 2023

github-actions bot commented Apr 9, 2023

aanm commented Apr 27, 2023

joestringer commented May 5, 2023

mvisonneau commented May 31, 2023

github-actions bot commented Jul 1, 2023

gandro commented Jul 6, 2023

github-actions bot commented Aug 6, 2023

christarazi commented Aug 7, 2023

github-actions bot commented Sep 7, 2023

jaffcheng commented Sep 7, 2023

gandro commented Sep 7, 2023

mvisonneau commented Sep 7, 2023

		@@ -423,6 +425,12 @@ func AutoComplete() error {

		InitDefaultPrefix(option.Config.DirectRoutingDevice)

pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes #23048

pkg/node: omit the generated spec.ipam.podCIDRs when running with some IPAM modes #23048

Conversation

mvisonneau commented Jan 11, 2023 • edited

thorn3r commented Jan 11, 2023

thorn3r commented Jan 11, 2023

mvisonneau commented Jan 12, 2023

thorn3r commented Jan 13, 2023 • edited by maintainer-s-little-helper bot

thorn3r left a comment

Choose a reason for hiding this comment

jaffcheng left a comment

Choose a reason for hiding this comment

maintainer-s-little-helper bot commented Jan 20, 2023

jaffcheng commented Jan 20, 2023

mvisonneau commented Jan 23, 2023

jaffcheng Jan 30, 2023

Choose a reason for hiding this comment

mvisonneau Jan 30, 2023

Choose a reason for hiding this comment

jaffcheng Jan 31, 2023

Choose a reason for hiding this comment

christarazi left a comment • edited

Choose a reason for hiding this comment

christarazi commented Mar 9, 2023

mvisonneau commented Mar 9, 2023

github-actions bot commented Apr 9, 2023

aanm commented Apr 27, 2023

joestringer commented May 5, 2023

mvisonneau commented May 31, 2023

github-actions bot commented Jul 1, 2023

gandro commented Jul 6, 2023

github-actions bot commented Aug 6, 2023

christarazi commented Aug 7, 2023

github-actions bot commented Sep 7, 2023

jaffcheng commented Sep 7, 2023

gandro commented Sep 7, 2023

mvisonneau commented Sep 7, 2023

mvisonneau commented Jan 11, 2023 •

edited

thorn3r commented Jan 13, 2023 •

edited by maintainer-s-little-helper bot

christarazi left a comment •

edited