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

Config option to set the default IP Pool #28818

Merged
merged 1 commit into from Nov 1, 2023
Merged

Config option to set the default IP Pool #28818

merged 1 commit into from Nov 1, 2023

Conversation

chaunceyjiang
Copy link
Member

Config option to customize the default IP Pool when using MultiPool

Fixes: #27131

Please ensure your pull request adheres to the following guidelines:

Fixes: #issue-number

 Config option to customize the default IP Pool when using MultiPool

@chaunceyjiang chaunceyjiang requested review from a team as code owners October 26, 2023 16:06
@maintainer-s-little-helper 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 Oct 26, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 26, 2023
@tklauser tklauser added the area/multipool Affects Multi-Pool IPAM label Oct 26, 2023
tklauser
tklauser requested changes Oct 26, 2023
View reviewed changes
pkg/ipam/types.go Outdated Show resolved Hide resolved
@tklauser tklauser requested a review from gandro October 26, 2023 17:58
@tklauser tklauser added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/ipam Impacts IP address management functionality. labels Oct 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 26, 2023
@tklauser
Copy link
Member

Requested a review from @gandro given he is currently working on addressing some tech-debt in the multi-pool IPAM code and this might conflict with his work.

@tommyp1ckles
Copy link
Contributor

tommyp1ckles commented Oct 26, 2023 •
edited

Nice improvement, changes generally make sense to me

@gandro Should this have some sort of validation that the pools in question exist?

@chaunceyjiang chaunceyjiang requested a review from a team as a code owner October 27, 2023 05:36
gandro
gandro reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

This is awesome work! Looks excellent to me overall. I've left some feedback regarding the pre-allocation

pkg/defaults/defaults.go Outdated Show resolved Hide resolved
gandro
gandro reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the update! One last minor nit

pkg/defaults/defaults.go Outdated Show resolved Hide resolved
gandro
gandro reviewed Oct 30, 2023
View reviewed changes
daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
@gandro
Copy link
Member

gandro commented Oct 30, 2023

/test

@chaunceyjiang
Copy link
Member Author

Error: buildx failed with: ERROR: failed to solve: docker.io/library/alpine:3.18.4@sha256:eece025e432126ce23f223450a0326fbebde39cdf496a85d8c016293fc851978: failed to do request: Head "https://registry-1.docker.io/v2/library/alpine/manifests/sha256:eece025e432126ce23f223450a0326fbebde39cdf496a85d8c016293fc851978": net/http: TLS handshake timeout

This is a GitHub error.

@chaunceyjiang
Copy link
Member Author

It seems GitHub is having troubles. Let me close and re-open the PR, that should re-trigger the workflows.

@chaunceyjiang chaunceyjiang reopened this Oct 31, 2023
@tommyp1ckles
Copy link
Contributor

This is a GitHub error.

Test image build may have failed causing a bunch of cascading test failures. Iet's retry.

@tommyp1ckles
Copy link
Contributor

/test

@gandro
Copy link
Member

gandro commented Oct 31, 2023 •
edited

tklauser
tklauser reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM, only some small spelling nits.

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
pkg/defaults/defaults.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
@chaunceyjiang
Config option to set the default IP Pool
1b846f0 
Config option to customize the default IP Pool when using MultiPool

Fixes: cilium#27131

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@gandro
Copy link
Member

gandro commented Oct 31, 2023 •
edited

/test

tklauser
tklauser approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 1, 2023
@gandro gandro merged commit e494302 into cilium:main Nov 1, 2023
62 checks passed
@chaunceyjiang chaunceyjiang deleted the default-ip-pool branch November 2, 2023 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro left review comments

@tklauser tklauser tklauser approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
area/ipam Impacts IP address management functionality. area/multipool Affects Multi-Pool IPAM kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Config option to customize the default IP Pool when using MultiPool
4 participants
@chaunceyjiang @tklauser @tommyp1ckles @gandro