CR Status Improvements

[gazarenkov]

Description

• Two-level health monitoring: Added Runtime condition alongside existing Deployed condition
  Deployed: Reflects Deployment/StatefulSet replica status
  Runtime: Provides Pod/Container-level health (crash loops, image pull failures, OOMKilled, etc.)
• Improved status messages
• Smart crash loop detection: Only reports crash if container is NOT currently Ready - containers that recover after restart are not flagged as failed
• Enabled plugins in status: status.plugins field lists enabled dynamic plugins when healthy, cleared when unhealthy
• Unit tests: Added ~490 lines of tests covering all container state scenarios
• Updated docs/design.md with full status condition reference tables and examples

Which issue(s) does this PR fix or relate to

https://redhat.atlassian.net/browse/RHIDP-15021

PR acceptance criteria

• Tests
• Documentation

Building Container Images for Testing

Need to test container images from this PR?

For Maintainers: To trigger a test image build, review the code and comment /build-images.
This always builds the HEAD of the PR branch.

For Contributors: Ask a maintainer to run /build-images.

Images will be built and pushed to Quay with links posted in comments.

[openshift-ci]

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rhdh-qodo-merge]

PR Summary by Qodo

Add Runtime condition and plugin list to Backstage CR status

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🐞 Bug fix 🕐 40+ Minutes

AI Description

• Add Runtime and Config status conditions for pod/container health and spec validation feedback.
• Populate status.plugins with enabled dynamic plugins only when Deployed+Runtime are healthy.
• Add unit/integration coverage and update design docs/CRD schema for the new status fields.

Diagram

