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

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

wants to merge 1 commit into from

Conversation

peppi-lotta
Copy link
Member

@peppi-lotta peppi-lotta commented Sep 18, 2024 •
edited
Loading

This commit makes it possible to:

  • Deploy IPAM with clusterctl
  • Reconsile CAPI's ipaddressclaims with this managers ippools

I have two PR's, one in metal-dev-env and one in CAPM3 to mach these changes

All three PRs' changes need to be tested together. Check test in the metal-dev-env PR.

What this PR does / why we need it: To make this IPAM into a provider for CAPI

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sunnatillo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 18, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 7 times, most recently from 4fcd34f to abe0f52 Compare September 19, 2024 09:01
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@peppi-lotta peppi-lotta mentioned this pull request Sep 19, 2024
Open
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 32d5986 to 2f06f8b Compare September 26, 2024 08:15
@peppi-lotta peppi-lotta mentioned this pull request Oct 2, 2024
Open
Rozzii
Rozzii previously requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

@peppi-lotta this is quite a big change so I prefer to review after all the CI checks are green.

Please fix the markdown and run the tests, then ping me please if everything is green.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 5 times, most recently from 5fa145b to b610263 Compare October 24, 2024 07:02
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 6b82fe0 to 8e486a5 Compare October 31, 2024 08:00
lentzi90
lentzi90 reviewed Nov 1, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Great job!
We could probably use generics to avoid the double functions for everything. However, I think it is good to keep it like this for now since it means we know that the original code did not change.
Just minor things below about naming.

config/default/kustomization.yaml Outdated Show resolved Hide resolved
config/default/kustomization.yaml Outdated Show resolved Hide resolved
metadata.yaml
kind: Metadata
releaseSeries:
- major: 1
minor: 9
Copy link
Member

Choose a reason for hiding this comment

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

We don't have v1.9 yet. Why is it here?

Copy link
Member Author

@peppi-lotta peppi-lotta Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I copied this straight as is in CAMP3. CAPI also has one minor release more than is the current in their metadata.yaml. Also in this PR in metal-dev-env I create a local override layer that is one minor release higher that the current actual release.

ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
@Rozzii Rozzii modified the milestone: IPAM - v1.9 Nov 6, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 3 times, most recently from 4f3b81d to 027c714 Compare November 7, 2024 11:19
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 1cb7909 to 3407185 Compare November 7, 2024 11:26
@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 7, 2024 •
edited
Loading

@peppi-lotta: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-ubuntu-e2e-integration-test-main abe0f52 link true /test metal3-ubuntu-e2e-integration-test-main
metal3-centos-e2e-integration-test-main abe0f52 link true /test metal3-centos-e2e-integration-test-main

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@peppi-lotta
This commit makes it possible to:
ab2256b 
- Deploy IPAM with clusterctl
- Reconsile CAPI's ipaddressclaims with this managers ippools

Signed-off-by: peppi-lotta <[email protected]>
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 3407185 to ab2256b Compare November 7, 2024 11:36
@Rozzii Rozzii self-requested a review November 8, 2024 08:19
@Rozzii Rozzii dismissed their stale review November 8, 2024 08:19

CI is green now

@Rozzii
Copy link
Member

Rozzii commented Nov 8, 2024

/hold
Feel free to review it and approve it but this feature will be merged after 1.9 release and will target 1.10 release.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2024
@Rozzii Rozzii added this to the IPAM - v1.10 milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@smoshiur1237 smoshiur1237 Awaiting requested review from smoshiur1237

@Rozzii Rozzii Awaiting requested review from Rozzii

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: IPAM WIP
Milestone
IPAM - v1.10
Development

Successfully merging this pull request may close these issues.
4 participants
@peppi-lotta @metal3-io-bot @Rozzii @lentzi90