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.

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

image-mirroring: Drop driver-toolkit from OKD 4.15 #43816

Merged
merged 1 commit into from Oct 10, 2023

Conversation

LorbusChris
Copy link
Member

No description provided.

@LorbusChris
Copy link
Member Author

/retest-required

@LorbusChris
Copy link
Member Author

/test build09-dry

@@ -128,7 +128,6 @@ registry.ci.openshift.org/origin/4.15:descheduler quay.io/openshift/origin-desch
registry.ci.openshift.org/origin/4.15:docker-builder quay.io/openshift/origin-docker-builder:4.15 quay.io/openshift/origin-docker-builder:4.15.0 quay.io/openshift/origin-docker-builder:latest
registry.ci.openshift.org/origin/4.15:docker-registry quay.io/openshift/origin-docker-registry:4.15 quay.io/openshift/origin-docker-registry:4.15.0 quay.io/openshift/origin-docker-registry:latest
registry.ci.openshift.org/origin/4.15:dpu-network-operator quay.io/openshift/origin-dpu-network-operator:4.15 quay.io/openshift/origin-dpu-network-operator:4.15.0 quay.io/openshift/origin-dpu-network-operator:latest
registry.ci.openshift.org/origin/4.15:driver-toolkit quay.io/openshift/origin-driver-toolkit:4.15 quay.io/openshift/origin-driver-toolkit:4.15.0 quay.io/openshift/origin-driver-toolkit:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is using those images?
DTK's customers are usually getting the image using oc adm release info ... --image-for driver-toolkit and getting an image from quay.io/openshift-release-dev/...

Copy link
Contributor

Choose a reason for hiding this comment

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

Before merging this PR, we need to make sure that OKD's users running oc adm release info ... --image-for driver-toolkit won't get those images. If this is the case then we shouldn't remove them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Customers shouldn't be using any OKD images unless they specifically want to do that. OKD won't have the driver-toolkit going forward, unless OKD-variant specific builds of that image are set up.

@@ -120,7 +120,6 @@ registry.ci.openshift.org/origin/4.13:descheduler quay.io/openshift/origin-desch
registry.ci.openshift.org/origin/4.13:docker-builder quay.io/openshift/origin-docker-builder:4.13 quay.io/openshift/origin-docker-builder:4.13.0
registry.ci.openshift.org/origin/4.13:docker-registry quay.io/openshift/origin-docker-registry:4.13 quay.io/openshift/origin-docker-registry:4.13.0
registry.ci.openshift.org/origin/4.13:dpu-network-operator quay.io/openshift/origin-dpu-network-operator:4.13 quay.io/openshift/origin-dpu-network-operator:4.13.0
registry.ci.openshift.org/origin/4.13:driver-toolkit quay.io/openshift/origin-driver-toolkit:4.13 quay.io/openshift/origin-driver-toolkit:4.13.0
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the specific details here on what's the right call, but high-level I'm thinking of this change to drop machine-os-content as affecting 4.15 and onwards only.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see what we would bother to remove those if machine-os-content is shipped in older versions anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Interested in what other stakeholders think, but IMO it doesn't seem worth the risk. Even if we did decide to do this in older releases, we should validate it in 4.15 first.

Copy link
Member Author

Choose a reason for hiding this comment

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

The driver toolkit image that is in OKD right now is actually an RHCOS one from Prow CI. It shouldn't be there and if nothing's image-referencing it, removing it from all current OKD versions (4.13+) should be safe afaict.

Removing driver-toolkit from 4.15 is required before switching the image-reference that it contains from machine-os-content to rhel-coreos, as the release controllers for OKD (and required CI tests like /test okd-images) will otherwise fail due to the OKD ImageStreams not containing an image of that name.

To be extra cautious, I can split this and do only 4.15 first. Please let me know.

Copy link
Member Author

Choose a reason for hiding this comment

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