graph TD
  A["BackstageReconciler"] --> B["reconcileStatus"] --> C["setRuntimeCondition"] --> D{{"Pods"}} --> E[("Backstage CR status")]
  B --> F["setDeployedCondition"] --> G{{"Deployment/StatefulSet"}} --> E
  B --> H["setEnabledPlugins"] --> I["BackstageModel"] --> E
  subgraph Legend
    direction LR
    _proc["Controller logic"] ~~~ _k8s{{"K8s resource"}} ~~~ _cr[("CR status")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Single condition with richer reasons/messages

• ➕ Avoids introducing/teaching multiple condition types to consumers
• ➕ Simplifies integration tests and external tooling that expects one condition
• ➖ Conflates rollout readiness with runtime health; harder to distinguish 'rolled out but crash-looping' vs 'still progressing'
• ➖ Less extensible if future health signals (e.g., config validity) need separate lifecycles

2. Derive runtime health from Deployment/ReplicaSet conditions/events only

• ➕ Avoids per-pod inspection and reduces list calls during reconcile
• ➕ Leans on native workload status fields
• ➖ Misses container-granular failures (image pull, init container failures, OOMKilled details)
• ➖ Slower/less precise feedback than direct Pod/ContainerStatus inspection

3. Add a dedicated status sub-structure (e.g., status.runtime/status.deployment) instead of conditions

• ➕ More structured, strongly-typed status for clients; easier to evolve with new fields
• ➕ Avoids overloading Condition messages for machine parsing
• ➖ Moves away from standard Kubernetes Condition patterns; harder to integrate with generic tooling
• ➖ Requires CRD/schema expansion and client updates beyond condition watchers

Recommendation: Keep the PR’s two-condition approach. Separating Deployed (workload readiness) from Runtime (pod/container health) matches Kubernetes best practices for multi-signal health, prevents false positives during brief readiness windows, and enables future expansion (e.g., Config validity) without conflating concerns. The added unit tests make the container-state heuristics (notably the ‘only flag crash loops when not Ready’) maintainable.

Files changed (17) +808 / -47

Enhancement (6) +232 / -30

current-types.goExpose new condition types/reasons in current API constants +14/-1

Expose new condition types/reasons in current API constants

• Adds Runtime and Config condition types alongside Deployed, and defines dedicated reason constants for runtime health (Running/ContainerFailed/Pending) and config validation (Invalid). This centralizes status taxonomy for controllers and clients.

api/current-types.go

backstage_types.goAdd Runtime/Config condition types and status.plugins field +15/-0

Add Runtime/Config condition types and status.plugins field

• Extends v1alpha5 condition taxonomy with Runtime and Config types plus corresponding reasons. Adds BackstageStatus.Plugins to publish enabled dynamic plugin package names when healthy.

api/v1alpha5/backstage_types.go

zz_generated.deepcopy.goRegenerate deepcopy for plugins field support (v1alpha5) +6/-1

Regenerate deepcopy for plugins field support (v1alpha5)

• Adds DeepCopy logic for the new BackstageStatus.Plugins slice and normalizes import aliasing. Ensures status objects copy safely across reconciler updates.

api/v1alpha5/zz_generated.deepcopy.go

backstage_controller.goWire new status reconciliation and type-specific error conditions +13/-8

Wire new status reconciliation and type-specific error conditions

• Routes preprocessing/model init failures to Config/Invalid and runtime object failures to Deployed/Failed, improving signal specificity. Switches from setDeploymentStatus to reconcileStatus and requeues when runtime/deployment isn’t ready to re-check pod/container health.

internal/controller/backstage_controller.go

backstage_status.goImplement Runtime condition, improved Deployed messages, and plugin publication +172/-18

Implement Runtime condition, improved Deployed messages, and plugin publication

• Introduces reconcileStatus to set Runtime (pod/container) and Deployed (workload) conditions, requiring both for overall readiness. Adds detailed container state inspection (waiting/terminated/crash-loop with 'ready' recovery logic), improves rollout messages to 'X/Y replicas ready', and publishes a sorted enabled-plugin list only when healthy.

internal/controller/backstage_status.go

runtime.goExpose enabled dynamic plugins from the model +12/-2

Expose enabled dynamic plugins from the model

• Adds BackstageModel.GetEnabledPlugins to retrieve enabled dynamic plugin packages (or an empty slice when absent). Also slightly simplifies validation loop error handling.

pkg/model/runtime.go

Bug fix (1) +11 / -1

install_plugins.shPreserve xargs failures and emit termination message fallback +11/-1

Preserve xargs failures and emit termination message fallback

• Captures xargs exit status from parallel plugin downloads and fails the script with a clear message if xargs failed but no FAILURE_LOG was produced. Improves diagnosability and prevents silent success on download failures.

plugin-installer/install_plugins.sh

Refactor (4) +4 / -4

zz_generated.deepcopy.goFix import aliasing in generated deepcopy (v1alpha1) +1/-1

Fix import aliasing in generated deepcopy (v1alpha1)

• Updates the metav1 import to use an explicit alias, reflecting regeneration/gofmt normalization. No behavioral change expected.

api/v1alpha1/zz_generated.deepcopy.go

zz_generated.deepcopy.goFix import aliasing in generated deepcopy (v1alpha2) +1/-1

Fix import aliasing in generated deepcopy (v1alpha2)

• Updates the apiextensions v1 import to use an explicit alias, reflecting regeneration/gofmt normalization. No behavioral change expected.

api/v1alpha2/zz_generated.deepcopy.go

zz_generated.deepcopy.goFix import aliasing in generated deepcopy (v1alpha3) +1/-1

Fix import aliasing in generated deepcopy (v1alpha3)

• Updates the apiextensions v1 import to use an explicit alias, reflecting regeneration/gofmt normalization. No behavioral change expected.

api/v1alpha3/zz_generated.deepcopy.go

zz_generated.deepcopy.goFix import aliasing in generated deepcopy (v1alpha4) +1/-1

Fix import aliasing in generated deepcopy (v1alpha4)

• Updates the apiextensions v1 import to use an explicit alias, reflecting regeneration/gofmt normalization. No behavioral change expected.

api/v1alpha4/zz_generated.deepcopy.go

Tests (3) +500 / -6

default-config_test.goUpdate integration assertions for two status conditions +7/-6

Update integration assertions for two status conditions

• Adjusts tests to expect both Deployed and Runtime conditions and uses meta.FindStatusCondition to assert by condition type rather than slice order. Improves stability as conditions expand.

integration_tests/default-config_test.go

rhdh-config_test.goEnsure namespaces are cleaned up after integration tests +2/-0

Ensure namespaces are cleaned up after integration tests

• Adds deleteNamespace calls after specific test flows to reduce resource leakage and cross-test interference in the suite.

integration_tests/rhdh-config_test.go

backstage_status_test.goAdd comprehensive unit tests for container/deploy/statefulset status logic +491/-0

Add comprehensive unit tests for container/deploy/statefulset status logic

• Adds ~490 LOC of tests covering main vs init container states, crash-loop recovery behavior, deployment/statefulset message formatting, and condition upsert semantics. Provides regression coverage for the new Runtime condition heuristics.

internal/controller/backstage_status_test.go

Documentation (1) +55 / -5

design.mdDocument Deployed vs Runtime conditions and plugins behavior +55/-5

Document Deployed vs Runtime conditions and plugins behavior

• Replaces the single-condition description with tables defining Deployed and Runtime condition semantics, reasons, and example messages. Adds an end-to-end status YAML example and clarifies that plugins are only listed when fully healthy.

docs/design.md

Other (2) +6 / -1

rhdh.redhat.com_backstages.yamlExtend CRD schema with status.plugins +6/-0

Extend CRD schema with status.plugins

• Adds the plugins array under status to the CRD OpenAPI schema, documenting that it is sorted alphabetically. Enables kubectl/clients to validate and display the field.

config/crd/bases/rhdh.redhat.com_backstages.yaml

deployment.yamlTidy default deployment env var comment +0/-1

Tidy default deployment env var comment

• Removes an outdated inline comment above CATALOG_INDEX_IMAGE. No functional change to the rendered deployment.

config/profile/rhdh/default-config/deployment.yaml

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Startup waits flagged failed 🐞 Bug ≡ Correctness

Description

checkContainerState() treats any non-empty container Waiting reason (except PodInitializing) as
ContainerFailed, which will mark Runtime as failed during normal startup/transient waits that the
design explicitly categorizes as Pending.

Code

internal/controller/backstage_status.go[R149-153]

+	if cs.State.Waiting != nil && cs.State.Waiting.Reason != "" {
+		if cs.State.Waiting.Reason == "PodInitializing" {
+			return "", "" // Normal state, not an error
+		}
+		return api.BackstageConditionReasonContainerFailed, fmt.Sprintf("%s %q: %s", prefix, cs.Name, cs.State.Waiting.Reason)

Relevance

●● Moderate

Status semantics change (Waiting reasons) could be debated vs design; prior status-condition
behavioral tweaks were sometimes rejected.

PR-#1949

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The design doc defines Pending as covering containers still starting; however the new logic returns
ContainerFailed for any Waiting reason other than PodInitializing, which will violate that
definition for transient startup waits.

internal/controller/backstage_status.go[141-155]
docs/design.md[104-111]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`checkContainerState()` returns `ContainerFailed` for almost all Waiting reasons. This conflicts with the design doc which states Pending should cover containers "still starting"; many normal startup states are represented as Waiting with a reason.

## Issue Context
You already correctly classify `ImagePullBackOff` and `CrashLoopBackOff` as failure scenarios. The issue is the default behavior for Waiting reasons is too aggressive.

## Fix Focus Areas
- internal/controller/backstage_status.go[141-155]
- docs/design.md[104-111]
- internal/controller/backstage_status_test.go[14-147]

### Suggested fix
1. Replace the current Waiting handling with a small classifier:
  - Treat known transient reasons as `Pending` (example set: `PodInitializing`, `ContainerCreating`).
  - Treat known failure reasons as `ContainerFailed` (keep `ImagePullBackOff`, `ErrImagePull`, `CrashLoopBackOff`, and optionally config errors like `CreateContainerConfigError`).
  - For unknown Waiting reasons, prefer `Pending` unless you have strong evidence it's terminal.
2. Add unit tests for a transient Waiting reason (e.g., `ContainerCreating`) asserting Runtime reason `Pending`, not `ContainerFailed`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Pod List() missing RBAC 📘 Rule violation ⛨ Security

Description

The controller now lists Pods to compute the Runtime status condition, but the operator/controller
RBAC does not grant access to the core pods resource, which can cause reconciliation and status
updates to fail with RBAC forbidden errors. This can leave the Runtime condition stuck in
Pending and trigger continuous requeues.

Code

internal/controller/backstage_status.go[R83-86]

+	labelSelector := client.MatchingLabels{
+		model.BackstageAppLabel: utils.BackstageAppLabelValue(backstage.Name),
+	}
+	if err := r.List(ctx, podList, client.InNamespace(backstage.Namespace), labelSelector); err != nil {

Relevance

●●● Strong

Missing RBAC for newly listed resource is a common runtime-breaker; team has accepted adding RBAC
for new kinds.

PR-#1651

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR-added runtime status computation calls r.List(...) on a corev1.PodList to derive the
Runtime condition, which requires Kubernetes RBAC permissions on the core pods resource.
However, the existing RBAC coverage (both the generated/annotated controller rules and the operator
ClusterRole in config/rbac/role.yaml) includes other core/app resources (e.g., configmaps,
secrets, services, PVCs, deployments, statefulsets, etc.) but omits pods, so the new Pod listing
operation is not authorized and would fail at runtime with forbidden errors, preventing the
condition from progressing beyond Pending.

Rule 8: Ensure operator RBAC includes required verbs for managed Kubernetes resource kinds
internal/controller/backstage_status.go[83-90]
config/rbac/role.yaml[7-21]
internal/controller/backstage_status.go[79-92]
internal/controller/backstage_controller.go[47-58]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Backstage controller now calls `r.List(...)` on `corev1.PodList` to compute/derive the `Runtime` status condition, but the operator/controller RBAC does not include permissions for the core `pods` resource. This can result in RBAC `forbidden` errors during reconciliation, leaving `Runtime` stuck in Pending and causing repeated requeues.

## Issue Context
The pod-listing behavior was introduced as part of the new Runtime condition computation, so RBAC must be updated to include at least `list` (and typically `get;list;watch`) on `pods`, and the repo’s manifest/RBAC generation pipeline (e.g., controller-gen/kustomize) should be rerun so the deployed Role/ClusterRole includes these permissions. Optionally, surfacing the underlying list error in the Runtime condition message can make RBAC/debugging failures easier to diagnose.

## Fix Focus Areas
- internal/controller/backstage_status.go[79-92]
- internal/controller/backstage_status.go[83-86]
- internal/controller/backstage_controller.go[47-58]
- config/rbac/role.yaml[7-21]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Terminating pods skew Runtime 🐞 Bug ☼ Reliability

Description

checkPodStates() evaluates all pods matching the Backstage app label and can return Pending/Failed
based on old terminating pods, so Runtime may stay unhealthy during rollouts even when the newest
replacement pod is Running/Ready.

Code

internal/controller/backstage_status.go[R111-115]

+	for _, pod := range pods {
+		// Check pod phase for terminal failures
+		if pod.Status.Phase == corev1.PodFailed {
+			return api.BackstageConditionReasonContainerFailed, fmt.Sprintf("pod %q failed", pod.Name)
+		}

Relevance

●● Moderate

Pod selection during rollouts (terminating/stale pods) is subtle; no strong repo precedent for
excluding them.

PR-#1652

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Pods are selected solely by the Backstage app label (set on the pod template/selector), and the scan
iterates all matching pods without excluding terminating pods, making stale rollout pods able to
affect the Runtime result.

internal/controller/backstage_status.go[104-136]
pkg/model/deployment.go[160-165]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Runtime condition scans every pod that matches the Backstage app label. During rolling updates, this label matches both new and old pods. Because the scan does not skip terminating pods, an old pod with non-ready containers can incorrectly determine the Runtime condition.

## Issue Context
The Deployment model sets the Backstage app label on the pod template and uses it in the selector, so all ReplicaSet pods (including old ones) will match.

## Fix Focus Areas
- internal/controller/backstage_status.go[104-136]
- pkg/model/deployment.go[160-165]

### Suggested fix
1. Filter pods before evaluation:
  - Skip pods with `pod.DeletionTimestamp != nil` (terminating).
  - Optionally skip `pod.Status.Phase == Succeeded`.
2. If all matching pods are skipped and none remain, report `Pending` with a clear message (e.g., "no active pods found").
3. Add unit tests for `checkPodStates()` covering a mix of terminating + healthy pods to ensure terminating pods don't force Pending/Failed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Unbounded 10s status polling 🐞 Bug ➹ Performance

Description

Reconcile() requeues every 10 seconds for any non-healthy status (including ContainerFailed),
causing repeated apply+status loops and API churn during persistent failures because the controller
does not watch Pods for status changes.

Code

internal/controller/backstage_controller.go[R119-123]

+	isReady := r.reconcileStatus(ctx, &backstage, *bsModel)
+	if !isReady {
+		// Requeue to check pod status again (for init container failures, etc.)
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+	}

Relevance

●● Moderate

Polling/requeue strategy is behavioral/perf tradeoff; no close precedent showing team will change it
now.

PR-#2084

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Reconcile explicitly requeues on any !isReady, and Runtime health is only true for reason Running;
combined with no Pod watches, this results in fixed-interval polling during persistent Runtime
failures.

internal/controller/backstage_controller.go[113-123]
internal/controller/backstage_status.go[76-102]
internal/controller/watchers.go[78-104]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The controller now requeues every 10 seconds whenever `reconcileStatus()` returns false. This includes terminal/persistent failure cases (e.g., Runtime `ContainerFailed`) and will continuously re-run reconciliation, including re-applying objects, which increases load.

## Issue Context
The controller watches Deployments/StatefulSets but not Pods, so polling is currently the only way Runtime can change. That makes it important to avoid tight polling for terminal failure modes.

## Fix Focus Areas
- internal/controller/backstage_controller.go[113-123]
- internal/controller/backstage_status.go[19-40]
- internal/controller/watchers.go[78-104]

### Suggested fix
1. Have `reconcileStatus()` (or `setRuntimeCondition`) return (ready, runtimeReason) so Reconcile can requeue selectively:
  - Requeue for `Pending` / `DeployInProgress`.
  - Do not requeue (or requeue with a much larger backoff) for `ContainerFailed` / `DeployFailed`.
2. Alternatively/additionally, add a Pod watch that enqueues the owning Backstage CR by label, which reduces the need for polling (requires pod RBAC as well).
3. If keeping polling, implement exponential backoff keyed by condition reason to avoid sustained 10s loops.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 18 rules

✅ Cross-repo context

  Explored: repo: redhat-developer/rhdh (sha: 4c5a4e85)

  Explored: repo: redhat-developer/rhdh-plugins (sha: 6ea99775)

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}