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

Remove old RHCOS metadata (not stream metadata) #5252

Merged

Conversation

cgwalters
Copy link
Member

In openshift/enhancements#679
we landed support for a stream metadata format already used
by FCOS and now by RHCOS/OCP consumers to find the bootimages.

We kept the old metadata because the UPI CI jobs used it.
In openshift/release#17482 I tried
to port the UPI CI jobs, but ran into huge levels of technical debt.

As best I can tell, the UPI CI jobs are not running on this repo
now and are likely broken for other reasons.

Let's remove the old data and avoid the confusing duplication.
Anyone who goes to work on the UPI CI jobs and sanitizes things
should be able to pick up the work to port to stream metadata
relatively easily.

@miabbott
Copy link
Member

/test help

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2021

@miabbott: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test e2e-aws
  • /test e2e-aws-upgrade
  • /test e2e-gcp-upgrade
  • /test e2e-metal-ipi-ovn-ipv6-required
  • /test gofmt
  • /test golint
  • /test govet
  • /test images
  • /test okd-images
  • /test okd-unit
  • /test okd-verify-codegen
  • /test openstack-manifests
  • /test shellcheck
  • /test tf-lint
  • /test unit
  • /test verify-codegen
  • /test verify-vendor
  • /test yaml-lint

The following commands are available to trigger optional jobs:

  • /test e2e-aws-disruptive
  • /test e2e-aws-fips
  • /test e2e-aws-proxy
  • /test e2e-aws-rhel8
  • /test e2e-aws-shared-vpc
  • /test e2e-aws-single-node
  • /test e2e-aws-upi
  • /test e2e-aws-workers-rhel7
  • /test e2e-aws-workers-rhel8
  • /test e2e-azure
  • /test e2e-azure-resourcegroup
  • /test e2e-azure-shared-vpc
  • /test e2e-azure-upi
  • /test e2e-azurestack
  • /test e2e-azurestack-upi
  • /test e2e-crc
  • /test e2e-gcp
  • /test e2e-gcp-shared-vpc
  • /test e2e-gcp-upi
  • /test e2e-gcp-upi-xpn
  • /test e2e-kubevirt
  • /test e2e-libvirt
  • /test e2e-metal
  • /test e2e-metal-assisted
  • /test e2e-metal-ipi
  • /test e2e-metal-ipi-ovn-dualstack
  • /test e2e-metal-ipi-ovn-ipv6
  • /test e2e-metal-ipi-virtualmedia
  • /test e2e-metal-single-node-live-iso
  • /test e2e-openstack
  • /test e2e-openstack-kuryr
  • /test e2e-openstack-parallel
  • /test e2e-openstack-proxy
  • /test e2e-openstack-upi
  • /test e2e-ovirt
  • /test e2e-vsphere
  • /test e2e-vsphere-upi
  • /test okd-e2e-aws
  • /test okd-e2e-aws-upgrade
  • /test okd-e2e-gcp
  • /test okd-e2e-gcp-upgrade
  • /test okd-e2e-vsphere
  • /test tf-fmt

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-installer-master-e2e-aws
  • pull-ci-openshift-installer-master-e2e-aws-fips
  • pull-ci-openshift-installer-master-e2e-aws-single-node
  • pull-ci-openshift-installer-master-e2e-aws-upgrade
  • pull-ci-openshift-installer-master-e2e-aws-workers-rhel7
  • pull-ci-openshift-installer-master-e2e-aws-workers-rhel8
  • pull-ci-openshift-installer-master-e2e-crc
  • pull-ci-openshift-installer-master-e2e-libvirt
  • pull-ci-openshift-installer-master-e2e-metal-ipi-ovn-ipv6
  • pull-ci-openshift-installer-master-e2e-metal-single-node-live-iso
  • pull-ci-openshift-installer-master-e2e-openstack
  • pull-ci-openshift-installer-master-e2e-openstack-kuryr
  • pull-ci-openshift-installer-master-e2e-ovirt
  • pull-ci-openshift-installer-master-gofmt
  • pull-ci-openshift-installer-master-golint
  • pull-ci-openshift-installer-master-govet
  • pull-ci-openshift-installer-master-images
  • pull-ci-openshift-installer-master-okd-images
  • pull-ci-openshift-installer-master-okd-unit
  • pull-ci-openshift-installer-master-okd-verify-codegen
  • pull-ci-openshift-installer-master-openstack-manifests
  • pull-ci-openshift-installer-master-shellcheck
  • pull-ci-openshift-installer-master-tf-fmt
  • pull-ci-openshift-installer-master-tf-lint
  • pull-ci-openshift-installer-master-unit
  • pull-ci-openshift-installer-master-verify-codegen
  • pull-ci-openshift-installer-master-verify-vendor
  • pull-ci-openshift-installer-master-yaml-lint

