Skip to content

Fix: remove kagenti.io/type label and config-hash on AgentRuntime deletion - #396

Merged
pdettori merged 3 commits into
rossoctl:mainfrom
rh-dnagornuks:ar-remove-label
Jun 8, 2026
Merged

Fix: remove kagenti.io/type label and config-hash on AgentRuntime deletion#396
pdettori merged 3 commits into
rossoctl:mainfrom
rh-dnagornuks:ar-remove-label

Conversation

@rh-dnagornuks

@rh-dnagornuks rh-dnagornuks commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

When an AgentRuntime CR is deleted the controller previously preserved kagenti.io/type on the workload and replaced kagenti.io/config-hash with a defaults-only value, so the webhook continued to inject sidecars into replacement pods. This PR removes both the label and the annotation on deletion so that the next rolling update produces pods without sidecars, returning the workload to its pre-AR state. Docs and tests were updated to match the new lifecycle.

Changes

  • handleDeletion: remove kagenti.io/type from workload metadata labels and PodTemplateSpec pod labels instead of preserving it
  • handleDeletion: delete kagenti.io/config-hash from PodTemplateSpec pod annotations instead of replacing it with a defaults-only hash; removes the now-unused ComputeDefaultsOnlyHash call
  • Unit test: inverted assertions to confirm label and annotation are absent after deletion
  • E2E Agent lifecycle > should clean up on deletion: updated to assert kagenti.io/type is removed from both metadata and PodTemplateSpec, and config-hash is absent
  • E2E Combined > should clean up on AgentRuntime deletion and stop injection (renamed from "maintain injection"): updated to assert label/annotation removal and that replacement pods have no sidecars; replaced manual pod-delete-and-recheck loop with WaitForDeploymentReady + pod-list assertions
  • combined-agent e2e fixture: removed kagenti.io/type from spec.selector.matchLabels, spec.template.metadata.labels, and metadata.labels — the controller owns this label's lifecycle; having it in the selector prevented the update during deletion (Kubernetes rejects removal of selector-required labels from the pod template)
  • docs/architecture.md, docs/controller-webhook-interaction.md, GETTING_STARTED.md: updated deletion lifecycle descriptions

Testing

  • make test and pre-commit run --all-files pass
  • On ROSA cluster, create AR targeting agent deployment and confirm sidecar injection
  • On ROSA cluster, delete AR and confirm that new pods don't receive sidecars

Related Issues

Resolves: RHAIENG-4933

@rh-dnagornuks
rh-dnagornuks marked this pull request as ready for review June 4, 2026 17:12
@rh-dnagornuks
rh-dnagornuks requested a review from a team as a code owner June 4, 2026 17:12
…etion

Previously the finalizer preserved kagenti.io/type on the target workload
and replaced the config-hash with a defaults-only value, leaving the
workload labelled after the AR was removed and sidecars still injecting
with platform defaults.

On AR deletion the controller now removes:
- kagenti.io/type from workload metadata labels and PodTemplateSpec pod
  labels — the webhook's pre-filter requires this label, so future pods
  are no longer mutated
- kagenti.io/config-hash from PodTemplateSpec pod annotations — its
  removal still triggers a rolling update so existing injected pods are
  replaced, and leaves the workload clean for any future AR

The managed-by label, skills annotation, and skill volumes continue to
be removed as before.

Update docs/architecture.md, docs/controller-webhook-interaction.md, and
GETTING_STARTED.md to reflect the new deletion behaviour.

Signed-off-by: Daniels Nagornuks <dnagornu@redhat.com>

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
…h removal

The e2e deletion tests were asserting the old behaviour where
kagenti.io/type was preserved and config-hash was updated to a
defaults-only value after AR deletion.

Update both deletion tests (AgentRuntime context and combined context) to
assert that:
- kagenti.io/type is absent from workload metadata and PodTemplateSpec
- kagenti.io/config-hash is absent from PodTemplateSpec

Update the combined-agent deletion test to verify that the rolling update
triggered by the PodTemplate change produces pods without sidecars, rather
than pods with defaults-only sidecar config.

Signed-off-by: Daniels Nagornuks <dnagornu@redhat.com>

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
…eletion

The combined-agent e2e fixture had kagenti.io/type: agent in its
spec.selector.matchLabels. Kubernetes rejects updates that remove a
selector-required label from spec.template.metadata.labels (422
Unprocessable Entity). handleDeletion was failing on every attempt
because RetryOnConflict only retries on 409 Conflict, not 422, causing
the finalizer to never be cleared and kubectl delete to block until the
30-minute test timeout.

Fix: remove kagenti.io/type from the fixture's selector and pod-template
labels. The AgentRuntime controller owns that label lifecycle — it adds
the label on AR creation and removes it on deletion. The fixture now
represents a clean pre-AR deployment, which is the typical use case.

Signed-off-by: Daniels Nagornuks <dnagornu@redhat.com>

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>

@pdettori pdettori left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Clean, well-structured PR. The deletion logic change is sound — removing kagenti.io/type instead of preserving it correctly ensures the webhook pre-filter skips future pods. The delete() calls on maps are idempotent and safe. The fixture change (removing kagenti.io/type from spec.selector.matchLabels) is well-motivated — Kubernetes rejects updates that remove a selector-required label from the pod template (422), which would cause the finalizer to never clear.

Areas reviewed: Go controller logic, unit tests, E2E tests, fixtures, docs, security
Commits: 3 commits, all signed-off ✓
CI status: All passing (E2E pending at review time)

Nit (non-blocking): Commit 1efe0fd uses Fix: (capitalized) prefix instead of fix: (lowercase conventional commit). Minor inconsistency with the other two commits.

@pdettori

pdettori commented Jun 8, 2026

Copy link
Copy Markdown
Member

The E2E failure is unrelated to this PR. The failed test is Skill Discovery E2E [Feature gate enabled] should populate linkedSkills from annotation — it timed out (180s) waiting for status.linkedSkills to contain "summarizer" but the field stayed empty. This is a pre-existing flaky/infrastructure issue with the skill discovery feature gate mechanism, not caused by the deletion lifecycle changes here.

@pdettori
pdettori merged commit 35aa453 into rossoctl:main Jun 8, 2026
18 of 20 checks passed
cwiklik pushed a commit that referenced this pull request Jun 16, 2026
…etion (#396)

(cherry picked from commit 35aa453)
Signed-off-by: cwiklik <cwiklikj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants