Skip to content

Conversation

@rccrdpccl
Copy link
Contributor

OKD doesn't use rhel-coreos, it uses stream-coreos instead

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@rccrdpccl rccrdpccl requested a review from carbonin April 11, 2025 10:20
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 11, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 11, 2025

@rccrdpccl: This pull request references MGMT-20369 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 bug to target the "4.19.0" version, but no target version was set.

Details

In response to this:

OKD doesn't use rhel-coreos, it uses stream-coreos instead

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2025
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2025
@rccrdpccl
Copy link
Contributor Author

/cc @jianzzha

@openshift-ci
Copy link

openshift-ci bot commented Apr 11, 2025

@rccrdpccl: GitHub didn't allow me to request PR reviews from the following users: jianzzha.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @jianzzha

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-sigs/prow repository.

@rccrdpccl
Copy link
Contributor Author

/retest

@codecov
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.30%. Comparing base (abd0143) to head (9a1128d).
Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
internal/oc/release.go 75.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7534      +/-   ##
==========================================
- Coverage   67.30%   67.30%   -0.01%     
==========================================
  Files         334      335       +1     
  Lines       42299    42486     +187     
==========================================
+ Hits        28470    28595     +125     
- Misses      11257    11306      +49     
- Partials     2572     2585      +13     
Files with missing lines Coverage Δ
internal/oc/release.go 75.45% <75.00%> (+2.06%) ⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rccrdpccl
Copy link
Contributor Author

/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 Apr 14, 2025
@rccrdpccl rccrdpccl force-pushed the handle-OKD-release-image branch from bfab14b to bc0632d Compare April 16, 2025 15:24
@rccrdpccl
Copy link
Contributor Author

/unhold
/retest

@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 Apr 16, 2025
@rccrdpccl
Copy link
Contributor Author

/retest

Copy link
Member

Choose a reason for hiding this comment

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

Is this consistent with how we're checking for OKD in other places? I remember seeing a check somewhere but I don't know how it's implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find any other parts where we check for OKD, apart from the OKD-RPMS image (which I think it's not really equivalent to this). Alternatively we could just try both and pick the first that works and not make it dependant on the release image (name). WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I actually think that is how we use that function. The problem is that we end up running it all the time which isn't great but ... I suppose it depends on how confident we are in the release image naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Prashanth684 would you have any recommendation on how to detect reliably OKD release image?

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately, there is no easy way to detect if release image is OKD outside of doing what oc does which is read the release image, parse the metadata and maybe check the componentversion to see if there is centos stream coreos string present there. The only way today is like you suggest to check for both and see which one works.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is checking the pullspec string right? so if a pullspec with digest is provided, this won't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, what you say is correct - this should be changed.
Currently we check for the image tagged okd-rpms within the release image (oc adm release info --image-for=okd-rpms --insecure=false <release image>) - however this seems that it's not working any longer from release 4.16 onward. This would mean OKD assisted installs for OCP > 4.15 are not working properly. Could you please confirm that this method is no longer valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

right...we don't build the okd-rpms image anymore. The openshift binaries are pulled from ART's mirror (for now).

Copy link
Contributor

Choose a reason for hiding this comment

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

you could check for stream-coreos instead as that is specific to OKD.

@rccrdpccl rccrdpccl force-pushed the handle-OKD-release-image branch from bc0632d to 9a1128d Compare April 28, 2025 13:54
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2025
@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, rccrdpccl

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

@carbonin
Copy link
Member

Looks like the OKD-specific job has been failing for quite a while now so it's unlikely that it's a problem with this PR

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 8f3da5d and 2 for PR HEAD 9a1128d in total

@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2025

@rccrdpccl: 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/okd-scos-e2e-aws-ovn 9a1128d link false /test okd-scos-e2e-aws-ovn

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

@openshift-merge-bot openshift-merge-bot bot merged commit 20e7243 into openshift:master Apr 28, 2025
19 of 20 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-api-server
This PR has been included in build ose-agent-installer-api-server-container-v4.20.0-202504290242.p0.g20e7243.assembly.stream.el9.
All builds following this will include this PR.

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants