Skip to content

fix test and include foremanctl in hosts and satellite_misxins #22150

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.

Sign up for GitHub

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

Open
nacoool wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix test and include foremanctl in hosts and satellite_misxins #22150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions robottelo/host_helpers/satellite_mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,45 +547,42 @@ def assert_install_assertions(self):
if settings.server.version.source != 'nightly':
assert settings.server.version.release == sat_version

# Check journald for errors using installation-method-aware service list
services = self.get_service_names()
service_units = ' '.join([f'-u "{svc}"' for svc in services])
result = self.execute(
f'journalctl --quiet --no-pager --boot --priority err {service_units}'
)
assert not result.stdout

# Check foreman production log
result = self.execute(r'grep --context=100 -E "\[E\|" /var/log/foreman/production.log')
if not is_open('SAT-21086'):
assert not result.stdout

# Check foreman-installer log (only relevant for satellite-installer method)
# Check foreman production log (only relevant for satellite-installer method)
if self.install_method == InstallMethod.INSTALLER:
result = self.execute(r'grep --context=100 -E "\[E\|" /var/log/foreman/production.log')
if not is_open('SAT-21086'):
assert not result.stdout
# 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:
filtered_errors = [
line
for line in result.stdout.splitlines()
if 'Connection refused' not in line
and 'attempt to connect to 127.0.0.1:3000' not in line
and 'failed to make connection to backend: localhost' not in line
]
assert not filtered_errors, f'Unexpected httpd errors:\n{chr(10).join(filtered_errors)}'

# Check candlepin logs
result = self.execute(r'grep -iR "error" /var/log/candlepin/*')
assert not result.stdout

httpd_log = self.execute('journalctl --unit=httpd')
assert 'WARNING' not in httpd_log.stdout

result = self.cli.Health.check()
assert 'FAIL' not in result.stdout
# Bug covers
if not is_open('SAT-47736'):
services = self.get_service_names()
service_units = ' '.join([f'-u "{svc}"' for svc in services])
# Check journald for errors using installation-method-aware service list
result = self.execute(
f'journalctl --quiet --no-pager --boot --grep ERROR {service_units}'
)
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/*')
Comment on lines +570 to +572

@sourcery-ai sourcery-ai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert not result.stdout

# Check candlepin logs
result = self.execute(r'grep -iR "error" /var/log/candlepin/*')
assert not result.stdout

# Check httpd logs
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
Comment on lines +583 to +585

@sourcery-ai sourcery-ai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

else:
result = self.cli.Health.check()
assert 'FAIL' not in result.stdout
39 changes: 36 additions & 3 deletions robottelo/hosts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,6 +1696,8 @@ class Capsule(ContentHost, CapsuleMixins):
rex_key_path = '~foreman-proxy/.ssh/id_rsa_foreman_proxy.pub'
product_rpm_name = 'satellite-capsule'
upstream_rpm_name = 'foreman-proxy'
container_rpm_name = 'satellitectl'
upstream_container = 'foremanctl'

def __init__(self, hostname, **kwargs):
kwargs.setdefault('net_type', settings.capsule.network_type)
Expand Down Expand Up @@ -1743,7 +1745,10 @@ def is_upstream(self):
:return: True if no downstream satellite RPMS are installed
: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
Comment on lines +1748 to +1750

@sourcery-ai sourcery-ai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) != 0

@cached_property
def is_stream(self):
Expand All @@ -1756,11 +1761,22 @@ def is_stream(self):
return False
return (
'stream' in self.execute(f'rpm -q --qf "%{{RELEASE}}" {self.product_rpm_name}').stdout
or self.is_satellitectl_available()
)

@cached_property
def version(self):
rpm_name = self.upstream_rpm_name if self.is_upstream else self.product_rpm_name
"""Get the version of the Capsule/Satellite installation.

Queries the appropriate RPM based on upstream vs downstream.
"""
product_rpm_name = (
self.container_rpm_name if self.is_satellitectl_available() else self.product_rpm_name
)
upstream_rpm_name = (
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

@ShimShtein ShimShtein Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nacoool nacoool Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have multiple tests which uses version function, it would break them.


@cached_property
Expand Down Expand Up @@ -1790,6 +1806,19 @@ def is_foremanctl_available(self):
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)
Comment on lines +1809 to +1818

@sourcery-ai sourcery-ai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result = self.execute('which satellitectl')
return result.status == 0

def detect_install_method(self):
"""Auto-detect which installation method to use.

Expand Down Expand Up @@ -2182,7 +2211,9 @@ def install_satellite_foremanctl(
)

# Install foremanctl
assert self.execute('dnf install -y foremanctl').status == 0, 'Failed to install foremanctl'
assert self.execute('dnf install -y satellitectl').status == 0, (
'Failed to install satellitectl'
)

if enable_fapolicyd:
assert self.execute('dnf -y install fapolicyd').status == 0
Expand Down Expand Up @@ -2364,6 +2395,8 @@ def load_remote_yaml_file(self, file_path):
class Satellite(Capsule, SatelliteMixins):
product_rpm_name = 'satellite'
upstream_rpm_name = 'foreman'
container_rpm_name = 'satellitectl'
upstream_container = 'foremanctl'

def __init__(self, hostname=None, **kwargs):
hostname = hostname or settings.server.hostname # instance attr set by broker.Host
Expand Down