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.

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

✨ Add support to set allocation_pools for subnet #1847

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

dulek
Copy link
Contributor

@dulek dulek commented Jan 30, 2024

What this PR does / why we need it:
This commit adds API that allows users to set allocations_pools in the subnet created by CAPO. This allows the users to restrict the IP address ranges that will be allocated automatically by OpenStack when creating Machines.

Users can utilize this to reserve addresses for VIPs (virtual IPs) or special nodes that will have predefined addresses and will be created later.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit ccfcb59
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65dcc9fea2e73c000826db1f
😎 Deploy Preview https://deploy-preview-1847--kubernetes-sigs-cluster-api-openstack.netlify.app/print
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2024
@mdbooth
Copy link
Contributor

mdbooth commented Jan 30, 2024

This doesn't feel right. allocationPools feels like a property of a specific created subnet. This is going to break when we (hopefully) do IPI dual-stack.

I was already wondering if NodeCIDRs should contain some kind of SubnetSpec rather than just a CIDR. It was already mentioned that this would allow specifying what kind of IPv6 address allocation to use. I would be much happier if allocationPools was a property of a SubnetSpec.

Thoughts? cc @MaysaMacedo

@EmilienM
Copy link
Contributor

EmilienM commented Jan 30, 2024

IMO life would be beautiful if we converted NodeCIDR into NodeCIDRs which is a list of subnet specs or something.

@mdbooth
Copy link
Contributor

mdbooth commented Jan 30, 2024

Another advantage of defining a SubnetSpec struct is that it would be extensible in v1beta1.

@jichenjc
Copy link
Contributor

This doesn't feel right. allocationPools feels like a property of a specific created subnet. This is going to break when we (hopefully) do IPI dual-stack.

this is useful in my openstack implementation especially 2 networks are in same CIDR with different allocation pool
I think it's not that helpful if we go IPI or everything under CAPO control, but it might be helpful for some customer
is using their existing env for CAPO (those resource under admin's input)

@MaysaMacedo
Copy link
Contributor

This doesn't feel right. allocationPools feels like a property of a specific created subnet. This is going to break when we (hopefully) do IPI dual-stack.

I was already wondering if NodeCIDRs should contain some kind of SubnetSpec rather than just a CIDR. It was already mentioned that this would allow specifying what kind of IPv6 address allocation to use. I would be much happier if allocationPools was a property of a SubnetSpec.

Thoughts? cc @MaysaMacedo

Just sharing what was discussed on the slack thread, having multiple SubnetSpec make sense to me, it would be good to name the field to something more representative of its content.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2024
@dulek
Copy link
Contributor Author

dulek commented Feb 5, 2024

This should be only merged once #1856 is in.

@dulek dulek force-pushed the allocation-pools branch 2 times, most recently from dc8e49b to cf8e814 Compare February 6, 2024 09:12
@EmilienM
Copy link
Contributor

please rebase on main

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2024
@mdbooth
Copy link
Contributor

mdbooth commented Feb 15, 2024

@dulek I rebased this for you, btw. No conflicts.

@dulek
Copy link
Contributor Author

dulek commented Feb 15, 2024

Thanks!

@EmilienM
Copy link
Contributor

@dulek is on vacation, I'll take care of rebase.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024
@EmilienM
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 21, 2024
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 22, 2024
@EmilienM
Copy link
Contributor

/cc MaysaMacedo
for double check

@EmilienM
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024
@MaysaMacedo
Copy link
Contributor

/lgtm

@jichenjc
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, jichenjc

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 Feb 23, 2024
This commit adds API that allows users to set `allocations_pools` in the
subnet created by CAPO. This allows the users to restrict the IP address
ranges that will be allocated automatically by OpenStack when creating
Machines.

Users can utilize this to reserve addresses for VIPs (virtual IPs) or
special nodes that will have predefined addresses and will be created
later.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2024
@MaysaMacedo
Copy link
Contributor

/lgtm

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

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024
@k8s-ci-robot k8s-ci-robot merged commit 1b0f1ea into kubernetes-sigs:main Feb 27, 2024
9 checks passed
@pierreprinetti pierreprinetti deleted the allocation-pools branch October 16, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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
Development

Successfully merging this pull request may close these issues.

6 participants