OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image #5139

EmilienM · 2024-11-15T17:18:40Z

What this PR does / why we need it:

Instead of forcing the users to provide an existing OpenStack Glance
image, we now let our CAPI provider to upload the image used in the
release payload and handle its lifecycle with ORC.

openshift-ci-robot · 2024-11-15T17:18:44Z

@EmilienM: This pull request references OSASINFRA-3492 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 task to target the "4.18.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:

Instead of forcing the users to provide an existing OpenStack Glance
image, we now let our CAPI provider to upload the image used in the
release payload and handle its lifecycle with ORC.

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 · 2024-11-15T17:19:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-11-15T17:20:01Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`110dfbd`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/6737827369ac7400081b21bf
😎 Deploy Preview	https://deploy-preview-5139--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

EmilienM · 2024-11-15T18:13:13Z

support/releaseinfo/releaseinfo.go

+type CoreOpenStackDisk struct {
+	Release string `json:"release"`
+	URL     string `json:"url"`
+}


I want to add the Hash as well.

EmilienM · 2024-11-15T18:13:56Z

/cc bryan-cox
since you worked on that topic.

mdbooth

Either needs wiring up to nodepool to use imageRef (preferrable), or we need to add a wait loop somewhere in here to ensure that the image is Available before creating any machines referencing it.

We don't want to do that second one, so... we should add an imageRef to Nodepool.

mdbooth · 2024-11-15T18:17:44Z

hypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.go