In response to this:

/test help

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.

@miabbott
Copy link
Member

/test e2e-aws-upi

let's just see how a UPI job handles this

@bgilbert
Copy link
Contributor

Should this PR remove hack/update-rhcos-bootimage.py also?

@miabbott
Copy link
Member

/retest

@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from e0baa27 to e1fa5e6 Compare September 30, 2021 13:41
@cgwalters
Copy link
Member Author

Should this PR remove hack/update-rhcos-bootimage.py also?

Yep, done!

Bigger picture, I think the next step here is adding a small controller process that generates the merged stream metadata server side by default, then we just copy from that.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM

@cgwalters
Copy link
Member Author

All the CI failures here seem unrelated to this PR; for e.g. e2e-aws-fips we got into cluster provisioning but failed tests, and there's no way this PR can cause test failures. traditional rhel worker jobs are failing to find packages. The UPI job seems to have bitrotted enough it fails trying to run much earlier than the code would be looking for the old rhcos.json.

Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

I am on board with ripping out the old metadata. I will create a card for the installer team to update the CI jobs appropriately.

This needs a rebase.

/approve

images/installer/Dockerfile.upi.ci.rhel8 Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 21, 2021
@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from e1fa5e6 to a6d3185 Compare October 21, 2021 20:12
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021
@cgwalters
Copy link
Member Author

cgwalters commented Oct 21, 2021

  • Rebased 🏄
  • Addressed comments
  • Now also removes old FCOS data
  • Move the config to a new subdir with an OWNERS file

@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from a6d3185 to 4437e49 Compare October 21, 2021 20:25
@cgwalters
Copy link
Member Author

Oops, right.

Man, I love that when Prow comments like:

  • This looks broken
    (second later)
  • This looks quite broken
    (second later)
  • WHAT IS THIS GARBAGE
    (second later)
  • I CAN'T EVEN
    (second later)
  • I'M DONE WITH HUMANS, GOING TO GO TALK TO SKYNET ABOUT THIS
    (ominous silence)

In openshift/enhancements#679
we landed support for a stream metadata format already used
by FCOS and now by RHCOS/OCP consumers to find the bootimages.

We kept the old metadata because the UPI CI jobs used it.
In openshift/release#17482 I tried
to port the UPI CI jobs, but ran into huge levels of technical debt.

As best I can tell, the UPI CI jobs are not running on this repo
now and are likely broken for other reasons.

Let's remove the old data and avoid the confusing duplication.
Anyone who goes to work on the UPI CI jobs and sanitizes things
should be able to pick up the work to port to stream metadata
relatively easily.
@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from 4437e49 to 73f13b8 Compare October 21, 2021 21:09
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM. Note that stream-metadata-rust will need the RHCOS stream metadata URL updated. Luckily it's retrieved there in a release-specific way.

Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/approve

I already approved earlier, but I wanted to make it explicitly that I also approve of the added commit the coreos team as owners.

data/data/coreos/OWNERS Outdated Show resolved Hide resolved
@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from 73f13b8 to 7699a8d Compare October 26, 2021 18:12
Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

Some minor nits but LGTM.

data/data/coreos/OWNERS Outdated Show resolved Hide resolved
OWNERS_ALIASES Outdated Show resolved Hide resolved
OWNERS_ALIASES Outdated Show resolved Hide resolved
It's more logically owned by the CoreOS team and this will
allow us to have a separate `OWNERS` file.

The `OWNERS` file is copied from the current one in openshift/os.
@cgwalters cgwalters force-pushed the remove-old-rhcos-json branch from 7699a8d to 61605bd Compare October 27, 2021 20:37
@cgwalters
Copy link
Member Author

Some minor nits but LGTM.

Thanks, fixed those.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

13 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2021

@cgwalters: 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-aws-upi e0baa27aa90d0b6716494563815e7a82b3b88319 link false /test e2e-aws-upi
ci/prow/e2e-aws-workers-rhel8 61605bd link false /test e2e-aws-workers-rhel8
ci/prow/e2e-aws-workers-rhel7 61605bd link false /test e2e-aws-workers-rhel7
ci/prow/e2e-crc 61605bd link false /test e2e-crc

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-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 7f60703 into openshift:master Oct 30, 2021
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Nov 30, 2021
This is only needed in rhcos.sh for old versions which lack the
openshift-install coreos-print-stream-json option, and the
file moved in openshift/installer#5252
so we should only copy if the "old" location is detected
openshift-merge-robot pushed a commit to openshift-metal3/dev-scripts that referenced this pull request Dec 3, 2021
This is only needed in rhcos.sh for old versions which lack the
openshift-install coreos-print-stream-json option, and the
file moved in openshift/installer#5252
so we should only copy if the "old" location is detected
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.

7 participants