Deep Code Review: PR #236

The sentence trails off mid-thought at line 114:

	namespace := app.GetResourceNamespace()

	// Check LimaVM first. If it exists and we can skip ConfigMap
	limaVM := &limav1alpha1.LimaVM{}
	limaVMErr := r.Get(ctx, client.ObjectKey{Name: limaVMName, Namespace: namespace}, limaVM)
	if limaVMErr != nil && !apierrors.IsNotFound(limaVMErr) {
		return ctrl.Result{}, limaVMErr

Fix: // Check whether the LimaVM already exists. If not, create the input ConfigMap and LimaVM.

	// Propagate spec.running from App into to the LimaVM.
	if limaVM.Spec.Running != app.Spec.Running {
		limaVM.Spec.Running = app.Spec.Running
		if err := r.Update(ctx, limaVM); err != nil {
			return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err)
		}
		return ctrl.Result{}, nil
	}

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

ObservedGeneration on a condition refers to the generation of the resource that owns the condition. Copying the LimaVM's value onto the App is semantically wrong — the two resources have independent generation sequences. Currently harmless because the LimaVM controller doesn't set ObservedGeneration (it stays 0), but any tooling that compares condition.observedGeneration >= resource.metadata.generation would be misled.

	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
	}

Fix: Use app.Generation instead, or omit it (zero value means "not tracking").

The comment says the 32KB limit is for "the whole object," but the limit comes from the CRD's maxLength: 32768 on the condition .message field. The etcd object limit is ~1.5 MB.

// conditionMessageMaxLen caps how long a condition message can be.
// The API server has a 32KB limit for the whole object, so we keep
// messages short to avoid failures, especially when errors get long
// (like repeated image download retries).
const conditionMessageMaxLen = 32768

Fix: // conditionMessageMaxLen matches the standard Kubernetes maxLength (32768) for condition messages defined in the CRD schema.

The return ctrl.Result{}, nil at line 177 stops the reconcile. The reconciler Owns(&limav1alpha1.LimaVM{}) but not Owns(&corev1.ConfigMap{}), so deleting the ConfigMap produces no watch event. The controller relies on the LimaVM emitting a future status update. In practice the LimaVM controller will still be processing (downloading images, starting the VM), so a watch event will arrive — but the coupling is implicit and fragile.

	// LimaVM exists — clean up the input ConfigMap if it still lingers from the
	// creation phase and return to requeueing on LimaVM updates.
	inputConfigMap := &corev1.ConfigMap{}
	if err := r.Get(ctx, client.ObjectKey{Name: inputConfigMapName, Namespace: namespace}, inputConfigMap); err == nil {
		if err := r.Delete(ctx, inputConfigMap); 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")
		return ctrl.Result{}, nil
	} else if !apierrors.IsNotFound(err) {
		log.Error(err, "Failed to fetch input ConfigMap")
	}

Fix: Fall through to propagate spec.running and mirror conditions in the same pass, or add Requeue: true.

The LimaVM controller truncates messages via truncateConditionMessage(), so messages should already be within limits. But the App controller has no independent guard. If a future LimaVM code path bypasses truncation, the App's status update would fail CRD validation.

	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
	}

Fix: Apply truncation defensively, or extract truncateConditionMessage to the base package so both controllers can share it.