From 4c1da82e2881717b2f501c593be6dea5cf30d5e6 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 21 Nov 2025 17:34:25 +0000 Subject: [PATCH] Handle VM guest agent errors without marking nodes unhealthy (related to #736) --- pkg/proxmox/cluster_client.go | 64 +++++++++++++++++------------- pkg/proxmox/cluster_client_test.go | 47 ++++++++++++++++++++++ 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/pkg/proxmox/cluster_client.go b/pkg/proxmox/cluster_client.go index aecc7a1..c3a1165 100644 --- a/pkg/proxmox/cluster_client.go +++ b/pkg/proxmox/cluster_client.go @@ -44,6 +44,33 @@ var transientRateLimitStatusCodes = map[int]struct{}{ 504: {}, } +// isVMSpecificError reports whether an error string is scoped to a single VM/guest agent +// and should not be treated as a node connectivity failure. +func isVMSpecificError(errStr string) bool { + if errStr == "" { + return false + } + lower := strings.ToLower(errStr) + + if strings.Contains(lower, "no qemu guest agent") || + strings.Contains(lower, "qemu guest agent is not running") || + strings.Contains(lower, "guest agent") { + return true + } + + // QMP guest agent operations can time out or fail per-VM (e.g. guest-get-fsinfo). + // These aren't node connectivity issues and should not mark endpoints unhealthy. + if strings.Contains(lower, "qmp command") { + return true + } + + if strings.Contains(lower, "guest-get-") { + return true + } + + return false +} + // NewClusterClient creates a new cluster-aware client func NewClusterClient(name string, config ClientConfig, endpoints []string) *ClusterClient { cc := &ClusterClient{ @@ -120,17 +147,9 @@ func (cc *ClusterClient) initialHealthCheck() { cc.mu.Lock() // Check if error is VM-specific (shouldn't affect health) - isVMSpecificError := false - if err != nil { - errStr := err.Error() - if strings.Contains(errStr, "No QEMU guest agent") || - strings.Contains(errStr, "QEMU guest agent is not running") || - strings.Contains(errStr, "guest agent") { - isVMSpecificError = true - } - } + vmSpecificErr := err != nil && isVMSpecificError(err.Error()) - if err == nil || isVMSpecificError { + if err == nil || vmSpecificErr { // Node is healthy - create a proper client with full timeout for actual use fullCfg := cc.config fullCfg.Host = ep @@ -149,7 +168,7 @@ func (cc *ClusterClient) initialHealthCheck() { delete(cc.lastError, ep) cc.lastHealthCheck[ep] = time.Now() cc.clients[ep] = fullClient // Store the full client, not test client - if isVMSpecificError { + if vmSpecificErr { log.Debug(). Str("cluster", cc.name). Str("endpoint", ep). @@ -456,17 +475,9 @@ func (cc *ClusterClient) recoverUnhealthyNodes(ctx context.Context) { cancel() // Check if error is VM-specific (shouldn't prevent recovery) - isVMSpecificError := false - if err != nil { - errStr := err.Error() - if strings.Contains(errStr, "No QEMU guest agent") || - strings.Contains(errStr, "QEMU guest agent is not running") || - strings.Contains(errStr, "guest agent") { - isVMSpecificError = true - } - } + vmSpecificErr := err != nil && isVMSpecificError(err.Error()) - if err == nil || isVMSpecificError { + if err == nil || vmSpecificErr { recoveredEndpoints <- ep // Store the client with original timeout @@ -480,7 +491,7 @@ func (cc *ClusterClient) recoverUnhealthyNodes(ctx context.Context) { cc.clients[ep] = fullClient cc.mu.Unlock() - if isVMSpecificError { + if vmSpecificErr { log.Info(). Str("cluster", cc.name). Str("endpoint", ep). @@ -578,17 +589,14 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien // Error 403 for storage operations means permission issue, not node health issue // Error 500 with "No QEMU guest agent configured" means VM-specific issue, not node failure // Error 500 with "QEMU guest agent is not running" means VM-specific issue, not node failure - // Error 500 with any "guest agent" message means VM-specific issue, not node failure + // Error 500 with any guest agent/QMP message means VM-specific issue, not node failure // Error 400 with "ds" parameter error means Proxmox 9.x doesn't support RRD data source filtering // JSON unmarshal errors are data format issues, not connectivity problems // Context deadline/timeout errors on storage endpoints mean storage issues, not node unreachability - if strings.Contains(errStr, "595") || + if isVMSpecificError(errStr) || + strings.Contains(errStr, "595") || (strings.Contains(errStr, "500") && strings.Contains(errStr, "hostname lookup")) || (strings.Contains(errStr, "500") && strings.Contains(errStr, "Name or service not known")) || - (strings.Contains(errStr, "500") && strings.Contains(errStr, "No QEMU guest agent configured")) || - (strings.Contains(errStr, "500") && strings.Contains(errStr, "QEMU guest agent is not running")) || - (strings.Contains(errStr, "500") && strings.Contains(errStr, "guest agent")) || - strings.Contains(errStr, "guest agent") || (strings.Contains(errStr, "403") && (strings.Contains(errStr, "storage") || strings.Contains(errStr, "datastore"))) || strings.Contains(errStr, "permission denied") || (strings.Contains(errStr, "400") && strings.Contains(errStr, "\"ds\"") && strings.Contains(errStr, "property is not defined in schema")) || diff --git a/pkg/proxmox/cluster_client_test.go b/pkg/proxmox/cluster_client_test.go index f1e63bc..4ee5c68 100644 --- a/pkg/proxmox/cluster_client_test.go +++ b/pkg/proxmox/cluster_client_test.go @@ -63,3 +63,50 @@ func TestClusterClientHandlesRateLimitWithoutMarkingUnhealthy(t *testing.T) { t.Fatalf("expected at least 2 requests to backend, got %d", requestCount) } } + +func TestClusterClientIgnoresGuestAgentTimeoutForHealth(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api2/json/nodes": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"data":[{"node":"test","status":"online","cpu":0,"maxcpu":1,"mem":0,"maxmem":1,"disk":0,"maxdisk":1,"uptime":1,"level":"normal"}]}`) + case "/api2/json/nodes/test/qemu/100/agent/get-fsinfo": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"data":null,"message":"VM 100 qmp command 'guest-get-fsinfo' failed - got timeout\n"}`) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + cfg := ClientConfig{ + Host: server.URL, + TokenName: "pulse@pve!token", + TokenValue: "sometokenvalue", + VerifySSL: false, + Timeout: 2 * time.Second, + } + + cc := NewClusterClient("test-cluster", cfg, []string{server.URL}) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + _, err := cc.GetVMFSInfo(ctx, "test", 100) + if err == nil { + t.Fatalf("expected VM guest agent timeout error, got nil") + } + + health := cc.GetHealthStatusWithErrors() + endpointHealth, ok := health[server.URL] + if !ok { + t.Fatalf("expected health entry for endpoint %s", server.URL) + } + if !endpointHealth.Healthy { + t.Fatalf("expected endpoint to remain healthy after VM-specific guest agent error, got %+v", endpointHealth) + } + if endpointHealth.LastError != "" { + t.Fatalf("expected last error to remain empty for VM-specific failures, got %q", endpointHealth.LastError) + } +}