Update iop for foremanctl - #22066
Conversation
ColeHiggins2
added
Tier 2 ACK
Reviewer's GuideThis PR adds foremanctl-based IoP (local Red Hat Lightspeed) management alongside the existing satellite-installer flow, branching behavior on the host install method and updating tests and helpers to use the appropriate mechanism and detection logic. Sequence diagram for IoP configuration branching by install_methodsequenceDiagram
participant Host
participant CDN
participant ForemanctlCLI
participant SatelliteInstallerCLI
Host->>CDN: register_to_cdn()
Host->>Host: enable_ipv6_podman_proxy()
alt install_method == FOREMANCTL
Host->>ForemanctlCLI: execute('foremanctl deploy --add-feature iop')
else installer
Host->>Host: get_iop_image_paths()
alt image_paths returned
Host->>Host: yaml.dump(image_paths)
Host->>Host: put(custom_hiera, /etc/foreman-installer/custom-hiera.yaml)
end
Host->>SatelliteInstallerCLI: execute(InstallerCommand('enable-iop', iop_ensure='present').get_command())
end
Host->>Host: iop_enabled
Note right of Host: Raises SatelliteHostError if result.status != 0 or iop_enabled is False
Sequence diagram for IoP enabled detection based on install_methodsequenceDiagram
participant Host
participant ForemanctlCLI
participant Shell
Host->>Host: iop_enabled
alt install_method == FOREMANCTL
Host->>ForemanctlCLI: list_foremanctl_features(enabled=True)
ForemanctlCLI-->>Host: features
Host->>Host: check 'iop' in features
else installer
Host->>Shell: execute('systemctl is-active iop-core-engine')
Shell-->>Host: result.status
Host->>Host: result.status == 0
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
PRT is currently failing with issues around the satellite rex keys. Not related to these code changes |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
Host.iop_enabled, consider preserving the existing API-based check for non-foremanctl installs and only falling back tosystemctl is-activewhen the API endpoint is unavailable, to avoid changing semantics for satellite-installer setups. - The foremanctl branch in
test_positive_install_iop_custom_certsomits use ofcerts_server_ca_certthat was previously passed viasatellite-installer; verify whether an equivalent CA bundle option is needed forforemanctl deployand wire it through if so. - You now import
InstallMethodin multiple places (module-level and insideiop_enabled); centralizing this import at module level and reusing it would simplify the code and avoid repeated imports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Host.iop_enabled`, consider preserving the existing API-based check for non-foremanctl installs and only falling back to `systemctl is-active` when the API endpoint is unavailable, to avoid changing semantics for satellite-installer setups.
- The foremanctl branch in `test_positive_install_iop_custom_certs` omits use of `certs_server_ca_cert` that was previously passed via `satellite-installer`; verify whether an equivalent CA bundle option is needed for `foremanctl deploy` and wire it through if so.
- You now import `InstallMethod` in multiple places (module-level and inside `iop_enabled`); centralizing this import at module level and reusing it would simplify the code and avoid repeated imports.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| result = self.execute('foremanctl deploy --add-feature iop', timeout='30m') | ||
| else: | ||
| # Set up container image path overrides for satellite-installer |
There was a problem hiding this comment.
How and where do you override the image paths for foremanctl to deploy IOP containers, as similar to traditional installer's custom hiera?
There was a problem hiding this comment.
I wasnt aware that we need to override the image paths for foremanctl. If that is necessary, I can add it in
There was a problem hiding this comment.
+1, I have the same concern about where foremanctl image overrides are handled in this path.
| @property | ||
| def iop_enabled(self): | ||
| """Return boolean indicating whether IoP (local Red Hat Lightspeed) is enabled.""" | ||
| return self.api.RHCloud().advisor_engine_config()['use_iop_mode'] |
There was a problem hiding this comment.
Just curious, Is this API not available on foremanctl deployment? or am I missing some update around this?
There was a problem hiding this comment.
Correct this endpoint should have been removed and it open here theforeman/foreman_rh_cloud#1226
There was a problem hiding this comment.
That's great! Could you do this cleanup in nailgun entities as well whenever you get a chance?
| result = self.execute('foremanctl deploy --add-feature iop', timeout='30m') | ||
| else: | ||
| # Set up container image path overrides for satellite-installer |
There was a problem hiding this comment.
+1, I have the same concern about where foremanctl image overrides are handled in this path.
| f' --certificate-source=custom_server' | ||
| f' --certificate-server-certificate /root/{certs_data["cert_file_name"]}' | ||
| f' --certificate-server-key /root/{certs_data["key_file_name"]}', |
There was a problem hiding this comment.
This foremanctl custom-certs path is missing the CA bundle flag. We pass server cert/key here, but not the foremanctl equivalent of certs_server_ca_cert. In tests/foreman/installer/test_install_foremanctl.py we include --certificate-server-ca-certificate ... for custom cert installs. Can we add that here too (likely /root/{certs_data["ca_bundle_file_name"]}) so this test validates the full custom-CA flow?
325318e to
de25fb9
Compare
|
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
| f, | ||
| sort_keys=False, | ||
| default_flow_style=False, | ||
| if self.install_method == InstallMethod.FOREMANCTL: |
There was a problem hiding this comment.
Jfyi, since today we use same templates for both scenario, install_method() has underlying detect_install_method() which would pass for both scenarios, so I'd suggest using settings.server.install_method instead
There was a problem hiding this comment.
What if there will be migration test:
SAT1 (rpm) -> SAT2 (containerized)
Then using static setting you won't be able to handle that.
|
@ColeHiggins2 Just a heads up, you can use |
|
|
PRT Result |
|
|
PRT Result |
|
|
PRT Result |
chris1984
left a comment
chris1984
left a comment
There was a problem hiding this comment.
Went through the changes and it looks pretty good.
|
|
PRT Result |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
For reference, we use systemd drop-ins to manage the images. https://github.com/theforeman/foremanctl/blob/master/docs/developer/deployment.md#image-management describes that. From that I'd expect that in robottelo everything is already set and no explicit image management should be needed in a containerized installation.
| return self.api.RHCloud().advisor_engine_config()['use_iop_mode'] | ||
|
|
||
| if self.install_method == InstallMethod.FOREMANCTL: | ||
| return 'iop' in self.list_foremanctl_features(enabled=True) |
There was a problem hiding this comment.
Wouldn't it be better to probe the real system and find a Smart Proxy with the iop feature? That works for both installation methods and doesn't require running any shell commands on the real system.
7 participants
IOP is currently enabled using satellite-installer. We need functionality for both satellite-installer and foremanctl.
Tests are failing with:
"requests.exceptions.HTTPError: 404 Client Error: Not Found for url:Satellite.com/api/v2/rh_cloud/advisor_engine_configUpdated methods to look at Installment type and set up iop accordingly.
Summary by Sourcery
Support IoP enable/disable flows for both satellite-installer and foremanctl based deployments while adjusting IoP status detection.
New Features:
Enhancements:
Tests: