fix test and include foremanctl in hosts and satellite_misxins - #22150
Open
nacoool wants to merge 1 commit into
Open
fix test and include foremanctl in hosts and satellite_misxins#22150nacoool wants to merge 1 commit into
nacoool wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideThis PR fixes the Sequence diagram for updated installation health assertionssequenceDiagram
participant SatelliteMixins
participant Host
participant Journald
participant foremanctl
participant SatelliteCLI
SatelliteMixins->>Host: assert_install_assertions()
SatelliteMixins->>Host: is_open(SAT-47736)
alt SAT-47736 not open
SatelliteMixins->>Host: get_service_names()
SatelliteMixins->>Journald: execute(journalctl --grep ERROR service_units)
SatelliteMixins->>Host: execute(grep -iR error /var/log/httpd/*)
SatelliteMixins->>Host: execute(grep -iR error /var/log/candlepin/*)
SatelliteMixins->>Journald: execute(journalctl --unit=httpd)
end
SatelliteMixins->>Host: is_satellitectl_available()
alt satellitectl available
SatelliteMixins->>foremanctl: execute(foremanctl health)
else satellitectl not available
SatelliteMixins->>SatelliteCLI: cli.Health.check()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
is_upstream, the combined status expression(rpm -q product_rpm_name).status and (rpm -q container_rpm_name).status != 0will evaluate to False when the product RPM is missing but the container RPM is present; if the intent is to treat either downstream RPM as making the host non-upstream, you likely want both checks explicitly!= 0rather than chained withand. - In
assert_install_assertions, when usingforemanctl healththe assertion checks'failed=0' in resultinstead ofresult.stdout, which will always be False and should be updated to inspect the command output string. - The new
journalctland httpd/candlepin log checks underSAT-47736are stricter (no filtering of expected transient httpd errors and--grep ERRORinstead of--priority err), which may introduce flakiness; consider preserving the previous transient error filters or documenting why these are safe to tighten.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `is_upstream`, the combined status expression `(rpm -q product_rpm_name).status and (rpm -q container_rpm_name).status != 0` will evaluate to False when the product RPM is missing but the container RPM is present; if the intent is to treat either downstream RPM as making the host non-upstream, you likely want both checks explicitly `!= 0` rather than chained with `and`.
- In `assert_install_assertions`, when using `foremanctl health` the assertion checks `'failed=0' in result` instead of `result.stdout`, which will always be False and should be updated to inspect the command output string.
- The new `journalctl` and httpd/candlepin log checks under `SAT-47736` are stricter (no filtering of expected transient httpd errors and `--grep ERROR` instead of `--priority err`), which may introduce flakiness; consider preserving the previous transient error filters or documenting why these are safe to tighten.
## Individual Comments
### Comment 1
<location path="robottelo/host_helpers/satellite_mixins.py" line_range="583-585" />
<code_context>
+ httpd_log = self.execute('journalctl --unit=httpd')
+ assert 'WARNING' not in httpd_log.stdout
+
+ if self.is_satellitectl_available():
+ result = self.execute('foremanctl health')
+ assert 'failed=0' in result
+ else:
+ result = self.cli.Health.check()
</code_context>
<issue_to_address>
**issue (bug_risk):** Use the command output when checking foremanctl health result.
`execute()` returns an object whose output is in `stdout`, as shown in the `httpd_log` assertion above. Here, the assertion checks `'failed=0' in result` instead of `result.stdout`, which will not correctly validate the command output and may always fail or raise. Please update this to inspect `result.stdout` (e.g. `assert 'failed=0' in result.stdout`).
</issue_to_address>
### Comment 2
<location path="robottelo/host_helpers/satellite_mixins.py" line_range="570-572" />
<code_context>
)
assert not result.stdout
- # Check httpd logs, filtering expected transient startup errors
- # (httpd may start before Foreman/containers are ready, causing brief "Connection refused")
- result = self.execute(r'grep -iR "error" /var/log/httpd/*')
- if result.stdout:
- filtered_errors = [
</code_context>
<issue_to_address>
**suggestion:** Restored httpd error check no longer filters known transient/expected errors.
The prior version excluded known transient startup errors (e.g., connection refused to 127.0.0.1:3000, temporary backend failures). The updated code now fails on any `error` match, which can make tests flaky when those benign cases occur. Please restore targeted filtering so the assertion only fails on unexpected errors.
Suggested implementation:
```python
# Check foreman-installer log
result = self.execute(
r'grep "\[ERROR" --context=100 /var/log/foreman-installer/satellite.log'
)
assert not result.stdout
# Check httpd logs, filtering expected transient startup errors
# (httpd may start before Foreman/containers are ready, causing brief "Connection refused")
result = self.execute(r'grep -iR "error" /var/log/httpd/*')
if result.stdout:
lines = result.stdout.splitlines()
filtered_errors = [
line
for line in lines
if not (
# Known transient startup errors / benign conditions:
"Connection refused" in line
or "connect() failed" in line
or "127.0.0.1:3000" in line
or "127.0.0.1:6000" in line
or "Temporary failure in name resolution" in line
or "proxy: error reading status line from remote server" in line
or "AH01102" in line # proxy: error reading status line from remote server
or "AH00959" in line # failed to make connection to backend
)
]
# Only fail if there are unexpected errors left after filtering
assert not "\n".join(filtered_errors)
```
You may want to refine the filtered error patterns to exactly match the previously accepted transient httpd errors in your codebase or logs (e.g., adjust IP/port combinations or Apache error codes). If there are helper utilities or constants already defined for log filtering elsewhere in robottelo, consider reusing them here instead of hardcoding the patterns.
</issue_to_address>
### Comment 3
<location path="robottelo/hosts.py" line_range="1748-1750" />
<code_context>
:rtype: bool
"""
- return self.execute(f'rpm -q {self.product_rpm_name}').status != 0
+ return (
+ self.execute(f'rpm -q {self.product_rpm_name}').status
+ and self.execute(f'rpm -q {self.container_rpm_name}').status
+ ) != 0
</code_context>
<issue_to_address>
**suggestion:** Simplify and clarify upstream detection logic to avoid subtle `and`/int behavior.
The current check depends on `and` returning an integer and on rpm exit codes being strictly 0/non‑0, which is easy to misread. Please rewrite the condition to compare each status explicitly (e.g., check each `.status != 0` and combine them) so the “both RPMs missing means upstream” intent is immediately clear.
</issue_to_address>
### Comment 4
<location path="robottelo/hosts.py" line_range="1809-1818" />
<code_context>
result = self.execute('which foremanctl')
return result.status == 0
+ def is_satellitectl_available(self):
+ """Check if satellitectl is installed on the system.
+
+ Only checks if the command exists, not if the package is available in repos.
+ This ensures auto-detection defaults to satellite-installer on clean systems.
+
+ :return: True if satellite command is installed
+ :rtype: bool
+ """
+ # Check if foremanctl command exists (already installed)
+ result = self.execute('which satellitectl')
+ return result.status == 0
</code_context>
<issue_to_address>
**nitpick:** Align the inline comment with the satellitectl check to avoid confusion.
The inline comment still refers to `foremanctl` while this method checks `satellitectl`. Please update the comment to mention `satellitectl` to clearly distinguish it from `is_foremanctl_available`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+583
to
+585
| if self.is_satellitectl_available(): | ||
| result = self.execute('foremanctl health') | ||
| assert 'failed=0' in result |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Use the command output when checking foremanctl health result.
execute() returns an object whose output is in stdout, as shown in the httpd_log assertion above. Here, the assertion checks 'failed=0' in result instead of result.stdout, which will not correctly validate the command output and may always fail or raise. Please update this to inspect result.stdout (e.g. assert 'failed=0' in result.stdout).
Comment on lines
+570
to
+572
| # Check httpd logs, filtering expected transient startup errors | ||
| # (httpd may start before Foreman/containers are ready, causing brief "Connection refused") | ||
| result = self.execute(r'grep -iR "error" /var/log/httpd/*') |
Contributor
There was a problem hiding this comment.
suggestion: Restored httpd error check no longer filters known transient/expected errors.
The prior version excluded known transient startup errors (e.g., connection refused to 127.0.0.1:3000, temporary backend failures). The updated code now fails on any error match, which can make tests flaky when those benign cases occur. Please restore targeted filtering so the assertion only fails on unexpected errors.
Suggested implementation:
# Check foreman-installer log
result = self.execute(
r'grep "\[ERROR" --context=100 /var/log/foreman-installer/satellite.log'
)
assert not result.stdout
# Check httpd logs, filtering expected transient startup errors
# (httpd may start before Foreman/containers are ready, causing brief "Connection refused")
result = self.execute(r'grep -iR "error" /var/log/httpd/*')
if result.stdout:
lines = result.stdout.splitlines()
filtered_errors = [
line
for line in lines
if not (
# Known transient startup errors / benign conditions:
"Connection refused" in line
or "connect() failed" in line
or "127.0.0.1:3000" in line
or "127.0.0.1:6000" in line
or "Temporary failure in name resolution" in line
or "proxy: error reading status line from remote server" in line
or "AH01102" in line # proxy: error reading status line from remote server
or "AH00959" in line # failed to make connection to backend
)
]
# Only fail if there are unexpected errors left after filtering
assert not "\n".join(filtered_errors)You may want to refine the filtered error patterns to exactly match the previously accepted transient httpd errors in your codebase or logs (e.g., adjust IP/port combinations or Apache error codes). If there are helper utilities or constants already defined for log filtering elsewhere in robottelo, consider reusing them here instead of hardcoding the patterns.
Comment on lines
+1748
to
+1750
| return ( | ||
| self.execute(f'rpm -q {self.product_rpm_name}').status | ||
| and self.execute(f'rpm -q {self.container_rpm_name}').status |
Contributor
There was a problem hiding this comment.
suggestion: Simplify and clarify upstream detection logic to avoid subtle and/int behavior.
The current check depends on and returning an integer and on rpm exit codes being strictly 0/non‑0, which is easy to misread. Please rewrite the condition to compare each status explicitly (e.g., check each .status != 0 and combine them) so the “both RPMs missing means upstream” intent is immediately clear.
Comment on lines
+1809
to
+1818
| def is_satellitectl_available(self): | ||
| """Check if satellitectl is installed on the system. | ||
|
|
||
| Only checks if the command exists, not if the package is available in repos. | ||
| This ensures auto-detection defaults to satellite-installer on clean systems. | ||
|
|
||
| :return: True if satellite command is installed | ||
| :rtype: bool | ||
| """ | ||
| # Check if foremanctl command exists (already installed) |
Contributor
There was a problem hiding this comment.
nitpick: Align the inline comment with the satellitectl check to avoid confusion.
The inline comment still refers to foremanctl while this method checks satellitectl. Please update the comment to mention satellitectl to clearly distinguish it from is_foremanctl_available.
ShimShtein
requested changes
Jul 20, 2026
| self.upstream_container if self.is_foremanctl_available() else self.upstream_rpm_name | ||
| ) | ||
| rpm_name = upstream_rpm_name if self.is_upstream else product_rpm_name | ||
| return self.execute(f'rpm -q --qf "%{{VERSION}}" {rpm_name}').stdout |
Contributor
There was a problem hiding this comment.
You already did rpm -q when invoked is_upstream. Instead of calling the same command twice, you can call it once and set the version as a byproduct. I mean the execution should look like this:
# run once in the initialization:
# Check for satellitectl
downstream_containerized_command = self.execute(f'rpm -q --qf "%{{VERSION}}" satellitectl')
upstream_containerized_command = self.execute(f'rpm -q --qf "%{{VERSION}}" foremanctl')
downstream_rpm_command = self.execute(f'rpm -q --qf "%{{VERSION}}" {product_rpm_name}')
upstream_rpm_command = self.execute(f'rpm -q --qf "%{{VERSION}}" {upstream_rpm_name}')
is_upstream = upstream_containerized_command.status == 0 || upstream_rpm_command.status == 0
is_containerized = upstream_containerized_command.status == 0 || downstream_containerized_command.status == 0
is_rpm = ! is_containerized
version = downstream_containerized_command.stdout || downstream_rpm_command.stdout || upstream_containerized_command.stdout || upstream_rpm_command.stdout This should give you all the indications that you need for later use.
Contributor
Author
There was a problem hiding this comment.
We have multiple tests which uses version function, it would break them.
nacoool
added
enhancement
nacoool
force-pushed
the
fix_installation_with_both_methods
branch
from
July 21, 2026 05:31
263eea7 to
570b7b6
Compare
Contributor
Author
|
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
Contributor
Author
|
Collaborator
|
PRT Result |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Statement
Test
test_installation_with_both_methodsinitially fails because of version issue.Deeper investigation reveals multiple failures happen.
Some of them are because of failures related to
foremanctl deploy.some of them is because it is not customised to include foremanctl.
Solution
Fix Test
test_installation_with_both_methodsby customising it so it can use version, is_stream, is_upstream etc..by including vars which we will use for foremanctl,
similar to
is_foremanctl_availableis_satellitectl_available is included.Related Issues
Summary by Sourcery
Update Satellite installation assertions and host helpers to support satellitectl/foremanctl-based deployments and fix version/health detection for mixed installation methods.
Bug Fixes:
Enhancements: