changes from review

Author: taskbot

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -1672,12 +1672,12 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 
 			for i := range discoveredBackends {
 				backend := &discoveredBackends[i]
-				if healthStat, found := healthStatus[backend.Name]; found {
+				if healthInfo, found := healthStatus[backend.Name]; found {
 					// Map vmcp health status to CRD backend status
 					// vmcp statuses: healthy, unhealthy, degraded, unknown
 					// CRD statuses: ready, unavailable, degraded, unknown
 					var newStatus string
-					switch healthStat {
+					switch healthInfo.Status {
 					case "healthy":
 						newStatus = mcpv1alpha1.BackendStatusReady
 					case "unhealthy":
@@ -1691,15 +1691,20 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 						continue
 					}
 
-					// Only log if status changed
+					// Update status if changed
 					if newStatus != backend.Status {
 						ctxLogger.V(1).Info("Backend health check updated status",
 							"name", backend.Name,
 							"old_status", backend.Status,
 							"new_status", newStatus,
-							"health_status", healthStat)
+							"health_status", healthInfo.Status)
 						backend.Status = newStatus
 					}
+
+					// Update LastHealthCheck with actual health check timestamp from vmcp
+					if !healthInfo.LastCheckTime.IsZero() {
+						backend.LastHealthCheck = metav1.NewTime(healthInfo.LastCheckTime)
+					}
 				}
 			}
 		} else {
@@ -2149,6 +2154,12 @@ func (*VirtualMCPServerReconciler) vmcpReferencesCompositeToolDefinition(
 	return false
 }
 
+// BackendHealthInfo contains health information for a single backend
+type BackendHealthInfo struct {
+	Status        string
+	LastCheckTime time.Time
+}
+
 // BackendHealthStatusResponse represents the health status response from the vmcp health API
 type BackendHealthStatusResponse struct {
 	Backends []struct {
@@ -2161,12 +2172,12 @@ type BackendHealthStatusResponse struct {
 	} `json:"backends"`
 }
 
-// queryVMCPHealthStatus queries the vmcp health endpoint and returns backend health status.
+// queryVMCPHealthStatus queries the vmcp health endpoint and returns backend health information.
 // Returns nil if health monitoring is not enabled or if there's an error.
 func (*VirtualMCPServerReconciler) queryVMCPHealthStatus(
 	ctx context.Context,
 	vmcpURL string,
-) map[string]string {
+) map[string]*BackendHealthInfo {
 	ctxLogger := log.FromContext(ctx)
 
 	// Construct health endpoint URL
@@ -2212,10 +2223,13 @@ func (*VirtualMCPServerReconciler) queryVMCPHealthStatus(
 		return nil
 	}
 
-	// Convert to map of backendID -> status
-	healthStatus := make(map[string]string)
+	// Convert to map of backendID -> health info
+	healthStatus := make(map[string]*BackendHealthInfo)
 	for _, backend := range healthResp.Backends {
-		healthStatus[backend.BackendID] = backend.Status
+		healthStatus[backend.BackendID] = &BackendHealthInfo{
+			Status:        backend.Status,
+			LastCheckTime: backend.LastCheckTime,
+		}
 	}
 
 	ctxLogger.V(1).Info("Retrieved health status from vmcp server",

test/e2e/thv-operator/virtualmcp/helpers.go

@@ -72,6 +72,44 @@ func WaitForVirtualMCPServerReady(
 	}, timeout, pollingInterval).Should(gomega.Succeed())
 }
 
+// WaitForVirtualMCPServerDeployed waits for a VirtualMCPServer deployment to be running
+// without requiring the Ready condition to be True. This is useful for health monitoring tests
+// where some backends may intentionally be unhealthy.
+func WaitForVirtualMCPServerDeployed(
+	ctx context.Context,
+	c client.Client,
+	name, namespace string,
+	timeout time.Duration,
+	pollingInterval time.Duration,
+) {
+	vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
+
+	gomega.Eventually(func() error {
+		if err := c.Get(ctx, types.NamespacedName{
+			Name:      name,
+			Namespace: namespace,
+		}, vmcpServer); err != nil {
+			return err
+		}
+
+		// Check that the VirtualMCPServer has a URL (indicating it's been deployed)
+		if vmcpServer.Status.URL == "" {
+			return fmt.Errorf("VirtualMCPServer URL not set yet")
+		}
+
+		// Check that pods are running (but not necessarily all backends healthy)
+		labels := map[string]string{
+			"app.kubernetes.io/name":     "vmcp",
+			"app.kubernetes.io/instance": name,
+		}
+		if err := checkPodsReady(ctx, c, namespace, labels); err != nil {
+			return fmt.Errorf("VirtualMCPServer pods not ready: %w", err)
+		}
+
+		return nil
+	}, timeout, pollingInterval).Should(gomega.Succeed())
+}
+
 // checkPodsReady checks if all pods matching the given labels are ready
 func checkPodsReady(ctx context.Context, c client.Client, namespace string, labels map[string]string) error {
 	podList := &corev1.PodList{}

test/e2e/thv-operator/virtualmcp/virtualmcp_health_monitoring_test.go

@@ -121,8 +121,10 @@ var _ = Describe("VirtualMCPServer Health Monitoring", Ordered, func() {
 		}
 		Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed())
 
-		By("Waiting for VirtualMCPServer to be ready")
-		WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
+		By("Waiting for VirtualMCPServer to be deployed")
+		// Use WaitForVirtualMCPServerDeployed instead of WaitForVirtualMCPServerReady
+		// because we intentionally have an unhealthy backend, so Ready will be False
+		WaitForVirtualMCPServerDeployed(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval)
 	})
 
 	AfterAll(func() {
@@ -179,9 +181,7 @@ var _ = Describe("VirtualMCPServer Health Monitoring", Ordered, func() {
 	})
 
 	It("should report healthy status for working backends", func() {
-		// Wait for health checks to run (at least 2 intervals to stabilize)
-		time.Sleep(15 * time.Second)
-
+		// Eventually will poll until health checks stabilize
 		vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
 		Eventually(func() error {
 			if err := k8sClient.Get(ctx, types.NamespacedName{
@@ -215,9 +215,8 @@ var _ = Describe("VirtualMCPServer Health Monitoring", Ordered, func() {
 	})
 
 	It("should report unhealthy status for failing backend", func() {
-		// Wait for unhealthy threshold to be exceeded (at least unhealthyThreshold * interval)
+		// Eventually will poll until the unhealthy threshold is exceeded
 		By(fmt.Sprintf("Waiting for unhealthy backend to fail %d consecutive health checks", unhealthyThreshold))
-		time.Sleep(time.Duration(unhealthyThreshold+1) * 5 * time.Second)
 
 		vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
 		Eventually(func() error {
@@ -263,10 +262,7 @@ var _ = Describe("VirtualMCPServer Health Monitoring", Ordered, func() {
 			}
 		}
 
-		// Wait for at least 2 health check intervals
-		time.Sleep(12 * time.Second)
-
-		// Verify timestamps have been updated
+		// Eventually will poll until timestamps are updated (after at least 2 health check intervals)
 		Eventually(func() error {
 			if err := k8sClient.Get(ctx, types.NamespacedName{
 				Name:      vmcpServerName,
@@ -320,9 +316,7 @@ var _ = Describe("VirtualMCPServer Health Monitoring", Ordered, func() {
 		}, timeout, pollingInterval).Should(Succeed())
 
 		By("Waiting for health monitoring to detect recovery")
-		// Give health monitor time to detect the recovery (a few health check intervals)
-		time.Sleep(20 * time.Second)
-
+		// Eventually will poll until health monitoring detects the recovery
 		vmcpServer := &mcpv1alpha1.VirtualMCPServer{}
 		Eventually(func() error {
 			if err := k8sClient.Get(ctx, types.NamespacedName{