Skip to content

Conversation

@jkyros
Copy link
Member

@jkyros jkyros commented Aug 16, 2022

The MCO needs access to image-references during bootstrap so it can reference additional images.

I'm open to more elegant ways to do this if you have concerns.

As for why...

In service to: https://issues.redhat.com/browse/MCO-291

Previously all the MCO needed was the machine-os-content image, which
became osImageURL, but now that we have new format images, extension
containers, and probably a pair (one new format image, one extension
container) per RHCOS major version (rhel-coreos-8, rhel-coreos-9, etc)
we need to consume more than one image out of the payload.

As a result, the MCO needs the image-references file so that it can read all of the
OS images contained within and select the one it prefers.

This dumps the image-references file out of the payload so the MCO can
consume it, and also adds an argument to the `machine-config-operator`
call to consume it.
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Simple and elegant IMO!

--infra-image="${MACHINE_CONFIG_INFRA_IMAGE}" \
--keepalived-image="${KEEPALIVED_IMAGE}" \
--coredns-image="${COREDNS_IMAGE}" \
--haproxy-image="${HAPROXY_IMAGE}" \
Copy link
Member

Choose a reason for hiding this comment

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

In theory we could also drop these arguments now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, yes, but I was a big chicken 😄

I figured we'd add the new argument -> make sure on our end everything works (I'm sure it does) -> come back for cleanup. If you think we should pull the old args out now to save time, we can?

Copy link
Member

Choose a reason for hiding this comment

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

We wouldn't pass CI here unless the new path worked entirely, right? (I guess there's possible corner cases on e.g. bare metal images not used in the default cloud cases, but we can probably just double check before merging)

I don't have a strong opinion here and will leave it to your choice!

@jkyros
Copy link
Member Author

jkyros commented Aug 18, 2022

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2022

@jkyros: 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-openstack 322d4fe link false /test e2e-openstack
ci/prow/e2e-metal-ipi 322d4fe link false /test e2e-metal-ipi
ci/prow/e2e-ibmcloud 322d4fe link false /test e2e-ibmcloud
ci/prow/e2e-libvirt 322d4fe link false /test e2e-libvirt

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

@patrickdillon
Copy link
Contributor

patrickdillon commented Aug 18, 2022

Simple and elegant IMO!

+1

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, patrickdillon

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2022
@cgwalters
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 3e65614 and 8 for PR HEAD 322d4fe in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 44e71ef and 7 for PR HEAD 322d4fe in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 44e71ef and 6 for PR HEAD 322d4fe in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 44e71ef and 5 for PR HEAD 322d4fe in total

@patrickdillon
Copy link
Contributor

/skip

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants