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

METAL-966: Add support for metal3 #175

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

METAL-966: Add support for metal3 #175

wants to merge 1 commit into from

Conversation

honza
Copy link
Member

@honza honza commented Jun 7, 2024

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 7, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 7, 2024 •
edited by openshift-ci bot
Loading

@honza: This pull request references METAL-966 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

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 bot requested review from damdo and nrb June 7, 2024 19:29
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2024
@honza honza force-pushed the add-metal3 branch 3 times, most recently from 2d6320e to 3e68300 Compare June 13, 2024 17:42
damdo
damdo reviewed Jul 1, 2024
View reviewed changes
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

Hey @honza the changes you are adding here look reasonable to me.

Although we recently merged #169 which slightly changes how we handle InfraCluster generation for each platform, and moves some of the switch/case around, would you be able to adapt this PR to that change?

Before merging this PR we would need to add e2e tests (similarly to what we already do with powervs/gcp/vsphere in this repo), which will require to us to get openshift/cluster-api-provider-metal3#18 merged first

@honza honza force-pushed the add-metal3 branch 3 times, most recently from 8d95a47 to ff1c497 Compare July 30, 2024 08:57
@honza honza force-pushed the add-metal3 branch 2 times, most recently from 410528e to 5eed1ef Compare August 6, 2024 08:45
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dtantsur
Once this PR has been reviewed and has the lgtm label, please assign damdo 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

@honza honza force-pushed the add-metal3 branch 2 times, most recently from bd716a1 to 1399782 Compare August 20, 2024 14:58
@hroyrh hroyrh mentioned this pull request Aug 28, 2024
Open
@honza honza force-pushed the add-metal3 branch 5 times, most recently from ef1f476 to 1fc88f7 Compare September 15, 2024 00:11
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024
@honza honza force-pushed the add-metal3 branch 2 times, most recently from d42a084 to 72cebea Compare November 7, 2024 13:35
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2024
@honza
Copy link
Member Author

honza commented Nov 14, 2024

We have started working on a CI job here: openshift/release#58839

@damdo
Copy link
Member

damdo commented Nov 14, 2024

Amazing news @honza thanks!

@dtantsur
Copy link
Member

Hi folks! Do you envision this change landing before the 4.18 branching?

@honza honza force-pushed the add-metal3 branch 2 times, most recently from 8017bd7 to d61d7f1 Compare November 18, 2024 15:25
@damdo
Copy link
Member

damdo commented Nov 18, 2024

Hey @dtantsur
For this to be ready to merge I'd like to see metal3 InfraCluster generation the same way we are now doing it for the other Infraclusters see: #169

Also I think we should get this retitled to Add metal3 provider support, as the concept of support is now wider if we consider the MAPI->CAPI migration work.

Also it would be best to have e2e presubmits passing on this PR before merging it.

@honza
Copy link
Member Author

honza commented Nov 19, 2024

/test e2e-metal3-capi-techpreview

@honza honza force-pushed the add-metal3 branch 4 times, most recently from 2e6341b to b168d2c Compare November 21, 2024 02:33
@hroyrh
Copy link

hroyrh commented Nov 21, 2024

/test e2e-metal3-capi-techpreview

1 similar comment
@hroyrh
Copy link

hroyrh commented Nov 21, 2024

/test e2e-metal3-capi-techpreview

@honza honza force-pushed the add-metal3 branch 2 times, most recently from 502eca1 to f5fbe4f Compare November 21, 2024 13:58
@hroyrh
Copy link

hroyrh commented Nov 21, 2024

/test e2e-metal3-capi-techpreview

damdo
damdo reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@damdo damdo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are a couple of quite important things for which we should create a follow up PR in the coming days.

For now I think it could be ok to merge if @JoelSpeed is also happy with it, but we should make sure we fix them ASAP.

e2e/framework/machineset.go
@@ -19,6 +19,7 @@ type machineSetParams struct {
failureDomain string
replicas int32
infrastructureRef corev1.ObjectReference
UserDataSecret string
Copy link
Member

Choose a reason for hiding this comment

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

This should probably have been unexported like the rest of the fields in this struct. But we can fix it in a followup PR I guess, if we are in a rush.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed; I wanted to avoid changing every call site. Happy to fix it in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are, including this PR, around 5 call sites, I'd prefer we just get that updated, rather than putting this off for the future

manifests/0000_30_cluster-api_01_credentials-request.yaml Outdated
apiVersion: cloudcredential.openshift.io/v1
kind: Metal3ProviderSpec
secretRef:
name: capv-manager-bootstrap-credentials
Copy link
Member

Choose a reason for hiding this comment

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

This needs changing as it conflicts with capv

Copy link
Member

@damdo damdo Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Although it doesn't break payload because it is on different platforms that will never encounter eachother.
We might want to rename (and tombstone?) cc. @JoelSpeed

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it doesn't break payload because it is on different platforms that will never encounter eachother.

We have plans in the future to allow multiple platforms to be installed, so, this would then conflict. Names must be unique, so this needs to be fixed

pkg/controllers/infracluster/infracluster_controller.go
Comment on lines +220 to +226
case configv1.BareMetalPlatformType:
baremetalCluster := &metal3v1.Metal3Cluster{}
if err := r.Get(ctx, client.ObjectKey{Namespace: defaultCAPINamespace, Name: r.Infra.Status.InfrastructureName}, baremetalCluster); err != nil && !kerrors.IsNotFound(err) {
return nil, fmt.Errorf("error getting InfraCluster object: %w", err)
}

infraCluster = baremetalCluster
Copy link
Member

Choose a reason for hiding this comment

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

This will need changing once we introduce the BareMetalCluster generation via this controller. In a followup PR if possible @honza, thank you!

@honza honza force-pushed the add-metal3 branch 2 times, most recently from 57ed1ad to a6da96c Compare November 22, 2024 14:57
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 22, 2024

@honza: 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/okd-scos-images 56b76dc link true /test okd-scos-images
ci/prow/e2e-aws-ovn-serial ab66ff3 link true /test e2e-aws-ovn-serial
ci/prow/e2e-aws-ovn-techpreview ab66ff3 link true /test e2e-aws-ovn-techpreview
ci/prow/e2e-gcp-ovn-techpreview ab66ff3 link true /test e2e-gcp-ovn-techpreview
ci/prow/regression-clusterinfra-cucushift-rehearse-capi-aws-ipi ab66ff3 link false /test regression-clusterinfra-cucushift-rehearse-capi-aws-ipi
ci/prow/okd-scos-e2e-aws-ovn ab66ff3 link false /test okd-scos-e2e-aws-ovn

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

@JoelSpeed JoelSpeed JoelSpeed left review comments

@damdo damdo damdo left review comments

@dtantsur dtantsur dtantsur approved these changes

@nrb nrb Awaiting requested review from nrb

Assignees
No one assigned
Labels
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
@honza @openshift-ci-robot @damdo @dtantsur @hroyrh @JoelSpeed @openshift-merge-robot