MGMT-20398: Don't start main agent until ironic reports it's ready#1518
MGMT-20398: Don't start main agent until ironic reports it's ready#1518pastequo wants to merge 1 commit into
Conversation
The check is based on a json file. If the file is missing, the feature is considered as not implemented or that we aren't in a ABI installation ; then the check is skipped. Assisted-by: Opencode
openshift-ci-robot
added
the
jira/valid-reference
|
@pastequo: This pull request references MGMT-20398 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 bug to target the "5.0.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. |
WalkthroughAdds ironic status file readiness validation to the assisted-installer-agent. A new ChangesIronic Status File Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pastequo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/util/status_file.go (1)
72-123: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftUnbounded polling loop with no cancellation/timeout.
waitForIronicReadinessblocks forever on transitional states (empty file,initializing, empty status), and it gates the whole agent startup inMain(). There's nocontext.Context, deadline, or max-wait, so if ironic never reacheswaiting for assisted agentthe agent hangs indefinitely with no way to cancel. Consider threading acontext.Contextwith a timeout (or an upper bound on retries) so the wait is cancellable and observable.As per path instructions: "context.Context for cancellation and timeouts".
Please confirm whether an indefinite, uncancellable wait is the intended product behavior here, or whether an upper bound is expected (the PR objective only states "don't start until ironic reports ready").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/status_file.go` around lines 72 - 123, The wait loop in waitForIronicReadiness is unbounded and cannot be cancelled, so it can block startup forever on empty or initializing statuses. Update waitForIronicReadiness (and its caller in Main) to accept a context.Context and enforce a timeout or max retries, then check ctx.Done() inside the polling loop and return a clear error when the wait is cancelled or exceeds the limit.Source: Path instructions
src/util/status_file_test.go (1)
35-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd context messages to assertions.
Most
Expect(...)calls (e.g. Lines 37, 45, 65, 121, 135, 150, 164, 183) lack failure messages, so a failure reports only the matcher output without identifying which case/condition failed. Adding short messages aids triage when a case regresses.As per coding guidelines: "Assertion messages - include meaningful failure messages, flag assertions without context messages".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/status_file_test.go` around lines 35 - 184, Most Expect assertions in this status file test are missing context messages, which makes failures hard to trace to a specific case. Update the assertions in the ValidateStatusFile test contexts to include short failure messages for each expectation, especially around the scenarios for empty input, nonexistent file, initializing, waiting, finalizing, unexpected status, invalid JSON, and unreadable file. Use the existing test case descriptions and the ValidateStatusFile symbol to keep the messages specific and consistent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/agent_config.go`:
- Around line 13-15: The struct field list in AgentConfig is not gofmt-aligned
after adding IronicStatusFilePath, so realign the type column for the fields in
this block to match gofmt output. Update the field declarations in the
AgentConfig struct so IntervalSecs, HostID, and IronicStatusFilePath use
consistent spacing and the file passes gofmt/golangci-lint.
In `@src/util/status_file_test.go`:
- Around line 168-185: The unreadable-file test in ValidateStatusFile is flaky
because chmod 0000 does not reliably block reads under root, so update the test
to simulate an actual read failure in a root-safe way or skip this case when
running as root. Use the existing ValidateStatusFile and statusFilePath setup to
point at an unreadable path or a directory-as-file scenario, and make sure any
os.Chmod calls in the test handle and assert their returned errors instead of
ignoring them.
- Around line 57-62: The background status-file write in the test helper
goroutines ignores the `os.WriteFile` error, which can leave
`ValidateStatusFile` polling until timeout instead of failing at the real cause.
Update the goroutines in `status_file_test.go` that write `StatusFileContent` to
check and handle the `os.WriteFile` return value, and apply the same fix to the
similar write blocks later in the file so any write failure is surfaced
immediately.
In `@src/util/status_file.go`:
- Around line 113-117: The empty-status branch in the status polling loop is
logging at Info on every iteration, which is inconsistent with the other
transitional paths and can flood logs during long waits. Update the `case ""`
branch in the status handling logic to use `logger.Debug` instead of
`logger.Info`, matching the existing `StatusInitializing` and empty-file
behavior in the same polling loop.
---
Nitpick comments:
In `@src/util/status_file_test.go`:
- Around line 35-184: Most Expect assertions in this status file test are
missing context messages, which makes failures hard to trace to a specific case.
Update the assertions in the ValidateStatusFile test contexts to include short
failure messages for each expectation, especially around the scenarios for empty
input, nonexistent file, initializing, waiting, finalizing, unexpected status,
invalid JSON, and unreadable file. Use the existing test case descriptions and
the ValidateStatusFile symbol to keep the messages specific and consistent.
In `@src/util/status_file.go`:
- Around line 72-123: The wait loop in waitForIronicReadiness is unbounded and
cannot be cancelled, so it can block startup forever on empty or initializing
statuses. Update waitForIronicReadiness (and its caller in Main) to accept a
context.Context and enforce a timeout or max retries, then check ctx.Done()
inside the polling loop and return a clear error when the wait is cancelled or
exceeds the limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66da9279-5f91-4566-aae5-99d15f3c9329
📒 Files selected for processing (4)
src/agent/main/main.gosrc/config/agent_config.gosrc/util/status_file.gosrc/util/status_file_test.go
| IntervalSecs int | ||
| HostID string | ||
| IronicStatusFilePath string |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
gofmt: fix struct field alignment.
golangci-lint reports the file is not gofmt-formatted at this block. The added field changes the longest field-name width, so the type column needs realignment; otherwise CI fails.
🔧 Proposed fix
- IntervalSecs int
- HostID string
- IronicStatusFilePath string
+ IntervalSecs int
+ HostID string
+ IronicStatusFilePath string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IntervalSecs int | |
| HostID string | |
| IronicStatusFilePath string | |
| IntervalSecs int | |
| HostID string | |
| IronicStatusFilePath string |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 13-13: File is not properly formatted
(gofmt)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/agent_config.go` around lines 13 - 15, The struct field list in
AgentConfig is not gofmt-aligned after adding IronicStatusFilePath, so realign
the type column for the fields in this block to match gofmt output. Update the
field declarations in the AgentConfig struct so IntervalSecs, HostID, and
IronicStatusFilePath use consistent spacing and the file passes
gofmt/golangci-lint.
Source: Linters/SAST tools
| go func() { | ||
| time.Sleep(20 * time.Millisecond) | ||
| content := StatusFileContent{Status: StatusWaitingForAgent} | ||
| data, _ := json.Marshal(content) | ||
| os.WriteFile(statusFilePath, data, 0644) | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the goroutine's os.WriteFile error.
If this background write fails, the status file never transitions, ValidateStatusFile keeps polling, and the test hangs until the Ginkgo suite timeout — failing opaquely rather than surfacing the real cause. Same pattern applies to Lines 78-83 and 99-104.
🛡️ Proposed fix
go func() {
time.Sleep(20 * time.Millisecond)
content := StatusFileContent{Status: StatusWaitingForAgent}
- data, _ := json.Marshal(content)
- os.WriteFile(statusFilePath, data, 0644)
+ data, err := json.Marshal(content)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(os.WriteFile(statusFilePath, data, 0644)).To(Succeed())
}()As per path instructions: "Never ignore error returns".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| time.Sleep(20 * time.Millisecond) | |
| content := StatusFileContent{Status: StatusWaitingForAgent} | |
| data, _ := json.Marshal(content) | |
| os.WriteFile(statusFilePath, data, 0644) | |
| }() | |
| go func() { | |
| time.Sleep(20 * time.Millisecond) | |
| content := StatusFileContent{Status: StatusWaitingForAgent} | |
| data, err := json.Marshal(content) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(os.WriteFile(statusFilePath, data, 0644)).To(Succeed()) | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 61-61: Error return value of os.WriteFile is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/status_file_test.go` around lines 57 - 62, The background
status-file write in the test helper goroutines ignores the `os.WriteFile`
error, which can leave `ValidateStatusFile` polling until timeout instead of
failing at the real cause. Update the goroutines in `status_file_test.go` that
write `StatusFileContent` to check and handle the `os.WriteFile` return value,
and apply the same fix to the similar write blocks later in the file so any
write failure is surfaced immediately.
Sources: Path instructions, Linters/SAST tools
| Context("when file becomes unreadable after creation", func() { | ||
| It("should return error", func() { | ||
| statusFilePath := filepath.Join(tempDir, "status.json") | ||
|
|
||
| // Create a valid file first | ||
| content := StatusFileContent{Status: StatusInitializing} | ||
| data, _ := json.Marshal(content) | ||
| err := os.WriteFile(statusFilePath, data, 0644) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| // Make it unreadable | ||
| os.Chmod(statusFilePath, 0000) | ||
| defer os.Chmod(statusFilePath, 0644) // Restore for cleanup | ||
|
|
||
| err = ValidateStatusFile(statusFilePath) | ||
| Expect(err).ToNot(BeNil()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unreadable-file test is flaky when the suite runs as root.
os.Chmod(statusFilePath, 0000) does not deny read access to the root user, so when the test runs as root (common in CI containers) the file remains readable, the status parses as StatusInitializing, and ValidateStatusFile blocks/polls instead of erroring — failing this expectation. Consider gating on non-root, or simulating the read failure differently (e.g. point at a path whose parent directory is non-traversable, or use a directory in place of the file).
Additionally, the os.Chmod return values on Lines 179-180 are ignored.
As per path instructions: "Never ignore error returns".
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 179-179: Error return value of os.Chmod is not checked
(errcheck)
[error] 180-180: Error return value of os.Chmod is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/status_file_test.go` around lines 168 - 185, The unreadable-file
test in ValidateStatusFile is flaky because chmod 0000 does not reliably block
reads under root, so update the test to simulate an actual read failure in a
root-safe way or skip this case when running as root. Use the existing
ValidateStatusFile and statusFilePath setup to point at an unreadable path or a
directory-as-file scenario, and make sure any os.Chmod calls in the test handle
and assert their returned errors instead of ignoring them.
Sources: Path instructions, Linters/SAST tools
| case "": | ||
| logger.Info("Ironic agent defined an empty state, waiting for readiness") | ||
| time.Sleep(waitingLoop) | ||
|
|
||
| continue |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log flooding: empty-status branch logs at Info every iteration.
The case "" branch logs at Info on every 2s poll while transitional StatusInitializing and the empty-file path use Debug. Since this loop can run indefinitely, this floods logs at Info level. Lower to Debug for consistency and to avoid noise.
🔧 Proposed fix
case "":
- logger.Info("Ironic agent defined an empty state, waiting for readiness")
+ logger.Debug("Ironic agent defined an empty state, waiting for readiness")
time.Sleep(waitingLoop)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "": | |
| logger.Info("Ironic agent defined an empty state, waiting for readiness") | |
| time.Sleep(waitingLoop) | |
| continue | |
| case "": | |
| logger.Debug("Ironic agent defined an empty state, waiting for readiness") | |
| time.Sleep(waitingLoop) | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/status_file.go` around lines 113 - 117, The empty-status branch in
the status polling loop is logging at Info on every iteration, which is
inconsistent with the other transitional paths and can flood logs during long
waits. Update the `case ""` branch in the status handling logic to use
`logger.Debug` instead of `logger.Info`, matching the existing
`StatusInitializing` and empty-file behavior in the same polling loop.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1518 +/- ##
==========================================
+ Coverage 56.49% 56.84% +0.35%
==========================================
Files 90 91 +1
Lines 4652 4709 +57
==========================================
+ Hits 2628 2677 +49
- Misses 1817 1826 +9
+ Partials 207 206 -1
🚀 New features to boost your workflow:
|
|
@pastequo: The following tests failed, say
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
The check is based on a json file.
If the file is missing, the feature is considered as not implemented or that we aren't in a ABI installation ; then the check is skipped.
Assisted-by: Opencode
Following this slack discussion
Will require another PR in the ironic agent repo to actually write the file
cc @CrystalChun @dtantsur
Summary by CodeRabbit
New Features
Bug Fixes