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

OSASINFRA-3690 OSASINFRA-3691: Add OpenStack CAPI-MAPI translation layer #188

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

OSASINFRA-3690 OSASINFRA-3691: Add OpenStack CAPI-MAPI translation layer #188

wants to merge 13 commits into from

Conversation

stephenfin
Copy link
Contributor

@stephenfin stephenfin commented Jul 15, 2024 •
edited
Loading

This is the OpenStack implementation for the MAPI <-> CAPI translation added in #173. It is similar to #233, which added IBM PowerVS (IBM Cloud) support recently.

The PR includes:

  • CAPO->MAPO conversion
  • MAPO->CAPO conversion
  • Unit tests for all error and warning paths
  • Fuzz tests
  • Integration tests for the controllers using MAPO/CAPO resources.

There are also a few additional cleanup patches includes. Where possible, these are kept in separate commits to avoid confusion.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 15, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2024
@stephenfin
Copy link
Contributor Author

/cc @mdbooth
/cc @EmilienM

@openshift-ci openshift-ci bot requested review from EmilienM and mdbooth July 15, 2024 14:36
@stephenfin
Copy link
Contributor Author

/cc @damdo

@openshift-ci openshift-ci bot requested a review from damdo July 15, 2024 14:36
@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch 3 times, most recently from 4086ad5 to 7410a66 Compare July 19, 2024 14:08
@damdo
Copy link
Member

damdo commented Sep 24, 2024

@stephenfin #173 is now merged with some minor tweaks from the original implementation. You might want to take a look there so we can also track OpenStack translation :)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2024
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 23, 2024
@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch from 7410a66 to 93f5369 Compare January 6, 2025 17:46
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 6, 2025

[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 joelspeed for approval. For more information see the 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

@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch 3 times, most recently from 08236e6 to 2c5af46 Compare January 10, 2025 18:28
@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch 4 times, most recently from ee49b17 to f927e55 Compare January 23, 2025 11:10
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2025
@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch from f927e55 to c10d4a8 Compare January 23, 2025 13:58
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2025
@stephenfin
Copy link
Contributor Author

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2025
@stephenfin stephenfin changed the title Add OpenStack CAPI-MAPI translation layer OSASINFRA-3690: OSASINFRA-3691: Add OpenStack CAPI-MAPI translation layer Jan 23, 2025
@stephenfin stephenfin mentioned this pull request Jan 23, 2025
Merged
@EmilienM
Copy link
Member

I finished a first pass and couldn't spot anything blocking, thanks for this huge work.
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 27, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 29, 2025

New changes are detected. LGTM label has been removed.

@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch 2 times, most recently from dc0b2a5 to 5edbcb0 Compare February 11, 2025 16:43
JoelSpeed
JoelSpeed reviewed Feb 20, 2025
View reviewed changes
@stephenfin
Disable SA1019 check
5b285ec 
The MAPI providers have deprecated fields that we still need to
be concerned with during conversion. Ignoring staticcheck errors
for all of these is a pain so set it globally instead.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
mapi2capi: Replace magic strings with constants
364cd29 
We have them. Use them. While here, also correct some doc strings.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
capi2mapi: Return MachineSet conversion errors earlier
64bcf9a 
Otherwise we can attempt to retrieve the Spec field from a nil object.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
capi2mapi: Rename AWS helpers
b4236f7 
Use consistent terminology to highlight things that are AWS-specific
and avoid conflicts with other platforms.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
chore: Bump cluster-api-actuator-pkg to the latest version
0715444 
We want these testutils module for OpenStack testing.

  go get github.com/openshift/cluster-api-actuator-pkg/testutils@latest
  go mod vendor
  go mod tidy

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
mapi2capi: Add errors module
2535922 
This is common code. It shouldn't be in the AWS-specific file.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
mapi2capi: Add OpenStack implementation
865230c 
Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
capi2mapi: Add OpenStack implementation
0b455bf 
Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
mapi2capi: Use APIVersion, Kind from objects
4548897 
Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
mapi2capi: Add OpenStack fuzz tests
45150f3 
We also replace the use of some hardcoded version attributes with the
library-provided versions. This should ease our lives if/when we bump
the API versions.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
capi2mapi: Add OpenStack fuzz tests
26607d8 
Once again, we replace the use of some hardcoded version attributes with
the library-provided versions.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
machinesetsync: Add additional logging
5bf9101 
This is helpful during debugging and shouldn't be too chatty. We also
fix the capitalisation of some existing logs to make them a little more
consistent.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
Wire up OpenStack capi2mapi/mapi2capi with controllers
23f3477 
These are complete. Time to integrate them with the Machine and
MachineSet sync controllers.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin stephenfin force-pushed the capi-mapi-conversion-pkg branch from d6f38d3 to 23f3477 Compare February 25, 2025 12:30
@stephenfin
Copy link
Contributor Author

@JoelSpeed Addressed all comments bar one that I don't understand.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 25, 2025 •
edited by openshift-ci bot
Loading

@stephenfin: This pull request references OSASINFRA-3690 which is a valid jira issue.

This pull request references OSASINFRA-3691 which is a valid jira issue.

In response to this:

This is the OpenStack implementation for the MAPI <-> CAPI translation added in #173. It is similar to #233, which added IBM PowerVS (IBM Cloud) support recently.

The PR includes:

  • CAPO->MAPO conversion
  • MAPO->CAPO conversion
  • Unit tests for all error and warning paths
  • Fuzz tests
  • Integration tests for the controllers using MAPO/CAPO resources.

There are also a few additional cleanup patches includes. Where possible, these are kept in separate commits to avoid confusion.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 25, 2025

@stephenfin: 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
ci/prow/e2e-azure-ovn-techpreview 23f3477 link false /test e2e-azure-ovn-techpreview
ci/prow/regression-clusterinfra-cucushift-rehearse-capi-aws-ipi 23f3477 link false /test regression-clusterinfra-cucushift-rehearse-capi-aws-ipi

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EmilienM EmilienM EmilienM left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@mdbooth mdbooth Awaiting requested review from mdbooth

@damdo damdo Awaiting requested review from damdo

@nrb nrb Awaiting requested review from nrb

Assignees

@EmilienM EmilienM

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@stephenfin @damdo @openshift-bot @openshift-ci-robot @EmilienM @JoelSpeed @openshift-merge-robot