+func reconcileOpenStackImageCR(ctx context.Context, client client.Client, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string) error {
+	openStackImage := orc.Image{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "rhcos" + hcluster.Name,


Suggested change

Name: "rhcos" + hcluster.Name,

Name: "rhcos-" + hcluster.Name,

We may also want to name it by the release image version so it can be shared between clusters 🤔

I don't know if the resource could be shared across multiple clusters, that's an interesting idea. I'll investigate for sure.

mdbooth · 2024-11-15T18:20:26Z

hypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.go

+		Spec: orc.ImageSpec{},
+	}
+
+	if _, err := createOrUpdate(ctx, client, &openStackImage, func() error {


TODO: check what createOrUpdate() does. Given that we have applyconfigs it's trivial for us to SSA these, so there's really no need for a function that actually does create or update.

From the doc:

// CreateOrUpdate is a copy of controllerutil.CreateOrUpdate with // an important difference: It copies a number of fields from the object // on the server to the mutated object if unset in the latter. This // avoids unnecessary updates when our code sets a whole struct that // has fields that get defaulted by the server.

mdbooth · 2024-11-15T18:22:04Z

hypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.go

+		return rhcosImageURL, rhcosImageRelease, fmt.Errorf("arch does not exist in release image, arch: %s", supportedArch)
+	}
+
+	rhcosImageURL = releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[supportedArch]].RHCOS.OpenStackDisk.URL


I'm 99% sure we can grab the SHA256s from here, too. It's probably in OpenStackDisk.

mdbooth · 2024-11-15T18:23:20Z

hypershift-operator/controllers/nodepool/openstack/openstack.go

-		// we plan to deliver the OpenStack provider as a dev preview.
-		return nil, fmt.Errorf("image name is required")
+		openStackMachineTemplate.Template.Spec.Image.ImageRef = &capiopenstackv1beta1.ResourceReference{
+			Name: "rhcos" + hcluster.Name,


Suggested change

Name: "rhcos" + hcluster.Name,

Name: "rhcos-" + hcluster.Name,

(Or version number)

mdbooth · 2024-11-15T18:24:51Z

support/releaseinfo/releaseinfo.go

@@ -69,14 +69,20 @@ type CoreOSImages struct {
 }

 type CoreRHCOSImage struct {
-	AzureDisk CoreAzureDisk `json:"azure-disk"`
+	AzureDisk     CoreAzureDisk     `json:"azure-disk"`
+	OpenStackDisk CoreOpenStackDisk `json:"openstack-disk"`


Are you sure this is required? Doesn't seem to be used. What do you expect to go here?

bryan-cox · 2024-11-15T18:40:37Z

hypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.go

@@ -498,3 +505,39 @@ func defaultWorkerSecurityGroupRules(machineCIDRs []string) []capo.SecurityGroup

 	return ingressRules
 }
+
+// lookupRHCOSImage looks up a release image and extracts the RHCOS image URL and release version
+func lookupRHCOSImage(ctx context.Context, client client.Client, hcluster *hyperv1.HostedCluster) (string, string, error) {


We should just leverage the existing lookupRHCOSImage and make it an exported function. We could pass in a platform field to differentiate between OSP and Azure.

Instead of forcing the users to provide an existing OpenStack Glance image, we now let our CAPI provider to upload the image used in the release payload and handle its lifecycle with ORC.

EmilienM · 2024-11-18T19:42:37Z

hypershift-operator/controllers/nodepool/capi.go

@@ -67,6 +67,13 @@ func (c *CAPI) Reconcile(ctx context.Context) error {
 		return err
 	}

+	// Reconcile ORC resources
+	if nodePool.Spec.Platform.Type == hyperv1.OpenStackPlatform {


I don't know if this pattern will be accepted. Suggestions are welcome!

@bryan-cox I realized that we probably need to handle the OpenStack RHCOS image by Nodepool, since the release image can be overridden per Nodepool?

EmilienM · 2024-11-18T19:42:56Z

hypershift-operator/controllers/nodepool/openstack/openstack.go

+func ReconcileOpenStackImageCR(ctx context.Context, client client.Client, createOrUpdate upsert.CreateOrUpdateFN, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, release *releaseinfo.ReleaseImage) error {
+	openStackImage := orc.Image{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "rhcos-" + hcluster.Name,


EmilienM · 2024-11-18T19:43:01Z

hypershift-operator/controllers/nodepool/openstack/openstack.go

+
+	openStackImageSpec.Resource = &orc.ImageResourceSpec{
+		// THIS IS NOT GOOD, NEEDS TO BE FIXED (something related to the image itself should be used, like version)
+		Name: "rhcos-" + hcluster.Name,


EmilienM · 2024-11-18T19:43:21Z

/test e2e-openstack

EmilienM · 2024-11-18T19:45:02Z

hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go

@@ -54,7 +54,7 @@ func defaultImage(releaseImage *releaseinfo.ReleaseImage) (string, string, error
 	return containerImage, split[1], nil
 }

-func unsupportedOpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage) (string, string, error) {
+func UnsupportedOpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage) (string, string, error) {


We probably want to move that function to utils or something

openshift-ci · 2024-11-18T22:37:01Z

@EmilienM: 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/unit	`042fd11`	link	true	`/test unit`
ci/prow/e2e-openstack	`042fd11`	link	false	`/test e2e-openstack`
ci/prow/okd-scos-e2e-aws-ovn	`042fd11`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-4-17	`042fd11`	link	true	`/test e2e-aws-4-17`

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.

EmilienM · 2024-11-19T00:23:04Z

For myself: we need to update RBAC:

{"level":"error","ts":"2024-11-18T21:20:51Z","msg":"Failed to reconcile NodePool","controller":"nodepool","controllerGroup":"hypershift.openshift.io","controllerKind":"NodePool","NodePool":{"name":"example-n42fg","namespace":"e2e-clusters-mgz8n"},"namespace":"e2e-clusters-mgz8n","name":"example-n42fg","reconcileID":"dc9ea120-8120-400a-af0a-3b265526072a","error":"images.openstack.k-orc.cloud is forbidden: User \"system:serviceaccount:hypershift:operator\" cannot create resource \"images\" in API group \"openstack.k-orc.cloud\" in the namespace \"e2e-clusters-mgz8n-example-n42fg\"","stacktrace":"github.com/openshift/hypershift/hypershift-operator/controllers/nodepool.(*NodePoolReconciler).Reconcile\n\t/hypershift/hypershift-operator/controllers/nodepool/nodepool_controller.go:205\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\t/hypershift/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:114\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/hypershift/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:311\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/hypershift/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:261\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/hypershift/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:222"}

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 15, 2024

openshift-ci bot added the do-not-merge/needs-area label Nov 15, 2024

openshift-ci bot requested review from csrwng and sjenning November 15, 2024 17:19

openshift-ci bot added area/cli Indicates the PR includes changes for CLI area/documentation Indicates the PR includes changes for documentation labels Nov 15, 2024

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Nov 15, 2024

EmilienM commented Nov 15, 2024

View reviewed changes

openshift-ci bot requested a review from bryan-cox November 15, 2024 18:13

mdbooth reviewed Nov 15, 2024

View reviewed changes

bryan-cox reviewed Nov 15, 2024

View reviewed changes

OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image

042fd11

Instead of forcing the users to provide an existing OpenStack Glance image, we now let our CAPI provider to upload the image used in the release payload and handle its lifecycle with ORC.

EmilienM force-pushed the orc branch from a0ad3b9 to 042fd11 Compare November 18, 2024 19:42

EmilienM commented Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image #5139

OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image #5139

EmilienM commented Nov 15, 2024

openshift-ci-robot commented Nov 15, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Nov 15, 2024

netlify bot commented Nov 15, 2024

EmilienM Nov 15, 2024

EmilienM commented Nov 15, 2024

mdbooth left a comment

mdbooth Nov 15, 2024

mdbooth Nov 15, 2024

EmilienM Nov 18, 2024

mdbooth Nov 15, 2024

EmilienM Nov 18, 2024

mdbooth Nov 15, 2024

mdbooth Nov 15, 2024

mdbooth Nov 15, 2024

bryan-cox Nov 15, 2024

EmilienM Nov 18, 2024

EmilienM Nov 18, 2024

EmilienM Nov 18, 2024

EmilienM Nov 18, 2024

EmilienM commented Nov 18, 2024

EmilienM Nov 18, 2024

openshift-ci bot commented Nov 18, 2024

EmilienM commented Nov 19, 2024

	Name: "rhcos" + hcluster.Name,
	Name: "rhcos-" + hcluster.Name,

OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image #5139

Are you sure you want to change the base?

OSASINFRA-3492: openstack: leverage ORC to handle RHCOS image #5139

Conversation

EmilienM commented Nov 15, 2024

openshift-ci-robot commented Nov 15, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Nov 15, 2024

netlify bot commented Nov 15, 2024

✅ Deploy Preview for hypershift-docs ready!

Choose a reason for hiding this comment

EmilienM commented Nov 15, 2024

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilienM commented Nov 18, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Nov 18, 2024

EmilienM commented Nov 19, 2024

openshift-ci-robot commented Nov 15, 2024 •

edited by openshift-ci bot

Loading