Skip to content

fix(#975): add diagnostic context to provisioning error logs #976

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

Draft
fullsend-ai-coder wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(#975): add diagnostic context to provisioning error logs #976

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
33 changes: 25 additions & 8 deletions pkg/reconciler/taskrun/dynamic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package taskrun

import (
"context"
"errors"
"fmt"
"strconv"
"time"
Expand Down Expand Up @@ -48,7 +47,11 @@ func (r DynamicResolver) Deallocate(taskRun *ReconcileTaskRun, ctx context.Conte
func (r DynamicResolver) Allocate(taskRun *ReconcileTaskRun, ctx context.Context, tr *v1.TaskRun, secretName string) (reconcile.Result, error) {
log := logr.FromContextOrDiscard(ctx)
if tr.Annotations[FailedHosts] != "" {
return reconcile.Result{}, errors.New("failed to provision host")
log.Error(nil, "all provisioning attempts exhausted",

@fullsend-ai-review fullsend-ai-review Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@qodo-app-for-konflux-ci qodo-app-for-konflux-ci Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

"failedHosts", tr.Annotations[FailedHosts],
"instanceTag", r.instanceTag,
)
return reconcile.Result{}, fmt.Errorf("failed to provision host, all attempts exhausted for instance tag %s (previously failed hosts: %s)", r.instanceTag, tr.Annotations[FailedHosts])
}

if tr.Annotations == nil {
Expand All @@ -59,8 +62,12 @@ func (r DynamicResolver) Allocate(taskRun *ReconcileTaskRun, ctx context.Context
startTime, err := strconv.ParseInt(allocStart, 10, 64)
if err == nil {
if startTime+r.timeout < time.Now().Unix() {
err = errors.New("timed out waiting for instance address")
log.Error(err, "timed out waiting for instance address")
err = fmt.Errorf("timed out waiting for instance address (instance: %s, instanceTag: %s, timeout: %ds)", tr.Annotations[CloudInstanceId], r.instanceTag, r.timeout)
log.Error(err, "timed out waiting for instance address",
"instanceId", tr.Annotations[CloudInstanceId],
"instanceTag", r.instanceTag,
"timeoutSeconds", r.timeout,
)
//ugh, try and unassign
terr := r.TerminateInstance(taskRun.client, ctx, cloud.InstanceIdentifier(tr.Annotations[CloudInstanceId]))
if terr != nil {
Expand All @@ -85,7 +92,10 @@ func (r DynamicResolver) Allocate(taskRun *ReconcileTaskRun, ctx context.Context
//An instance already exists, so get its IP address
address, err := r.GetInstanceAddress(taskRun.client, ctx, cloud.InstanceIdentifier(tr.Annotations[CloudInstanceId]))
if err != nil { // A permanent error occurred when fetching the IP address for the VM
log.Error(err, "failed to get instance address for cloud host")
log.Error(err, "failed to get instance address for cloud host",
"instanceId", tr.Annotations[CloudInstanceId],
"instanceTag", r.instanceTag,
)
//Try to delete the instance and unassign it from the TaskRun
terr := r.TerminateInstance(taskRun.client, ctx, cloud.InstanceIdentifier(tr.Annotations[CloudInstanceId]))
if terr != nil {
Expand Down Expand Up @@ -124,9 +134,13 @@ func (r DynamicResolver) Allocate(taskRun *ReconcileTaskRun, ctx context.Context
if unassignErr != nil {
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",

@qodo-app-for-konflux-ci qodo-app-for-konflux-ci Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

@qodo-app-for-konflux-ci qodo-app-for-konflux-ci Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

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

"instanceId", tr.Annotations[CloudInstanceId],
"instanceTag", r.instanceTag,
"address", address,
)
}
return reconcile.Result{}, err
return reconcile.Result{}, fmt.Errorf("failed to provision cloud host (instance: %s, address: %s): %w", tr.Annotations[CloudInstanceId], address, err)
}
return reconcile.Result{}, nil
} else { // A transient error (that wasn't returned) occurred when fetching the IP address for the VM
Expand Down Expand Up @@ -194,7 +208,10 @@ func (r DynamicResolver) Allocate(taskRun *ReconcileTaskRun, ctx context.Context
if err != nil {
launchErr := err
//launch failed
log.Error(err, "Failed to create cloud host")
log.Error(err, "Failed to create cloud host",
"instanceTag", r.instanceTag,
"platform", r.platform,
)
failureCount := 0
existingFailureString := tr.Annotations[CloudFailures]
if existingFailureString != "" {
Expand Down
12 changes: 10 additions & 2 deletions pkg/reconciler/taskrun/dynamicpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,11 @@ func (a DynamicHostPool) Allocate(r *ReconcileTaskRun, ctx context.Context, tr *
if len(hostPool.hosts) > 0 {
_, allocationErr = hostPool.Allocate(r, ctx, tr, secretName)
if allocationErr != nil && !errors.Is(allocationErr, ErrAllHostsFailed) {
log.Error(allocationErr, "could not allocate host from pool")
log.Error(allocationErr, "could not allocate host from pool",
"instanceTag", a.instanceTag,
"platform", a.platform,
"poolSize", len(hostPool.hosts),
)
return reconcile.Result{}, allocationErr
}
if allocationErr == nil && (tr.Labels == nil || tr.Labels[constant.WaitingForPlatformLabel] == "") {
Expand Down Expand Up @@ -143,7 +147,11 @@ func (a DynamicHostPool) Allocate(r *ReconcileTaskRun, ctx context.Context, tr *
taskRunID := fmt.Sprintf("%s:%s", tr.Namespace, tr.Name)
inst, err := a.cloudProvider.LaunchInstance(r.client, ctx, taskRunID, a.instanceTag, a.additionalInstanceTags)
if err != nil {
return reconcile.Result{}, err
log.Error(err, "failed to launch new instance for dynamic pool",
"instanceTag", a.instanceTag,
"platform", a.platform,
)
return reconcile.Result{}, fmt.Errorf("failed to launch instance for dynamic pool (instanceTag: %s, platform: %s): %w", a.instanceTag, a.platform, err)
}

log.Info("allocated instance", "instance", inst)
Expand Down
8 changes: 6 additions & 2 deletions pkg/reconciler/taskrun/hostpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,19 @@ func (hp HostPool) Allocate(r *ReconcileTaskRun, ctx context.Context, tr *v1.Tas

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",

@qodo-app-for-konflux-ci qodo-app-for-konflux-ci Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

"host", selected.Name,
"address", selected.Address,
"platform", hp.targetPlatform,
)
delete(tr.Labels, constant.AssignedHost)
controllerutil.RemoveFinalizer(tr, PipelineFinalizer)
updateErr := UpdateTaskRunWithRetry(ctx, r.client, r.apiReader, tr)
if updateErr != nil {
log.Error(updateErr, "Could not unassign task after provisioning failure")
return reconcile.Result{}, err
}
return reconcile.Result{}, fmt.Errorf("failed to provision host: %v", err)
return reconcile.Result{}, fmt.Errorf("failed to provision host %s (%s): %w", selected.Name, selected.Address, err)
}
return reconcile.Result{}, nil
}
Expand Down
40 changes: 40 additions & 0 deletions pkg/reconciler/taskrun/provision_dynamic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,46 @@ var _ = Describe("Test Dynamic Host Provisioning", func() {
})
})

When("error messages contain diagnostic context", func() {

It("should include failed hosts and instance tag in error when all provisioning attempts are exhausted", func(ctx SpecContext) {
createUserTaskRun(ctx, client, "test-err-ctx", "linux/arm64")
tr := getUserTaskRun(ctx, client, "test-err-ctx")
// Simulate that a previous host already failed
tr.Annotations = map[string]string{FailedHosts: "host-abc"}
Expect(client.Update(ctx, tr)).ShouldNot(HaveOccurred())

_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: userNamespace, Name: "test-err-ctx"}})
Expect(err).Should(HaveOccurred())
Expect(err.Error()).Should(ContainSubstring("host-abc"))
Expect(err.Error()).Should(ContainSubstring("all attempts exhausted"))
})

It("should include instance ID and timeout in error when instance address times out", func(ctx SpecContext) {
cloudImpl.TimeoutGetAddress = true
defer func() { cloudImpl.TimeoutGetAddress = false }()

createUserTaskRun(ctx, client, "test-timeout-ctx", "linux/arm64")
// 1st reconcile: launches instance
_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: userNamespace, Name: "test-timeout-ctx"}})
Expect(err).ShouldNot(HaveOccurred())

// Verify instance was created and has an ID
tr := getUserTaskRun(ctx, client, "test-timeout-ctx")
Expect(tr.Annotations[CloudInstanceId]).ShouldNot(BeEmpty())

@fullsend-ai-review fullsend-ai-review Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

instanceID := tr.Annotations[CloudInstanceId]

// Wait for timeout (allocation-timeout is 2 seconds in test config)

@qodo-app-for-konflux-ci qodo-app-for-konflux-ci Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

time.Sleep(time.Second * 3)

// Reconcile after timeout
_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: userNamespace, Name: "test-timeout-ctx"}})
Expect(err).Should(HaveOccurred())
Expect(err.Error()).Should(ContainSubstring("timed out"))
Expect(err.Error()).Should(ContainSubstring(instanceID))
})
})

// Tests for buildDynamicResolver function - only the sad paths since happy paths are thoroughly tested elsewhere
When("testing buildDynamicResolver error paths", func() {
It("should use default instance tag when platform config doesn't specify one", func(ctx SpecContext) {
Expand Down
14 changes: 14 additions & 0 deletions pkg/reconciler/taskrun/provision_dynamicpool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,20 @@ var _ = Describe("Test Dynamic Pool Host Provisioning", func() {
})
})

When("error messages contain diagnostic context", func() {

It("should include instance tag and platform in error when launch fails in dynamic pool", func(ctx SpecContext) {
cloudImpl.FailLaunch = true
defer func() { cloudImpl.FailLaunch = false }()

createUserTaskRun(ctx, client, "test-pool-launch-err", "linux/arm64")
_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: userNamespace, Name: "test-pool-launch-err"}})
Expect(err).Should(HaveOccurred())
Expect(err.Error()).Should(ContainSubstring("dynamic pool"))
Expect(err.Error()).Should(ContainSubstring("launch failed"))
})
})

// Tests for buildDynamicHostPool function
When("testing buildDynamicHostPool error paths", func() {
It("should use default instance tag when platform config doesn't specify one", func(ctx SpecContext) {
Expand Down
47 changes: 47 additions & 0 deletions pkg/reconciler/taskrun/provision_static_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,53 @@ var _ = Describe("Test Static Host Provisioning", func() {
})
})

When("error messages contain diagnostic context", func() {

It("should include host details in error when all hosts have been tried", func(ctx SpecContext) {
tr := runUserPipeline(ctx, client, reconciler, "test-err-hosts")
provision1 := getProvisionTaskRun(ctx, client, tr)
host1 := provision1.Labels[AssignedHost]

// Fail the first host
provision1.Status.CompletionTime = &metav1.Time{Time: time.Now()}
provision1.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: v1.ConditionFalse,
})
Expect(client.Status().Update(ctx, provision1)).ShouldNot(HaveOccurred())
_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: provision1.Namespace, Name: provision1.Name}})
Expect(err).ShouldNot(HaveOccurred())
Expect(client.Delete(ctx, provision1)).Should(Succeed())

// Reconcile the user task to try the next host
tr = getUserTaskRun(ctx, client, "test-err-hosts")
_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: tr.Namespace, Name: tr.Name}})
Expect(err).ShouldNot(HaveOccurred())

// Fail the second host
provision2 := getProvisionTaskRun(ctx, client, tr)
provision2.Status.CompletionTime = &metav1.Time{Time: time.Now()}
provision2.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: v1.ConditionFalse,
})
Expect(client.Status().Update(ctx, provision2)).ShouldNot(HaveOccurred())
_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: provision2.Namespace, Name: provision2.Name}})
Expect(err).ShouldNot(HaveOccurred())

// Final reconcile should fail and include failed host names
tr = getUserTaskRun(ctx, client, "test-err-hosts")
_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: tr.Namespace, Name: tr.Name}})
Expect(err).Should(HaveOccurred())
Expect(err.Error()).Should(ContainSubstring(host1))
Expect(err.Error()).Should(ContainSubstring("all available hosts"))

secret := getSecret(ctx, client, tr)
Expect(secret.Data["error"]).ShouldNot(BeEmpty())
Expect(string(secret.Data["error"])).Should(ContainSubstring(host1))
})
})

When("when provisioning succeeds", func() {

// It tests a specific failure case where the provisioner TaskRun reports
Expand Down
Loading