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

MCO-424: Drop machine-os-content references #3364

Merged
merged 1 commit into from Oct 2, 2023

Conversation

cgwalters
Copy link
Member

xref https://github.com/openshift/enhancements/blob/master/enhancements/ocp-coreos-layering/ocp-coreos-layering.md

This is part of https://issues.redhat.com/browse/MCO-392

Since we've landed the new format image usage in the latest nightly, it's time to try dropping it from our image references entirely.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2022
@cgwalters cgwalters changed the title WIP: Drop machine-os-content references Drop machine-os-content references Oct 4, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2022
@cgwalters
Copy link
Member Author

OK we're passing our own CI here. I think we can likely do this, and then the actual removal would hinge on openshift/driver-toolkit#101

@yuqi-zhang
Copy link
Contributor

/test e2e-hypershift

Wanted to check if this was actually added or not

@yuqi-zhang
Copy link
Contributor

/test okd-scos-images

@cgwalters
Copy link
Member Author

cgwalters commented Oct 6, 2022

Let's merge openshift/driver-toolkit#102 first, because this repository is the real logical owner of machine-os-content, and we have tons of existing CI jobs setup here we can use to gain signal for its final removal.
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 12, 2023
@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 13, 2023
@openshift-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Jun 12, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 12, 2023

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

jlebon added a commit to jlebon/os that referenced this pull request Sep 27, 2023
This was previously enabled (openshift#1048) and then disabled again (openshift#1084)
because `oc` doesn't know how to handle multiple images with those
labels in the release payload. We'll need to solve this eventually if
we want to be able to ship multiple OS images in the payload (that's
tracked in openshift#1047), but we don't need to block on this if we can remove
the legacy `machine-os-content` at the same time.

See also: openshift/driver-toolkit#101
See also: openshift/machine-config-operator#3364
@cgwalters cgwalters reopened this Sep 27, 2023
@cgwalters
Copy link
Member Author

Let's merge openshift/driver-toolkit#102 first, because this repository is the real logical owner of machine-os-content, and we have tons of existing CI jobs setup here we can use to gain signal for its final removal.

While this is true, I think it will actually be better to leave driver-toolkit as the last thing holding the bag. We can merge this, and then do the driver-toolkit change and the change to openshift/os#1374 as the same time.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2023

@cgwalters: 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/prow/e2e-aws c78dbe1 link true /test e2e-aws

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-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2023
@cgwalters
Copy link
Member Author

See discussion in openshift/os#1374

@cgwalters cgwalters changed the title Drop machine-os-content references MCO-424: Drop machine-os-content references Sep 27, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 27, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 27, 2023

@cgwalters: This pull request references MCO-424 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.15.0" version, but no target version was set.

In response to this:

xref https://github.com/openshift/enhancements/blob/master/enhancements/ocp-coreos-layering/ocp-coreos-layering.md

This is part of https://issues.redhat.com/browse/MCO-392

Since we've landed the new format image usage in the latest nightly, it's time to try dropping it from our image references entirely.

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.

jlebon added a commit to jlebon/os that referenced this pull request Sep 27, 2023
This was previously enabled (openshift#1048) and then disabled again (openshift#1084)
because `oc` doesn't know how to handle multiple images with those
labels in the release payload. We'll need to solve this eventually if
we want to be able to ship multiple OS images in the payload (that's
tracked in openshift#1047), but we don't need to block on this if we can remove
the legacy `machine-os-content` at the same time.

See also: openshift/driver-toolkit#101
See also: openshift/machine-config-operator#3364
@cgwalters
Copy link
Member Author

OK yep, green for CI across the board here, and this is basically purely deleting dead code. Risk here is IMO quite low.

Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Reading through the context I think this should be safe to drop on our side. Offhand I don't think any non-CI covered paths (hypershift, IBM cloud, onceFrom etc.) uses this anymore as well

/lgtm
/hold

For QE verification in case we're missing something

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

openshift-ci-robot commented Sep 29, 2023

@cgwalters: This pull request references MCO-424 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.15.0" version, but no target version was set.

In response to this:

xref https://github.com/openshift/enhancements/blob/master/enhancements/ocp-coreos-layering/ocp-coreos-layering.md

This is part of https://issues.redhat.com/browse/MCO-392

Since we've landed the new format image usage in the latest nightly, it's time to try dropping it from our image references entirely.

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, travier, yuqi-zhang

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:
  • OWNERS [cgwalters,yuqi-zhang]

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

@cgwalters
Copy link
Member Author

For QE verification in case we're missing something

In this case what would QE do that would not be covered by presubmits?

@yuqi-zhang
Copy link
Contributor

I thought of it as they have a larger test suite compared to CI, but I guess that might not be relevant for this PR. Feel free to unhold if we believe this doesn't require additional testing

@cgwalters
Copy link
Member Author

cgwalters commented Oct 2, 2023

Yes I think QE capacity is better focused on other things honestly (like #3865 )

At this point the technical debt from this dis-proportionally impacts the CoreOS team. But I also know if something breaks from this the first bug contact will be the MCO team as usual.

Dunno, bottom line we can see if someone from the QE team says "yes there's some test we have which may be relevant" or not. We have a large complex system, in theory somehow maybe somehow this breaks scaleup from old bootimages for example, but I don't think that's likely.

@yuqi-zhang
Copy link
Contributor

/hold cancel

I'm convinced 👍

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 2, 2023

@cgwalters: This pull request references MCO-424 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.15.0" version, but no target version was set.

In response to this:

xref https://github.com/openshift/enhancements/blob/master/enhancements/ocp-coreos-layering/ocp-coreos-layering.md

This is part of https://issues.redhat.com/browse/MCO-392

Since we've landed the new format image usage in the latest nightly, it's time to try dropping it from our image references entirely.

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.

@openshift-ci openshift-ci bot merged commit 67767a5 into openshift:master Oct 2, 2023
14 checks passed
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants