Deep Review: PR #236

ObservedGeneration on a condition conventionally refers to the .metadata.generation of the resource that owns the condition. Copying the LimaVM's value into the App makes it reference the LimaVM's generation, which is meaningless on the App resource. Currently harmless because the LimaVM controller does not set ObservedGeneration (defaults to 0), but this becomes incorrect if it ever does.

	statusChanged := false
	for _, cond := range limaVM.Status.Conditions {
		statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
			Type:               cond.Type,
			Status:             cond.Status,
			Reason:             cond.Reason,
			Message:            cond.Message,
			ObservedGeneration: cond.ObservedGeneration,
			LastTransitionTime: cond.LastTransitionTime,
		}) || statusChanged
	}

The LimaVM controller's setCondition at limavm_controller.go:427-437 does not set ObservedGeneration:

// setCondition updates or adds a condition in the LimaVM status.
func (r *LimaVMReconciler) setCondition(limaVM *v1alpha1.LimaVM, conditionType string, status metav1.ConditionStatus, reason, message string) {
	changed := apimeta.SetStatusCondition(&limaVM.Status.Conditions, metav1.Condition{
		Type:    conditionType,
		Status:  status,
		Reason:  reason,
		Message: message,
	})
	if changed && r.Recorder != nil {
		r.Recorder.Eventf(limaVM, nil, corev1.EventTypeNormal, "ConditionChanged", conditionType, message)
	}
}

Fix: Set ObservedGeneration: app.Generation, or omit the field.

If r.Get returns a transient error (e.g., API timeout), the controller logs it but continues to ctrl.Result{}, nil, forgoing retry. The input ConfigMap has an ownerReference to the App, so Kubernetes GC handles cleanup eventually. The App controller also re-reconciles on LimaVM status changes, so the window is small. Still, returning the error would be more correct.

	inputCM := &corev1.ConfigMap{}
	if err := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputCM); err == nil {
		if err := r.Delete(ctx, inputCM); err != nil && !apierrors.IsNotFound(err) {
			return ctrl.Result{}, fmt.Errorf("failed to delete input ConfigMap: %w", err)
		}
		log.Info("Deleted input ConfigMap after LimaVM created its own copy")
	} else if !apierrors.IsNotFound(err) {
		log.Error(err, "Failed to fetch input ConfigMap")
	}

Fix: Return ctrl.Result{}, err on transient Get failures.

Same pattern as above, but during deletion. The input ConfigMap has an ownerReference, so GC provides a safety net. But the controller should not proceed to remove the cleanup finalizer before verifying cleanup succeeded.

	case apierrors.IsNotFound(err):
		// LimaVM is gone. Clean up the input ConfigMap if it still exists
		inputCM := &corev1.ConfigMap{}
		if cmErr := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputCM); cmErr == nil {
			if cmErr := r.Delete(ctx, inputCM); cmErr != nil && !apierrors.IsNotFound(cmErr) {
				return ctrl.Result{}, fmt.Errorf("failed to delete input ConfigMap during cleanup: %w", cmErr)
			}
		} else if !apierrors.IsNotFound(cmErr) {
			log.Error(cmErr, "Failed to fetch input ConfigMap during cleanup")
		}
		return ctrl.Result{}, base.RemoveCleanupFinalizer(ctx, r.Client, &app)

Fix: Return ctrl.Result{}, cmErr when cmErr is not nil and not NotFound.

The loop over limaVM.Status.Conditions adds or updates conditions on the App but never removes conditions that no longer exist on the LimaVM. In practice, LimaVM conditions transition between states rather than disappearing, so this risk is low. If the LimaVM is deleted and recreated with a different condition set, stale conditions would linger.

	// Mirror LimaVM status conditions onto the App status.
	statusChanged := false
	for _, cond := range limaVM.Status.Conditions {
		statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
			Type:               cond.Type,
			Status:             cond.Status,
			Reason:             cond.Reason,
			Message:            cond.Message,
			ObservedGeneration: cond.ObservedGeneration,
			LastTransitionTime: cond.LastTransitionTime,
		}) || statusChanged
	}
	if statusChanged {
		if err := r.Status().Update(ctx, &app); err != nil {
			log.Error(err, "Unable to update App status")
			return ctrl.Result{}, err
		}
	}

Fix: Before the loop, diff condition types and remove any App conditions absent from the LimaVM. Or accept this as a known limitation with a comment.

The test file has no local_teardown_file. The existing LimaVM tests (e.g., limavm-instance.bats) define one to clean up $RDD_LIMA_HOME/<instance>. Without it, the default teardown runs rdd svc stop, which kills the control plane mid-deletion, potentially leaving orphaned instance directories.

local_setup_file() {
    setup_rdd_control_plane "app,limavm"
}

Fix: Add a local_teardown_file that cleans up ${RDD_LIMA_HOME}/${VM_NAME}, matching the pattern in limavm-instance.bats.

The PR removes the Recorder events.EventRecorder field and the setCondition helper that emitted ConditionChanged events. The old code was a stub, and the App controller is now a thin orchestration layer — the LimaVM controller emits the real lifecycle events. Still, emitting events on App status transitions would improve observability.

Fix: Restore the Recorder and emit an event when statusChanged is true after condition mirroring.

	// Propagate spec.running from App into to the LimaVM.

Fix: // Propagate spec.running from App to the LimaVM.