Drop support for RHEL 8#6130
Conversation
- [MCO-2282] Remove code paths for RHEL8 to RHEL9 upgrades
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThis PR removes RHEL 8 support from machine-config-daemon by eliminating the RHEL 8 builder stage, consolidating SSH key paths to a single constant, removing OS version checks, simplifying SSH key handling logic, and updating daemon behaviors and tests to standardize on RHEL 9+ systems. ChangesDrop RHEL 8 Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: proietfb 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/daemon/update.go (1)
2482-2492:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRe-create
/home/core/.sshexplicitly before the subdir.
mkdir -p -m 0700 /home/core/.ssh/authorized_keys.donly guarantees0700on the finalauthorized_keys.dcomponent. If/home/core/.sshis missing, it gets recreated with the runuser umask, so this helper no longer guarantees the parent-directory permissions its comment promises and the tests assert.Suggested fix
func createSSHKeyDir(authKeyDir string) error { klog.Infof("Creating missing SSH key dir at %q", authKeyDir) mkdir := func(dir string) error { return exec.Command("runuser", "-u", constants.CoreUserName, "--", "mkdir", "-m", "0700", "-p", dir).Run() } - // Create the SSH key directory (/home/core/.ssh/authorized_keys.d). - return mkdir(filepath.Dir(constants.RHCOSDefaultSSHKeyPath)) + if err := mkdir(constants.CoreUserSSHPath); err != nil { + return err + } + return mkdir(authKeyDir) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/daemon/update.go` around lines 2482 - 2492, The createSSHKeyDir function currently only runs mkdir on the leaf directory (using filepath.Dir(constants.RHCOSDefaultSSHKeyPath)), which can leave the parent (/home/core/.ssh) with incorrect permissions; update createSSHKeyDir to explicitly create the parent directory first and then the target subdirectory using the same runuser mkdir wrapper (the inner mkdir closure) so both the parent (/home/core/.ssh) and the subdir (authorized_keys.d) are created with mode 0700; reference the createSSHKeyDir function, the mkdir closure, and constants.RHCOSDefaultSSHKeyPath when making the change.test/helpers/utils.go (1)
1672-1679:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the fallback
os-releasepath in the debug bundle.This list still uses
/usr/lib/osrelease, so failed-node artifacts miss the standard fallback OS release file. Since this hunk is already updating the collected paths, switch it to/usr/lib/os-release.Suggested fix
nodeFilesToWrite := []string{ "/etc/machine-config-daemon/currentconfig", "/etc/os-release", - "/usr/lib/osrelease", + "/usr/lib/os-release", constants.RHCOSDefaultSSHKeyPath, "/etc/machine-config-daemon/node-annotation.json.bak", "/etc/ignition-machine-config-encapsulated.json.bak", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/utils.go` around lines 1672 - 1679, The nodeFilesToWrite slice in test/helpers/utils.go contains an incorrect fallback path "/usr/lib/osrelease"; update that entry to the standard "/usr/lib/os-release" so the debug bundle includes the proper fallback os-release file. Locate the nodeFilesToWrite variable and replace the "/usr/lib/osrelease" string with "/usr/lib/os-release" (the rest of the slice entries remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-1of2/mcd_test.go`:
- Around line 317-320: The call to helpers.WaitForRenderedConfig ignores its
error return causing oldInfraRenderedConfig to be empty on failure; change the
call to capture the error (e.g., oldInfraRenderedConfig, err :=
helpers.WaitForRenderedConfig(...)) and fail fast (use
require.NoError/require.Nil or t.Fatalf) immediately after to stop the test when
the baseline render does not complete; update references to
oldInfraRenderedConfig accordingly so subsequent rollback logic uses a valid
rendered config.
---
Outside diff comments:
In `@pkg/daemon/update.go`:
- Around line 2482-2492: The createSSHKeyDir function currently only runs mkdir
on the leaf directory (using filepath.Dir(constants.RHCOSDefaultSSHKeyPath)),
which can leave the parent (/home/core/.ssh) with incorrect permissions; update
createSSHKeyDir to explicitly create the parent directory first and then the
target subdirectory using the same runuser mkdir wrapper (the inner mkdir
closure) so both the parent (/home/core/.ssh) and the subdir (authorized_keys.d)
are created with mode 0700; reference the createSSHKeyDir function, the mkdir
closure, and constants.RHCOSDefaultSSHKeyPath when making the change.
In `@test/helpers/utils.go`:
- Around line 1672-1679: The nodeFilesToWrite slice in test/helpers/utils.go
contains an incorrect fallback path "/usr/lib/osrelease"; update that entry to
the standard "/usr/lib/os-release" so the debug bundle includes the proper
fallback os-release file. Locate the nodeFilesToWrite variable and replace the
"/usr/lib/osrelease" string with "/usr/lib/os-release" (the rest of the slice
entries remain unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a33d8eb-2950-426d-9716-1b1a923fdb0c
📒 Files selected for processing (10)
DockerfileDockerfile.rhel7pkg/daemon/constants/constants.gopkg/daemon/daemon.gopkg/daemon/osrelease/osrelease.gopkg/daemon/update.gotemplates/common/on-prem/files/NetworkManager-resolv-prepender.yamltest/e2e-1of2/mcd_test.gotest/e2e-single-node/sno_mcd_test.gotest/helpers/utils.go
💤 Files with no reviewable changes (3)
- pkg/daemon/osrelease/osrelease.go
- Dockerfile.rhel7
- Dockerfile
| _, err := cs.MachineConfigs().Create(context.TODO(), oldInfraConfig, metav1.CreateOptions{}) | ||
| require.Nil(t, err) | ||
| oldInfraRenderedConfig, err := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name) | ||
| oldInfraRenderedConfig, _ := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name) | ||
| infraNode := helpers.GetSingleNodeByRole(t, cs, "infra") |
There was a problem hiding this comment.
Fail fast if the baseline render never completes.
If WaitForRenderedConfig fails here, oldInfraRenderedConfig becomes empty and the rollback path later waits for the wrong target, which obscures the original setup failure.
Suggested fix
_, err := cs.MachineConfigs().Create(context.TODO(), oldInfraConfig, metav1.CreateOptions{})
require.Nil(t, err)
- oldInfraRenderedConfig, _ := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name)
+ oldInfraRenderedConfig, err := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name)
+ require.NoError(t, err)
infraNode := helpers.GetSingleNodeByRole(t, cs, "infra")As per coding guidelines, **/*.go: Never ignore error returns.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := cs.MachineConfigs().Create(context.TODO(), oldInfraConfig, metav1.CreateOptions{}) | |
| require.Nil(t, err) | |
| oldInfraRenderedConfig, err := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name) | |
| oldInfraRenderedConfig, _ := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name) | |
| infraNode := helpers.GetSingleNodeByRole(t, cs, "infra") | |
| _, err := cs.MachineConfigs().Create(context.TODO(), oldInfraConfig, metav1.CreateOptions{}) | |
| require.Nil(t, err) | |
| oldInfraRenderedConfig, err := helpers.WaitForRenderedConfig(t, cs, "infra", oldInfraConfig.Name) | |
| require.NoError(t, err) | |
| infraNode := helpers.GetSingleNodeByRole(t, cs, "infra") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e-1of2/mcd_test.go` around lines 317 - 320, The call to
helpers.WaitForRenderedConfig ignores its error return causing
oldInfraRenderedConfig to be empty on failure; change the call to capture the
error (e.g., oldInfraRenderedConfig, err := helpers.WaitForRenderedConfig(...))
and fail fast (use require.NoError/require.Nil or t.Fatalf) immediately after to
stop the test when the baseline render does not complete; update references to
oldInfraRenderedConfig accordingly so subsequent rollback logic uses a valid
rendered config.
|
/payload-jobs periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-azure-mco-disruptive-techpreview-3of3 |
|
/test e2e-openstack |
|
@proietfb: The following test 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. |
1 participant
- What I did
Dropped support for RHEL8
- How to verify it
All CI tests should pass even without RHEL8 custom paths
- Description for the changelog
Summary by CodeRabbit