OCPEDGE-2217: Add TNF ABI workflow to existing assisted installer flow #8457
Conversation
Add getHostConfigDir() and loadFencingCredentials() to support reading fencing credentials from fencing-credentials.yaml. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add applyHostConfigByHostname() and update ApplyHostConfigs() to apply fencing credentials to hosts by matching hostname. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add tests for loadFencingCredentials() and applyHostConfigByHostname() covering valid files, missing files, validation errors, and API calls. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
openshift-ci-robot
added
the
jira/valid-reference
|
@fonta-rh: This pull request references OCPEDGE-2217 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. DetailsIn 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds loading and parsing of fencing-credentials.yaml from HOST_CONFIG_DIR and applies fencing credentials to hosts by hostname during host configuration, with validation, error propagation, and tests covering parsing and API update scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/XL
|
@fonta-rh: This pull request references OCPEDGE-2217 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. DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@fonta-rh: This pull request references OCPEDGE-2217 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.22" instead. DetailsIn 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. |
|
@fonta-rh: This pull request references OCPEDGE-2217 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.22" instead. DetailsIn 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![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 (2)
cmd/agentbasedinstaller/host_config.go (1)
75-99: Consider detecting duplicate hostnames in fencing credentials.If the YAML file contains multiple entries with the same hostname, the latter entry silently overwrites the former. This could mask configuration errors. Consider adding a check:
credentialsMap := make(map[string]*models.FencingCredentialsParams) for i, cred := range fcFile.Credentials { if cred.Hostname == "" { return nil, fmt.Errorf("fencing credential at index %d has empty hostname", i) } + if _, exists := credentialsMap[cred.Hostname]; exists { + return nil, fmt.Errorf("duplicate fencing credential for hostname: %s", cred.Hostname) + } + if cred.Address == nil {cmd/agentbasedinstaller/host_config_test.go (1)
235-395: Good test coverage for hostname-based configuration.Tests cover key scenarios including skip conditions and success/failure paths. Consider adding a test case for malformed inventory JSON to verify error handling:
Context("when host has malformed inventory JSON", func() { It("should return error", func() { testLogger, _ := test.NewNullLogger() host := &models.Host{ ID: &testHostID, InfraEnvID: testInfraEnvID, Inventory: `{invalid json`, } fencingCreds := map[string]*models.FencingCredentialsParams{ "master-0": { Address: strPtr("redfish+https://example.com"), Username: strPtr("admin"), Password: strPtr("password"), }, } err := applyHostConfigByHostname(ctx, testLogger, bmInventory, host, fencingCreds) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("failed to unmarshal")) }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/agentbasedinstaller/host_config.go(3 hunks)cmd/agentbasedinstaller/host_config_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/agentbasedinstaller/host_config_test.gocmd/agentbasedinstaller/host_config.go
🧬 Code graph analysis (2)
cmd/agentbasedinstaller/host_config_test.go (3)
internal/installcfg/installcfg.go (1)
CertificateVerification(235-235)models/fencing_credentials_params.go (1)
FencingCredentialsParams(21-38)models/host_update_params.go (1)
HostUpdateParams(22-51)
cmd/agentbasedinstaller/host_config.go (4)
models/fencing_credentials_params.go (1)
FencingCredentialsParams(21-38)internal/common/test_configuration.go (1)
Address(615-618)internal/installcfg/installcfg.go (1)
CertificateVerification(235-235)models/host_update_params.go (1)
HostUpdateParams(22-51)
🔇 Additional comments (7)
cmd/agentbasedinstaller/host_config.go (4)
34-42: LGTM!Simple and correct implementation for environment variable lookup with a sensible default.
111-138: LGTM!Good design to load fencing credentials once before the loop, and the error handling follows the established pattern consistently.
266-276: LGTM!Error handling correctly wraps API errors into
UpdateFailurewith the necessary context. Theconfigfield is nil here, butUpdateFailure.Hostname()correctly falls back toinventory.Hostnamewhich is always populated at this point.
246-278: Consider checking if fencing credentials need updating before calling API.Unlike
applyHostConfig()which checks if configuration changes are needed before updating, this function always callsV2UpdateHostwhen matching credentials are found. This could result in unnecessary API calls on repeated executions.Consider comparing existing host fencing credentials (if available in the model) before making the update call, similar to how
applyRoleandapplyRootDeviceHintscheck existing state.cmd/agentbasedinstaller/host_config_test.go (3)
1-17: LGTM!Standard and appropriate imports for Ginkgo/Gomega-based unit tests.
19-233: LGTM!Comprehensive test coverage for
loadFencingCredentialsincluding:
- Missing file handling
- Valid credentials parsing with optional fields
- Validation of required fields
- Invalid YAML and strict parsing behavior
- Installer compatibility verification
402-432: LGTM!Clean and focused mock transport implementation that properly tracks API call invocations and supports error injection for failure testing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8457 +/- ##
==========================================
+ Coverage 43.33% 43.51% +0.17%
==========================================
Files 405 411 +6
Lines 70793 71140 +347
==========================================
+ Hits 30681 30959 +278
- Misses 37368 37425 +57
- Partials 2744 2756 +12
🚀 New features to boost your workflow:
|
Add validation to return an error if the fencing credentials file contains multiple entries with the same hostname, preventing silent overwrites that could mask configuration errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add test case to verify applyHostConfigByHostname returns an error when host inventory contains invalid JSON. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/agentbasedinstaller/host_config_test.go (2)
19-233: Comprehensive test coverage for fencing credentials parsing.The test suite thoroughly covers valid inputs, validation errors, strict parsing, and edge cases. The tests properly set up and clean up temporary directories, and validation scenarios are well-structured.
Consider adding a test case for duplicate hostnames in the credentials YAML to verify the behavior is as expected (currently the map would silently overwrite the first entry with the second).
It("should handle duplicate hostnames by using the last occurrence", func() { content := `credentials: - hostname: master-0 address: redfish+https://192.168.111.1:8000/redfish/v1/Systems/abc username: admin password: password123 - hostname: master-0 address: redfish+https://192.168.111.1:8000/redfish/v1/Systems/xyz username: admin2 password: password456 ` err := os.WriteFile(filepath.Join(tempDir, "fencing-credentials.yaml"), []byte(content), 0600) Expect(err).NotTo(HaveOccurred()) creds, err := loadFencingCredentials(tempDir) Expect(err).NotTo(HaveOccurred()) Expect(creds).To(HaveLen(1)) // Verify which entry wins (last one in this case) Expect(*creds["master-0"].Username).To(Equal("admin2")) })
425-455: Mock transport implementation is appropriate for testing.The mock correctly implements the runtime transport interface, captures API call parameters for verification, and supports error injection. The type switch properly handles V2UpdateHostParams and returns appropriate responses.
Minor: Line 454 could be simplified from
fmt.Errorf("%s", errMsg)to justerrors.New(errMsg):func (m *mockHostConfigTransport) SetUpdateError(errMsg string) { - m.updateError = fmt.Errorf("%s", errMsg) + m.updateError = errors.New(errMsg) }But this is purely a style preference in test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/agentbasedinstaller/host_config.go(3 hunks)cmd/agentbasedinstaller/host_config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/agentbasedinstaller/host_config.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/agentbasedinstaller/host_config_test.go
🔇 Additional comments (1)
cmd/agentbasedinstaller/host_config_test.go (1)
235-418: Excellent test coverage for applyHostConfigByHostname.The test suite comprehensively covers all code paths including early-return scenarios, the happy path with proper API parameter verification, and error cases. The mock transport correctly captures API interactions and supports error injection for testing failure scenarios.
|
@fonta-rh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
Summary
Add support for Two-Node Fencing (TNF) in the Agent-Based Installer workflow.
Background
Two-Node Fencing (TNF) enables highly-available OpenShift clusters with only 2 control plane nodes by using BMC-based fencing to ensure safe failover. The fencing credentials (BMC address, username, password) are provided in install-config.yaml, controlPlane block and must be applied to hosts during installation.
Changes
This PR adds the ABI client-side support for TNF:
The fencing credentials file is read once and credentials are applied to hosts via the existing V2UpdateHost API. The existing handleFencing() in builder.go then reads these credentials from hosts when generating the final install-config.
Related PR
Works in tandem with an installer PR which generates the fencing-credentials.yaml file from install-config.yaml and adds client-side validations
List all the issues related to this PR
https://issues.redhat.com/browse/OCPEDGE-2217
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist