OKD-322: Update UPI dockerfile to work on CentOS/RHEL 10#10322
OKD-322: Update UPI dockerfile to work on CentOS/RHEL 10#10322jatinsu wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jatinsu: This pull request references OKD-322 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
jatinsu
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| yum install --setopt=tsflags=nodocs -y \ | ||
| gettext \ | ||
| google-cloud-cli-447.0.0-1 \ | ||
| google-cloud-cli \ |
There was a problem hiding this comment.
447.0.0-1 isn't in the rhel 10 repo anymore. This pin is related to https://issues.redhat.com/browse/OCPBUGS-22830. How can I test to see if this issue still exists?
There was a problem hiding this comment.
Just bump it to a newer version (you should pin it to a specific version so it doesn't break) and you can run a gcp upi presubmit test on the pr
There was a problem hiding this comment.
@jatinsu can you pin this to a specific version please?
There was a problem hiding this comment.
Just pinned it. I had to move the version check into the install command since rhel 9 doesn't have 561 (I also made the if condition to be much simpler, I didn't notice that there was a platform ID)
|
@jatinsu: This pull request references OKD-322 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 story to target the "4.22.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
| RUN . /etc/os-release && \ | ||
| MAJOR_VERSION=$(echo "${VERSION_ID}" | cut -d. -f1) && \ | ||
| if [ "${MAJOR_VERSION}" = "10" ]; then \ | ||
| sh -c 'echo -e "[google-cloud-cli]\nname=Google Cloud CLI\nbaseurl=https://packages.cloud.google.com/yum/repos/cloud-sdk-el10-x86_64\nenabled=1\ngpgcheck=1\nrepo_gpgcheck=0\ngpgkey=https://packages.cloud.google.com/yum/doc/rpm-package-key-v10.gpg" > /etc/yum.repos.d/google-cloud-sdk.repo'; \ | ||
| else \ | ||
| sh -c 'echo -e "[google-cloud-cli]\nname=Google Cloud CLI\nbaseurl=https://packages.cloud.google.com/yum/repos/cloud-sdk-el9-x86_64\nenabled=1\ngpgcheck=1\nrepo_gpgcheck=0\ngpgkey=https://packages.cloud.google.com/yum/doc/rpm-package-key.gpg" > /etc/yum.repos.d/google-cloud-sdk.repo'; \ | ||
| fi | ||
|
|
There was a problem hiding this comment.
Isn't this image always rhel9 (see the builder image on line 9? I don't think we want to make this conditional
There was a problem hiding this comment.
OKD is attempting to move to CentOS 10, and the way we do this is by replacing the base image with a cs10 image. That's why there's a conditional. If you prefer I could do it based of a installation failure similar to what is done here.
There was a problem hiding this comment.
@patrickdillon Sorry, would you mind taking a look again? I can change the conditional to what's done here if needed
There was a problem hiding this comment.
OKD is attempting to move to CentOS 10, and the way we do this is by replacing the base image with a cs10 image.
This UPI image is only used to host tools to interact with cloud sdks and do tasks in CI steps... the base image here shouldn't really have any effect on okd?
There was a problem hiding this comment.
This UPI image is only used to host tools to interact with cloud sdks and do tasks in CI steps... the base image here shouldn't really have any effect on okd?
So the reason why the base image affects OKD is because this UPI image is actually built separately for OKD, and the UPI installer fails at build time when the base image is swapped to CentOS 10. Do you think it'd make more sense to keep the base image as is? I think art has historically always replaced it iirc.
Here's the failure logs that happen when we swap the base image to CentOS 10 if you want some more context. Sorry for the initial confusion!
There was a problem hiding this comment.
Yes i think that makes more sense, because we would be removing an incorrect config and not adding more complexity.
This upi image should be changed ao we mirror it rather than build in ci
There was a problem hiding this comment.
I say “incorrect “ because this image really has no consequence on okd, the installer is copied in from another image
There was a problem hiding this comment.
Just got back from the ART team. I'll close this PR, we'll continue with using rhel9. Thanks for looking at this!
WalkthroughUpdated the CI builder base image to a newer version (golang 1.25, OpenShift 4.22). Made google-cloud-cli installation platform-aware, using different versions and repository configurations for EL10 versus EL9 systems based on Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
images/installer/Dockerfile.upi.ci (1)
52-59: Fail fast for unsupported platforms instead of defaulting to EL9.Right now, any non-
platform:el10value falls into the EL9 branch. That can mask future base-image changes and produce confusing package failures. Prefer explicitel9/el10handling with an error for anything else.Suggested refactor
RUN . /etc/os-release && \ - if [ "${PLATFORM_ID}" = "platform:el10" ]; then \ + if [ "${PLATFORM_ID}" = "platform:el10" ]; then \ echo -e "[google-cloud-cli]\nname=Google Cloud CLI\nbaseurl=https://packages.cloud.google.com/yum/repos/cloud-sdk-el10-x86_64\nenabled=1\ngpgcheck=1\nrepo_gpgcheck=0\ngpgkey=https://packages.cloud.google.com/yum/doc/rpm-package-key-v10.gpg" > /etc/yum.repos.d/google-cloud-sdk.repo; \ GOOGLE_CLOUD_CLI_PKG="google-cloud-cli-561.0.0-1.el10"; \ - else \ + elif [ "${PLATFORM_ID}" = "platform:el9" ]; then \ echo -e "[google-cloud-cli]\nname=Google Cloud CLI\nbaseurl=https://packages.cloud.google.com/yum/repos/cloud-sdk-el9-x86_64\nenabled=1\ngpgcheck=1\nrepo_gpgcheck=0\ngpgkey=https://packages.cloud.google.com/yum/doc/rpm-package-key.gpg" > /etc/yum.repos.d/google-cloud-sdk.repo; \ GOOGLE_CLOUD_CLI_PKG="google-cloud-cli-447.0.0-1"; \ + else \ + echo "Unsupported PLATFORM_ID: ${PLATFORM_ID}" >&2; \ + exit 1; \ fi && \As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/installer/Dockerfile.upi.ci` around lines 52 - 59, The current conditional treats any non-`platform:el10` as EL9 which can mask unsupported platforms; modify the conditional around the sourced PLATFORM_ID (from /etc/os-release) to explicitly handle `platform:el10` and `platform:el9` (setting the google-cloud repo content and GOOGLE_CLOUD_CLI_PKG accordingly) and add an else branch that prints a clear error to stderr and exits non-zero to fail fast for unsupported PLATFORM_ID values; update the block that writes /etc/yum.repos.d/google-cloud-sdk.repo and sets GOOGLE_CLOUD_CLI_PKG to reflect these three explicit branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@images/installer/Dockerfile.upi.ci`:
- Around line 52-59: The current conditional treats any non-`platform:el10` as
EL9 which can mask unsupported platforms; modify the conditional around the
sourced PLATFORM_ID (from /etc/os-release) to explicitly handle `platform:el10`
and `platform:el9` (setting the google-cloud repo content and
GOOGLE_CLOUD_CLI_PKG accordingly) and add an else branch that prints a clear
error to stderr and exits non-zero to fail fast for unsupported PLATFORM_ID
values; update the block that writes /etc/yum.repos.d/google-cloud-sdk.repo and
sets GOOGLE_CLOUD_CLI_PKG to reflect these three explicit branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cddfd450-de10-4a28-b86b-6ad37e10cbfa
📒 Files selected for processing (1)
images/installer/Dockerfile.upi.ci
|
/retest-required |
1 similar comment
|
/retest-required |
|
@jatinsu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Closed as we will be using a rhel9 base image for Upi-installer |
3 participants
GCloud cli repo has changed for CentOS/RHEL 10 based images. Documentation that I used to update this is here: https://docs.cloud.google.com/sdk/docs/install-sdk#rpm. The reason for this PR now is because OKD is attempting to use CentOS 10 base images in 4.22 onward, and the upi installer is failing because of the outdated Gcloud repo.