feat(env-check): remove dependency of system package manager from node env checker #3406
Conversation
WalkthroughThe changes modify the Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
controller/monitor/environment_check_monitor.go (6)
181-182: Use a more descriptive variable name for clarity
The variablepackageProbeExecutablesis fine, but consider a shorter or more descriptive name (e.g.,pkgExecutables) for readability.
204-207: Harmonize package naming and definitions
For CentOS/Fedora/Red Hat-based systems, the listed package keys ("nfs-utils", "iscsi-initiator-utils") are correct, but remember to verify that each distribution consistently uses these names. Some variants might use alternative naming.
238-239: Initialize with capacity
As an optional optimization, consider initializing the slices with the approximate capacity to reduce allocations if you expect a stable set of packages.
240-241: Use a single statement for shellCmd
DefiningshellCmdis concise, but you may inline it directly into the method call for clarity since it is used only once.
244-244: Log error details
When a package is not found, consider logging the actual stderr or the failing command's output to aid troubleshooting.
247-247: Add debug logs for installed packages
You might want to add a debug log here to confirm that<pkg>was successfully recognized/installed to further assist with troubleshooting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/monitor/environment_check_monitor.go(4 hunks)
🔇 Additional comments (6)
controller/monitor/environment_check_monitor.go (6)
191-194: Consistent naming for package keys
The package keys ("nfs-common", "open-iscsi", etc.) align with Ubuntu/Debian naming, which is clear. Ensure these name-to-executable mappings match typical Debian package naming conventions to avoid ambiguity.
209-212: Ensure correct SUSE naming
The SUSE-based distributions commonly include these packages under these names. Make sure you’ve validated each SUSE variant (e.g., SLES vs. openSUSE) still uses "nfs-client" and "open-iscsi".
214-217: Arch-based distributions
These mapping entries look correct for Arch-based distributions. The typical package naming is "nfs-utils", "open-iscsi", etc.
219-222: Gentoo naming
Gentoo uses category-based naming. The definitions here ("net-fs/nfs-utils", etc.) appear consistent with standard Gentoo naming conventions.
250-253: Combine messages for multiple missing packages
When multiple packages are missing, the code uses a single message summarizing them. This is a good practice that prevents multiple conditions from being updated repeatedly.
258-258: Confirmation message for installed packages
The success condition states that all required packages are installed. This is clear and helpful for diagnosing environment completeness at a glance.
COLDTURNIP
force-pushed
the
10019-pkg_check_without_pkg_mgr
branch
from
bedf1fa to
d5266d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controller/monitor/environment_check_monitor.go (2)
Line range hint
191-222: Consider adding package name commentsThe package-to-executable mapping is comprehensive but could benefit from comments explaining why specific executables were chosen for verification, especially for packages that have multiple potential executables.
Apply this diff to improve documentation:
case strings.Contains(osImage, "ubuntu"): fallthrough case strings.Contains(osImage, "debian"): - packageProbeExecutables["nfs-common"] = "mount.nfs" - packageProbeExecutables["open-iscsi"] = "iscsiadm" - packageProbeExecutables["cryptsetup"] = "cryptsetup" - packageProbeExecutables["dmsetup"] = "dmsetup" + // NFS client support + packageProbeExecutables["nfs-common"] = "mount.nfs" + // iSCSI initiator support + packageProbeExecutables["open-iscsi"] = "iscsiadm" + // Disk encryption support + packageProbeExecutables["cryptsetup"] = "cryptsetup" + // Device mapper support + packageProbeExecutables["dmsetup"] = "dmsetup"
344-360: LGTM: Well-implemented helper functionThe
checkPackageInstalledfunction is well-structured with appropriate error handling and debug logging. The use ofsh -cfor executing thecommandshell function is correct.Two minor suggestions for consideration:
- Consider adding a context parameter for timeout/cancellation control
- Consider sorting the returned package lists for consistent output ordering
-func (m *EnvironmentCheckMonitor) checkPackageInstalled(packageProbeExecutables map[string]string, namespaces []lhtypes.Namespace) (installed, notInstalled []string, err error) { +func (m *EnvironmentCheckMonitor) checkPackageInstalled(ctx context.Context, packageProbeExecutables map[string]string, namespaces []lhtypes.Namespace) (installed, notInstalled []string, err error) { nsexec, err := lhns.NewNamespaceExecutor(lhtypes.ProcessNone, lhtypes.HostProcDirectory, namespaces) if err != nil { return nil, nil, err } for pkg, executable := range packageProbeExecutables { shellCmd := fmt.Sprintf("command -v %s", executable) - if _, err := nsexec.Execute(nil, "sh", []string{"-c", shellCmd}, lhtypes.ExecuteDefaultTimeout); err != nil { + if _, err := nsexec.Execute(ctx, "sh", []string{"-c", shellCmd}, lhtypes.ExecuteDefaultTimeout); err != nil { m.logger.WithError(err).Debugf("Package %v is not found", pkg) notInstalled = append(notInstalled, pkg) } else { installed = append(installed, pkg) } } + sort.Strings(installed) + sort.Strings(notInstalled) return installed, notInstalled, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/monitor/environment_check_monitor.go(4 hunks)
🔇 Additional comments (2)
controller/monitor/environment_check_monitor.go (2)
181-182: LGTM: Improved package verification approach
The new approach of using executable probing instead of package manager queries is more reliable and distribution-agnostic. This change successfully removes the dependency on system package managers.
230-246: Verify error handling for command execution
The error handling for package verification looks good, but we should ensure the command execution timeout is appropriate for slower systems.
✅ Verification successful
Let me gather more information about the package verification implementation to understand the command execution context better.
Let me try a different approach to find the package verification implementation.
The default timeout of 1 minute is appropriate for package verification
The package verification implementation uses a simple command -v check which is a lightweight operation that typically completes within milliseconds. The current timeout of 1 minute (defined as ExecuteDefaultTimeout = time.Minute) provides more than sufficient time even for slower systems, considering:
- The command being executed is a basic shell built-in command
- It only checks for executable existence in PATH
- No heavy I/O or computation is involved
- The operation is performed sequentially for each package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the default timeout is sufficient for command execution
# Test: Search for timeout definition
rg -A 2 'ExecuteDefaultTimeout.*='
Length of output: 1394
Script:
#!/bin/bash
# Check the package verification implementation
ast-grep --pattern 'func (m *Monitor) checkPackageInstalled($$$) ($$$) {
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Search for the checkPackageInstalled function implementation
rg -A 20 "func.*checkPackageInstalled"
Length of output: 2032
|
@mergify backport v1.7.x |
✅ Backports have been created
|
|
@mergify backport v1.8.x |
✅ Backports have been created
|
…nager longhorn/longhorn-10019 Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
10019-pkg_check_without_pkg_mgr
branch
from
d5266d3 to
66a361b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controller/monitor/environment_check_monitor.go (1)
Line range hint
181-222: LGTM! Robust approach to package verification.The new implementation that verifies packages by checking for their executables is more reliable than querying package managers. This approach:
- Reduces dependency on system package managers
- Handles different package names across distributions
- Focuses on actual executable availability rather than package installation status
Consider enhancing the error message for unknown OS.
The error message could be more helpful by listing the required executables instead of just mentioning "required packages".
- fmt.Sprintf("Unable to verify the required packages because the OS image '%v' is unknown to the Longhorn system. Please ensure the required packages are installed.", osImage)) + fmt.Sprintf("Unable to verify the required packages because the OS image '%v' is unknown to the Longhorn system. Please ensure the following executables are available: mount.nfs, iscsiadm, cryptsetup, dmsetup", osImage))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/monitor/environment_check_monitor.go(4 hunks)
🔇 Additional comments (2)
controller/monitor/environment_check_monitor.go (2)
344-360: LGTM! Well-implemented package verification.
The implementation:
- Uses the portable
command -vinstead ofwhich - Properly handles errors and provides debug logging
- Returns both installed and not installed packages for comprehensive status reporting
238-246: LGTM! Improved condition messages.
The condition messages now provide more detailed information by:
- Listing missing packages when verification fails
- Including the list of successfully installed packages in the success message
longhorn/longhorn-10019 Signed-off-by: Raphanus Lo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controller/monitor/environment_check_monitor.go (2)
224-225: Consider logging unsupported OS for troubleshooting.While skipping the environment check for unsupported OS is appropriate, consider adding a debug log to help with troubleshooting.
default: - // unsupported host platform, skip environment check condition + m.logger.Debugf("Skipping environment check for unsupported OS: %s", osImage) return
342-358: Enhance error handling and logging in checkPackageInstalled.The function implementation is good, but could be improved with more specific error handling and detailed logging.
func (m *EnvironmentCheckMonitor) checkPackageInstalled(packageProbeExecutables map[string]string, namespaces []lhtypes.Namespace) (installed, notInstalled []string, err error) { nsexec, err := lhns.NewNamespaceExecutor(lhtypes.ProcessNone, lhtypes.HostProcDirectory, namespaces) if err != nil { - return nil, nil, err + return nil, nil, errors.Wrap(err, "failed to create namespace executor") } for pkg, executable := range packageProbeExecutables { shellCmd := fmt.Sprintf("command -v %s", executable) if _, err := nsexec.Execute(nil, "sh", []string{"-c", shellCmd}, lhtypes.ExecuteDefaultTimeout); err != nil { - m.logger.WithError(err).Debugf("Package %v is not found", pkg) + m.logger.WithError(err).Debugf("Package %v (executable: %v) is not found", pkg, executable) notInstalled = append(notInstalled, pkg) } else { installed = append(installed, pkg) } } return installed, notInstalled, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/monitor/environment_check_monitor.go(4 hunks)
🔇 Additional comments (2)
controller/monitor/environment_check_monitor.go (2)
Line range hint 181-224: LGTM! Improved package verification approach.
The new implementation correctly maps packages to their corresponding executables for each supported OS distribution. This approach is more reliable as it directly verifies the availability of required executables rather than depending on package manager queries.
Line range hint 191-222: Verify package mappings across distributions.
The package-to-executable mappings look correct for each distribution. However, let's verify the consistency of required executables across all distributions.
✅ Verification successful
Package-to-executable mappings are consistent across distributions
The verification confirms that all distributions check for the same set of core executables:
mount.nfsfor NFS support (via different packages: nfs-utils, nfs-common, nfs-client, net-fs/nfs-utils)iscsiadmfor iSCSI support (via different packages: open-iscsi, iscsi-initiator-utils, sys-block/open-iscsi)cryptsetupfor encryption support (via packages: cryptsetup, sys-fs/cryptsetup)dmsetupfor device mapper support (via different packages: device-mapper, dmsetup, sys-fs/lvm2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all distributions check for the same set of core executables
# Expected: All distributions should check for mount.nfs, iscsiadm, cryptsetup, and dmsetup
echo "Analyzing package probe executables across distributions..."
rg -A 4 'packageProbeExecutables\[.*\] = ".*"' | sort | uniq -c | sort -nr
Length of output: 2435
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10019
What this PR does / why we need it:
Instead of reading the system package database, check the executables needed by the system, to eliminate the system management tool chain issue.
Special notes for your reviewer:
Additional documentation or context