-
Notifications
You must be signed in to change notification settings - Fork 460
MCO-1976: [Dev] Fix RHEL9-specific MCD Logic for RHEL10/CentOS10 Compatibility #5415
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-1976: [Dev] Fix RHEL9-specific MCD Logic for RHEL10/CentOS10 Compatibility #5415
Conversation
|
@dkhater-redhat: This pull request references MCO-1976 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.21.0" version, but no target version was set. In 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. |
|
@dkhater-redhat: This pull request references MCO-1976 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.21.0" version, but no target version was set. In 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. |
|
@dkhater-redhat: This pull request references MCO-1976 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.21.0" version, but no target version was set. In 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. |
|
@dkhater-redhat: This pull request references MCO-1976 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.21.0" version, but no target version was set. In 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. |
|
@dkhater-redhat: This pull request references MCO-1976 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.21.0" version, but no target version was set. In 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. |
|
/jira refresh |
|
@dkhater-redhat: This pull request references MCO-1976 which is a valid jira issue. In 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. |
7c07907 to
2cabd9c
Compare
|
All password test cases passed using rhel9 and rhel10 After configuring RHEL10 in worker nodes and RHEL9 in master nodes we can see this; Both, master and worker nodes are using the right binary (the already existing one) We have no access to a rhel10 payload, so we couldnt test the changes rhel10 -> rhel10 and rhel10 -> rhel9 No issues where found. /label qe-approved |
|
@dkhater-redhat: This pull request references MCO-1976 which is a valid jira issue. In 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. |
|
/remove-label cherry-pick-approved |
|
@sergiordlr: This PR has been marked as verified by In 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. |
|
/lgtm |
isabella-janssen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Change looks clear & change was verified by QE.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkhater-redhat, isabella-janssen, umohnani8 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:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkhater-redhat, isabella-janssen, umohnani8 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:
Approvers can indicate their approval by writing |
|
@dkhater-redhat: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
| if sourceMajor == "9" && targetMajor == "8" { | ||
|
|
||
| // When container is newer than target, use target-compatible binary | ||
| switch { | ||
| case sourceMajor == "10" && targetMajor == "9": | ||
| sourceBinarySuffix = ".rhel9" | ||
| klog.Info("container is rhel10, target is rhel9") | ||
| case sourceMajor == "10" && targetMajor == "8": | ||
| sourceBinarySuffix = ".rhel8" | ||
| klog.Info("container is rhel10, target is rhel8") | ||
| case sourceMajor == "9" && targetMajor == "8": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my 2 cents, but I don't think continuing with version-specific conditionals is the right direction. This piece of code will need a revisit each time there's a new major, like now.
WDYT about something like this?
diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go
index e990021ed..01170f701 100644
--- a/pkg/daemon/daemon.go
+++ b/pkg/daemon/daemon.go
@@ -14,7 +14,9 @@ import (
"os/exec"
"os/signal"
"path/filepath"
+ "regexp"
"slices"
+ "strconv"
"strings"
"sync"
"syscall"
@@ -531,20 +533,34 @@ func ReexecuteForTargetRoot(target string) error {
return fmt.Errorf("failed to get target OS: %w", err)
}
- var sourceBinarySuffix string
+ sourceBinary := "/usr/bin/machine-config-daemon"
if sourceOsVersion.IsLikeRHEL() && targetOsVersion.IsLikeRHEL() {
- sourceMajor := sourceOsVersion.BaseVersionMajor()
- targetMajor := targetOsVersion.BaseVersionMajor()
- if sourceMajor == "9" && targetMajor == "8" {
- sourceBinarySuffix = ".rhel8"
- klog.Info("container is rhel9, target is rhel8")
+ sourceVersion, err := strconv.Atoi(sourceOsVersion.BaseVersionMajor())
+ if err != nil {
+ return fmt.Errorf("failed to convert source major version: %w", err)
+ }
+ targetVersion, err := strconv.Atoi(targetOsVersion.BaseVersionMajor())
+ if err != nil {
+ return fmt.Errorf("failed to convert target major version: %w", err)
+ }
+ if sourceVersion > targetVersion {
+ suffixes, err := getVersionedBinarySuffixes(sourceBinary + "*")
+ if err != nil {
+ return fmt.Errorf("failed to get machine-config-daemon versioned binaries: %w", err)
+ }
+ suffix, exists := suffixes[targetVersion]
+ if exists {
+ sourceBinary += suffix
+ }
+ klog.Infof("container is rhel%d, target is rhel%d", sourceVersion, targetVersion)
} else {
- klog.Infof("using appropriate binary for source=rhel-%s target=rhel-%s", sourceMajor, targetMajor)
+ klog.Infof("using appropriate binary for source=rhel-%d target=rhel-%d", sourceVersion, targetVersion)
}
+
} else {
klog.Info("assuming we can use container binary chroot() to host")
}
- sourceBinary := "/usr/bin/machine-config-daemon" + sourceBinarySuffix
+
src, err := os.Open(sourceBinary)
if err != nil {
return fmt.Errorf("opening %s: %w", sourceBinary, err)
@@ -596,6 +612,33 @@ func ReexecuteForTargetRoot(target string) error {
return syscall.Exec(targetBin, newArgv, newEnv)
}
+var regexDigits = regexp.MustCompile(`(\d)*$`)
+
+func getVersionedBinarySuffixes(basePath string) (map[int]string, error) {
+ result := map[int]string{}
+ matches, err := filepath.Glob(basePath)
+ if err != nil {
+ return nil, err
+ }
+ for _, match := range matches {
+ extension := filepath.Ext(match)
+ if extension != "" {
+ versionBytes := regexDigits.Find([]byte(extension))
+ if versionBytes == nil {
+ // Skip, is a binary that doesn't have the pattern we are looking for
+ continue
+ }
+
+ version, err := strconv.Atoi(string(versionBytes))
+ if err != nil {
+ return nil, err
+ }
+ result[version] = extension
+ }
+ }
+ return result, nil
+}
+
// worker runs a worker thread that just dequeues items, processes them, and marks them done.
// It enforces that the syncHandler is never invoked concurrently with the same key.
func (dn *Daemon) worker() {|
/retest-required |
|
Spoke with Pablo and I completely agree (and actually considered that when creating this PR, but opted for this method because we have time constraints and the if statement isn't too bad for now). We agreed that we will address this for RHEL 11. We are in a time crunch due to the soft freeze. |
@dkhater-redhat Do you think it's worth creating a tech debt card in Jira to track this to make the proposal is not lost? |
de97fee
into
openshift:main
|
tech debt card to address pablo's callout https://issues.redhat.com/browse/MCO-1989 |
- What I did
Added RHEL10 support to the MCD for SSH key path selection and binary compatibility across RHEL versions.
Changes made:
SSH Key Path Selection (
pkg/daemon/update.go:2291): AddedIsEL10()check touseNewSSHKeyPath()so RHEL10 correctly uses the new SSH key path (/home/core/.ssh/authorized_keys.d/ignition) instead of the legacy path (/home/core/.ssh/authorized_keys).Binary Selection (
pkg/daemon/daemon.go:540-548): ExtendedReexecuteForTargetRoot()to handle RHEL10 containers managing RHEL8 or RHEL9 hosts by selecting the appropriate binary suffix (.rhel8or.rhel9).This addresses items [2] and [4] from the RHEL10 MCD compatibility spike, ensuring the MCD works correctly on RHEL10 and can manage older RHEL versions during dual-stream operations.
- How to verify it
For SSH Key Path (Item 2):
On a RHEL10 cluster:
oc debug node/
chroot /host
ls -la /home/core/.ssh/authorized_keys.d/ignition
For Binary Selection (Item 4):
When RHEL10 container images are available:
oc logs -n openshift-machine-config-operator -l k8s-app=machine-config-daemon | grep "container is rhel10"
- "container is rhel10, target is rhel9" (for RHEL9 nodes)
- "container is rhel10, target is rhel8" (for RHEL8 nodes)
Note: Item 4 also requires Dockerfile updates to include compiled .rhel9 binary when building RHEL10 container images. When the MCO team builds RHEL10-based container images, we will be able to see this change.
- Description for the changelog