fix(#975): add diagnostic context to provisioning error logs - #976
fix(#975): add diagnostic context to provisioning error logs#976fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Replace generic "failed to provision host" messages with errors that include instance IDs, instance tags, platform, addresses, and failed host lists. This enables Splunk queries to distinguish AWS capacity errors from SSH failures, network issues, and timeouts. Changes: - dynamic.go: Include failed hosts list and instance tag when all provisioning attempts are exhausted. Add instance ID and timeout duration to timeout errors. Add structured log fields (instanceId, instanceTag, address) to all error log calls. - hostpool.go: Add host name, address, and platform to the provisioning task launch failure log. Use %w instead of %v for proper error chain preservation. - dynamicpool.go: Add error log with instanceTag and platform when LaunchInstance fails (previously silent). Add structured fields to pool allocation failure log. - Tests: Add diagnostic context verification tests for dynamic, static, and dynamic pool provisioning to assert error messages contain instance IDs, host names, and descriptive context. Closes #975
|
🤖 Finished Review · ✅ Success · Started 9:40 PM UTC · Completed 9:47 PM UTC |
Review — approvePR: #976 — fix(#975): add diagnostic context to provisioning error logs SummaryThis PR enriches provisioning error messages and structured log fields across the three allocation strategies (dynamic, dynamic pool, static host pool) so that Splunk queries can distinguish AWS capacity errors from SSH failures, network issues, and timeouts. The changes directly address #975, which documented complete loss of diagnostic visibility in MPC error logs. Dimension AnalysisCorrectness ✅
Security ✅No concerns. Logged data (instance IDs, tags, addresses, platform strings) is operational infrastructure metadata, not user data or credentials. No new external inputs are trusted. Intent & Coherence ✅The change is well-scoped to the issue's acceptance criteria: enriching error messages to enable log-based diagnosis. No unrelated behavioral changes. The PR description accurately reflects the diff. Style & Conventions ✅
Documentation ✅No in-repo documentation is affected by this logging-only change. Cross-Repo Contracts ✅Error messages are consumed internally (error secrets, controller logs). No external API surface changed. Low-Severity Observations
VerdictThe changes are correct, well-tested, and directly address the operational issue described in #975. Approving. Labels: PR fixes a bug in error logging with low-risk enrichment of error messages |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| log := logr.FromContextOrDiscard(ctx) | ||
| if tr.Annotations[FailedHosts] != "" { | ||
| return reconcile.Result{}, errors.New("failed to provision host") | ||
| log.Error(nil, "all provisioning attempts exhausted", |
There was a problem hiding this comment.
[low] style
log.Error(nil, ...) passes nil as the error argument. While valid in logr, this is unusual in this codebase where most callers pass a non-nil error. Consider creating the fmt.Errorf first and passing it to both the log and the return, as done in the timeout path a few lines below.
Suggested fix: Create the error with fmt.Errorf first, then pass it to log.Error and return it.
|
|
||
| // Verify instance was created and has an ID | ||
| tr := getUserTaskRun(ctx, client, "test-timeout-ctx") | ||
| Expect(tr.Annotations[CloudInstanceId]).ShouldNot(BeEmpty()) |
There was a problem hiding this comment.
[low] testing
time.Sleep(time.Second * 3) is timing-sensitive. The 3-second sleep must exceed the 2-second allocation-timeout, but slow CI environments could make this flaky. This follows the pre-existing pattern in the file.
Suggested fix: Consider increasing the sleep margin or refactoring to use Eventually with a polling interval in a future cleanup.
fullsend-ai-review
Bot
added
ready-for-merge
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 76.92% 75.86% -1.06%
==========================================
Files 26 26
Lines 2817 2847 +30
==========================================
- Hits 2167 2160 -7
- Misses 452 492 +40
+ Partials 198 195 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/agentic_review |
Code Review by Qodo
Context used✅ Compliance rules (platform):
2 rules 1. Instance ID lost in error
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
| log.Error(unassignErr, "failed to unassign instance from task after provisioning failure") | ||
| } else { | ||
| log.Error(err, "failed to provision cloud host") | ||
| log.Error(err, "failed to provision cloud host", |
There was a problem hiding this comment.
1. Dynamic provision error untested 📘 Rule violation ▣ Testability
The modified provisioning-task failure path in DynamicResolver.Allocate adds new executable error/logging lines, but there is no test scenario that can reach it given the current test harness always provides the SSH secret and the fake client create path does not fail. This violates the requirement that every new/modified executable line in the patch is exercised by automated tests (or has an explicit coverage justification).
Agent Prompt
## Issue description
The PR modifies the `launchProvisioningTask(...)` error-handling block in `DynamicResolver.Allocate`, but the existing tests do not exercise this branch. Per compliance, each new/changed executable line must be executed by at least one automated test (or explicitly justified).
## Issue Context
`launchProvisioningTask(...)` returns an error primarily when the SSH secret is missing or when the TaskRun create call fails. The current dynamic test setup always creates the `awskeys` secret, so the new wrapped error/log lines in this branch are unlikely to be executed.
## Fix Focus Areas
- pkg/reconciler/taskrun/dynamic.go[123-144]
- pkg/reconciler/taskrun/taskrun.go[971-983]
- pkg/reconciler/taskrun/taskrun_helpers_test.go[303-327]
- pkg/reconciler/taskrun/provision_dynamic_test.go[90-170]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if err != nil { | ||
| //ugh, try and unassign | ||
| log.Error(err, "failed to launch provisioning task, unassigning host") | ||
| log.Error(err, "failed to launch provisioning task, unassigning host", |
There was a problem hiding this comment.
2. Hostpool provision error untested 📘 Rule violation ▣ Testability
The modified HostPool.Allocate provisioning-task launch failure path adds/changes executable error/logging lines, but the test harness always provides the awskeys secret and does not introduce a failing create path to execute this branch. This violates the requirement that every new/modified executable line in the patch is exercised by automated tests (or has an explicit coverage justification).
Agent Prompt
## Issue description
The PR changes executable lines in `HostPool.Allocate` in the error path when `launchProvisioningTask(...)` fails (adds structured fields and changes returned error formatting/wrapping). There is no test that forces `launchProvisioningTask(...)` to fail, so these modified lines are not exercised.
## Issue Context
`launchProvisioningTask(...)` fails when it cannot find the SSH secret or cannot create the provisioning TaskRun. The static test setup always includes the `awskeys` secret, so this path is not covered.
## Fix Focus Areas
- pkg/reconciler/taskrun/hostpool.go[134-151]
- pkg/reconciler/taskrun/taskrun.go[971-983]
- pkg/reconciler/taskrun/taskrun_helpers_test.go[270-279]
- pkg/reconciler/taskrun/provision_static_test.go[22-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| log := logr.FromContextOrDiscard(ctx) | ||
| if tr.Annotations[FailedHosts] != "" { | ||
| return reconcile.Result{}, errors.New("failed to provision host") | ||
| log.Error(nil, "all provisioning attempts exhausted", |
There was a problem hiding this comment.
3. Nil error in error() 🐞 Bug ◔ Observability
DynamicResolver.Allocate calls log.Error(nil, ...) when provisioning attempts are exhausted, emitting an error-level log without an error value and diverging from the repo’s normal logging pattern. This reduces log consistency/utility and makes the returned error (created afterward) unavailable to the logger.
Agent Prompt
### Issue description
`DynamicResolver.Allocate` logs an error with `log.Error(nil, ...)` in the “attempts exhausted” path, so the error-level log does not carry an error value.
### Issue Context
Elsewhere in this repo, error-level logs consistently include a non-nil error (even if synthetic) to provide consistent metadata.
### Fix Focus Areas
- pkg/reconciler/taskrun/dynamic.go[47-55]
### Suggested fix
Create the error first (same one you return) and pass it to `log.Error(err, ...)` (or switch this log to `log.Info(...)` if you intentionally don’t want an error object).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| log.Error(unassignErr, "failed to unassign instance from task after provisioning failure") | ||
| } else { | ||
| log.Error(err, "failed to provision cloud host") | ||
| log.Error(err, "failed to provision cloud host", |
There was a problem hiding this comment.
4. Instance id lost in error 🐞 Bug ≡ Correctness
When launchProvisioningTask fails, DynamicResolver.Allocate deletes CloudInstanceId from tr.Annotations before constructing the new structured log fields and wrapped error that read tr.Annotations[CloudInstanceId]. If termination succeeds, the returned error/logs can show an empty instance ID, defeating the added diagnostic context and affecting user-visible error secrets.
Agent Prompt
### Issue description
In the provisioning-task launch failure path, the code deletes `tr.Annotations[CloudInstanceId]` and then immediately uses `tr.Annotations[CloudInstanceId]` to populate new log fields and the returned wrapped error string. This can erase the instance ID from the very diagnostics this PR adds.
### Issue Context
Errors returned from host allocation are surfaced to users via `createErrorSecret` as part of `Error allocating host: ...`, so losing the instance ID reduces debuggability.
### Fix Focus Areas
- pkg/reconciler/taskrun/dynamic.go[123-144]
- pkg/reconciler/taskrun/taskrun.go[463-469]
### Suggested fix
Store `instanceID := tr.Annotations[CloudInstanceId]` in a local variable before calling `TerminateInstance` / deleting the annotation. Use `instanceID` for:
- the structured log field `instanceId`
- the returned `fmt.Errorf(... instance: %s ...)`
Optionally, also use it in the terminate call for clarity.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Expect(tr.Annotations[CloudInstanceId]).ShouldNot(BeEmpty()) | ||
| instanceID := tr.Annotations[CloudInstanceId] | ||
|
|
||
| // Wait for timeout (allocation-timeout is 2 seconds in test config) |
There was a problem hiding this comment.
5. Sleep-based timeout test 🐞 Bug ☼ Reliability
The new dynamic provisioning test uses time.Sleep(3s) to trigger the allocation timeout, which slows the test suite and can be timing-flaky in loaded CI environments. The timeout logic is purely computed from AllocationStartTimeAnnotation and r.timeout, so the test can simulate timeout deterministically by backdating the annotation instead of sleeping.
Agent Prompt
### Issue description
A test waits on wall-clock time (`time.Sleep`) to exceed the allocation timeout, increasing runtime and introducing timing sensitivity.
### Issue Context
The production timeout check compares `AllocationStartTimeAnnotation + r.timeout` against `time.Now().Unix()`.
### Fix Focus Areas
- pkg/reconciler/taskrun/provision_dynamic_test.go[301-323]
- pkg/reconciler/taskrun/dynamic.go[60-85]
### Suggested fix
Replace `time.Sleep(...)` by updating the TaskRun’s `AllocationStartTimeAnnotation` to an older timestamp (e.g., `time.Now().Add(-5*time.Second).Unix()`) and `client.Update(...)` the TaskRun before the second reconcile. This keeps coverage while making the test fast and deterministic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2 participants
Replace generic "failed to provision host" messages with errors that include instance IDs, instance tags, platform, addresses, and failed host lists. This enables Splunk queries to distinguish AWS capacity errors from SSH failures, network issues, and timeouts.
Changes:
all provisioning attempts are exhausted. Add instance ID and
timeout duration to timeout errors. Add structured log fields
(instanceId, instanceTag, address) to all error log calls.
provisioning task launch failure log. Use %w instead of %v
for proper error chain preservation.
when LaunchInstance fails (previously silent). Add structured
fields to pool allocation failure log.
static, and dynamic pool provisioning to assert error messages
contain instance IDs, host names, and descriptive context.
Closes #975
Post-script verification
agent/975-improve-error-logging)8ae7a0c24d48998f9ddc19c7d68db4b4b25fdc85..HEAD)