On a related note, is it too late to switch to a more agnostic image name, like simply coreos? That would make separate builds for OCP and OKD for some components like the MCO superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

To be extra cautious, I can split this and do only 4.15 first. Please let me know.

Can't we just remove it for 4.15? Is it an issue to keep machine-os-content in older versions?

On a related note, is it too late to switch to a more agnostic image name, like simply coreos? That would make separate builds for OCP and OKD for some components like the MCO superfluous.

You mean instead of rhel-coreos? I don't mind as long as DTK's image-references is updated accordingly but I won't mix those 2 efforts anyway. Let's keep this PR simple IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. machine-os-content will continue to exist in OKD 4.13 and 4.14.

This change is really just about removing an OCP image from OKD (driver-toolkit, which happens to reference machine-os-content today). The change in this PR is now as simple as can be, PTAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes rhel-coreos. We're doing separate OKD / OCP builds for a couple of components, where the only difference is the base OS image-reference they contain (see e.g. https://github.com/openshift/machine-config-operator/blob/master/Dockerfile#L24-L35)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I am OK with it. Probably better to discuss it with the coreOS folks.
Is it related to this PR though?

@openshift-ci-robot
Copy link
Contributor

[REHEARSALNOTIFIER]
@LorbusChris: no rehearsable tests are affected by this change

Interacting with pj-rehearse

Comment: /pj-rehearse to run up to 10 rehearsals
Comment: /pj-rehearse skip to opt-out of rehearsals
Comment: /pj-rehearse {test-name}, with each test separated by a space, to run one or more specific rehearsals
Comment: /pj-rehearse more to run up to 20 rehearsals
Comment: /pj-rehearse max to run up to 35 rehearsals
Comment: /pj-rehearse auto-ack to run up to 10 rehearsals, and add the rehearsals-ack label on success
Comment: /pj-rehearse abort to abort all active rehearsals

Once you are satisfied with the results of the rehearsals, comment: /pj-rehearse ack to unblock merge. When the rehearsals-ack label is present on your PR, merge will no longer be blocked by rehearsals.
If you would like the rehearsals-ack label removed, comment: /pj-rehearse reject to re-block merging.

@LorbusChris LorbusChris changed the title Import SCOS images for OKD 4.15, don't mirror driver-toolkit image-mirroring: Drop driver-toolkit from OKD 4.15 Oct 4, 2023
@LorbusChris LorbusChris changed the title image-mirroring: Drop driver-toolkit from OKD 4.15 image-mirroring: Drop driver-toolkit from OKD 4.15 Oct 4, 2023
@jlebon
Copy link
Member

jlebon commented Oct 4, 2023

This looks sane to me. Thanks for working on this!
/approve

Will let @ybettan do the final stamping.

Re. naming, I think a better approach is probably to not care about the name at all and just focus on the machine-os label bit. But yeah, let's keep that discussion separate from this effort. :) (Probably worth a ticket somewhere... against the MCO maybe?)

@ybettan
Copy link
Contributor

ybettan commented Oct 5, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2023
@ybettan
Copy link
Contributor

ybettan commented Oct 5, 2023

/assign @jupierce

@jupierce
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlebon, jupierce, LorbusChris, ybettan

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2023

@LorbusChris: The following test 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/build-farm/build09-dry b96b93ea48118921e60bf1a9e1ba4a840823fde6 link false /test build09-dry

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

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 1c2a745 and 2 for PR HEAD 12912fd in total

@openshift-ci openshift-ci bot merged commit be6330b into openshift:master Oct 10, 2023
9 checks passed
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2023

@LorbusChris: Updated the image-mirror-mappings configmap in namespace ci at cluster app.ci using the following files:

  • key mapping_origin_4_15 using file core-services/image-mirroring/openshift/mapping_origin_4_15

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 kubernetes/test-infra repository.

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. rehearsals-ack Signifies that rehearsal jobs have been acknowledged
Projects
None yet
6 participants