From 4c0a6444cd5b928bdcbe1222e821d52cd6bbda09 Mon Sep 17 00:00:00 2001 From: Pulse Monitor Date: Fri, 5 Sep 2025 22:29:29 +0000 Subject: [PATCH] fix: improve cluster health checks and handle VMFileSystem unmarshal errors (addresses #405) - Made cluster health checks less aggressive to prevent false unhealthy states - Fixed JSON unmarshal error when Proxmox returns object instead of array for VMFileSystem - Increased initial health check timeouts from 2s to 5s for better reliability - Added handling for JSON unmarshal errors as data format issues, not connectivity problems - Improved recovery check interval from 5s to 10s to reduce excessive health checks - Changed log levels from WARN to DEBUG for transient connectivity issues --- pkg/proxmox/client.go | 34 ++++++++++++++++++++++++++++------ pkg/proxmox/cluster_client.go | 32 ++++++++++++++++++-------------- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index a159c8f00..76c128c99 100644 --- a/pkg/proxmox/client.go +++ b/pkg/proxmox/client.go @@ -905,17 +905,39 @@ func (c *Client) GetVMFSInfo(ctx context.Context, node string, vmid int) ([]VMFi } defer resp.Body.Close() - var result struct { + // First, read the response body into bytes so we can try multiple unmarshal attempts + bodyBytes, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + // Try to unmarshal as an array first (expected format) + var arrayResult struct { Data struct { Result []VMFileSystem `json:"result"` } `json:"data"` } - - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return nil, err + if err := json.Unmarshal(bodyBytes, &arrayResult); err == nil && arrayResult.Data.Result != nil { + return arrayResult.Data.Result, nil } - - return result.Data.Result, nil + + // If that fails, try as an object (might be an error response or different format) + var objectResult struct { + Data struct { + Result interface{} `json:"result"` + } `json:"data"` + } + if err := json.Unmarshal(bodyBytes, &objectResult); err == nil { + // If result is an object, it might be an error or empty response + // Return empty array to indicate no filesystem info available + log.Debug(). + Interface("result", objectResult.Data.Result). + Msg("GetVMFSInfo received object instead of array, returning empty") + return []VMFileSystem{}, nil + } + + // If both fail, return error + return nil, fmt.Errorf("unexpected response format from guest agent get-fsinfo") } // GetVMStatus returns detailed VM status including balloon info diff --git a/pkg/proxmox/cluster_client.go b/pkg/proxmox/cluster_client.go index 1d25ec6c2..8a9450b98 100644 --- a/pkg/proxmox/cluster_client.go +++ b/pkg/proxmox/cluster_client.go @@ -61,16 +61,19 @@ func (cc *ClusterClient) initialHealthCheck() { return } + // For multi-node clusters, do a very quick check but don't mark unhealthy immediately + // This prevents nodes from being marked unhealthy due to temporary startup conditions + var wg sync.WaitGroup for _, endpoint := range cc.endpoints { wg.Add(1) go func(ep string) { defer wg.Done() - // Try a quick connection test + // Try a quick connection test with slightly longer timeout for initial check cfg := cc.config cfg.Host = ep - cfg.Timeout = 2 * time.Second + cfg.Timeout = 5 * time.Second testClient, err := NewClient(cfg) if err != nil { @@ -85,8 +88,8 @@ func (cc *ClusterClient) initialHealthCheck() { return } - // Quick test - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + // Quick test with slightly longer timeout for initial check + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) _, err = testClient.GetNodes(ctx) cancel() @@ -302,10 +305,10 @@ func (cc *ClusterClient) recoverUnhealthyNodes(ctx context.Context) { now := time.Now() for endpoint, healthy := range cc.nodeHealth { if !healthy { - // Skip if we checked this endpoint recently (within 5 seconds) - // Reduced from 30 seconds to allow faster recovery + // Skip if we checked this endpoint recently (within 10 seconds) + // Balance between recovery speed and avoiding excessive checks if lastCheck, exists := cc.lastHealthCheck[endpoint]; exists { - if now.Sub(lastCheck) < 5*time.Second { + if now.Sub(lastCheck) < 10*time.Second { continue } } @@ -422,13 +425,16 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien // Error 500 with hostname lookup failure means a node reference issue, not endpoint failure // 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 + // JSON unmarshal errors are data format issues, not connectivity problems if 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, "403") && (strings.Contains(errStr, "storage") || strings.Contains(errStr, "datastore"))) || - strings.Contains(errStr, "permission denied") { + strings.Contains(errStr, "permission denied") || + strings.Contains(errStr, "json: cannot unmarshal") || + strings.Contains(errStr, "unexpected response format") { // This is likely a node-specific failure, not an endpoint failure // Return the error but don't mark the endpoint as unhealthy log.Debug(). @@ -526,10 +532,9 @@ func (cc *ClusterClient) GetVMs(ctx context.Context, node string) ([]VM, error) return nil }) - // If we get "no healthy nodes" error, return empty list instead of error - // This prevents VMs from disappearing when cluster has connectivity issues + // Don't return error for transient connectivity issues - preserve UI state if err != nil && strings.Contains(err.Error(), "no healthy nodes available") { - log.Warn(). + log.Debug(). Str("cluster", cc.name). Str("node", node). Err(err). @@ -551,10 +556,9 @@ func (cc *ClusterClient) GetContainers(ctx context.Context, node string) ([]Cont return nil }) - // If we get "no healthy nodes" error, return empty list instead of error - // This prevents containers from disappearing when cluster has connectivity issues + // Don't return error for transient connectivity issues - preserve UI state if err != nil && strings.Contains(err.Error(), "no healthy nodes available") { - log.Warn(). + log.Debug(). Str("cluster", cc.name). Str("node", node). Err(err).