Dynamically discover release image OS metadata #205

ironcladlou · 2021-05-04T18:50:17Z

Before this commit, CoreOS metadata was hard-coded in this Git repo and required
manual maintenance to map OCP release versions to compatible AMIs. Now, the
metadata is discovered automatically as part of the general release image
metadata lookup, as the CoreOS metadata is part of the payload as of
openshift/installer#4760.

After this commit, HyperShift will only be compatible with OCP release images
which embed the CoreOS metadata.

ironcladlou · 2021-05-04T18:50:23Z

/test e2e-aws

openshift-ci-robot · 2021-05-04T18:50:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ironcladlou]

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

wking · 2021-05-04T18:58:04Z

control-plane-operator/releaseinfo/pod_provider.go

+				{
+					Name:    "read-coreos-metadata",
+					Image:   image,
+					Command: []string{"/usr/bin/cat", "/release-manifests/0000_50_installer_coreos-bootimages.yaml"},


is this invoked in a shell where we can use wildcards? And maybe regardless, might be worth PRing a comment to the installer source like "renaming this manifest will break a hard-coded path in the HyperShift contro-plane operator: $LINK". Or can we make this lookup robust to installer-side renames by loading manifests and then iterating over them to find the one we want?

Couple of things we use which definitely need more thought:

/release-manifests/image-references

/release-manifests/0000_50_installer_coreos-bootimages.yaml

In a given release payload, some questions about those files:

Are they considered part of the payload "contract" somehow, e.g. by nature of having a well-known location?

Are their contents guaranteed to be serializations of some public API types? For example, image-references is a serialized ImageStream, but bootimages is a serialized ConfigMap which embeds a JSON document whose schema I'm not aware of

cc @derekwaynecarr

/release-manifests/image-references is payload contract, e.g. here and here is the CVO requiring that file to exist. The CVO requires manifests to exist in release-manifests/, uses their filenames for ordering, but otherwise isn't opinionated about what they are called, and does not require any manifest-filename consistency between releases.

It's possible that the installer might drop their boot-image ConfigMap, but seems unlikely, and even less likely if you let them know that you're consuming it.

I wonder if it makes sense for the OS metadata bits to become part of the contract? Otherwise it's not clear to me how providers would piece together the compatible payload/AMI (for example) versions from the outside in order to specify the AMI explicitly on the HostedCluster. @derekwaynecarr, thoughts here?

@staebler let’s discuss the above.

The contract is to call openshift-install coreos print-stream-json. There is no contract for extracting the CoreOS image stream from the release payload directly.

The main questions for me:

Where is the published schema for the coreos metadata JSON?

How should that metadata (conforming to the schema) be made available to users given a release payload image spec (e.g. is there an API call users can make to get the CoreOS metadata for that release, is the the JSON encoded in the image metadata itself, etc.)

I would ask the same questions regarding the ImageStream for component images embedded in the payload. In that case the schema is clear (it's an ImageStream which is public versioned schema), but the "how do users get it" is outstanding

oc adm release info -o json $RELEASE_IMAGE_PULLSPEC will include the image references. I'm fine porting that logic to library-go, for other folks who want to vendor it.

ironcladlou · 2021-05-04T19:13:50Z

/test e2e-aws

ironcladlou · 2021-05-04T20:55:43Z

hypershift-operator/controllers/nodepool/nodepool_controller.go

-		ami, err := r.ImageProvider.Image(hcluster)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to obtain AMI: %w", err)
+		var ami string


@enxebre @sjenning should we be using the value from spec if provided and only discovering if no value was given?

This ami lookup is dictated by nodePool.Spec.Release.Image which also dictates the MCS version used for this nodePool, the MCS dictates the ign payload and therefore the nodePool version. So the ami could even be a status.Field in our MCS resource. This way we'd capture and expose atomically anything that manages nodePool versioning all together in the MCS logic. Thoughts?

It makes sense — although this is platform specific, and there's currently no struct type in MCS.Status to express that sort of thing. So maybe something like:

status: version: ... platform: type: aws aws: ami: ...

Or something... the info is already embedded in NodePool via NodePoolPlatform, so not sure if it makes sense to embed a NodePoolPlatform in MCS.Status?

I also added some TODOs around some possible gaps in the NodePool API here (AWS region, architecture).

I think we should move the status API additions to a followup if that's okay

enxebre · 2021-05-05T09:38:44Z

hypershift-operator/controllers/nodepool/nodepool_controller.go

-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to obtain AMI: %w", err)
+		var ami string
+		if arch, foundArch := releaseImage.StreamMetadata.Architectures["x86_64"]; !foundArch {


Do you think this new logic deserves its own unit testable function?

I do have some tests in progress I can add to the PR, yes

I did extract some of the logic into a function, but it's so small it doesn't seem worth unit testing at the moment — I do think the reconciliation code itself deserves tests but that would be a much larger refactor (there aren't any tests at all for this controller currently, and I think it needs some restructuring to make that manageable), so I'm not tackling it in this PR

enxebre · 2021-05-05T09:42:18Z

hypershift-operator/controllers/nodepool/nodepool_controller.go

+		var ami string
+		if arch, foundArch := releaseImage.StreamMetadata.Architectures["x86_64"]; !foundArch {
+			return ctrl.Result{}, fmt.Errorf("couldn't find OS metadata for architecture %q", "x64_64")
+		} else {


can we signal all this new errors in a nodePool condition as well?

That seems like it would be useful

I incorporated a new AMIDiscoveryFailed condition to surface this failure

ironcladlou · 2021-05-05T11:41:00Z

/test e2e-aws

Before this commit, CoreOS metadata was hard-coded in this Git repo and required manual maintenance to map OCP release versions to compatible AMIs. Now, the metadata is discovered automatically as part of the general release image metadata lookup, as the CoreOS metadata is part of the payload as of openshift/installer#4760. After this commit, HyperShift will only be compatible with OCP release images which embed the CoreOS metadata.

ironcladlou · 2021-05-05T13:41:28Z

/test e2e-aws

sjenning · 2021-05-05T17:18:25Z

/lgtm

openshift-ci-robot requested review from csrwng and sjenning May 4, 2021 18:50

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2021

ironcladlou mentioned this pull request May 4, 2021

sync 4.8 rhcos build metadata #170

Closed

wking reviewed May 4, 2021

View reviewed changes

ironcladlou force-pushed the discover-aws-images branch from 87aa5f6 to 9a18cd1 Compare May 4, 2021 19:13

ironcladlou commented May 4, 2021

View reviewed changes

enxebre reviewed May 5, 2021

View reviewed changes

ironcladlou force-pushed the discover-aws-images branch from 9a18cd1 to bcd7de4 Compare May 5, 2021 13:40

openshift-ci-robot assigned sjenning May 5, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021

openshift-merge-robot merged commit fec57f3 into openshift:main May 5, 2021

Dynamically discover release image OS metadata #205

Dynamically discover release image OS metadata #205

Uh oh!

Conversation

ironcladlou commented May 4, 2021

Uh oh!

ironcladlou commented May 4, 2021

Uh oh!

openshift-ci-robot commented May 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented May 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented May 5, 2021

Uh oh!

ironcladlou commented May 5, 2021

Uh oh!

sjenning commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wking May 6, 2021 •

edited

Loading

enxebre May 5, 2021 •

edited

Loading