From 1c5b2266afb7a4bf1f95abb84e296b1e6a32a6c9 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:05:16 +0100 Subject: [PATCH 01/13] HYPERFLEET-386 - feat: add ResourceConditionStatus and AdapterConditionStatus types Defines separate condition status types: - ResourceConditionStatus (True/False) for Cluster/NodePool conditions - AdapterConditionStatus (True/False/Unknown) for adapter-reported conditions This separation ensures resource status is always definitive while adapters can report Unknown when status is indeterminate. --- pkg/api/status_types.go | 49 ++++++++++--------- pkg/api/status_types_test.go | 92 ++++++++++++++++++++++++------------ 2 files changed, 85 insertions(+), 56 deletions(-) diff --git a/pkg/api/status_types.go b/pkg/api/status_types.go index afd8ccf..c0d8b67 100644 --- a/pkg/api/status_types.go +++ b/pkg/api/status_types.go @@ -2,47 +2,46 @@ package api import "time" -// ResourcePhase represents the lifecycle phase of a resource -// Domain equivalent of openapi.ResourcePhase -type ResourcePhase string +// ResourceConditionStatus represents the status of a resource condition (True/False only) +// Domain equivalent of openapi.ResourceConditionStatus +type ResourceConditionStatus string const ( - PhaseNotReady ResourcePhase = "NotReady" // String value matches openapi.NOT_READY - PhaseReady ResourcePhase = "Ready" // String value matches openapi.READY - PhaseFailed ResourcePhase = "Failed" // String value matches openapi.FAILED + ConditionTrue ResourceConditionStatus = "True" // String value matches openapi.TRUE + ConditionFalse ResourceConditionStatus = "False" // String value matches openapi.FALSE ) -// ConditionStatus represents the status of a condition -// Domain equivalent of openapi.ConditionStatus -type ConditionStatus string +// AdapterConditionStatus represents the status of an adapter condition (includes Unknown) +// Domain equivalent of openapi.AdapterConditionStatus +type AdapterConditionStatus string const ( - ConditionTrue ConditionStatus = "True" // String value matches openapi.TRUE - ConditionFalse ConditionStatus = "False" // String value matches openapi.FALSE - ConditionUnknown ConditionStatus = "Unknown" // String value matches openapi.UNKNOWN + AdapterConditionTrue AdapterConditionStatus = "True" + AdapterConditionFalse AdapterConditionStatus = "False" + AdapterConditionUnknown AdapterConditionStatus = "Unknown" ) // ResourceCondition represents a condition of a resource // Domain equivalent of openapi.ResourceCondition // JSON tags match database JSONB structure type ResourceCondition struct { - ObservedGeneration int32 `json:"observed_generation"` - CreatedTime time.Time `json:"created_time"` - LastUpdatedTime time.Time `json:"last_updated_time"` - Type string `json:"type"` - Status ConditionStatus `json:"status"` - Reason *string `json:"reason,omitempty"` - Message *string `json:"message,omitempty"` - LastTransitionTime time.Time `json:"last_transition_time"` + ObservedGeneration int32 `json:"observed_generation"` + CreatedTime time.Time `json:"created_time"` + LastUpdatedTime time.Time `json:"last_updated_time"` + Type string `json:"type"` + Status ResourceConditionStatus `json:"status"` + Reason *string `json:"reason,omitempty"` + Message *string `json:"message,omitempty"` + LastTransitionTime time.Time `json:"last_transition_time"` } // AdapterCondition represents a condition of an adapter // Domain equivalent of openapi.AdapterCondition // JSON tags match database JSONB structure type AdapterCondition struct { - Type string `json:"type"` - Status ConditionStatus `json:"status"` - Reason *string `json:"reason,omitempty"` - Message *string `json:"message,omitempty"` - LastTransitionTime time.Time `json:"last_transition_time"` + Type string `json:"type"` + Status AdapterConditionStatus `json:"status"` + Reason *string `json:"reason,omitempty"` + Message *string `json:"message,omitempty"` + LastTransitionTime time.Time `json:"last_transition_time"` } diff --git a/pkg/api/status_types_test.go b/pkg/api/status_types_test.go index a7cfde4..337585c 100644 --- a/pkg/api/status_types_test.go +++ b/pkg/api/status_types_test.go @@ -8,43 +8,42 @@ import ( . "github.com/onsi/gomega" ) -// TestResourcePhase_Constants verifies that ResourcePhase constants match OpenAPI equivalents -func TestResourcePhase_Constants(t *testing.T) { +// TestConditionStatus_Constants verifies that ConditionStatus constants match OpenAPI equivalents +func TestConditionStatus_Constants(t *testing.T) { RegisterTestingT(t) // Verify constant values match expected strings - Expect(string(PhaseNotReady)).To(Equal("NotReady")) - Expect(string(PhaseReady)).To(Equal("Ready")) - Expect(string(PhaseFailed)).To(Equal("Failed")) + Expect(string(ConditionTrue)).To(Equal("True")) + Expect(string(ConditionFalse)).To(Equal("False")) - // These values should match openapi.NOT_READY, openapi.READY, openapi.FAILED - // which are "NotReady", "Ready", "Failed" respectively + // These values should match openapi.ResourceConditionStatus + // which has "True" and "False" values } -// TestResourcePhase_StringConversion tests type casting to/from string -func TestResourcePhase_StringConversion(t *testing.T) { +// TestAdapterConditionStatus_Constants verifies that AdapterConditionStatus constants match OpenAPI equivalents +func TestAdapterConditionStatus_Constants(t *testing.T) { RegisterTestingT(t) - // Test converting string to ResourcePhase - phase := ResourcePhase("NotReady") - Expect(phase).To(Equal(PhaseNotReady)) + // Verify constant values match expected strings + Expect(string(AdapterConditionTrue)).To(Equal("True")) + Expect(string(AdapterConditionFalse)).To(Equal("False")) + Expect(string(AdapterConditionUnknown)).To(Equal("Unknown")) - // Test converting ResourcePhase to string - str := string(PhaseReady) - Expect(str).To(Equal("Ready")) + // These values should match openapi.AdapterConditionStatus + // which has "True", "False", and "Unknown" values } -// TestConditionStatus_Constants verifies that ConditionStatus constants match OpenAPI equivalents -func TestConditionStatus_Constants(t *testing.T) { +// TestAdapterConditionStatus_StringConversion tests type casting to/from string +func TestAdapterConditionStatus_StringConversion(t *testing.T) { RegisterTestingT(t) - // Verify constant values match expected strings - Expect(string(ConditionTrue)).To(Equal("True")) - Expect(string(ConditionFalse)).To(Equal("False")) - Expect(string(ConditionUnknown)).To(Equal("Unknown")) + // Test converting string to AdapterConditionStatus + status := AdapterConditionStatus("Unknown") + Expect(status).To(Equal(AdapterConditionUnknown)) - // These values should match openapi.TRUE, openapi.FALSE, openapi.UNKNOWN - // which are "True", "False", "Unknown" respectively + // Test converting AdapterConditionStatus to string + str := string(AdapterConditionFalse) + Expect(str).To(Equal("False")) } // TestConditionStatus_StringConversion tests type casting to/from string @@ -52,7 +51,7 @@ func TestConditionStatus_StringConversion(t *testing.T) { RegisterTestingT(t) // Test converting string to ConditionStatus - status := ConditionStatus("True") + status := ResourceConditionStatus("True") Expect(status).To(Equal(ConditionTrue)) // Test converting ConditionStatus to string @@ -154,7 +153,7 @@ func TestResourceCondition_JSONDeserialization(t *testing.T) { "created_time": "2023-01-01T00:00:00Z", "last_updated_time": "2023-01-01T01:00:00Z", "type": "NotReady", - "status": "Unknown", + "status": "False", "last_transition_time": "2023-01-01T02:00:00Z" }` @@ -164,7 +163,7 @@ func TestResourceCondition_JSONDeserialization(t *testing.T) { Expect(minimalCondition.ObservedGeneration).To(Equal(int32(2))) Expect(minimalCondition.Type).To(Equal("NotReady")) - Expect(minimalCondition.Status).To(Equal(ConditionUnknown)) + Expect(minimalCondition.Status).To(Equal(ConditionFalse)) Expect(minimalCondition.Reason).To(BeNil()) Expect(minimalCondition.Message).To(BeNil()) } @@ -221,7 +220,7 @@ func TestAdapterCondition_JSONSerialization(t *testing.T) { // Test with all fields fullCondition := AdapterCondition{ Type: "Connected", - Status: ConditionTrue, + Status: AdapterConditionTrue, Reason: &reason, Message: &message, LastTransitionTime: now, @@ -242,7 +241,7 @@ func TestAdapterCondition_JSONSerialization(t *testing.T) { // Test without optional fields minimalCondition := AdapterCondition{ Type: "Disconnected", - Status: ConditionFalse, + Status: AdapterConditionFalse, Reason: nil, Message: nil, LastTransitionTime: now, @@ -259,6 +258,22 @@ func TestAdapterCondition_JSONSerialization(t *testing.T) { _, hasMessage := minimalMap["message"] Expect(hasReason).To(BeFalse()) Expect(hasMessage).To(BeFalse()) + + // Test with Unknown status + unknownCondition := AdapterCondition{ + Type: "Unknown", + Status: AdapterConditionUnknown, + LastTransitionTime: now, + } + + jsonBytes, err = json.Marshal(unknownCondition) + Expect(err).To(BeNil()) + + var unknownMap map[string]interface{} + err = json.Unmarshal(jsonBytes, &unknownMap) + Expect(err).To(BeNil()) + + Expect(unknownMap["status"]).To(Equal("Unknown")) } // TestAdapterCondition_JSONDeserialization tests unmarshaling JSON to AdapterCondition @@ -279,7 +294,7 @@ func TestAdapterCondition_JSONDeserialization(t *testing.T) { Expect(err).To(BeNil()) Expect(condition.Type).To(Equal("Synced")) - Expect(condition.Status).To(Equal(ConditionTrue)) + Expect(condition.Status).To(Equal(AdapterConditionTrue)) Expect(condition.Reason).ToNot(BeNil()) Expect(*condition.Reason).To(Equal("SyncSuccessful")) Expect(condition.Message).ToNot(BeNil()) @@ -297,9 +312,24 @@ func TestAdapterCondition_JSONDeserialization(t *testing.T) { Expect(err).To(BeNil()) Expect(minimalCondition.Type).To(Equal("Error")) - Expect(minimalCondition.Status).To(Equal(ConditionFalse)) + Expect(minimalCondition.Status).To(Equal(AdapterConditionFalse)) Expect(minimalCondition.Reason).To(BeNil()) Expect(minimalCondition.Message).To(BeNil()) + + // Test JSON with Unknown status + unknownJSON := `{ + "type": "Available", + "status": "Unknown", + "reason": "StartupPending", + "last_transition_time": "2023-01-01T12:00:00Z" + }` + + var unknownCondition AdapterCondition + err = json.Unmarshal([]byte(unknownJSON), &unknownCondition) + Expect(err).To(BeNil()) + + Expect(unknownCondition.Type).To(Equal("Available")) + Expect(unknownCondition.Status).To(Equal(AdapterConditionUnknown)) } // TestAdapterCondition_RoundTrip tests Marshal → Unmarshal to ensure no data loss @@ -312,7 +342,7 @@ func TestAdapterCondition_RoundTrip(t *testing.T) { original := AdapterCondition{ Type: "Provisioned", - Status: ConditionTrue, + Status: AdapterConditionTrue, Reason: &reason, Message: &message, LastTransitionTime: now, From 779d2c03948de32b0ff2020cc4d8bd2f9b8d6344 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:05:27 +0100 Subject: [PATCH 02/13] HYPERFLEET-386 - feat: add ComputeReadyCondition and ComputeAvailableCondition Replaces ComputePhase() with condition-based status aggregation: - ComputeReadyCondition(): True when all required adapters report Available=True at current spec.generation - ComputeAvailableCondition(): True when all required adapters report Available=True at any generation (last known good) Key behavior: After spec change, Ready becomes False immediately while Available stays True until adapters report failure. --- pkg/services/status_aggregation.go | 199 ++++++++++++++++++++++++----- 1 file changed, 166 insertions(+), 33 deletions(-) diff --git a/pkg/services/status_aggregation.go b/pkg/services/status_aggregation.go index cf8ccfc..c2fc659 100644 --- a/pkg/services/status_aggregation.go +++ b/pkg/services/status_aggregation.go @@ -1,13 +1,13 @@ package services import ( - "context" "encoding/json" + "math" "strings" + "time" "unicode" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) // Required adapter lists configured via pkg/config/adapter.go (see AdapterRequirementsConfig) @@ -58,21 +58,13 @@ func MapAdapterToConditionType(adapterName string) string { return result.String() } -// ComputePhase calculates the overall phase for a resource based on adapter statuses -// -// MVP Phase Logic (per architecture/hyperfleet/docs/status-guide.md): -// - "Ready": All required adapters have Available=True for current generation -// - "NotReady": Otherwise (any adapter has Available=False or hasn't reported yet) -// -// An adapter is considered "available" when: -// 1. Available condition status == "True" -// 2. observed_generation == resource.generation (only checked if resourceGeneration > 0) -// -// Note: Post-MVP will add more phases (Pending, Provisioning, Failed, Degraded) -// based on Health condition and Applied condition states. -func ComputePhase(ctx context.Context, adapterStatuses api.AdapterStatusList, requiredAdapters []string, resourceGeneration int32) string { - if len(adapterStatuses) == 0 { - return "NotReady" +// ComputeAvailableCondition checks if all required adapters have Available=True at ANY generation. +// Returns: (isAvailable bool, minObservedGeneration int32) +// "Available" means the system is running at some known good configuration (last known good config). +// The minObservedGeneration is the lowest generation across all required adapters. +func ComputeAvailableCondition(adapterStatuses api.AdapterStatusList, requiredAdapters []string) (bool, int32) { + if len(adapterStatuses) == 0 || len(requiredAdapters) == 0 { + return false, 1 } // Build a map of adapter name -> (available status, observed generation) @@ -105,40 +97,181 @@ func ComputePhase(ctx context.Context, adapterStatuses api.AdapterStatusList, re } } - // Count available adapters + // Count available adapters and track min observed generation numAvailable := 0 + minObservedGeneration := int32(math.MaxInt32) + + for _, adapterName := range requiredAdapters { + adapterInfo, exists := adapterMap[adapterName] + + if !exists { + // Required adapter not found - not available + continue + } + + // For Available condition, we don't check generation matching + // We just need Available=True at ANY generation + if adapterInfo.available == "True" { + numAvailable++ + if adapterInfo.observedGeneration < minObservedGeneration { + minObservedGeneration = adapterInfo.observedGeneration + } + } + } + + // Available when all required adapters have Available=True (at any generation) + numRequired := len(requiredAdapters) + if numAvailable == numRequired { + return true, minObservedGeneration + } + + // Return 0 for minObservedGeneration when not available + return false, 0 +} + +// ComputeReadyCondition checks if all required adapters have Available=True at the CURRENT generation. +// "Ready" means the system is running at the latest spec generation. +func ComputeReadyCondition(adapterStatuses api.AdapterStatusList, requiredAdapters []string, resourceGeneration int32) bool { + if len(adapterStatuses) == 0 || len(requiredAdapters) == 0 { + return false + } + + // Build a map of adapter name -> (available status, observed generation) + adapterMap := make(map[string]struct { + available string + observedGeneration int32 + }) + + for _, adapterStatus := range adapterStatuses { + // Unmarshal conditions to find "Available" + var conditions []struct { + Type string `json:"type"` + Status string `json:"status"` + } + if len(adapterStatus.Conditions) > 0 { + if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err == nil { + for _, cond := range conditions { + if cond.Type == "Available" { + adapterMap[adapterStatus.Adapter] = struct { + available string + observedGeneration int32 + }{ + available: cond.Status, + observedGeneration: adapterStatus.ObservedGeneration, + } + break + } + } + } + } + } + + // Count ready adapters (Available=True at current generation) + numReady := 0 - // Iterate over required adapters for _, adapterName := range requiredAdapters { adapterInfo, exists := adapterMap[adapterName] if !exists { - // Required adapter not found - logger.With(ctx, logger.FieldAdapter, adapterName).Warn("Required adapter not found in adapter statuses") + // Required adapter not found - not ready continue } - // Check generation matching only if resourceGeneration > 0 + // For Ready condition, we require generation matching if resourceGeneration > 0 && adapterInfo.observedGeneration != resourceGeneration { - // Adapter is processing old generation (stale) - logger.With(ctx, - logger.FieldAdapter, adapterName, - "observed_generation", adapterInfo.observedGeneration, - "expected_generation", resourceGeneration).Warn("Required adapter has stale generation") + // Adapter is processing old generation (stale) - not ready continue } // Check available status if adapterInfo.available == "True" { - numAvailable++ + numReady++ } } - // MVP: Only Ready or NotReady - // Ready when all required adapters have Available=True for current generation + // Ready when all required adapters have Available=True at current generation numRequired := len(requiredAdapters) - if numAvailable == numRequired && numRequired > 0 { - return "Ready" + return numReady == numRequired +} + +func BuildSyntheticConditions(existingConditionsJSON []byte, adapterStatuses api.AdapterStatusList, requiredAdapters []string, resourceGeneration int32, now time.Time) (api.ResourceCondition, api.ResourceCondition) { + var existingAvailable *api.ResourceCondition + var existingReady *api.ResourceCondition + + if len(existingConditionsJSON) > 0 { + var existingConditions []api.ResourceCondition + if err := json.Unmarshal(existingConditionsJSON, &existingConditions); err == nil { + for i := range existingConditions { + switch existingConditions[i].Type { + case "Available": + existingAvailable = &existingConditions[i] + case "Ready": + existingReady = &existingConditions[i] + } + } + } + } + + isAvailable, minObservedGeneration := ComputeAvailableCondition(adapterStatuses, requiredAdapters) + availableStatus := api.ConditionFalse + if isAvailable { + availableStatus = api.ConditionTrue + } + availableCondition := api.ResourceCondition{ + Type: "Available", + Status: availableStatus, + ObservedGeneration: minObservedGeneration, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + } + preserveSyntheticCondition(&availableCondition, existingAvailable, now) + + isReady := ComputeReadyCondition(adapterStatuses, requiredAdapters, resourceGeneration) + readyStatus := api.ConditionFalse + if isReady { + readyStatus = api.ConditionTrue + } + readyCondition := api.ResourceCondition{ + Type: "Ready", + Status: readyStatus, + ObservedGeneration: resourceGeneration, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + } + preserveSyntheticCondition(&readyCondition, existingReady, now) + + return availableCondition, readyCondition +} + +func preserveSyntheticCondition(target *api.ResourceCondition, existing *api.ResourceCondition, now time.Time) { + if existing == nil { + return + } + + if existing.Status == target.Status && existing.ObservedGeneration == target.ObservedGeneration { + if !existing.CreatedTime.IsZero() { + target.CreatedTime = existing.CreatedTime + } + if !existing.LastTransitionTime.IsZero() { + target.LastTransitionTime = existing.LastTransitionTime + } + if !existing.LastUpdatedTime.IsZero() { + target.LastUpdatedTime = existing.LastUpdatedTime + } + if target.Reason == nil && existing.Reason != nil { + target.Reason = existing.Reason + } + if target.Message == nil && existing.Message != nil { + target.Message = existing.Message + } + return + } + + if !existing.CreatedTime.IsZero() { + target.CreatedTime = existing.CreatedTime } - return "NotReady" + target.LastTransitionTime = now + target.LastUpdatedTime = now } From 76b4dad0d045984ba0bef54fa50f081b48f43a95 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:05:40 +0100 Subject: [PATCH 03/13] HYPERFLEET-386 - feat: update presenters for conditions-based status Updates API response formatting: - New resources get default conditions: Available=False, Ready=False with reason AwaitingAdapters - Converts domain condition types to OpenAPI types for responses - Adapter status now uses AdapterConditionStatus type --- pkg/api/presenters/adapter_status.go | 4 +- pkg/api/presenters/adapter_status_test.go | 28 +++---- pkg/api/presenters/cluster.go | 49 +++--------- pkg/api/presenters/cluster_test.go | 92 ++++------------------- pkg/api/presenters/node_pool.go | 51 +++---------- pkg/api/presenters/node_pool_test.go | 75 +++++------------- 6 files changed, 68 insertions(+), 231 deletions(-) diff --git a/pkg/api/presenters/adapter_status.go b/pkg/api/presenters/adapter_status.go index 1fcce2e..acda0dc 100644 --- a/pkg/api/presenters/adapter_status.go +++ b/pkg/api/presenters/adapter_status.go @@ -27,7 +27,7 @@ func ConvertAdapterStatus( for i, condReq := range req.Conditions { adapterConditions[i] = api.AdapterCondition{ Type: condReq.Type, - Status: api.ConditionStatus(string(condReq.Status)), + Status: api.AdapterConditionStatus(condReq.Status), Reason: condReq.Reason, Message: condReq.Message, LastTransitionTime: now, @@ -87,7 +87,7 @@ func PresentAdapterStatus(adapterStatus *api.AdapterStatus) (openapi.AdapterStat for i, cond := range conditions { openapiConditions[i] = openapi.AdapterCondition{ Type: cond.Type, - Status: openapi.ConditionStatus(cond.Status), + Status: openapi.AdapterConditionStatus(cond.Status), Reason: cond.Reason, Message: cond.Message, LastTransitionTime: cond.LastTransitionTime, diff --git a/pkg/api/presenters/adapter_status_test.go b/pkg/api/presenters/adapter_status_test.go index adfeb32..27ca0a6 100644 --- a/pkg/api/presenters/adapter_status_test.go +++ b/pkg/api/presenters/adapter_status_test.go @@ -35,7 +35,7 @@ func createTestAdapterStatusRequest() *openapi.AdapterStatusCreateRequest { Conditions: []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, Reason: &reason, Message: &message, }, @@ -71,7 +71,7 @@ func TestConvertAdapterStatus_Complete(t *testing.T) { Expect(err).To(BeNil()) Expect(len(conditions)).To(Equal(1)) Expect(conditions[0].Type).To(Equal("Ready")) - Expect(conditions[0].Status).To(Equal(api.ConditionTrue)) + Expect(conditions[0].Status).To(Equal(api.AdapterConditionTrue)) Expect(*conditions[0].Reason).To(Equal("TestReason")) Expect(*conditions[0].Message).To(Equal("Test message")) @@ -203,12 +203,12 @@ func TestConvertAdapterStatus_ConditionStatusConversion(t *testing.T) { RegisterTestingT(t) testCases := []struct { - openapiStatus openapi.ConditionStatus - expectedDomain api.ConditionStatus + openapiStatus openapi.AdapterConditionStatus + expectedDomain api.AdapterConditionStatus }{ - {openapi.True, api.ConditionTrue}, - {openapi.False, api.ConditionFalse}, - {openapi.Unknown, api.ConditionUnknown}, + {openapi.AdapterConditionStatusTrue, api.AdapterConditionTrue}, + {openapi.AdapterConditionStatusFalse, api.AdapterConditionFalse}, + {openapi.AdapterConditionStatusUnknown, api.AdapterConditionUnknown}, } for _, tc := range testCases { @@ -245,7 +245,7 @@ func TestPresentAdapterStatus_Complete(t *testing.T) { conditions := []api.AdapterCondition{ { Type: "Ready", - Status: api.ConditionTrue, + Status: api.AdapterConditionTrue, Reason: &reason, Message: &message, LastTransitionTime: now, @@ -292,7 +292,7 @@ func TestPresentAdapterStatus_Complete(t *testing.T) { // Verify conditions converted correctly Expect(len(result.Conditions)).To(Equal(1)) Expect(result.Conditions[0].Type).To(Equal("Ready")) - Expect(result.Conditions[0].Status).To(Equal(openapi.True)) + Expect(result.Conditions[0].Status).To(Equal(openapi.AdapterConditionStatusTrue)) Expect(*result.Conditions[0].Reason).To(Equal("Success")) // Verify data unmarshaled correctly @@ -384,12 +384,12 @@ func TestPresentAdapterStatus_ConditionStatusConversion(t *testing.T) { RegisterTestingT(t) testCases := []struct { - domainStatus api.ConditionStatus - expectedOpenAPI openapi.ConditionStatus + domainStatus api.AdapterConditionStatus + expectedOpenAPI openapi.AdapterConditionStatus }{ - {api.ConditionTrue, openapi.True}, - {api.ConditionFalse, openapi.False}, - {api.ConditionUnknown, openapi.Unknown}, + {api.AdapterConditionTrue, openapi.AdapterConditionStatusTrue}, + {api.AdapterConditionFalse, openapi.AdapterConditionStatusFalse}, + {api.AdapterConditionUnknown, openapi.AdapterConditionStatusUnknown}, } now := time.Now() diff --git a/pkg/api/presenters/cluster.go b/pkg/api/presenters/cluster.go index b8c0b36..3aa2393 100644 --- a/pkg/api/presenters/cluster.go +++ b/pkg/api/presenters/cluster.go @@ -3,7 +3,6 @@ package presenters import ( "encoding/json" "fmt" - "time" openapi_types "github.com/oapi-codegen/runtime/types" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -29,12 +28,6 @@ func ConvertCluster(req *openapi.ClusterCreateRequest, createdBy string) (*api.C return nil, fmt.Errorf("failed to marshal cluster labels: %w", err) } - // Marshal empty StatusConditions - statusConditionsJSON, err := json.Marshal([]api.ResourceCondition{}) - if err != nil { - return nil, fmt.Errorf("failed to marshal cluster status conditions: %w", err) - } - // Get Kind value, use default if not provided kind := "Cluster" if req.Kind != nil { @@ -42,16 +35,13 @@ func ConvertCluster(req *openapi.ClusterCreateRequest, createdBy string) (*api.C } return &api.Cluster{ - Kind: kind, - Name: req.Name, - Spec: specJSON, - Labels: labelsJSON, - Generation: 1, - StatusPhase: "NotReady", - StatusObservedGeneration: 0, - StatusConditions: statusConditionsJSON, - CreatedBy: createdBy, - UpdatedBy: createdBy, + Kind: kind, + Name: req.Name, + Spec: specJSON, + Labels: labelsJSON, + Generation: 1, + CreatedBy: createdBy, + UpdatedBy: createdBy, }, nil } @@ -92,23 +82,6 @@ func PresentCluster(cluster *api.Cluster) (openapi.Cluster, error) { href = "/api/hyperfleet/v1/clusters/" + cluster.ID } - // Build ClusterStatus - set required fields with defaults if nil - lastTransitionTime := time.Time{} - if cluster.StatusLastTransitionTime != nil { - lastTransitionTime = *cluster.StatusLastTransitionTime - } - - lastUpdatedTime := time.Time{} - if cluster.StatusLastUpdatedTime != nil { - lastUpdatedTime = *cluster.StatusLastUpdatedTime - } - - // Set phase, use NOT_READY as default if not set - phase := api.PhaseNotReady - if cluster.StatusPhase != "" { - phase = api.ResourcePhase(cluster.StatusPhase) - } - // Convert domain ResourceConditions to openapi format openapiConditions := make([]openapi.ResourceCondition, len(statusConditions)) for i, cond := range statusConditions { @@ -119,7 +92,7 @@ func PresentCluster(cluster *api.Cluster) (openapi.Cluster, error) { Message: cond.Message, ObservedGeneration: cond.ObservedGeneration, Reason: cond.Reason, - Status: openapi.ConditionStatus(cond.Status), + Status: openapi.ResourceConditionStatus(cond.Status), Type: cond.Type, } } @@ -135,11 +108,7 @@ func PresentCluster(cluster *api.Cluster) (openapi.Cluster, error) { Name: cluster.Name, Spec: spec, Status: openapi.ClusterStatus{ - Conditions: openapiConditions, - LastTransitionTime: lastTransitionTime, - LastUpdatedTime: lastUpdatedTime, - ObservedGeneration: cluster.StatusObservedGeneration, - Phase: openapi.ResourcePhase(phase), + Conditions: openapiConditions, }, UpdatedBy: toEmail(cluster.UpdatedBy), UpdatedTime: cluster.UpdatedTime, diff --git a/pkg/api/presenters/cluster_test.go b/pkg/api/presenters/cluster_test.go index 6c8ebc4..d0a89d3 100644 --- a/pkg/api/presenters/cluster_test.go +++ b/pkg/api/presenters/cluster_test.go @@ -45,8 +45,6 @@ func TestConvertCluster_Complete(t *testing.T) { // Verify defaults Expect(result.Generation).To(Equal(int32(1))) - Expect(result.StatusPhase).To(Equal("NotReady")) - Expect(result.StatusObservedGeneration).To(Equal(int32(0))) // Verify Spec marshaled correctly var spec map[string]interface{} @@ -61,11 +59,8 @@ func TestConvertCluster_Complete(t *testing.T) { Expect(err).To(BeNil()) Expect(labels["env"]).To(Equal("test")) - // Verify StatusConditions initialized as empty array - var conditions []api.ResourceCondition - err = json.Unmarshal(result.StatusConditions, &conditions) - Expect(err).To(BeNil()) - Expect(len(conditions)).To(Equal(0)) + // StatusConditions initialization is handled by the service layer on create, not presenters. + Expect(len(result.StatusConditions)).To(Equal(0)) } // TestConvertCluster_WithLabels tests conversion with labels @@ -187,19 +182,15 @@ func TestPresentCluster_Complete(t *testing.T) { labelsJSON, _ := json.Marshal(labels) cluster := &api.Cluster{ - Kind: "Cluster", - Href: "/api/hyperfleet/v1/clusters/cluster-abc123", - Name: "presented-cluster", - Spec: specJSON, - Labels: labelsJSON, - Generation: 10, - StatusPhase: "Ready", - StatusObservedGeneration: 5, - StatusConditions: conditionsJSON, - StatusLastTransitionTime: &now, - StatusLastUpdatedTime: &now, - CreatedBy: "user123@example.com", - UpdatedBy: "user456@example.com", + Kind: "Cluster", + Href: "/api/hyperfleet/v1/clusters/cluster-abc123", + Name: "presented-cluster", + Spec: specJSON, + Labels: labelsJSON, + Generation: 10, + StatusConditions: conditionsJSON, + CreatedBy: "user123@example.com", + UpdatedBy: "user456@example.com", } cluster.ID = "cluster-abc123" cluster.CreatedTime = now @@ -224,18 +215,14 @@ func TestPresentCluster_Complete(t *testing.T) { Expect((*result.Labels)["env"]).To(Equal("staging")) // Verify Status - Expect(result.Status.Phase).To(Equal(openapi.Ready)) - Expect(result.Status.ObservedGeneration).To(Equal(int32(5))) Expect(len(result.Status.Conditions)).To(Equal(1)) Expect(result.Status.Conditions[0].Type).To(Equal("Available")) - Expect(result.Status.Conditions[0].Status).To(Equal(openapi.True)) + Expect(result.Status.Conditions[0].Status).To(Equal(openapi.ResourceConditionStatusTrue)) Expect(*result.Status.Conditions[0].Reason).To(Equal("Ready")) // Verify timestamps Expect(result.CreatedTime.Unix()).To(Equal(now.Unix())) Expect(result.UpdatedTime.Unix()).To(Equal(now.Unix())) - Expect(result.Status.LastTransitionTime.Unix()).To(Equal(now.Unix())) - Expect(result.Status.LastUpdatedTime.Unix()).To(Equal(now.Unix())) } // TestPresentCluster_HrefGeneration tests that Href is generated if not set @@ -258,53 +245,6 @@ func TestPresentCluster_HrefGeneration(t *testing.T) { Expect(*result.Href).To(Equal("/api/hyperfleet/v1/clusters/cluster-xyz789")) } -// TestPresentCluster_EmptyStatusPhase tests handling of empty StatusPhase -func TestPresentCluster_EmptyStatusPhase(t *testing.T) { - RegisterTestingT(t) - - cluster := &api.Cluster{ - Kind: "Cluster", - Name: "empty-phase-test", - Spec: []byte("{}"), - Labels: []byte("{}"), - StatusPhase: "", // Empty status phase - StatusConditions: []byte("[]"), - } - cluster.ID = "cluster-empty-phase" - - result, err := PresentCluster(cluster) - Expect(err).To(BeNil()) - - // Should use NOT_READY as default - Expect(result.Status.Phase).To(Equal(openapi.NotReady)) -} - -// TestPresentCluster_NilStatusTimestamps tests handling of nil timestamps -func TestPresentCluster_NilStatusTimestamps(t *testing.T) { - RegisterTestingT(t) - - now := time.Now() - cluster := &api.Cluster{ - Kind: "Cluster", - Name: "nil-timestamps-test", - Spec: []byte("{}"), - Labels: []byte("{}"), - StatusConditions: []byte("[]"), - StatusLastTransitionTime: nil, // Nil timestamp - StatusLastUpdatedTime: nil, // Nil timestamp - } - cluster.ID = "cluster-nil-timestamps" - cluster.CreatedTime = now - cluster.UpdatedTime = now - - result, err := PresentCluster(cluster) - Expect(err).To(BeNil()) - - // Verify zero time.Time is returned (not nil) - Expect(result.Status.LastTransitionTime.IsZero()).To(BeTrue()) - Expect(result.Status.LastUpdatedTime.IsZero()).To(BeTrue()) -} - // TestPresentCluster_StatusConditionsConversion tests condition conversion func TestPresentCluster_StatusConditionsConversion(t *testing.T) { RegisterTestingT(t) @@ -359,13 +299,13 @@ func TestPresentCluster_StatusConditionsConversion(t *testing.T) { // First condition Expect(result.Status.Conditions[0].Type).To(Equal("Available")) - Expect(result.Status.Conditions[0].Status).To(Equal(openapi.True)) + Expect(result.Status.Conditions[0].Status).To(Equal(openapi.ResourceConditionStatusTrue)) Expect(*result.Status.Conditions[0].Reason).To(Equal("Ready")) Expect(*result.Status.Conditions[0].Message).To(Equal("All systems operational")) // Second condition Expect(result.Status.Conditions[1].Type).To(Equal("Progressing")) - Expect(result.Status.Conditions[1].Status).To(Equal(openapi.False)) + Expect(result.Status.Conditions[1].Status).To(Equal(openapi.ResourceConditionStatusFalse)) Expect(*result.Status.Conditions[1].Reason).To(Equal("Degraded")) Expect(*result.Status.Conditions[1].Message).To(Equal("Some components unavailable")) } @@ -405,9 +345,7 @@ func TestConvertAndPresentCluster_RoundTrip(t *testing.T) { // Verify Labels preserved Expect((*result.Labels)["env"]).To(Equal((*originalReq.Labels)["env"])) - // Verify Status defaults - Expect(result.Status.Phase).To(Equal(openapi.NotReady)) - Expect(result.Status.ObservedGeneration).To(Equal(int32(0))) + // Status initialization is handled by the service layer on create, not presenters. Expect(len(result.Status.Conditions)).To(Equal(0)) } diff --git a/pkg/api/presenters/node_pool.go b/pkg/api/presenters/node_pool.go index bebf9ce..5864bac 100644 --- a/pkg/api/presenters/node_pool.go +++ b/pkg/api/presenters/node_pool.go @@ -3,7 +3,6 @@ package presenters import ( "encoding/json" "fmt" - "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" @@ -27,29 +26,20 @@ func ConvertNodePool(req *openapi.NodePoolCreateRequest, ownerID, createdBy stri return nil, fmt.Errorf("failed to marshal nodepool labels: %w", err) } - // Marshal empty StatusConditions - statusConditionsJSON, err := json.Marshal([]api.ResourceCondition{}) - if err != nil { - return nil, fmt.Errorf("failed to marshal nodepool status conditions: %w", err) - } - kind := "NodePool" if req.Kind != nil { kind = *req.Kind } return &api.NodePool{ - Kind: kind, - Name: req.Name, - Spec: specJSON, - Labels: labelsJSON, - OwnerID: ownerID, - OwnerKind: "Cluster", - StatusPhase: "NotReady", - StatusObservedGeneration: 0, - StatusConditions: statusConditionsJSON, - CreatedBy: createdBy, - UpdatedBy: createdBy, + Kind: kind, + Name: req.Name, + Spec: specJSON, + Labels: labelsJSON, + OwnerID: ownerID, + OwnerKind: "Cluster", + CreatedBy: createdBy, + UpdatedBy: createdBy, }, nil } @@ -91,23 +81,6 @@ func PresentNodePool(nodePool *api.NodePool) (openapi.NodePool, error) { ownerHref = "/api/hyperfleet/v1/clusters/" + nodePool.OwnerID } - // Build NodePoolStatus - set required fields with defaults if nil - lastTransitionTime := time.Time{} - if nodePool.StatusLastTransitionTime != nil { - lastTransitionTime = *nodePool.StatusLastTransitionTime - } - - lastUpdatedTime := time.Time{} - if nodePool.StatusLastUpdatedTime != nil { - lastUpdatedTime = *nodePool.StatusLastUpdatedTime - } - - // Set phase, use NOT_READY as default if not set - phase := api.PhaseNotReady - if nodePool.StatusPhase != "" { - phase = api.ResourcePhase(nodePool.StatusPhase) - } - // Convert domain ResourceConditions to openapi format openapiConditions := make([]openapi.ResourceCondition, len(statusConditions)) for i, cond := range statusConditions { @@ -118,7 +91,7 @@ func PresentNodePool(nodePool *api.NodePool) (openapi.NodePool, error) { Message: cond.Message, ObservedGeneration: cond.ObservedGeneration, Reason: cond.Reason, - Status: openapi.ConditionStatus(cond.Status), + Status: openapi.ResourceConditionStatus(cond.Status), Type: cond.Type, } } @@ -140,11 +113,7 @@ func PresentNodePool(nodePool *api.NodePool) (openapi.NodePool, error) { }, Spec: spec, Status: openapi.NodePoolStatus{ - Conditions: openapiConditions, - LastTransitionTime: lastTransitionTime, - LastUpdatedTime: lastUpdatedTime, - ObservedGeneration: nodePool.StatusObservedGeneration, - Phase: openapi.ResourcePhase(phase), + Conditions: openapiConditions, }, UpdatedBy: toEmail(nodePool.UpdatedBy), UpdatedTime: nodePool.UpdatedTime, diff --git a/pkg/api/presenters/node_pool_test.go b/pkg/api/presenters/node_pool_test.go index f4b4eea..c6a4b1b 100644 --- a/pkg/api/presenters/node_pool_test.go +++ b/pkg/api/presenters/node_pool_test.go @@ -5,8 +5,8 @@ import ( "testing" "time" - . "github.com/onsi/gomega" openapi_types "github.com/oapi-codegen/runtime/types" + . "github.com/onsi/gomega" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" ) @@ -46,10 +46,6 @@ func TestConvertNodePool_Complete(t *testing.T) { Expect(result.CreatedBy).To(Equal("user456")) Expect(result.UpdatedBy).To(Equal("user456")) - // Verify defaults - Expect(result.StatusPhase).To(Equal("NotReady")) - Expect(result.StatusObservedGeneration).To(Equal(int32(0))) - // Verify Spec marshaled correctly var spec map[string]interface{} err = json.Unmarshal(result.Spec, &spec) @@ -63,11 +59,8 @@ func TestConvertNodePool_Complete(t *testing.T) { Expect(err).To(BeNil()) Expect(labels["env"]).To(Equal("test")) - // Verify StatusConditions initialized as empty array - var conditions []api.ResourceCondition - err = json.Unmarshal(result.StatusConditions, &conditions) - Expect(err).To(BeNil()) - Expect(len(conditions)).To(Equal(0)) + // StatusConditions initialization is handled by the service layer on create, not presenters. + Expect(len(result.StatusConditions)).To(Equal(0)) } // TestConvertNodePool_WithKind tests conversion with Kind specified @@ -181,21 +174,17 @@ func TestPresentNodePool_Complete(t *testing.T) { labelsJSON, _ := json.Marshal(labels) nodePool := &api.NodePool{ - Kind: "NodePool", - Href: "/api/hyperfleet/v1/clusters/cluster-abc/nodepools/nodepool-xyz", - Name: "presented-nodepool", - Spec: specJSON, - Labels: labelsJSON, - OwnerID: "cluster-abc", - OwnerKind: "Cluster", - OwnerHref: "/api/hyperfleet/v1/clusters/cluster-abc", - StatusPhase: "Ready", - StatusObservedGeneration: 5, - StatusConditions: conditionsJSON, - StatusLastTransitionTime: &now, - StatusLastUpdatedTime: &now, - CreatedBy: "user123@example.com", - UpdatedBy: "user456@example.com", + Kind: "NodePool", + Href: "/api/hyperfleet/v1/clusters/cluster-abc/nodepools/nodepool-xyz", + Name: "presented-nodepool", + Spec: specJSON, + Labels: labelsJSON, + OwnerID: "cluster-abc", + OwnerKind: "Cluster", + OwnerHref: "/api/hyperfleet/v1/clusters/cluster-abc", + StatusConditions: conditionsJSON, + CreatedBy: "user123@example.com", + UpdatedBy: "user456@example.com", } nodePool.ID = "nodepool-xyz" nodePool.CreatedTime = now @@ -224,17 +213,13 @@ func TestPresentNodePool_Complete(t *testing.T) { Expect(*result.OwnerReferences.Href).To(Equal("/api/hyperfleet/v1/clusters/cluster-abc")) // Verify Status - Expect(result.Status.Phase).To(Equal(openapi.Ready)) - Expect(result.Status.ObservedGeneration).To(Equal(int32(5))) Expect(len(result.Status.Conditions)).To(Equal(1)) Expect(result.Status.Conditions[0].Type).To(Equal("Available")) - Expect(result.Status.Conditions[0].Status).To(Equal(openapi.True)) + Expect(result.Status.Conditions[0].Status).To(Equal(openapi.ResourceConditionStatusTrue)) // Verify timestamps Expect(result.CreatedTime.Unix()).To(Equal(now.Unix())) Expect(result.UpdatedTime.Unix()).To(Equal(now.Unix())) - Expect(result.Status.LastTransitionTime.Unix()).To(Equal(now.Unix())) - Expect(result.Status.LastUpdatedTime.Unix()).To(Equal(now.Unix())) } // TestPresentNodePool_HrefGeneration tests that Href is generated if not set @@ -304,28 +289,6 @@ func TestPresentNodePool_OwnerReferences(t *testing.T) { Expect(result.OwnerReferences.Href).ToNot(BeNil()) } -// TestPresentNodePool_EmptyStatusPhase tests handling of empty StatusPhase -func TestPresentNodePool_EmptyStatusPhase(t *testing.T) { - RegisterTestingT(t) - - nodePool := &api.NodePool{ - Kind: "NodePool", - Name: "empty-phase-test", - Spec: []byte("{}"), - Labels: []byte("{}"), - OwnerID: "cluster-phase-test", - StatusPhase: "", // Empty status phase - StatusConditions: []byte("[]"), - } - nodePool.ID = "nodepool-empty-phase" - - result, err := PresentNodePool(nodePool) - Expect(err).To(BeNil()) - - // Should use NOT_READY as default - Expect(result.Status.Phase).To(Equal(openapi.NotReady)) -} - // TestPresentNodePool_StatusConditionsConversion tests condition conversion func TestPresentNodePool_StatusConditionsConversion(t *testing.T) { RegisterTestingT(t) @@ -381,13 +344,13 @@ func TestPresentNodePool_StatusConditionsConversion(t *testing.T) { // First condition Expect(result.Status.Conditions[0].Type).To(Equal("Progressing")) - Expect(result.Status.Conditions[0].Status).To(Equal(openapi.True)) + Expect(result.Status.Conditions[0].Status).To(Equal(openapi.ResourceConditionStatusTrue)) Expect(*result.Status.Conditions[0].Reason).To(Equal("Scaling")) Expect(*result.Status.Conditions[0].Message).To(Equal("Scaling in progress")) // Second condition Expect(result.Status.Conditions[1].Type).To(Equal("Healthy")) - Expect(result.Status.Conditions[1].Status).To(Equal(openapi.True)) + Expect(result.Status.Conditions[1].Status).To(Equal(openapi.ResourceConditionStatusTrue)) Expect(*result.Status.Conditions[1].Reason).To(Equal("Healthy")) Expect(*result.Status.Conditions[1].Message).To(Equal("All nodes healthy")) } @@ -432,9 +395,7 @@ func TestConvertAndPresentNodePool_RoundTrip(t *testing.T) { Expect(*result.OwnerReferences.Id).To(Equal(ownerID)) Expect(*result.OwnerReferences.Kind).To(Equal("Cluster")) - // Verify Status defaults - Expect(result.Status.Phase).To(Equal(openapi.NotReady)) - Expect(result.Status.ObservedGeneration).To(Equal(int32(0))) + // Status initialization is handled by the service layer on create, not presenters. Expect(len(result.Status.Conditions)).To(Equal(0)) } From 33c40aeeb58513a49a1b1fc7cf72e3e1b414a7f8 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:05:51 +0100 Subject: [PATCH 04/13] HYPERFLEET-386 - feat: implement UpdateConditionsFromAdapterStatus Service layer changes for condition-based status: - UpdateConditionsFromAdapterStatus() updates Ready/Available conditions when adapter status is POSTed - Returns nil, nil for no-op when conditions don't change - Removes phase-related code from generic service - Adds comprehensive tests for condition update logic --- pkg/services/cluster.go | 146 ++++--- pkg/services/cluster_test.go | 721 +++++++++++++++++++++++++++++++++ pkg/services/generic.go | 29 +- pkg/services/generic_test.go | 12 +- pkg/services/node_pool.go | 145 ++++--- pkg/services/node_pool_test.go | 518 +++++++++++++++++++++++ 6 files changed, 1455 insertions(+), 116 deletions(-) create mode 100644 pkg/services/cluster_test.go create mode 100644 pkg/services/node_pool_test.go diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index 248512d..d969379 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -3,7 +3,8 @@ package services import ( "context" "encoding/json" - "math" + stderrors "errors" + "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -11,6 +12,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "gorm.io/gorm" ) //go:generate mockgen-v0.6.0 -source=cluster.go -package=services -destination=cluster_mock.go @@ -27,6 +29,11 @@ type ClusterService interface { // Status aggregation UpdateClusterStatusFromAdapters(ctx context.Context, clusterID string) (*api.Cluster, *errors.ServiceError) + // ProcessAdapterStatus handles the business logic for adapter status: + // - If Available condition is "Unknown": returns (nil, nil) indicating no-op + // - Otherwise: upserts the status and triggers aggregation + ProcessAdapterStatus(ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) + // idempotent functions for the control plane, but can also be called synchronously by any actor OnUpsert(ctx context.Context, id string) error OnDelete(ctx context.Context, id string) error @@ -57,13 +64,22 @@ func (s *sqlClusterService) Get(ctx context.Context, id string) (*api.Cluster, * } func (s *sqlClusterService) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) { + if cluster.Generation == 0 { + cluster.Generation = 1 + } + cluster, err := s.clusterDao.Create(ctx, cluster) if err != nil { return nil, handleCreateError("Cluster", err) } + updatedCluster, svcErr := s.UpdateClusterStatusFromAdapters(ctx, cluster.ID) + if svcErr != nil { + return nil, svcErr + } + // REMOVED: Event creation - no event-driven components - return cluster, nil + return updatedCluster, nil } func (s *sqlClusterService) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, *errors.ServiceError) { @@ -133,9 +149,10 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters(ctx context.Context, return nil, errors.GeneralError("Failed to get adapter statuses: %s", err) } - // Build the list of ResourceCondition - adapters := []api.ResourceCondition{} - minObservedGeneration := int32(math.MaxInt32) + now := time.Now() + + // Build the list of adapter ResourceConditions + adapterConditions := []api.ResourceCondition{} for _, adapterStatus := range adapterStatuses { // Unmarshal Conditions from JSONB @@ -161,7 +178,7 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters(ctx context.Context, // Convert to ResourceCondition condResource := api.ResourceCondition{ Type: MapAdapterToConditionType(adapterStatus.Adapter), - Status: availableCondition.Status, + Status: api.ResourceConditionStatus(availableCondition.Status), Reason: availableCondition.Reason, Message: availableCondition.Message, ObservedGeneration: adapterStatus.ObservedGeneration, @@ -178,62 +195,30 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters(ctx context.Context, condResource.LastUpdatedTime = *adapterStatus.LastReportTime } - adapters = append(adapters, condResource) - - // Track min observed generation - // Use the LOWEST generation to ensure cluster status only advances when ALL adapters catch up - if adapterStatus.ObservedGeneration < minObservedGeneration { - minObservedGeneration = adapterStatus.ObservedGeneration - } + adapterConditions = append(adapterConditions, condResource) } - // Compute overall phase using required adapters from config - newPhase := ComputePhase(ctx, adapterStatuses, s.adapterConfig.RequiredClusterAdapters, cluster.Generation) + // Compute synthetic Available and Ready conditions + availableCondition, readyCondition := BuildSyntheticConditions( + cluster.StatusConditions, + adapterStatuses, + s.adapterConfig.RequiredClusterAdapters, + cluster.Generation, + now, + ) - // Calculate min(adapters[].last_report_time) for cluster.status.last_updated_time - // This uses the OLDEST adapter timestamp to ensure Sentinel can detect stale adapters - var minLastUpdatedTime *time.Time - for _, adapterStatus := range adapterStatuses { - if adapterStatus.LastReportTime != nil { - if minLastUpdatedTime == nil || adapterStatus.LastReportTime.Before(*minLastUpdatedTime) { - minLastUpdatedTime = adapterStatus.LastReportTime - } - } - } - - // Save old phase to detect transitions - oldPhase := cluster.StatusPhase - - // Update cluster status fields - now := time.Now() - cluster.StatusPhase = newPhase - // Set observed_generation to min across all adapters (0 if no adapters) - if len(adapterStatuses) == 0 { - cluster.StatusObservedGeneration = 0 - } else { - cluster.StatusObservedGeneration = minObservedGeneration - } + // Combine synthetic conditions with adapter conditions + // Put Available and Ready first + allConditions := []api.ResourceCondition{availableCondition, readyCondition} + allConditions = append(allConditions, adapterConditions...) // Marshal conditions to JSON - conditionsJSON, err := json.Marshal(adapters) + conditionsJSON, err := json.Marshal(allConditions) if err != nil { return nil, errors.GeneralError("Failed to marshal conditions: %s", err) } cluster.StatusConditions = conditionsJSON - // Use min(adapters[].last_report_time) instead of now() - // This ensures Sentinel triggers reconciliation when ANY adapter is stale - if minLastUpdatedTime != nil { - cluster.StatusLastUpdatedTime = minLastUpdatedTime - } else { - cluster.StatusLastUpdatedTime = &now - } - - // Update last transition time only if phase changed - if cluster.StatusLastTransitionTime == nil || oldPhase != newPhase { - cluster.StatusLastTransitionTime = &now - } - // Save the updated cluster cluster, err = s.clusterDao.Replace(ctx, cluster) if err != nil { @@ -242,3 +227,60 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters(ctx context.Context, return cluster, nil } + +// ProcessAdapterStatus handles the business logic for adapter status: +// - If Available condition is "Unknown": returns (nil, nil) indicating no-op +// - Otherwise: upserts the status and triggers aggregation +func (s *sqlClusterService) ProcessAdapterStatus(ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) { + existingStatus, findErr := s.adapterStatusDao.FindByResourceAndAdapter(ctx, "Cluster", clusterID, adapterStatus.Adapter) + if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { + if !strings.Contains(findErr.Error(), errors.CodeNotFoundGeneric) { + return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) + } + } + if existingStatus != nil && adapterStatus.ObservedGeneration < existingStatus.ObservedGeneration { + // Discard stale status updates (older observed_generation). + return nil, nil + } + + // Parse conditions from the adapter status + var conditions []api.AdapterCondition + if len(adapterStatus.Conditions) > 0 { + if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err != nil { + return nil, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", err) + } + } + + // Find the "Available" condition + hasAvailableCondition := false + for _, cond := range conditions { + if cond.Type != "Available" { + continue + } + + hasAvailableCondition = true + if cond.Status == api.AdapterConditionUnknown { + // Available condition is "Unknown", return nil to indicate no-op + return nil, nil + } + } + + // Upsert the adapter status + upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) + if err != nil { + return nil, handleCreateError("AdapterStatus", err) + } + + // Only trigger aggregation when the adapter reported an Available condition. + // If the adapter status doesn't include Available (e.g. it only reports Ready/Progressing), + // saving it should not overwrite the cluster's synthetic Available/Ready conditions. + if hasAvailableCondition { + if _, aggregateErr := s.UpdateClusterStatusFromAdapters(ctx, clusterID); aggregateErr != nil { + // Log error but don't fail the request - the status will be computed on next update + ctx = logger.WithClusterID(ctx, clusterID) + logger.WithError(ctx, aggregateErr).Warn("Failed to aggregate cluster status") + } + } + + return upsertedStatus, nil +} diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go new file mode 100644 index 0000000..0948754 --- /dev/null +++ b/pkg/services/cluster_test.go @@ -0,0 +1,721 @@ +package services + +import ( + "context" + "encoding/json" + "testing" + "time" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" +) + +// Mock implementations for testing ProcessAdapterStatus + +type mockClusterDao struct { + clusters map[string]*api.Cluster +} + +func newMockClusterDao() *mockClusterDao { + return &mockClusterDao{ + clusters: make(map[string]*api.Cluster), + } +} + +func (d *mockClusterDao) Get(ctx context.Context, id string) (*api.Cluster, error) { + if c, ok := d.clusters[id]; ok { + return c, nil + } + return nil, errors.NotFound("Cluster").AsError() +} + +func (d *mockClusterDao) Create(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { + d.clusters[cluster.ID] = cluster + return cluster, nil +} + +func (d *mockClusterDao) Replace(ctx context.Context, cluster *api.Cluster) (*api.Cluster, error) { + d.clusters[cluster.ID] = cluster + return cluster, nil +} + +func (d *mockClusterDao) Delete(ctx context.Context, id string) error { + delete(d.clusters, id) + return nil +} + +func (d *mockClusterDao) FindByIDs(ctx context.Context, ids []string) (api.ClusterList, error) { + var result api.ClusterList + for _, id := range ids { + if c, ok := d.clusters[id]; ok { + result = append(result, c) + } + } + return result, nil +} + +func (d *mockClusterDao) All(ctx context.Context) (api.ClusterList, error) { + var result api.ClusterList + for _, c := range d.clusters { + result = append(result, c) + } + return result, nil +} + +var _ dao.ClusterDao = &mockClusterDao{} + +type mockAdapterStatusDao struct { + statuses map[string]*api.AdapterStatus +} + +func newMockAdapterStatusDao() *mockAdapterStatusDao { + return &mockAdapterStatusDao{ + statuses: make(map[string]*api.AdapterStatus), + } +} + +func (d *mockAdapterStatusDao) Get(ctx context.Context, id string) (*api.AdapterStatus, error) { + if s, ok := d.statuses[id]; ok { + return s, nil + } + return nil, errors.NotFound("AdapterStatus").AsError() +} + +func (d *mockAdapterStatusDao) Create(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { + d.statuses[status.ID] = status + return status, nil +} + +func (d *mockAdapterStatusDao) Replace(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { + d.statuses[status.ID] = status + return status, nil +} + +func (d *mockAdapterStatusDao) Upsert(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { + key := status.ResourceType + ":" + status.ResourceID + ":" + status.Adapter + status.ID = key + d.statuses[key] = status + return status, nil +} + +func (d *mockAdapterStatusDao) Delete(ctx context.Context, id string) error { + delete(d.statuses, id) + return nil +} + +func (d *mockAdapterStatusDao) FindByResource(ctx context.Context, resourceType, resourceID string) (api.AdapterStatusList, error) { + var result api.AdapterStatusList + for _, s := range d.statuses { + if s.ResourceType == resourceType && s.ResourceID == resourceID { + result = append(result, s) + } + } + return result, nil +} + +func (d *mockAdapterStatusDao) FindByResourcePaginated(ctx context.Context, resourceType, resourceID string, offset, limit int) (api.AdapterStatusList, int64, error) { + statuses, _ := d.FindByResource(ctx, resourceType, resourceID) + return statuses, int64(len(statuses)), nil +} + +func (d *mockAdapterStatusDao) FindByResourceAndAdapter(ctx context.Context, resourceType, resourceID, adapter string) (*api.AdapterStatus, error) { + for _, s := range d.statuses { + if s.ResourceType == resourceType && s.ResourceID == resourceID && s.Adapter == adapter { + return s, nil + } + } + return nil, errors.NotFound("AdapterStatus").AsError() +} + +func (d *mockAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, error) { + var result api.AdapterStatusList + for _, s := range d.statuses { + result = append(result, s) + } + return result, nil +} + +var _ dao.AdapterStatusDao = &mockAdapterStatusDao{} + +// TestProcessAdapterStatus_UnknownCondition tests that Unknown Available condition returns nil (no-op) +func TestProcessAdapterStatus_UnknownCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := "test-cluster-id" + + // Create adapter status with Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: "Available", + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil for Unknown status") + + // Verify nothing was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") +} + +// TestProcessAdapterStatus_TrueCondition tests that True Available condition upserts and aggregates +func TestProcessAdapterStatus_TrueCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := "test-cluster-id" + + // Create the cluster first + cluster := &api.Cluster{ + Generation: 1, + } + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Create adapter status with Available=True + conditions := []api.AdapterCondition{ + { + Type: "Available", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should return the upserted status") + Expect(result.Adapter).To(Equal("test-adapter")) + + // Verify the status was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for True condition") +} + +// TestProcessAdapterStatus_FalseCondition tests that False Available condition upserts and aggregates +func TestProcessAdapterStatus_FalseCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := "test-cluster-id" + + // Create the cluster first + cluster := &api.Cluster{ + Generation: 1, + } + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Create adapter status with Available=False + conditions := []api.AdapterCondition{ + { + Type: "Available", + Status: api.AdapterConditionFalse, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should return the upserted status") + + // Verify the status was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for False condition") +} + +// TestProcessAdapterStatus_NoAvailableCondition tests when there's no Available condition +func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := "test-cluster-id" + + // Create the cluster first + fixedNow := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + initialConditions := []api.ResourceCondition{ + { + Type: "Available", + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + { + Type: "Ready", + Status: api.ConditionFalse, + ObservedGeneration: 7, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + } + initialConditionsJSON, _ := json.Marshal(initialConditions) + + cluster := &api.Cluster{ + Generation: 7, + StatusConditions: initialConditionsJSON, + } + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + initialClusterStatusConditions := append(api.Cluster{}.StatusConditions, cluster.StatusConditions...) + + // Create adapter status with Health condition (no Available) + conditions := []api.AdapterCondition{ + { + Type: "Health", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should proceed when no Available condition") + + // Verify the status was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(1), "Status should be stored when no Available condition") + + // Verify that saving a non-Available condition did not overwrite cluster Available/Ready + storedCluster, _ := clusterDao.Get(ctx, clusterID) + Expect(storedCluster.StatusConditions).To(Equal(initialClusterStatusConditions), "Cluster status conditions should not be overwritten when adapter status lacks Available") +} + +// TestProcessAdapterStatus_MultipleConditions_AvailableUnknown tests multiple conditions with Available=Unknown +func TestProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := "test-cluster-id" + + // Create adapter status with multiple conditions including Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: "Ready", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: "Available", + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + { + Type: "Progressing", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil when Available=Unknown") + + // Verify nothing was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") +} + +func TestClusterAvailableReadyTransitions(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + // Keep this small so we can cover transitions succinctly. + adapterConfig.RequiredClusterAdapters = []string{"validation", "dns"} + + service := NewClusterService(clusterDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + clusterID := "test-cluster-id" + + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + getSynth := func() (api.ResourceCondition, api.ResourceCondition) { + stored, getErr := clusterDao.Get(ctx, clusterID) + Expect(getErr).To(BeNil()) + + var conds []api.ResourceCondition + Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) + Expect(len(conds)).To(BeNumerically(">=", 2)) + + var available, ready *api.ResourceCondition + for i := range conds { + switch conds[i].Type { + case "Available": + available = &conds[i] + case "Ready": + ready = &conds[i] + } + } + Expect(available).ToNot(BeNil()) + Expect(ready).ToNot(BeNil()) + return *available, *ready + } + + upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { + conditions := []api.AdapterCondition{ + {Type: "Available", Status: available, LastTransitionTime: time.Now()}, + } + conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() + + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: adapter, + ObservedGeneration: observedGen, + Conditions: conditionsJSON, + CreatedTime: &now, + LastReportTime: &now, + } + + _, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + Expect(err).To(BeNil()) + } + + // No adapter statuses yet. + _, err := service.UpdateClusterStatusFromAdapters(ctx, clusterID) + Expect(err).To(BeNil()) + avail, ready := getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + Expect(ready.ObservedGeneration).To(Equal(int32(1))) + + // Partial adapters: still not Available/Ready. + upsert("validation", api.AdapterConditionTrue, 1) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // All required adapters available at gen=1 => Available=True, Ready=True. + upsert("dns", api.AdapterConditionTrue, 1) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionTrue)) + Expect(ready.ObservedGeneration).To(Equal(int32(1))) + + // Bump resource generation => Ready flips to False; Available remains True. + clusterDao.clusters[clusterID].Generation = 2 + _, err = service.UpdateClusterStatusFromAdapters(ctx, clusterID) + Expect(err).To(BeNil()) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + Expect(ready.ObservedGeneration).To(Equal(int32(2))) + + // One adapter updates to gen=2 => Ready still False; Available still True (minObservedGeneration still 1). + upsert("validation", api.AdapterConditionTrue, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // One adapter updates to gen=1 => Ready still False; Available still True (minObservedGeneration still 1). + // This is an edge case where an adapter reports a gen=1 status after a gen=2 status. + // Since we don't allow downgrading observed generations, we should not overwrite the cluster conditions. + // And Available should remain True, but in reality it should be False. + // This should be an unexpected edge case, since once a resource changes generation, all adapters should report a gen=2 status. + // So, while we are keeping Available True for gen=1, there should be soon an update to gen=2, which will overwrite the Available condition. + upsert("validation", api.AdapterConditionFalse, 1) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) // <-- this is the edge case + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // All required adapters at gen=2 => Ready becomes True, Available minObservedGeneration becomes 2. + upsert("dns", api.AdapterConditionTrue, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(2))) + Expect(ready.Status).To(Equal(api.ConditionTrue)) + + // One required adapter goes False => both Available and Ready become False. + upsert("dns", api.AdapterConditionFalse, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(avail.ObservedGeneration).To(Equal(int32(0))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // Available=Unknown is a no-op (does not store, does not overwrite cluster conditions). + prevStatus := append(api.Cluster{}.StatusConditions, clusterDao.clusters[clusterID].StatusConditions...) + unknownConds := []api.AdapterCondition{{Type: "Available", Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}} + unknownJSON, _ := json.Marshal(unknownConds) + unknownStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "dns", + Conditions: unknownJSON, + } + result, svcErr := service.ProcessAdapterStatus(ctx, clusterID, unknownStatus) + Expect(svcErr).To(BeNil()) + Expect(result).To(BeNil()) + Expect(clusterDao.clusters[clusterID].StatusConditions).To(Equal(prevStatus)) +} + +func TestClusterStaleAdapterStatusUpdatePolicy(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + adapterConfig.RequiredClusterAdapters = []string{"validation", "dns"} + + service := NewClusterService(clusterDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + clusterID := "test-cluster-id" + + cluster := &api.Cluster{Generation: 2} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + getAvailable := func() api.ResourceCondition { + stored, getErr := clusterDao.Get(ctx, clusterID) + Expect(getErr).To(BeNil()) + + var conds []api.ResourceCondition + Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) + for i := range conds { + if conds[i].Type == "Available" { + return conds[i] + } + } + Expect(true).To(BeFalse(), "Available condition not found") + return api.ResourceCondition{} + } + + upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { + conditions := []api.AdapterCondition{ + {Type: "Available", Status: available, LastTransitionTime: time.Now()}, + } + conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() + + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: adapter, + ObservedGeneration: observedGen, + Conditions: conditionsJSON, + CreatedTime: &now, + LastReportTime: &now, + } + + _, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + Expect(err).To(BeNil()) + } + + // Current generation statuses => Available=True at observed_generation=2. + upsert("validation", api.AdapterConditionTrue, 2) + upsert("dns", api.AdapterConditionTrue, 2) + available := getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) + + // Stale True should not override newer True. + upsert("validation", api.AdapterConditionTrue, 1) + available = getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) + + // Stale False is more restrictive and should override. + upsert("validation", api.AdapterConditionFalse, 1) + available = getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) +} + +func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + adapterConfig.RequiredClusterAdapters = []string{"validation"} + + service := NewClusterService(clusterDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + clusterID := "test-cluster-id" + + fixedNow := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + initialConditions := []api.ResourceCondition{ + { + Type: "Available", + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + { + Type: "Ready", + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + } + initialConditionsJSON, _ := json.Marshal(initialConditions) + + cluster := &api.Cluster{ + Generation: 1, + StatusConditions: initialConditionsJSON, + } + cluster.ID = clusterID + created, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + var createdConds []api.ResourceCondition + Expect(json.Unmarshal(created.StatusConditions, &createdConds)).To(Succeed()) + Expect(len(createdConds)).To(BeNumerically(">=", 2)) + + var createdAvailable, createdReady *api.ResourceCondition + for i := range createdConds { + switch createdConds[i].Type { + case "Available": + createdAvailable = &createdConds[i] + case "Ready": + createdReady = &createdConds[i] + } + } + Expect(createdAvailable).ToNot(BeNil()) + Expect(createdReady).ToNot(BeNil()) + Expect(createdAvailable.CreatedTime).To(Equal(fixedNow)) + Expect(createdAvailable.LastTransitionTime).To(Equal(fixedNow)) + Expect(createdAvailable.LastUpdatedTime).To(Equal(fixedNow)) + Expect(createdReady.CreatedTime).To(Equal(fixedNow)) + Expect(createdReady.LastTransitionTime).To(Equal(fixedNow)) + Expect(createdReady.LastUpdatedTime).To(Equal(fixedNow)) + + updated, err := service.UpdateClusterStatusFromAdapters(ctx, clusterID) + Expect(err).To(BeNil()) + + var updatedConds []api.ResourceCondition + Expect(json.Unmarshal(updated.StatusConditions, &updatedConds)).To(Succeed()) + Expect(len(updatedConds)).To(BeNumerically(">=", 2)) + + var updatedAvailable, updatedReady *api.ResourceCondition + for i := range updatedConds { + switch updatedConds[i].Type { + case "Available": + updatedAvailable = &updatedConds[i] + case "Ready": + updatedReady = &updatedConds[i] + } + } + Expect(updatedAvailable).ToNot(BeNil()) + Expect(updatedReady).ToNot(BeNil()) + Expect(updatedAvailable.CreatedTime).To(Equal(fixedNow)) + Expect(updatedAvailable.LastTransitionTime).To(Equal(fixedNow)) + Expect(updatedAvailable.LastUpdatedTime).To(Equal(fixedNow)) + Expect(updatedReady.CreatedTime).To(Equal(fixedNow)) + Expect(updatedReady.LastTransitionTime).To(Equal(fixedNow)) + Expect(updatedReady.LastUpdatedTime).To(Equal(fixedNow)) +} diff --git a/pkg/services/generic.go b/pkg/services/generic.go index f20a998..84d59a0 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -165,9 +165,18 @@ func (s *sqlGenericService) buildSearchValues(listCtx *listContext, d *dao.Gener if err != nil { return "", nil, errors.BadRequest("Failed to parse search query: %s", listCtx.args.Search) } + + // Extract condition queries (status.conditions.xxx) before field name mapping + // These require special JSONB containment handling that TSL doesn't support + tableName := (*d).GetTableName() + tslTree, conditionExprs, serviceErr := db.ExtractConditionQueries(tslTree, tableName) + if serviceErr != nil { + return "", nil, serviceErr + } + // apply field name mapping first (status.xxx -> status_xxx, labels.xxx -> labels->>'xxx') // this must happen before treeWalkForRelatedTables to prevent treating "status" and "labels" as related resources - tslTree, serviceErr := db.FieldNameWalk(tslTree, *listCtx.disallowedFields) + tslTree, serviceErr = db.FieldNameWalk(tslTree, *listCtx.disallowedFields) if serviceErr != nil { return "", nil, serviceErr } @@ -194,6 +203,24 @@ func (s *sqlGenericService) buildSearchValues(listCtx *listContext, d *dao.Gener if err != nil { return "", nil, errors.GeneralError("%s", err.Error()) } + + // Combine the base SQL with condition expressions + if len(conditionExprs) > 0 { + for _, condExpr := range conditionExprs { + condSQL, condValues, err := condExpr.ToSql() + if err != nil { + return "", nil, errors.GeneralError("%s", err.Error()) + } + // Append condition to the main SQL with AND + if sql == "" { + sql = condSQL + } else { + sql = fmt.Sprintf("(%s) AND (%s)", sql, condSQL) + } + values = append(values, condValues...) + } + } + return sql, values, nil } diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index f66a309..dc570b6 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -56,17 +56,7 @@ func TestSQLTranslation(t *testing.T) { "sql": "username IN (?)", "values": ConsistOf("ooo.openshift"), }, - // Test status.xxx field mapping - { - "search": "status.phase = 'NotReady'", - "sql": "status_phase = ?", - "values": ConsistOf("NotReady"), - }, - { - "search": "status.last_updated_time < '2025-01-01T00:00:00Z'", - "sql": "status_last_updated_time < ?", - "values": ConsistOf("2025-01-01T00:00:00Z"), - }, + // Test status.conditions field mapping (use status.conditions.='' syntax for condition queries) // Test labels.xxx field mapping { "search": "labels.environment = 'production'", diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index 17e1fc4..8309c1e 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -3,7 +3,8 @@ package services import ( "context" "encoding/json" - "math" + stderrors "errors" + "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -11,6 +12,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "gorm.io/gorm" ) //go:generate mockgen-v0.6.0 -source=node_pool.go -package=services -destination=node_pool_mock.go @@ -27,6 +29,11 @@ type NodePoolService interface { // Status aggregation UpdateNodePoolStatusFromAdapters(ctx context.Context, nodePoolID string) (*api.NodePool, *errors.ServiceError) + // ProcessAdapterStatus handles the business logic for adapter status: + // - If Available condition is "Unknown": returns (nil, nil) indicating no-op + // - Otherwise: upserts the status and triggers aggregation + ProcessAdapterStatus(ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) + // idempotent functions for the control plane, but can also be called synchronously by any actor OnUpsert(ctx context.Context, id string) error OnDelete(ctx context.Context, id string) error @@ -57,13 +64,22 @@ func (s *sqlNodePoolService) Get(ctx context.Context, id string) (*api.NodePool, } func (s *sqlNodePoolService) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, *errors.ServiceError) { + if nodePool.Generation == 0 { + nodePool.Generation = 1 + } + nodePool, err := s.nodePoolDao.Create(ctx, nodePool) if err != nil { return nil, handleCreateError("NodePool", err) } + updatedNodePool, svcErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePool.ID) + if svcErr != nil { + return nil, svcErr + } + // REMOVED: Event creation - no event-driven components - return nodePool, nil + return updatedNodePool, nil } func (s *sqlNodePoolService) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, *errors.ServiceError) { @@ -131,9 +147,10 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(ctx context.Contex return nil, errors.GeneralError("Failed to get adapter statuses: %s", err) } - // Build the list of ResourceCondition - adapters := []api.ResourceCondition{} - minObservedGeneration := int32(math.MaxInt32) + now := time.Now() + + // Build the list of adapter ResourceConditions + adapterConditions := []api.ResourceCondition{} for _, adapterStatus := range adapterStatuses { // Unmarshal Conditions from JSONB @@ -159,7 +176,7 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(ctx context.Contex // Convert to ResourceCondition condResource := api.ResourceCondition{ Type: MapAdapterToConditionType(adapterStatus.Adapter), - Status: availableCondition.Status, + Status: api.ResourceConditionStatus(availableCondition.Status), Reason: availableCondition.Reason, Message: availableCondition.Message, ObservedGeneration: adapterStatus.ObservedGeneration, @@ -176,62 +193,30 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(ctx context.Contex condResource.LastUpdatedTime = *adapterStatus.LastReportTime } - adapters = append(adapters, condResource) - - // Track min observed generation - // Use the LOWEST generation to ensure nodepool status only advances when ALL adapters catch up - if adapterStatus.ObservedGeneration < minObservedGeneration { - minObservedGeneration = adapterStatus.ObservedGeneration - } + adapterConditions = append(adapterConditions, condResource) } - // Compute overall phase using required adapters from config - newPhase := ComputePhase(ctx, adapterStatuses, s.adapterConfig.RequiredNodePoolAdapters, nodePool.Generation) + // Compute synthetic Available and Ready conditions + availableCondition, readyCondition := BuildSyntheticConditions( + nodePool.StatusConditions, + adapterStatuses, + s.adapterConfig.RequiredNodePoolAdapters, + nodePool.Generation, + now, + ) - // Calculate min(adapters[].last_report_time) for nodepool.status.last_updated_time - // This uses the OLDEST adapter timestamp to ensure Sentinel can detect stale adapters - var minLastUpdatedTime *time.Time - for _, adapterStatus := range adapterStatuses { - if adapterStatus.LastReportTime != nil { - if minLastUpdatedTime == nil || adapterStatus.LastReportTime.Before(*minLastUpdatedTime) { - minLastUpdatedTime = adapterStatus.LastReportTime - } - } - } - - // Save old phase to detect transitions - oldPhase := nodePool.StatusPhase - - // Update nodepool status fields - now := time.Now() - nodePool.StatusPhase = newPhase - // Set observed_generation to min across all adapters (0 if no adapters, matching DB default) - if len(adapterStatuses) == 0 { - nodePool.StatusObservedGeneration = 0 - } else { - nodePool.StatusObservedGeneration = minObservedGeneration - } + // Combine synthetic conditions with adapter conditions + // Put Available and Ready first + allConditions := []api.ResourceCondition{availableCondition, readyCondition} + allConditions = append(allConditions, adapterConditions...) // Marshal conditions to JSON - conditionsJSON, err := json.Marshal(adapters) + conditionsJSON, err := json.Marshal(allConditions) if err != nil { return nil, errors.GeneralError("Failed to marshal conditions: %s", err) } nodePool.StatusConditions = conditionsJSON - // Use min(adapters[].last_report_time) instead of now() - // This ensures Sentinel triggers reconciliation when ANY adapter is stale - if minLastUpdatedTime != nil { - nodePool.StatusLastUpdatedTime = minLastUpdatedTime - } else { - nodePool.StatusLastUpdatedTime = &now - } - - // Update last transition time only if phase changed - if nodePool.StatusLastTransitionTime == nil || oldPhase != newPhase { - nodePool.StatusLastTransitionTime = &now - } - // Save the updated nodepool nodePool, err = s.nodePoolDao.Replace(ctx, nodePool) if err != nil { @@ -240,3 +225,59 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(ctx context.Contex return nodePool, nil } + +// ProcessAdapterStatus handles the business logic for adapter status: +// - If Available condition is "Unknown": returns (nil, nil) indicating no-op +// - Otherwise: upserts the status and triggers aggregation +func (s *sqlNodePoolService) ProcessAdapterStatus(ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, *errors.ServiceError) { + existingStatus, findErr := s.adapterStatusDao.FindByResourceAndAdapter(ctx, "NodePool", nodePoolID, adapterStatus.Adapter) + if findErr != nil && !stderrors.Is(findErr, gorm.ErrRecordNotFound) { + if !strings.Contains(findErr.Error(), errors.CodeNotFoundGeneric) { + return nil, errors.GeneralError("Failed to get adapter status: %s", findErr) + } + } + if existingStatus != nil && adapterStatus.ObservedGeneration < existingStatus.ObservedGeneration { + // Discard stale status updates (older observed_generation). + return nil, nil + } + + // Parse conditions from the adapter status + var conditions []api.AdapterCondition + if len(adapterStatus.Conditions) > 0 { + if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err != nil { + return nil, errors.GeneralError("Failed to unmarshal adapter status conditions: %s", err) + } + } + + // Find the "Available" condition + hasAvailableCondition := false + for _, cond := range conditions { + if cond.Type != "Available" { + continue + } + + hasAvailableCondition = true + if cond.Status == api.AdapterConditionUnknown { + // Available condition is "Unknown", return nil to indicate no-op + return nil, nil + } + } + + // Upsert the adapter status + upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) + if err != nil { + return nil, handleCreateError("AdapterStatus", err) + } + + // Only trigger aggregation when the adapter reported an Available condition. + // If the adapter status doesn't include Available, saving it should not overwrite + // the nodepool's synthetic Available/Ready conditions. + if hasAvailableCondition { + if _, aggregateErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID); aggregateErr != nil { + // Log error but don't fail the request - the status will be computed on next update + logger.With(ctx, logger.FieldNodePoolID, nodePoolID).WithError(aggregateErr).Warn("Failed to aggregate nodepool status") + } + } + + return upsertedStatus, nil +} diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go new file mode 100644 index 0000000..24e0527 --- /dev/null +++ b/pkg/services/node_pool_test.go @@ -0,0 +1,518 @@ +package services + +import ( + "context" + "encoding/json" + "testing" + "time" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" +) + +// Mock implementations for testing NodePool ProcessAdapterStatus + +type mockNodePoolDao struct { + nodePools map[string]*api.NodePool +} + +func newMockNodePoolDao() *mockNodePoolDao { + return &mockNodePoolDao{ + nodePools: make(map[string]*api.NodePool), + } +} + +func (d *mockNodePoolDao) Get(ctx context.Context, id string) (*api.NodePool, error) { + if np, ok := d.nodePools[id]; ok { + return np, nil + } + return nil, errors.NotFound("NodePool").AsError() +} + +func (d *mockNodePoolDao) Create(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { + d.nodePools[nodePool.ID] = nodePool + return nodePool, nil +} + +func (d *mockNodePoolDao) Replace(ctx context.Context, nodePool *api.NodePool) (*api.NodePool, error) { + d.nodePools[nodePool.ID] = nodePool + return nodePool, nil +} + +func (d *mockNodePoolDao) Delete(ctx context.Context, id string) error { + delete(d.nodePools, id) + return nil +} + +func (d *mockNodePoolDao) FindByIDs(ctx context.Context, ids []string) (api.NodePoolList, error) { + var result api.NodePoolList + for _, id := range ids { + if np, ok := d.nodePools[id]; ok { + result = append(result, np) + } + } + return result, nil +} + +func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { + var result api.NodePoolList + for _, np := range d.nodePools { + result = append(result, np) + } + return result, nil +} + +var _ dao.NodePoolDao = &mockNodePoolDao{} + +// TestNodePoolProcessAdapterStatus_UnknownCondition tests that Unknown Available condition returns nil (no-op) +func TestNodePoolProcessAdapterStatus_UnknownCondition(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + // Create adapter status with Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: "Available", + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil for Unknown status") + + // Verify nothing was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) + Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") +} + +// TestNodePoolProcessAdapterStatus_TrueCondition tests that True Available condition upserts and aggregates +func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + // Create the nodepool first + nodePool := &api.NodePool{ + Generation: 1, + } + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + // Create adapter status with Available=True + conditions := []api.AdapterCondition{ + { + Type: "Available", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).ToNot(BeNil(), "ProcessAdapterStatus should return the upserted status") + Expect(result.Adapter).To(Equal("test-adapter")) + + // Verify the status was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) + Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for True condition") +} + +// TestNodePoolProcessAdapterStatus_MultipleConditions_AvailableUnknown tests multiple conditions with Available=Unknown +func TestNodePoolProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := config.NewAdapterRequirementsConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + // Create adapter status with multiple conditions including Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: "Ready", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: "Available", + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil when Available=Unknown") + + // Verify nothing was stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) + Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") +} + +func TestNodePoolAvailableReadyTransitions(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + adapterConfig.RequiredNodePoolAdapters = []string{"validation", "hypershift"} + + service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + nodePool := &api.NodePool{Generation: 1} + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + getSynth := func() (api.ResourceCondition, api.ResourceCondition) { + stored, getErr := nodePoolDao.Get(ctx, nodePoolID) + Expect(getErr).To(BeNil()) + + var conds []api.ResourceCondition + Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) + Expect(len(conds)).To(BeNumerically(">=", 2)) + + var available, ready *api.ResourceCondition + for i := range conds { + switch conds[i].Type { + case "Available": + available = &conds[i] + case "Ready": + ready = &conds[i] + } + } + Expect(available).ToNot(BeNil()) + Expect(ready).ToNot(BeNil()) + return *available, *ready + } + + upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { + conditions := []api.AdapterCondition{ + {Type: "Available", Status: available, LastTransitionTime: time.Now()}, + } + conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() + + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: adapter, + ObservedGeneration: observedGen, + Conditions: conditionsJSON, + CreatedTime: &now, + LastReportTime: &now, + } + + _, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + Expect(err).To(BeNil()) + } + + // No adapter statuses yet. + _, err := service.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID) + Expect(err).To(BeNil()) + avail, ready := getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + Expect(ready.ObservedGeneration).To(Equal(int32(1))) + + // Partial adapters: still not Available/Ready. + upsert("validation", api.AdapterConditionTrue, 1) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // All required adapters available at gen=1 => Available=True, Ready=True. + upsert("hypershift", api.AdapterConditionTrue, 1) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionTrue)) + + // Bump resource generation => Ready flips to False; Available remains True. + nodePoolDao.nodePools[nodePoolID].Generation = 2 + _, err = service.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID) + Expect(err).To(BeNil()) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + Expect(ready.ObservedGeneration).To(Equal(int32(2))) + + // One adapter updates to gen=2 => Ready still False; Available still True (minObservedGeneration still 1). + upsert("validation", api.AdapterConditionTrue, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(1))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // All required adapters at gen=2 => Ready becomes True, Available minObservedGeneration becomes 2. + upsert("hypershift", api.AdapterConditionTrue, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionTrue)) + Expect(avail.ObservedGeneration).To(Equal(int32(2))) + Expect(ready.Status).To(Equal(api.ConditionTrue)) + + // One required adapter goes False => both Available and Ready become False. + upsert("hypershift", api.AdapterConditionFalse, 2) + avail, ready = getSynth() + Expect(avail.Status).To(Equal(api.ConditionFalse)) + Expect(avail.ObservedGeneration).To(Equal(int32(0))) + Expect(ready.Status).To(Equal(api.ConditionFalse)) + + // Adapter status with no Available condition should not overwrite synthetic conditions. + prevStatus := append(api.NodePool{}.StatusConditions, nodePoolDao.nodePools[nodePoolID].StatusConditions...) + nonAvailableConds := []api.AdapterCondition{{Type: "Health", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}} + nonAvailableJSON, _ := json.Marshal(nonAvailableConds) + nonAvailableStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "hypershift", + ObservedGeneration: 2, + Conditions: nonAvailableJSON, + } + result, svcErr := service.ProcessAdapterStatus(ctx, nodePoolID, nonAvailableStatus) + Expect(svcErr).To(BeNil()) + Expect(result).ToNot(BeNil()) + Expect(nodePoolDao.nodePools[nodePoolID].StatusConditions).To(Equal(prevStatus)) + + // Available=Unknown is a no-op (does not store, does not overwrite nodepool conditions). + prevStatus = append(api.NodePool{}.StatusConditions, nodePoolDao.nodePools[nodePoolID].StatusConditions...) + unknownConds := []api.AdapterCondition{{Type: "Available", Status: api.AdapterConditionUnknown, LastTransitionTime: time.Now()}} + unknownJSON, _ := json.Marshal(unknownConds) + unknownStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "hypershift", + Conditions: unknownJSON, + } + result, svcErr = service.ProcessAdapterStatus(ctx, nodePoolID, unknownStatus) + Expect(svcErr).To(BeNil()) + Expect(result).To(BeNil()) + Expect(nodePoolDao.nodePools[nodePoolID].StatusConditions).To(Equal(prevStatus)) +} + +func TestNodePoolStaleAdapterStatusUpdatePolicy(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + adapterConfig.RequiredNodePoolAdapters = []string{"validation", "hypershift"} + + service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + nodePool := &api.NodePool{Generation: 2} + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + getAvailable := func() api.ResourceCondition { + stored, getErr := nodePoolDao.Get(ctx, nodePoolID) + Expect(getErr).To(BeNil()) + + var conds []api.ResourceCondition + Expect(json.Unmarshal(stored.StatusConditions, &conds)).To(Succeed()) + for i := range conds { + if conds[i].Type == "Available" { + return conds[i] + } + } + Expect(true).To(BeFalse(), "Available condition not found") + return api.ResourceCondition{} + } + + upsert := func(adapter string, available api.AdapterConditionStatus, observedGen int32) { + conditions := []api.AdapterCondition{ + {Type: "Available", Status: available, LastTransitionTime: time.Now()}, + } + conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() + + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: adapter, + ObservedGeneration: observedGen, + Conditions: conditionsJSON, + CreatedTime: &now, + LastReportTime: &now, + } + + _, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + Expect(err).To(BeNil()) + } + + // Current generation statuses => Available=True at observed_generation=2. + upsert("validation", api.AdapterConditionTrue, 2) + upsert("hypershift", api.AdapterConditionTrue, 2) + available := getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) + + // Stale True should not override newer True. + upsert("validation", api.AdapterConditionTrue, 1) + available = getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) + + // Stale False is more restrictive and should override. + upsert("validation", api.AdapterConditionFalse, 1) + available = getAvailable() + Expect(available.Status).To(Equal(api.ConditionTrue)) + Expect(available.ObservedGeneration).To(Equal(int32(2))) +} + +func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + adapterConfig := config.NewAdapterRequirementsConfig() + adapterConfig.RequiredNodePoolAdapters = []string{"validation"} + + service := NewNodePoolService(nodePoolDao, adapterStatusDao, adapterConfig) + + ctx := context.Background() + nodePoolID := "test-nodepool-id" + + fixedNow := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + initialConditions := []api.ResourceCondition{ + { + Type: "Available", + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + { + Type: "Ready", + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: fixedNow, + CreatedTime: fixedNow, + LastUpdatedTime: fixedNow, + }, + } + initialConditionsJSON, _ := json.Marshal(initialConditions) + + nodePool := &api.NodePool{ + Generation: 1, + StatusConditions: initialConditionsJSON, + } + nodePool.ID = nodePoolID + created, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + var createdConds []api.ResourceCondition + Expect(json.Unmarshal(created.StatusConditions, &createdConds)).To(Succeed()) + Expect(len(createdConds)).To(BeNumerically(">=", 2)) + + var createdAvailable, createdReady *api.ResourceCondition + for i := range createdConds { + switch createdConds[i].Type { + case "Available": + createdAvailable = &createdConds[i] + case "Ready": + createdReady = &createdConds[i] + } + } + Expect(createdAvailable).ToNot(BeNil()) + Expect(createdReady).ToNot(BeNil()) + Expect(createdAvailable.CreatedTime).To(Equal(fixedNow)) + Expect(createdAvailable.LastTransitionTime).To(Equal(fixedNow)) + Expect(createdAvailable.LastUpdatedTime).To(Equal(fixedNow)) + Expect(createdReady.CreatedTime).To(Equal(fixedNow)) + Expect(createdReady.LastTransitionTime).To(Equal(fixedNow)) + Expect(createdReady.LastUpdatedTime).To(Equal(fixedNow)) + + updated, err := service.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID) + Expect(err).To(BeNil()) + + var updatedConds []api.ResourceCondition + Expect(json.Unmarshal(updated.StatusConditions, &updatedConds)).To(Succeed()) + Expect(len(updatedConds)).To(BeNumerically(">=", 2)) + + var updatedAvailable, updatedReady *api.ResourceCondition + for i := range updatedConds { + switch updatedConds[i].Type { + case "Available": + updatedAvailable = &updatedConds[i] + case "Ready": + updatedReady = &updatedConds[i] + } + } + Expect(updatedAvailable).ToNot(BeNil()) + Expect(updatedReady).ToNot(BeNil()) + Expect(updatedAvailable.CreatedTime).To(Equal(fixedNow)) + Expect(updatedAvailable.LastTransitionTime).To(Equal(fixedNow)) + Expect(updatedAvailable.LastUpdatedTime).To(Equal(fixedNow)) + Expect(updatedReady.CreatedTime).To(Equal(fixedNow)) + Expect(updatedReady.LastTransitionTime).To(Equal(fixedNow)) + Expect(updatedReady.LastUpdatedTime).To(Equal(fixedNow)) +} From 0a7b6a356f27fdebcc13d43eec34725fb9e30fc9 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:03 +0100 Subject: [PATCH 05/13] HYPERFLEET-386 - feat: return 204 No Content for no-op status updates Handler changes for condition-based status: - Add handleCreateWithNoContent() framework helper for 204 responses - POST /clusters/{id}/statuses returns: - 201 Created when conditions changed - 204 No Content when adapter report received but no condition change - Same behavior for nodepool status endpoints --- pkg/handlers/cluster_status.go | 15 +++++------- pkg/handlers/framework.go | 41 +++++++++++++++++++++++++++++++++ pkg/handlers/nodepool_status.go | 13 +++++------ 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/pkg/handlers/cluster_status.go b/pkg/handlers/cluster_status.go index 16825cb..c7636b9 100644 --- a/pkg/handlers/cluster_status.go +++ b/pkg/handlers/cluster_status.go @@ -8,7 +8,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/presenters" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" ) @@ -89,17 +88,15 @@ func (h clusterStatusHandler) Create(w http.ResponseWriter, r *http.Request) { return nil, errors.GeneralError("Failed to convert adapter status: %v", convErr) } - // Upsert (create or update based on resource_type + resource_id + adapter) - adapterStatus, err := h.adapterStatusService.Upsert(ctx, newStatus) + // Process adapter status (handles Unknown status and upsert + aggregation) + adapterStatus, err := h.clusterService.ProcessAdapterStatus(ctx, clusterID, newStatus) if err != nil { return nil, err } - // Trigger status aggregation - if _, aggregateErr := h.clusterService.UpdateClusterStatusFromAdapters(ctx, clusterID); aggregateErr != nil { - // Log error but don't fail the request - the status will be computed on next update - ctx = logger.WithClusterID(ctx, clusterID) - logger.WithError(ctx, aggregateErr).Warn("Failed to aggregate cluster status") + // If result is nil, return nil to signal 204 No Content + if adapterStatus == nil { + return nil, nil } status, presErr := presenters.PresentAdapterStatus(adapterStatus) @@ -111,5 +108,5 @@ func (h clusterStatusHandler) Create(w http.ResponseWriter, r *http.Request) { handleError, } - handle(w, r, cfg, http.StatusCreated) + handleCreateWithNoContent(w, r, cfg) } diff --git a/pkg/handlers/framework.go b/pkg/handlers/framework.go index 8e2fd75..62a6767 100755 --- a/pkg/handlers/framework.go +++ b/pkg/handlers/framework.go @@ -134,3 +134,44 @@ func handleList(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { } writeJSONResponse(w, r, http.StatusOK, results) } + +// handleCreateWithNoContent handles create requests that may return 204 No Content +// If action returns (nil, nil), it means a no-op and returns 204 No Content +// Otherwise, it returns 201 Created with the result +func handleCreateWithNoContent(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { + if cfg.ErrorHandler == nil { + cfg.ErrorHandler = handleError + } + + bytes, err := io.ReadAll(r.Body) + if err != nil { + handleError(r, w, errors.MalformedRequest("Unable to read request body: %s", err)) + return + } + + err = json.Unmarshal(bytes, &cfg.MarshalInto) + if err != nil { + handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) + return + } + + for _, v := range cfg.Validate { + err := v() + if err != nil { + cfg.ErrorHandler(r, w, err) + return + } + } + + result, serviceErr := cfg.Action() + + switch { + case serviceErr != nil: + cfg.ErrorHandler(r, w, serviceErr) + case result == nil: + // No-op case: return 204 No Content + w.WriteHeader(http.StatusNoContent) + default: + writeJSONResponse(w, r, http.StatusCreated, result) + } +} diff --git a/pkg/handlers/nodepool_status.go b/pkg/handlers/nodepool_status.go index 7accf5f..0c1fffe 100644 --- a/pkg/handlers/nodepool_status.go +++ b/pkg/handlers/nodepool_status.go @@ -89,16 +89,15 @@ func (h nodePoolStatusHandler) Create(w http.ResponseWriter, r *http.Request) { return nil, errors.GeneralError("Failed to convert adapter status: %v", convErr) } - // Upsert (create or update based on resource_type + resource_id + adapter) - adapterStatus, err := h.adapterStatusService.Upsert(ctx, newStatus) + // Process adapter status (handles Unknown status and upsert + aggregation) + adapterStatus, err := h.nodePoolService.ProcessAdapterStatus(ctx, nodePoolID, newStatus) if err != nil { return nil, err } - // Trigger status aggregation - if _, aggregateErr := h.nodePoolService.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID); aggregateErr != nil { - // Log error but don't fail the request - the status will be computed on next update - logger.With(ctx, logger.FieldNodePoolID, nodePoolID).WithError(aggregateErr).Warn("Failed to aggregate nodepool status") + // If result is nil, return nil to signal 204 No Content + if adapterStatus == nil { + return nil, nil } status, presErr := presenters.PresentAdapterStatus(adapterStatus) @@ -110,5 +109,5 @@ func (h nodePoolStatusHandler) Create(w http.ResponseWriter, r *http.Request) { handleError, } - handle(w, r, cfg, http.StatusCreated) + handleCreateWithNoContent(w, r, cfg) } From 1854b7b7f7a5d3a23a0846d5db51ddf47b549b5f Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:16 +0100 Subject: [PATCH 06/13] HYPERFLEET-386 - feat: add condition-based search and GIN index Database and search changes: - Add conditionsNodeConverter() for TSL to SQL translation - New search syntax: status.conditions.Ready='True' - Remove status_phase column from migrations - Add GIN index for efficient condition queries Query translation: status.conditions.Ready='True' becomes: jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') ->> 'status' = 'True' --- .../migrations/202511111044_add_clusters.go | 23 +- .../migrations/202511111055_add_node_pools.go | 23 +- .../202601210001_add_conditions_gin_index.go | 46 +++ pkg/db/migrations/migration_structs.go | 1 + pkg/db/sql_helpers.go | 181 +++++++++- pkg/db/sql_helpers_test.go | 330 ++++++++++++++++++ 6 files changed, 555 insertions(+), 49 deletions(-) create mode 100644 pkg/db/migrations/202601210001_add_conditions_gin_index.go create mode 100644 pkg/db/sql_helpers_test.go diff --git a/pkg/db/migrations/202511111044_add_clusters.go b/pkg/db/migrations/202511111044_add_clusters.go index 4e2e90b..18cbeb4 100644 --- a/pkg/db/migrations/202511111044_add_clusters.go +++ b/pkg/db/migrations/202511111044_add_clusters.go @@ -35,11 +35,7 @@ func addClusters() *gormigrate.Migration { -- Version control generation INTEGER NOT NULL DEFAULT 1, - -- Status fields (flattened for efficient querying) - status_phase VARCHAR(50) NOT NULL DEFAULT 'NotReady', - status_last_transition_time TIMESTAMPTZ NULL, - status_observed_generation INTEGER NOT NULL DEFAULT 0, - status_last_updated_time TIMESTAMPTZ NULL, + -- Status (conditions-only model) status_conditions JSONB NULL, -- Audit fields @@ -62,27 +58,10 @@ func addClusters() *gormigrate.Migration { return err } - // Create index on status_phase for filtering - if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_clusters_status_phase ON clusters(status_phase);").Error; err != nil { - return err - } - - // Create index on status_last_updated_time for search optimization - // Sentinel queries frequently filter by this field to find stale resources - if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_clusters_status_last_updated_time ON clusters(status_last_updated_time);").Error; err != nil { - return err - } - return nil }, Rollback: func(tx *gorm.DB) error { // Drop indexes first - if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_status_last_updated_time;").Error; err != nil { - return err - } - if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_status_phase;").Error; err != nil { - return err - } if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_name;").Error; err != nil { return err } diff --git a/pkg/db/migrations/202511111055_add_node_pools.go b/pkg/db/migrations/202511111055_add_node_pools.go index e19ea37..a157193 100644 --- a/pkg/db/migrations/202511111055_add_node_pools.go +++ b/pkg/db/migrations/202511111055_add_node_pools.go @@ -38,11 +38,7 @@ func addNodePools() *gormigrate.Migration { -- Version control generation INTEGER NOT NULL DEFAULT 1, - -- Status fields (flattened for efficient querying) - status_phase VARCHAR(50) NOT NULL DEFAULT 'NotReady', - status_last_transition_time TIMESTAMPTZ NULL, - status_observed_generation INTEGER NOT NULL DEFAULT 0, - status_last_updated_time TIMESTAMPTZ NULL, + -- Status (conditions-only model) status_conditions JSONB NULL, -- Audit fields @@ -65,17 +61,6 @@ func addNodePools() *gormigrate.Migration { return err } - // Create index on status_phase for filtering - if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_node_pools_status_phase ON node_pools(status_phase);").Error; err != nil { - return err - } - - // Create index on status_last_updated_time for search optimization - // Sentinel queries frequently filter by this field to find stale resources - if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_node_pools_status_last_updated_time ON node_pools(status_last_updated_time);").Error; err != nil { - return err - } - // Add foreign key constraint to clusters addFKSQL := ` ALTER TABLE node_pools @@ -96,12 +81,6 @@ func addNodePools() *gormigrate.Migration { } // Drop indexes - if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_status_last_updated_time;").Error; err != nil { - return err - } - if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_status_phase;").Error; err != nil { - return err - } if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_owner_id;").Error; err != nil { return err } diff --git a/pkg/db/migrations/202601210001_add_conditions_gin_index.go b/pkg/db/migrations/202601210001_add_conditions_gin_index.go new file mode 100644 index 0000000..2860f04 --- /dev/null +++ b/pkg/db/migrations/202601210001_add_conditions_gin_index.go @@ -0,0 +1,46 @@ +package migrations + +import ( + "github.com/go-gormigrate/gormigrate/v2" + "gorm.io/gorm" +) + +// addConditionsGinIndex adds expression indexes on the Ready condition +// within status_conditions JSONB columns for efficient lookups. +func addConditionsGinIndex() *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "202601210001", + Migrate: func(tx *gorm.DB) error { + // Create expression index on clusters for Ready condition lookups + if err := tx.Exec(` + CREATE INDEX IF NOT EXISTS idx_clusters_ready_status + ON clusters USING BTREE (( + jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') + )); + `).Error; err != nil { + return err + } + + // Create expression index on node_pools for Ready condition lookups + if err := tx.Exec(` + CREATE INDEX IF NOT EXISTS idx_node_pools_ready_status + ON node_pools USING BTREE (( + jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') + )); + `).Error; err != nil { + return err + } + + return nil + }, + Rollback: func(tx *gorm.DB) error { + if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_ready_status;").Error; err != nil { + return err + } + if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_ready_status;").Error; err != nil { + return err + } + return nil + }, + } +} diff --git a/pkg/db/migrations/migration_structs.go b/pkg/db/migrations/migration_structs.go index c422587..2077fd7 100755 --- a/pkg/db/migrations/migration_structs.go +++ b/pkg/db/migrations/migration_structs.go @@ -30,6 +30,7 @@ var MigrationList = []*gormigrate.Migration{ addClusters(), addNodePools(), addAdapterStatus(), + addConditionsGinIndex(), } // Model represents the base model struct. All entities will have this struct embedded. diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index d3eb450..920b355 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -6,6 +6,7 @@ import ( "regexp" "strings" + sq "github.com/Masterminds/squirrel" "github.com/jinzhu/inflection" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/yaacov/tree-search-language/pkg/tsl" @@ -52,12 +53,9 @@ func hasProperty(n tsl.Node) bool { } // Field mapping rules for user-friendly syntax to database columns +// Note: status.phase was removed - use status.conditions.Ready='True' or status.conditions.Available='True' instead var statusFieldMappings = map[string]string{ - "status.last_updated_time": "status_last_updated_time", - "status.last_transition_time": "status_last_transition_time", - "status.phase": "status_phase", - "status.observed_generation": "status_observed_generation", - "status.conditions": "status_conditions", + "status.conditions": "status_conditions", } // getField gets the sql field associated with a name. @@ -147,6 +145,179 @@ func propertiesNodeConverter(n tsl.Node) tsl.Node { return propertyNode } +// Condition type validation pattern: PascalCase condition types (e.g., Ready, Available, Progressing) +var conditionTypePattern = regexp.MustCompile(`^[A-Z][a-zA-Z0-9]*$`) + +// Condition status validation: must be True, False, or Unknown +var validConditionStatuses = map[string]bool{ + "True": true, + "False": true, + "Unknown": true, +} + +// startsWithConditions checks if a field starts with status.conditions. +func startsWithConditions(s string) bool { + return strings.HasPrefix(s, "status.conditions.") +} + +// hasCondition returns true if node has a status.conditions. identifier on left hand side. +func hasCondition(n tsl.Node) bool { + // Get the left side operator. + l, ok := n.Left.(tsl.Node) + if !ok { + return false + } + + // If left side is not a `status.conditions.` identifier, return. + leftStr, ok := l.Left.(string) + if !ok || l.Func != tsl.IdentOp || !startsWithConditions(leftStr) { + return false + } + + return true +} + +// conditionsNodeConverter handles status.conditions.='' queries +// Transforms: status.conditions.Ready='True' -> +// jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")') ->> 'status' = 'True' +// This uses the BTREE expression index on Ready conditions for efficient lookups. +func conditionsNodeConverter(n tsl.Node) (interface{}, *errors.ServiceError) { + // Get the left side operator. + l, ok := n.Left.(tsl.Node) + if !ok { + return nil, errors.BadRequest("invalid condition query structure") + } + + // Extract the full field path + leftStr, ok := l.Left.(string) + if !ok { + return nil, errors.BadRequest("expected string for left side of condition") + } + + // Extract condition type from path (e.g., "status.conditions.Ready" -> "Ready") + parts := strings.Split(leftStr, ".") + if len(parts) != 3 || parts[0] != "status" || parts[1] != "conditions" { + return nil, errors.BadRequest("invalid condition field path: %s", leftStr) + } + conditionType := parts[2] + + // Validate condition type to prevent SQL injection + if !conditionTypePattern.MatchString(conditionType) { + return nil, errors.BadRequest("condition type '%s' is invalid: must be PascalCase (e.g., Ready, Available)", conditionType) + } + + // Get the right side value (the expected status) + r, ok := n.Right.(tsl.Node) + if !ok { + return nil, errors.BadRequest("invalid condition query structure: missing right side") + } + + rightStr, ok := r.Left.(string) + if !ok { + return nil, errors.BadRequest("expected string for right side of condition") + } + + // Validate condition status + if !validConditionStatuses[rightStr] { + return nil, errors.BadRequest("condition status '%s' is invalid: must be True, False, or Unknown", rightStr) + } + + // Only support equality operator for condition queries + if n.Func != tsl.EqOp { + return nil, errors.BadRequest("only equality operator (=) is supported for condition queries") + } + + // Build query using jsonb_path_query_first to leverage BTREE expression index + // The index is: CREATE INDEX ... USING BTREE ((jsonb_path_query_first(status_conditions, '$[*] ? (@.type == "Ready")'))) + jsonPath := fmt.Sprintf(`$[*] ? (@.type == "%s")`, conditionType) + + return sq.Expr("jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", jsonPath, rightStr), nil +} + +// ConditionExpression represents an extracted condition query as a Squirrel expression +type ConditionExpression struct { + Expr sq.Sqlizer +} + +// ExtractConditionQueries walks the TSL tree and extracts condition queries, +// returning the modified tree (with condition nodes replaced) and the extracted conditions. +// This is necessary because the TSL library doesn't support JSONB containment operators. +func ExtractConditionQueries(n tsl.Node, tableName string) (tsl.Node, []sq.Sqlizer, *errors.ServiceError) { + var conditions []sq.Sqlizer + modifiedTree, err := extractConditionsWalk(n, tableName, &conditions) + return modifiedTree, conditions, err +} + +// extractConditionsWalk recursively walks the tree and extracts condition queries +func extractConditionsWalk(n tsl.Node, tableName string, conditions *[]sq.Sqlizer) (tsl.Node, *errors.ServiceError) { + // Check if this node is a condition query + if hasCondition(n) { + expr, err := conditionsNodeConverter(n) + if err != nil { + return n, err + } + *conditions = append(*conditions, expr.(sq.Sqlizer)) + + // Replace with a placeholder that always evaluates to true + // This allows the rest of the tree to be processed normally + return tsl.Node{ + Func: tsl.EqOp, + Left: tsl.Node{Func: tsl.NumberOp, Left: float64(1)}, + Right: tsl.Node{Func: tsl.NumberOp, Left: float64(1)}, + }, nil + } + + // For non-condition nodes, recursively process children + var newLeft, newRight interface{} + var serviceErr *errors.ServiceError + + if n.Left != nil { + switch v := n.Left.(type) { + case tsl.Node: + newLeftNode, err := extractConditionsWalk(v, tableName, conditions) + if err != nil { + return n, err + } + newLeft = newLeftNode + default: + newLeft = n.Left + } + } + + if n.Right != nil { + switch v := n.Right.(type) { + case tsl.Node: + newRightNode, err := extractConditionsWalk(v, tableName, conditions) + if err != nil { + return n, err + } + newRight = newRightNode + case []tsl.Node: + var newRightNodes []tsl.Node + for _, rightNode := range v { + newRightNode, err := extractConditionsWalk(rightNode, tableName, conditions) + if err != nil { + return n, err + } + newRightNodes = append(newRightNodes, newRightNode) + } + newRight = newRightNodes + default: + newRight = n.Right + } + } + + if serviceErr != nil { + return n, serviceErr + } + + return tsl.Node{ + Func: n.Func, + Left: newLeft, + Right: newRight, + }, nil +} + // FieldNameWalk walks on the filter tree and check/replace // the search fields names: // a. the the field name is valid. diff --git a/pkg/db/sql_helpers_test.go b/pkg/db/sql_helpers_test.go new file mode 100644 index 0000000..98b75c1 --- /dev/null +++ b/pkg/db/sql_helpers_test.go @@ -0,0 +1,330 @@ +package db + +import ( + "testing" + + "github.com/yaacov/tree-search-language/pkg/tsl" +) + +func TestConditionsNodeConverter(t *testing.T) { + tests := []struct { + name string + field string + value string + expectedSQL string + expectedArgs []interface{} + expectError bool + errorContains string + }{ + { + name: "Ready condition True", + field: "status.conditions.Ready", + value: "True", + expectedSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectedArgs: []interface{}{`$[*] ? (@.type == "Ready")`, "True"}, + expectError: false, + }, + { + name: "Ready condition False", + field: "status.conditions.Ready", + value: "False", + expectedSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectedArgs: []interface{}{`$[*] ? (@.type == "Ready")`, "False"}, + expectError: false, + }, + { + name: "Available condition True", + field: "status.conditions.Available", + value: "True", + expectedSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectedArgs: []interface{}{`$[*] ? (@.type == "Available")`, "True"}, + expectError: false, + }, + { + name: "Available condition Unknown", + field: "status.conditions.Available", + value: "Unknown", + expectedSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectedArgs: []interface{}{`$[*] ? (@.type == "Available")`, "Unknown"}, + expectError: false, + }, + { + name: "Progressing condition", + field: "status.conditions.Progressing", + value: "True", + expectedSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectedArgs: []interface{}{`$[*] ? (@.type == "Progressing")`, "True"}, + expectError: false, + }, + { + name: "Invalid condition status", + field: "status.conditions.Ready", + value: "Invalid", + expectError: true, + errorContains: "condition status 'Invalid' is invalid", + }, + { + name: "Invalid condition type - lowercase", + field: "status.conditions.ready", + value: "True", + expectError: true, + errorContains: "must be PascalCase", + }, + { + name: "Invalid condition type - with underscore", + field: "status.conditions.Ready_Status", + value: "True", + expectError: true, + errorContains: "must be PascalCase", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Build a TSL node that represents: status.conditions.X = 'Y' + node := tsl.Node{ + Func: tsl.EqOp, + Left: tsl.Node{ + Func: tsl.IdentOp, + Left: tt.field, + }, + Right: tsl.Node{ + Func: tsl.StringOp, + Left: tt.value, + }, + } + + result, err := conditionsNodeConverter(node) + + if tt.expectError { + if err == nil { + t.Errorf("Expected error containing %q, but got nil", tt.errorContains) + return + } + if tt.errorContains != "" && err.Error() != "" { + // Check if error message contains expected string + found := false + if errMsg := err.Error(); errMsg != "" { + found = true + } + if !found { + t.Logf("Error message: %s", err.Error()) + } + } + return + } + + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } + + // Get SQL from the sqlizer + sqlizer := result.(interface { + ToSql() (string, []interface{}, error) + }) + sql, args, sqlErr := sqlizer.ToSql() + if sqlErr != nil { + t.Errorf("Failed to convert to SQL: %v", sqlErr) + return + } + + if sql != tt.expectedSQL { + t.Errorf("Expected SQL %q, got %q", tt.expectedSQL, sql) + } + + if len(args) != len(tt.expectedArgs) { + t.Errorf("Expected %d args, got %d", len(tt.expectedArgs), len(args)) + return + } + + for i, expectedArg := range tt.expectedArgs { + if args[i] != expectedArg { + t.Errorf("Expected arg[%d] = %q, got %q", i, expectedArg, args[i]) + } + } + }) + } +} + +func TestExtractConditionQueries(t *testing.T) { + tests := []struct { + name string + searchQuery string + expectedConditions int + expectedConditionSQL string + expectError bool + }{ + { + name: "Single condition query", + searchQuery: "status.conditions.Ready='True'", + expectedConditions: 1, + expectedConditionSQL: "jsonb_path_query_first(status_conditions, ?::jsonpath) ->> 'status' = ?", + expectError: false, + }, + { + name: "No condition queries", + searchQuery: "name='test'", + expectedConditions: 0, + expectError: false, + }, + { + name: "Mixed query with condition", + searchQuery: "name='test' AND status.conditions.Ready='True'", + expectedConditions: 1, + expectError: false, + }, + { + name: "Multiple condition queries", + searchQuery: "status.conditions.Ready='True' AND status.conditions.Available='True'", + expectedConditions: 2, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Parse the search query + tslTree, err := tsl.ParseTSL(tt.searchQuery) + if err != nil { + t.Fatalf("Failed to parse TSL: %v", err) + } + + // Extract condition queries + _, conditions, serviceErr := ExtractConditionQueries(tslTree, "clusters") + + if tt.expectError { + if serviceErr == nil { + t.Error("Expected error but got nil") + } + return + } + + if serviceErr != nil { + t.Errorf("Unexpected error: %v", serviceErr) + return + } + + if len(conditions) != tt.expectedConditions { + t.Errorf("Expected %d conditions, got %d", tt.expectedConditions, len(conditions)) + return + } + + // Verify the SQL of extracted conditions + if tt.expectedConditions > 0 && tt.expectedConditionSQL != "" { + sql, _, sqlErr := conditions[0].ToSql() + if sqlErr != nil { + t.Errorf("Failed to convert condition to SQL: %v", sqlErr) + return + } + if sql != tt.expectedConditionSQL { + t.Errorf("Expected condition SQL %q, got %q", tt.expectedConditionSQL, sql) + } + } + }) + } +} + +func TestHasCondition(t *testing.T) { + tests := []struct { + name string + field string + expected bool + }{ + { + name: "Valid condition field", + field: "status.conditions.Ready", + expected: true, + }, + { + name: "Status field without conditions prefix", + field: "status.other_field", + expected: false, + }, + { + name: "Labels field", + field: "labels.environment", + expected: false, + }, + { + name: "Simple field", + field: "name", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + node := tsl.Node{ + Func: tsl.EqOp, + Left: tsl.Node{ + Func: tsl.IdentOp, + Left: tt.field, + }, + Right: tsl.Node{ + Func: tsl.StringOp, + Left: "value", + }, + } + + result := hasCondition(node) + if result != tt.expected { + t.Errorf("Expected hasCondition(%q) = %v, got %v", tt.field, tt.expected, result) + } + }) + } +} + +func TestConditionTypeValidation(t *testing.T) { + tests := []struct { + name string + condType string + expectMatch bool + }{ + {"Valid - Ready", "Ready", true}, + {"Valid - Available", "Available", true}, + {"Valid - Progressing", "Progressing", true}, + {"Valid - CustomCondition", "CustomCondition", true}, + {"Valid - With numbers", "Ready2", true}, + {"Invalid - lowercase", "ready", false}, + {"Invalid - starts with number", "2Ready", false}, + {"Invalid - contains underscore", "Ready_State", false}, + {"Invalid - contains hyphen", "Ready-State", false}, + {"Invalid - empty", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := conditionTypePattern.MatchString(tt.condType) + if result != tt.expectMatch { + t.Errorf("conditionTypePattern.MatchString(%q) = %v, expected %v", tt.condType, result, tt.expectMatch) + } + }) + } +} + +func TestConditionStatusValidation(t *testing.T) { + tests := []struct { + status string + expectValid bool + }{ + {"True", true}, + {"False", true}, + {"Unknown", true}, + {"true", false}, + {"false", false}, + {"unknown", false}, + {"Yes", false}, + {"No", false}, + {"", false}, + } + + for _, tt := range tests { + t.Run(tt.status, func(t *testing.T) { + result := validConditionStatuses[tt.status] + if result != tt.expectValid { + t.Errorf("validConditionStatuses[%q] = %v, expected %v", tt.status, result, tt.expectValid) + } + }) + } +} From 81ea64bb3783185977d175c4a7f29aeb818dc88c Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:27 +0100 Subject: [PATCH 07/13] HYPERFLEET-386 - feat: update OpenAPI spec for conditions-based status Breaking API changes: - Remove status.phase field from Cluster and NodePool - Add status.conditions[] array with Ready/Available conditions - ResourceCondition.status enum: True, False (not Unknown) - AdapterCondition.status enum: True, False, Unknown --- openapi/openapi.yaml | 230 ++++++++++++++++++++----------------------- 1 file changed, 108 insertions(+), 122 deletions(-) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 8028955..81624ba 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.0.0 info: title: HyperFleet API - version: 1.0.3 + version: 1.0.4 contact: name: HyperFleet Team license: @@ -49,8 +49,8 @@ paths: Create a new cluster resource. **Note**: The `status` object in the response is read-only and computed by the service. - It is NOT part of the request body. Initially, status.phase will be "NotReady" and - status.conditions will be empty until status conditions are POSTed. + It is NOT part of the request body. Initially, + status.conditions will include mandatory "Available" and "Ready" conditions. parameters: [] responses: '201': @@ -419,7 +419,7 @@ components: required: false description: |- Filter results using TSL (Tree Search Language) query syntax. - Examples: `status.phase='NotReady'`, `name in ('c1','c2')`, `labels.region='us-east'` + Examples: `status.conditions.Ready='True'`, `name in ('c1','c2')`, `labels.region='us-east'` schema: type: string explode: false @@ -428,14 +428,12 @@ components: type: object required: - type - - status - last_transition_time + - status properties: type: type: string description: Condition type - status: - $ref: '#/components/schemas/ConditionStatus' reason: type: string description: Machine-readable reason code @@ -447,12 +445,21 @@ components: format: date-time description: |- When this condition last transitioned status (API-managed) - Only updated when status changes (True/False/Unknown), not when reason/message changes + Only updated when status changes (True/False), not when reason/message changes + status: + $ref: '#/components/schemas/AdapterConditionStatus' description: |- Condition in AdapterStatus Used for standard Kubernetes condition types: "Available", "Applied", "Health" Note: observed_generation is at AdapterStatus level, not per-condition, since all conditions in one AdapterStatus share the same observed generation + AdapterConditionStatus: + type: string + enum: + - 'True' + - 'False' + - Unknown + description: Status value for adapter conditions AdapterStatus: type: object required: @@ -758,11 +765,23 @@ components: updated_time: '2021-01-01T00:00:00Z' generation: 1 status: - phase: Ready - last_transition_time: '2021-01-01T00:00:00Z' - observed_generation: 1 - last_updated_time: '2021-01-01T00:00:00Z' conditions: + - type: Ready + status: 'True' + reason: ResourceReady + message: All conditions successful for current spec generation + observed_generation: 1 + created_time: '2021-01-01T10:00:00Z' + last_updated_time: '2021-01-01T10:00:00Z' + last_transition_time: '2021-01-01T10:00:00Z' + - type: Available + status: 'True' + reason: ResourceAvailable + message: All conditions successful for observed_generation + observed_generation: 1 + created_time: '2021-01-01T10:00:00Z' + last_updated_time: '2021-01-01T10:00:00Z' + last_transition_time: '2021-01-01T10:00:00Z' - type: ValidationSuccessful status: 'True' reason: All validations passed @@ -849,47 +868,28 @@ components: ClusterStatus: type: object required: - - phase - - last_transition_time - - observed_generation - - last_updated_time - conditions properties: - phase: - $ref: '#/components/schemas/ResourcePhase' - last_transition_time: - type: string - format: date-time - description: |- - When cluster last transitioned (used by Sentinel for backoff) - Updated when conditions are reported if the phase changes - observed_generation: - type: integer - format: int32 - description: |- - Last generation processed - Updated when conditions are reported. - This will be the lowest value of each condition's observed_generation values - The phase value is based on this generation - last_updated_time: - type: string - format: date-time - description: |- - Time of the last update - Updated when conditions are reported. - Computed as min(conditions[].last_updated_time) to detect stale adapters. - Uses earliest (not latest) timestamp to ensure Sentinel can detect if any adapter has stopped reporting. conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' + minItems: 2 + description: |- + List of status conditions for the cluster. + + **Mandatory conditions**: + - `type: "Available"`: indicates if the cluster is accessible and functional. + - `type: "Ready"`: indicates if the cluster has reached its desired state. + + These conditions are present immediately upon resource creation. description: |- Cluster status computed from all status conditions. This object is computed by the service and CANNOT be modified directly. It is aggregated from condition updates posted to `/clusters/{id}/statuses`. - Provides quick overview of all reported conditions and aggregated phase. + Provides quick overview of all reported conditions. ConditionRequest: type: object required: @@ -899,7 +899,7 @@ components: type: type: string status: - $ref: '#/components/schemas/ConditionStatus' + $ref: '#/components/schemas/AdapterConditionStatus' reason: type: string message: @@ -907,16 +907,8 @@ components: description: |- Condition data for create/update requests (from adapters) observed_generation and observed_time are now at AdapterStatusCreateRequest level - ConditionStatus: - type: string - enum: - - 'True' - - 'False' - - Unknown - description: Status value for conditions Error: type: object - description: RFC 9457 Problem Details error format with HyperFleet extensions required: - type - title @@ -959,40 +951,10 @@ components: example: abc123def456 errors: type: array - description: Field-level validation errors (for validation failures) items: $ref: '#/components/schemas/ValidationError' - ValidationError: - type: object - description: Field-level validation error detail - required: - - field - - message - properties: - field: - type: string - description: JSON path to the field that failed validation - example: spec.name - value: - description: The invalid value that was provided (if safe to include) - constraint: - type: string - description: The validation constraint that was violated - enum: - - required - - min - - max - - min_length - - max_length - - pattern - - enum - - format - - unique - example: required - message: - type: string - description: Human-readable error message for this field - example: Cluster name is required + description: Field-level validation errors (for validation failures) + description: RFC 9457 Problem Details error format with HyperFleet extensions NodePool: type: object required: @@ -1068,11 +1030,23 @@ components: kind: Cluster href: https://api.hyperfleet.com/v1/clusters/cluster-123 status: - phase: Ready - last_transition_time: '2021-01-01T10:00:00Z' - observed_generation: 1 - last_updated_time: '2021-01-01T10:02:00Z' conditions: + - type: Ready + status: 'True' + reason: ResourceReady + message: All conditions successful for observed_generation + observed_generation: 1 + created_time: '2021-01-01T10:00:00Z' + last_updated_time: '2021-01-01T10:00:00Z' + last_transition_time: '2021-01-01T10:00:00Z' + - type: Available + status: 'True' + reason: ResourceAvailable + message: All conditions successful for current spec generation + observed_generation: 1 + created_time: '2021-01-01T10:00:00Z' + last_updated_time: '2021-01-01T10:00:00Z' + last_transition_time: '2021-01-01T10:00:00Z' - type: ValidationSuccessful status: 'True' reason: All validations passed @@ -1212,39 +1186,21 @@ components: NodePoolStatus: type: object required: - - phase - - observed_generation - - last_transition_time - - last_updated_time - conditions properties: - phase: - $ref: '#/components/schemas/ResourcePhase' - observed_generation: - type: integer - format: int32 - minimum: 1 - description: |- - Last generation processed - Updated when conditions are reported. - This will be the lowest value of each condition's observed_generation values - The phase value is based on this generation - last_transition_time: - type: string - format: date-time - description: When NodePool last transitioned (used by Sentinel for backoff) - last_updated_time: - type: string - format: date-time - description: |- - Time of the last update - Updated when conditions are reported. - Computed as min(conditions[].last_updated_time) to detect stale adapters. - Uses earliest (not latest) timestamp to ensure Sentinel can detect if any adapter has stopped reporting. conditions: type: array items: $ref: '#/components/schemas/ResourceCondition' + minItems: 2 + description: |- + List of status conditions for the nodepool. + + **Mandatory conditions**: + - `type: "Available"`: indicates if the nodepool is accessible and functional. + - `type: "Ready"`: indicates if the nodepool has reached its desired state. + + These conditions are present immediately upon resource creation. description: |- NodePool status computed from all status conditions. @@ -1270,8 +1226,8 @@ components: type: object required: - type - - status - last_transition_time + - status - observed_generation - created_time - last_updated_time @@ -1279,8 +1235,6 @@ components: type: type: string description: Condition type - status: - $ref: '#/components/schemas/ConditionStatus' reason: type: string description: Machine-readable reason code @@ -1292,7 +1246,9 @@ components: format: date-time description: |- When this condition last transitioned status (API-managed) - Only updated when status changes (True/False/Unknown), not when reason/message changes + Only updated when status changes (True/False), not when reason/message changes + status: + $ref: '#/components/schemas/ResourceConditionStatus' observed_generation: type: integer format: int32 @@ -1312,13 +1268,43 @@ components: Condition in Cluster/NodePool status Used for semantic condition types: "ValidationSuccessful", "DNSSuccessful", "NodePoolSuccessful", etc. Includes observed_generation and last_updated_time to track adapter-specific state - ResourcePhase: + ResourceConditionStatus: type: string enum: - - NotReady - - Ready - - Failed - description: Phase of a resource (Cluster or NodePool) + - 'True' + - 'False' + description: Status value for resource conditions + ValidationError: + type: object + required: + - field + - message + properties: + field: + type: string + description: JSON path to the field that failed validation + example: spec.name + value: + description: The invalid value that was provided (if safe to include) + constraint: + type: string + enum: + - required + - min + - max + - min_length + - max_length + - pattern + - enum + - format + - unique + description: The validation constraint that was violated + example: required + message: + type: string + description: Human-readable error message for this field + example: Cluster name is required + description: Field-level validation error detail securitySchemes: BearerAuth: type: http From 823ac7e5d0819bad3edb53d50c4a372da5f07780 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:37 +0100 Subject: [PATCH 08/13] HYPERFLEET-386 - refactor: remove phase validation from types Removes obsolete phase-related code: - Remove phase validation from cluster types - Remove phase validation from node pool types - Remove associated phase tests --- pkg/api/cluster_types.go | 11 ++--------- pkg/api/cluster_types_test.go | 30 ------------------------------ pkg/api/node_pool_types.go | 11 ++--------- pkg/api/node_pool_types_test.go | 32 -------------------------------- 4 files changed, 4 insertions(+), 80 deletions(-) diff --git a/pkg/api/cluster_types.go b/pkg/api/cluster_types.go index aa68980..8d734f7 100644 --- a/pkg/api/cluster_types.go +++ b/pkg/api/cluster_types.go @@ -21,12 +21,8 @@ type Cluster struct { // Version control Generation int32 `json:"generation" gorm:"default:1;not null"` - // Status fields (expanded to database columns) - StatusPhase string `json:"status_phase" gorm:"default:'NotReady'"` - StatusLastTransitionTime *time.Time `json:"status_last_transition_time,omitempty"` - StatusObservedGeneration int32 `json:"status_observed_generation" gorm:"default:0"` - StatusLastUpdatedTime *time.Time `json:"status_last_updated_time,omitempty"` - StatusConditions datatypes.JSON `json:"status_conditions" gorm:"type:jsonb"` + // Status (conditions-only model with synthetic Available/Ready conditions) + StatusConditions datatypes.JSON `json:"status_conditions" gorm:"type:jsonb"` // Audit fields CreatedBy string `json:"created_by" gorm:"size:255;not null"` @@ -52,9 +48,6 @@ func (c *Cluster) BeforeCreate(tx *gorm.DB) error { if c.Generation == 0 { c.Generation = 1 } - if c.StatusPhase == "" { - c.StatusPhase = string(PhaseNotReady) - } // Set Href if not already set if c.Href == "" { c.Href = "/api/hyperfleet/v1/clusters/" + c.ID diff --git a/pkg/api/cluster_types_test.go b/pkg/api/cluster_types_test.go index 6c46458..a5cbd8a 100644 --- a/pkg/api/cluster_types_test.go +++ b/pkg/api/cluster_types_test.go @@ -126,35 +126,6 @@ func TestCluster_BeforeCreate_GenerationPreserved(t *testing.T) { Expect(cluster.Generation).To(Equal(int32(5))) } -// TestCluster_BeforeCreate_StatusPhaseDefault tests StatusPhase default value -func TestCluster_BeforeCreate_StatusPhaseDefault(t *testing.T) { - RegisterTestingT(t) - - // Test default StatusPhase - cluster := &Cluster{ - Name: "test-cluster", - } - - err := cluster.BeforeCreate(nil) - Expect(err).To(BeNil()) - Expect(cluster.StatusPhase).To(Equal("NotReady")) -} - -// TestCluster_BeforeCreate_StatusPhasePreserved tests StatusPhase is not overwritten -func TestCluster_BeforeCreate_StatusPhasePreserved(t *testing.T) { - RegisterTestingT(t) - - // Test StatusPhase preservation - cluster := &Cluster{ - Name: "test-cluster", - StatusPhase: "Ready", - } - - err := cluster.BeforeCreate(nil) - Expect(err).To(BeNil()) - Expect(cluster.StatusPhase).To(Equal("Ready")) -} - // TestCluster_BeforeCreate_HrefGeneration tests Href auto-generation func TestCluster_BeforeCreate_HrefGeneration(t *testing.T) { RegisterTestingT(t) @@ -200,6 +171,5 @@ func TestCluster_BeforeCreate_Complete(t *testing.T) { Expect(cluster.ID).ToNot(BeEmpty()) Expect(cluster.Kind).To(Equal("Cluster")) // Kind is preserved, not auto-set Expect(cluster.Generation).To(Equal(int32(1))) - Expect(cluster.StatusPhase).To(Equal("NotReady")) Expect(cluster.Href).To(Equal("/api/hyperfleet/v1/clusters/" + cluster.ID)) } diff --git a/pkg/api/node_pool_types.go b/pkg/api/node_pool_types.go index 1f04bea..64af77d 100644 --- a/pkg/api/node_pool_types.go +++ b/pkg/api/node_pool_types.go @@ -30,12 +30,8 @@ type NodePool struct { // Foreign key relationship Cluster *Cluster `gorm:"foreignKey:OwnerID;references:ID"` - // Status fields (expanded) - StatusPhase string `json:"status_phase" gorm:"default:'NotReady'"` - StatusObservedGeneration int32 `json:"status_observed_generation" gorm:"default:0"` - StatusLastTransitionTime *time.Time `json:"status_last_transition_time,omitempty"` - StatusLastUpdatedTime *time.Time `json:"status_last_updated_time,omitempty"` - StatusConditions datatypes.JSON `json:"status_conditions" gorm:"type:jsonb"` + // Status (conditions-only model with synthetic Available/Ready conditions) + StatusConditions datatypes.JSON `json:"status_conditions" gorm:"type:jsonb"` // Audit fields CreatedBy string `json:"created_by" gorm:"size:255;not null"` @@ -64,9 +60,6 @@ func (np *NodePool) BeforeCreate(tx *gorm.DB) error { if np.OwnerKind == "" { np.OwnerKind = "Cluster" } - if np.StatusPhase == "" { - np.StatusPhase = string(PhaseNotReady) - } // Set Href if not already set if np.Href == "" { np.Href = fmt.Sprintf("/api/hyperfleet/v1/clusters/%s/nodepools/%s", np.OwnerID, np.ID) diff --git a/pkg/api/node_pool_types_test.go b/pkg/api/node_pool_types_test.go index 72b5bbb..97f23c4 100644 --- a/pkg/api/node_pool_types_test.go +++ b/pkg/api/node_pool_types_test.go @@ -131,37 +131,6 @@ func TestNodePool_BeforeCreate_OwnerKindPreserved(t *testing.T) { Expect(nodepool.OwnerKind).To(Equal("CustomOwner")) } -// TestNodePool_BeforeCreate_StatusPhaseDefault tests StatusPhase default value -func TestNodePool_BeforeCreate_StatusPhaseDefault(t *testing.T) { - RegisterTestingT(t) - - // Test default StatusPhase - nodepool := &NodePool{ - Name: "test-nodepool", - OwnerID: "cluster-123", - } - - err := nodepool.BeforeCreate(nil) - Expect(err).To(BeNil()) - Expect(nodepool.StatusPhase).To(Equal("NotReady")) -} - -// TestNodePool_BeforeCreate_StatusPhasePreserved tests StatusPhase is not overwritten -func TestNodePool_BeforeCreate_StatusPhasePreserved(t *testing.T) { - RegisterTestingT(t) - - // Test StatusPhase preservation - nodepool := &NodePool{ - Name: "test-nodepool", - OwnerID: "cluster-123", - StatusPhase: "Ready", - } - - err := nodepool.BeforeCreate(nil) - Expect(err).To(BeNil()) - Expect(nodepool.StatusPhase).To(Equal("Ready")) -} - // TestNodePool_BeforeCreate_HrefGeneration tests Href auto-generation func TestNodePool_BeforeCreate_HrefGeneration(t *testing.T) { RegisterTestingT(t) @@ -241,7 +210,6 @@ func TestNodePool_BeforeCreate_Complete(t *testing.T) { Expect(nodepool.ID).ToNot(BeEmpty()) Expect(nodepool.Kind).To(Equal("NodePool")) // Kind is preserved, not auto-set Expect(nodepool.OwnerKind).To(Equal("Cluster")) - Expect(nodepool.StatusPhase).To(Equal("NotReady")) Expect(nodepool.Href).To(Equal("/api/hyperfleet/v1/clusters/cluster-complete/nodepools/" + nodepool.ID)) Expect(nodepool.OwnerHref).To(Equal("/api/hyperfleet/v1/clusters/cluster-complete")) } From 5dd7445333812f5f32e0cd0cb5f1eaceec12b9df Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:46 +0100 Subject: [PATCH 09/13] HYPERFLEET-386 - test: update test factories for conditions Updates test helpers to use conditions instead of phase: - Cluster factory creates default Ready/Available conditions - NodePool factory creates default Ready/Available conditions --- test/factories/clusters.go | 53 ++++++++++++++++++++++++++-------- test/factories/node_pools.go | 55 +++++++++++++++++++++++++++--------- 2 files changed, 83 insertions(+), 25 deletions(-) diff --git a/test/factories/clusters.go b/test/factories/clusters.go index deeb107..583ccc9 100644 --- a/test/factories/clusters.go +++ b/test/factories/clusters.go @@ -59,23 +59,52 @@ func reloadCluster(dbSession *gorm.DB, cluster *api.Cluster) error { return dbSession.First(cluster, "id = ?", cluster.ID).Error } -// NewClusterWithStatus creates a cluster with specific status phase and last_updated_time +// NewClusterWithStatus creates a cluster with specific status conditions // dbFactory parameter is needed to update database fields -func NewClusterWithStatus(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time) (*api.Cluster, error) { +// The isAvailable and isReady parameters control which synthetic conditions are set +func NewClusterWithStatus(f *Factories, dbFactory db.SessionFactory, id string, isAvailable, isReady bool) (*api.Cluster, error) { cluster, err := f.NewCluster(id) if err != nil { return nil, err } - // Update database record with status fields - dbSession := dbFactory.New(context.Background()) - updates := map[string]interface{}{ - "status_phase": phase, + now := time.Now() + availableStatus := api.ConditionFalse + if isAvailable { + availableStatus = api.ConditionTrue + } + readyStatus := api.ConditionFalse + if isReady { + readyStatus = api.ConditionTrue } - if lastUpdatedTime != nil { - updates["status_last_updated_time"] = lastUpdatedTime + + conditions := []api.ResourceCondition{ + { + Type: "Available", + Status: availableStatus, + ObservedGeneration: cluster.Generation, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + }, + { + Type: "Ready", + Status: readyStatus, + ObservedGeneration: cluster.Generation, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + }, } - err = dbSession.Model(cluster).Updates(updates).Error + + conditionsJSON, err := json.Marshal(conditions) + if err != nil { + return nil, err + } + + // Update database record with status conditions + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(cluster).Update("status_conditions", conditionsJSON).Error if err != nil { return nil, err } @@ -113,9 +142,9 @@ func NewClusterWithLabels(f *Factories, dbFactory db.SessionFactory, id string, return cluster, nil } -// NewClusterWithStatusAndLabels creates a cluster with both status and labels -func NewClusterWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.Cluster, error) { - cluster, err := NewClusterWithStatus(f, dbFactory, id, phase, lastUpdatedTime) +// NewClusterWithStatusAndLabels creates a cluster with both status conditions and labels +func NewClusterWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, isAvailable, isReady bool, labels map[string]string) (*api.Cluster, error) { + cluster, err := NewClusterWithStatus(f, dbFactory, id, isAvailable, isReady) if err != nil { return nil, err } diff --git a/test/factories/node_pools.go b/test/factories/node_pools.go index c47e614..990de22 100644 --- a/test/factories/node_pools.go +++ b/test/factories/node_pools.go @@ -79,23 +79,52 @@ func reloadNodePool(dbSession *gorm.DB, nodePool *api.NodePool) error { return dbSession.First(nodePool, "id = ?", nodePool.ID).Error } -// NewNodePoolWithStatus creates a node pool with specific status phase and last_updated_time +// NewNodePoolWithStatus creates a node pool with specific status conditions // dbFactory parameter is needed to update database fields -func NewNodePoolWithStatus(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time) (*api.NodePool, error) { +// The isAvailable and isReady parameters control which synthetic conditions are set +func NewNodePoolWithStatus(f *Factories, dbFactory db.SessionFactory, id string, isAvailable, isReady bool) (*api.NodePool, error) { nodePool, err := f.NewNodePool(id) if err != nil { return nil, err } - // Update database record with status fields - dbSession := dbFactory.New(context.Background()) - updates := map[string]interface{}{ - "status_phase": phase, - } - if lastUpdatedTime != nil { - updates["status_last_updated_time"] = lastUpdatedTime + now := time.Now() + availableStatus := api.ConditionFalse + if isAvailable { + availableStatus = api.ConditionTrue + } + readyStatus := api.ConditionFalse + if isReady { + readyStatus = api.ConditionTrue + } + + conditions := []api.ResourceCondition{ + { + Type: "Available", + Status: availableStatus, + ObservedGeneration: nodePool.Generation, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + }, + { + Type: "Ready", + Status: readyStatus, + ObservedGeneration: nodePool.Generation, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + }, + } + + conditionsJSON, err := json.Marshal(conditions) + if err != nil { + return nil, err } - err = dbSession.Model(nodePool).Updates(updates).Error + + // Update database record with status conditions + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(nodePool).Update("status_conditions", conditionsJSON).Error if err != nil { return nil, err } @@ -133,9 +162,9 @@ func NewNodePoolWithLabels(f *Factories, dbFactory db.SessionFactory, id string, return nodePool, nil } -// NewNodePoolWithStatusAndLabels creates a node pool with both status and labels -func NewNodePoolWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.NodePool, error) { - nodePool, err := NewNodePoolWithStatus(f, dbFactory, id, phase, lastUpdatedTime) +// NewNodePoolWithStatusAndLabels creates a node pool with both status conditions and labels +func NewNodePoolWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, isAvailable, isReady bool, labels map[string]string) (*api.NodePool, error) { + nodePool, err := NewNodePoolWithStatus(f, dbFactory, id, isAvailable, isReady) if err != nil { return nil, err } From d7cac7fa0bf5b132d2a092a7538fb1acd4845d60 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:06:57 +0100 Subject: [PATCH 10/13] HYPERFLEET-386 - test: add integration tests for conditions End-to-end test coverage: - Tests for condition updates via adapter status POST - Updated contract tests for new response format - Tests for status.conditions.* search syntax --- test/integration/adapter_status_test.go | 142 ++++++- test/integration/api_contract_test.go | 32 +- test/integration/search_field_mapping_test.go | 346 ++++++++++-------- 3 files changed, 335 insertions(+), 185 deletions(-) diff --git a/test/integration/adapter_status_test.go b/test/integration/adapter_status_test.go index 4997f28..dff37a7 100644 --- a/test/integration/adapter_status_test.go +++ b/test/integration/adapter_status_test.go @@ -45,7 +45,7 @@ func TestClusterStatusPost(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("AdapterReady"), }, }, @@ -80,7 +80,7 @@ func TestClusterStatusGet(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, }, }, nil, @@ -121,7 +121,7 @@ func TestNodePoolStatusPost(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.False, + Status: openapi.AdapterConditionStatusFalse, Reason: util.PtrString("Initializing"), }, }, @@ -156,7 +156,7 @@ func TestNodePoolStatusGet(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, }, }, nil, @@ -193,7 +193,7 @@ func TestAdapterStatusPaging(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, }, }, nil, @@ -237,7 +237,7 @@ func TestAdapterStatusIdempotency(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.False, + Status: openapi.AdapterConditionStatusFalse, Reason: util.PtrString("Initializing"), }, }, @@ -249,7 +249,7 @@ func TestAdapterStatusIdempotency(t *testing.T) { Expect(resp1.StatusCode()).To(Equal(http.StatusCreated)) Expect(resp1.JSON201).NotTo(BeNil()) Expect(resp1.JSON201.Adapter).To(Equal("idempotency-test-adapter")) - Expect(resp1.JSON201.Conditions[0].Status).To(Equal(openapi.False)) + Expect(resp1.JSON201.Conditions[0].Status).To(Equal(openapi.AdapterConditionStatusFalse)) // Second POST: Update the same adapter with different conditions data2 := map[string]interface{}{ @@ -261,7 +261,7 @@ func TestAdapterStatusIdempotency(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, Reason: util.PtrString("AdapterReady"), }, }, @@ -273,7 +273,7 @@ func TestAdapterStatusIdempotency(t *testing.T) { Expect(resp2.StatusCode()).To(Equal(http.StatusCreated)) Expect(resp2.JSON201).NotTo(BeNil()) Expect(resp2.JSON201.Adapter).To(Equal("idempotency-test-adapter")) - Expect(resp2.JSON201.Conditions[0].Status).To(Equal(openapi.True)) + Expect(resp2.JSON201.Conditions[0].Status).To(Equal(openapi.AdapterConditionStatusTrue)) // GET all statuses - should have only ONE status for "idempotency-test-adapter" listResp, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) @@ -292,7 +292,125 @@ func TestAdapterStatusIdempotency(t *testing.T) { // Verify: should have exactly ONE entry for this adapter (updated, not duplicated) Expect(adapterCount).To(Equal(1), "Adapter should be updated, not duplicated") - Expect(finalStatus.Conditions[0].Status).To(Equal(openapi.True), "Conditions should be updated to latest") + Expect(finalStatus.Conditions[0].Status).To(Equal(openapi.AdapterConditionStatusTrue), "Conditions should be updated to latest") +} + +// TestClusterStatusPost_UnknownReturns204 tests that posting Unknown Available status returns 204 No Content +func TestClusterStatusPost_UnknownReturns204(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Create an adapter status with Available=Unknown + statusInput := newAdapterStatusRequest( + "test-adapter-unknown", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: "Available", + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("StartupPending"), + }, + }, + nil, + ) + + resp, err := client.PostClusterStatusesWithResponse(ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), "Expected 204 No Content for Unknown status") + + // Verify the status was NOT stored + listResp, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(listResp.JSON200).NotTo(BeNil()) + + // Check that no adapter status with "test-adapter-unknown" exists + for _, s := range listResp.JSON200.Items { + Expect(s.Adapter).NotTo(Equal("test-adapter-unknown"), "Unknown status should not be stored") + } +} + +// TestNodePoolStatusPost_UnknownReturns204 tests that posting Unknown Available status returns 204 No Content +func TestNodePoolStatusPost_UnknownReturns204(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a nodepool (which also creates its parent cluster) + nodePool, err := h.Factories.NewNodePools(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Create an adapter status with Available=Unknown + statusInput := newAdapterStatusRequest( + "test-nodepool-adapter-unknown", + 1, + []openapi.ConditionRequest{ + { + Type: "Available", + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("StartupPending"), + }, + }, + nil, + ) + + resp, err := client.PostNodePoolStatusesWithResponse(ctx, nodePool.OwnerID, nodePool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred(), "Error posting nodepool status: %v", err) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), "Expected 204 No Content for Unknown status") + + // Verify the status was NOT stored + listResp, err := client.GetNodePoolsStatusesWithResponse(ctx, nodePool.OwnerID, nodePool.ID, nil, test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred()) + Expect(listResp.JSON200).NotTo(BeNil()) + + // Check that no adapter status with "test-nodepool-adapter-unknown" exists + for _, s := range listResp.JSON200.Items { + Expect(s.Adapter).NotTo(Equal("test-nodepool-adapter-unknown"), "Unknown status should not be stored") + } +} + +// TestClusterStatusPost_MultipleConditionsWithUnknownAvailable tests that Unknown Available is detected among multiple conditions +func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) { + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Create an adapter status with multiple conditions including Available=Unknown + statusInput := newAdapterStatusRequest( + "test-adapter-multi-unknown", + cluster.Generation, + []openapi.ConditionRequest{ + { + Type: "Ready", + Status: openapi.AdapterConditionStatusTrue, + }, + { + Type: "Available", + Status: openapi.AdapterConditionStatusUnknown, + Reason: util.PtrString("StartupPending"), + }, + { + Type: "Progressing", + Status: openapi.AdapterConditionStatusTrue, + }, + }, + nil, + ) + + resp, err := client.PostClusterStatusesWithResponse(ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx)) + Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), "Expected 204 No Content when Available=Unknown among multiple conditions") } // TestAdapterStatusPagingEdgeCases tests edge cases in pagination @@ -314,7 +432,7 @@ func TestAdapterStatusPagingEdgeCases(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, }, }, nil, @@ -356,7 +474,7 @@ func TestAdapterStatusPagingEdgeCases(t *testing.T) { []openapi.ConditionRequest{ { Type: "Ready", - Status: openapi.True, + Status: openapi.AdapterConditionStatusTrue, }, }, nil, diff --git a/test/integration/api_contract_test.go b/test/integration/api_contract_test.go index 8bf4f48..811c284 100644 --- a/test/integration/api_contract_test.go +++ b/test/integration/api_contract_test.go @@ -9,32 +9,20 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" ) -// TestAPIContract_ResourcePhaseConstants verifies that domain and OpenAPI constants are in sync -// This test ensures that pkg/api.ResourcePhase constants match the generated openapi.ResourcePhase constants -func TestAPIContract_ResourcePhaseConstants(t *testing.T) { - RegisterTestingT(t) - - // Verify domain constants match OpenAPI generated constants - // If OpenAPI spec changes, this test will fail and alert us to update domain constants - Expect(string(api.PhaseNotReady)).To(Equal(string(openapi.NotReady)), - "api.PhaseNotReady must match openapi.NotReady") - Expect(string(api.PhaseReady)).To(Equal(string(openapi.Ready)), - "api.PhaseReady must match openapi.Ready") - Expect(string(api.PhaseFailed)).To(Equal(string(openapi.Failed)), - "api.PhaseFailed must match openapi.Failed") -} - // TestAPIContract_ConditionStatusConstants verifies that domain and OpenAPI constants are in sync -// This test ensures that pkg/api.ConditionStatus constants match the generated openapi.ConditionStatus constants +// This test ensures that pkg/api.ConditionStatus constants match the generated openapi condition status constants func TestAPIContract_ConditionStatusConstants(t *testing.T) { RegisterTestingT(t) // Verify domain constants match OpenAPI generated constants // If OpenAPI spec changes, this test will fail and alert us to update domain constants - Expect(string(api.ConditionTrue)).To(Equal(string(openapi.True)), - "api.ConditionTrue must match openapi.True") - Expect(string(api.ConditionFalse)).To(Equal(string(openapi.False)), - "api.ConditionFalse must match openapi.False") - Expect(string(api.ConditionUnknown)).To(Equal(string(openapi.Unknown)), - "api.ConditionUnknown must match openapi.Unknown") + // Both AdapterConditionStatus and ResourceConditionStatus use the same values + Expect(string(api.ConditionTrue)).To(Equal(string(openapi.AdapterConditionStatusTrue)), + "api.ConditionTrue must match openapi.AdapterConditionStatusTrue") + Expect(string(api.ConditionFalse)).To(Equal(string(openapi.AdapterConditionStatusFalse)), + "api.ConditionFalse must match openapi.AdapterConditionStatusFalse") + Expect(string(api.ConditionTrue)).To(Equal(string(openapi.ResourceConditionStatusTrue)), + "api.ConditionTrue must match openapi.ResourceConditionStatusTrue") + Expect(string(api.ConditionFalse)).To(Equal(string(openapi.ResourceConditionStatusFalse)), + "api.ConditionFalse must match openapi.ResourceConditionStatusFalse") } diff --git a/test/integration/search_field_mapping_test.go b/test/integration/search_field_mapping_test.go index 04fe7de..88d67dd 100644 --- a/test/integration/search_field_mapping_test.go +++ b/test/integration/search_field_mapping_test.go @@ -1,10 +1,8 @@ package integration import ( - "fmt" "net/http" "testing" - "time" . "github.com/onsi/gomega" @@ -13,103 +11,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/test/factories" ) -// TestSearchStatusPhaseMapping verifies that status.phase user-friendly syntax -// correctly maps to status_phase database field -func TestSearchStatusPhaseMapping(t *testing.T) { - RegisterTestingT(t) - h, client := test.RegisterIntegration(t) - - account := h.NewRandAccount() - ctx := h.NewAuthenticatedContext(account) - - // Create NotReady cluster using new factory method - notReadyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "NotReady", nil) - Expect(err).NotTo(HaveOccurred()) - - // Create Ready cluster - readyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", nil) - Expect(err).NotTo(HaveOccurred()) - - // Query NotReady clusters using user-friendly syntax - searchStr := "status.phase='NotReady'" - search := openapi.SearchParams(searchStr) - params := &openapi.GetClustersParams{ - Search: &search, - } - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusOK)) - list := resp.JSON200 - Expect(list).NotTo(BeNil()) - Expect(list.Total).To(BeNumerically(">=", 1)) - - // Verify all returned clusters are NotReady - foundNotReady := false - for _, item := range list.Items { - if *item.Id == notReadyCluster.ID { - foundNotReady = true - // Status field structure depends on openapi.yaml - // Assuming status.phase exists - Expect(item.Status.Phase).To(Equal(openapi.NotReady)) - } - // Should not contain readyCluster - Expect(*item.Id).NotTo(Equal(readyCluster.ID)) - } - Expect(foundNotReady).To(BeTrue(), "Expected to find the NotReady cluster") -} - -// TestSearchStatusLastUpdatedTimeMapping verifies that status.last_updated_time -// user-friendly syntax correctly maps to status_last_updated_time database field -// and time comparison works correctly -func TestSearchStatusLastUpdatedTimeMapping(t *testing.T) { - RegisterTestingT(t) - h, client := test.RegisterIntegration(t) - - account := h.NewRandAccount() - ctx := h.NewAuthenticatedContext(account) - - now := time.Now() - oldTime := now.Add(-2 * time.Hour) - recentTime := now.Add(-30 * time.Minute) - - // Create old cluster (2 hours ago) - oldCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", &oldTime) - Expect(err).NotTo(HaveOccurred()) - - // Create recent cluster (30 minutes ago) - recentCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", &recentTime) - Expect(err).NotTo(HaveOccurred()) - - // Query clusters updated before 1 hour ago - threshold := now.Add(-1 * time.Hour) - searchStr := fmt.Sprintf("status.last_updated_time < '%s'", threshold.Format(time.RFC3339)) - search := openapi.SearchParams(searchStr) - params := &openapi.GetClustersParams{ - Search: &search, - } - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusOK)) - list := resp.JSON200 - Expect(list).NotTo(BeNil()) - - // Should return at least oldCluster - Expect(list.Total).To(BeNumerically(">=", 1)) - - // Verify oldCluster is in results but recentCluster is not - foundOld := false - for _, item := range list.Items { - if *item.Id == oldCluster.ID { - foundOld = true - } - // Should not contain recentCluster (updated 30 mins ago) - Expect(*item.Id).NotTo(Equal(recentCluster.ID)) - } - Expect(foundOld).To(BeTrue(), "Expected to find the old cluster") -} - // TestSearchLabelsMapping verifies that labels.xxx user-friendly syntax // correctly maps to JSONB query labels->>'xxx' func TestSearchLabelsMapping(t *testing.T) { @@ -193,13 +94,13 @@ func TestSearchCombinedQuery(t *testing.T) { account := h.NewRandAccount() ctx := h.NewAuthenticatedContext(account) - // Create cluster with NotReady status and us-east region + // Create cluster with NotReady status (Available=False, Ready=False) and us-east region matchCluster, err := factories.NewClusterWithStatusAndLabels( &h.Factories, h.DBFactory, h.NewID(), - "NotReady", - nil, + false, // isAvailable + false, // isReady map[string]string{"region": "us-east"}, ) Expect(err).NotTo(HaveOccurred()) @@ -209,25 +110,25 @@ func TestSearchCombinedQuery(t *testing.T) { &h.Factories, h.DBFactory, h.NewID(), - "NotReady", - nil, + false, // isAvailable + false, // isReady map[string]string{"region": "us-west"}, ) Expect(err).NotTo(HaveOccurred()) - // Create cluster with Ready status and us-east region - wrongStatusCluster, err := factories.NewClusterWithStatusAndLabels( + // Create cluster with Ready status (Available=True, Ready=True) and us-east region + _, err = factories.NewClusterWithStatusAndLabels( &h.Factories, h.DBFactory, h.NewID(), - "Ready", - nil, + true, // isAvailable + true, // isReady map[string]string{"region": "us-east"}, ) Expect(err).NotTo(HaveOccurred()) - // Query using combined AND condition - searchStr := "status.phase='NotReady' and labels.region='us-east'" + // Query using combined AND condition with labels (labels search still works) + searchStr := "labels.region='us-east'" search := openapi.SearchParams(searchStr) params := &openapi.GetClustersParams{ Search: &search, @@ -240,44 +141,82 @@ func TestSearchCombinedQuery(t *testing.T) { Expect(list).NotTo(BeNil()) Expect(list.Total).To(BeNumerically(">=", 1)) - // Should only return matchCluster + // Should return matchCluster and wrongStatusCluster but not wrongRegionCluster foundMatch := false for _, item := range list.Items { if *item.Id == matchCluster.ID { foundMatch = true - Expect(item.Status.Phase).To(Equal(openapi.NotReady)) } - // Should not contain wrongRegionCluster or wrongStatusCluster + // Should not contain wrongRegionCluster Expect(*item.Id).NotTo(Equal(wrongRegionCluster.ID)) - Expect(*item.Id).NotTo(Equal(wrongStatusCluster.ID)) } Expect(foundMatch).To(BeTrue(), "Expected to find the matching cluster") } -// TestSearchNodePoolFieldMapping verifies that NodePool also supports -// the same field mapping as Cluster -func TestSearchNodePoolFieldMapping(t *testing.T) { +// TestSearchNodePoolLabelsMapping verifies that NodePool also supports +// the labels field mapping +func TestSearchNodePoolLabelsMapping(t *testing.T) { RegisterTestingT(t) h, client := test.RegisterIntegration(t) account := h.NewRandAccount() ctx := h.NewAuthenticatedContext(account) - // Create NotReady NodePool - notReadyNP, err := factories.NewNodePoolWithStatus(&h.Factories, h.DBFactory, h.NewID(), "NotReady", nil) + // Test labels mapping for NodePools + npWithLabels, err := factories.NewNodePoolWithLabels(&h.Factories, h.DBFactory, h.NewID(), map[string]string{ + "environment": "test", + }) + Expect(err).NotTo(HaveOccurred()) + + searchLabelsStr := "labels.environment='test'" + searchLabels := openapi.SearchParams(searchLabelsStr) + labelsParams := &openapi.GetNodePoolsParams{ + Search: &searchLabels, + } + labelsResp, labelsErr := client.GetNodePoolsWithResponse(ctx, labelsParams, test.WithAuthToken(ctx)) + + Expect(labelsErr).NotTo(HaveOccurred()) + Expect(labelsResp.StatusCode()).To(Equal(http.StatusOK)) + labelsList := labelsResp.JSON200 + Expect(labelsList).NotTo(BeNil()) + + foundLabeled := false + for _, item := range labelsList.Items { + if *item.Id == npWithLabels.ID { + foundLabeled = true + } + } + Expect(foundLabeled).To(BeTrue(), "Expected to find the labeled node pool") +} + +// TestSearchStatusConditionsMapping verifies that status.conditions.='' +// user-friendly syntax correctly maps to JSONB containment query +func TestSearchStatusConditionsMapping(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create cluster with Ready=True, Available=True + readyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), true, true) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with Ready=False, Available=True + notReadyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), true, false) Expect(err).NotTo(HaveOccurred()) - // Create Ready NodePool - readyNP, err := factories.NewNodePoolWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", nil) + // Create cluster with Ready=False, Available=False + notAvailableCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), false, false) Expect(err).NotTo(HaveOccurred()) - // Query NotReady NodePools using user-friendly syntax - searchStr := "status.phase='NotReady'" + // Search for Ready=True + searchStr := "status.conditions.Ready='True'" search := openapi.SearchParams(searchStr) - params := &openapi.GetNodePoolsParams{ + params := &openapi.GetClustersParams{ Search: &search, } - resp, err := client.GetNodePoolsWithResponse(ctx, params, test.WithAuthToken(ctx)) + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) Expect(err).NotTo(HaveOccurred()) Expect(resp.StatusCode()).To(Equal(http.StatusOK)) @@ -285,41 +224,146 @@ func TestSearchNodePoolFieldMapping(t *testing.T) { Expect(list).NotTo(BeNil()) Expect(list.Total).To(BeNumerically(">=", 1)) - // Verify NotReady NodePool is in results - foundNotReady := false + // Verify only readyCluster is returned + foundReady := false for _, item := range list.Items { - if *item.Id == notReadyNP.ID { - foundNotReady = true - Expect(item.Status.Phase).To(Equal(openapi.NotReady)) + if *item.Id == readyCluster.ID { + foundReady = true } - // Should not contain readyNP - Expect(*item.Id).NotTo(Equal(readyNP.ID)) + // Should not contain notReadyCluster or notAvailableCluster + Expect(*item.Id).NotTo(Equal(notReadyCluster.ID)) + Expect(*item.Id).NotTo(Equal(notAvailableCluster.ID)) } - Expect(foundNotReady).To(BeTrue(), "Expected to find the NotReady node pool") + Expect(foundReady).To(BeTrue(), "Expected to find the ready cluster") + + // Search for Available=True + searchAvailableStr := "status.conditions.Available='True'" + searchAvailable := openapi.SearchParams(searchAvailableStr) + availableParams := &openapi.GetClustersParams{ + Search: &searchAvailable, + } + availableResp, err := client.GetClustersWithResponse(ctx, availableParams, test.WithAuthToken(ctx)) - // Also test labels mapping for NodePools - npWithLabels, err := factories.NewNodePoolWithLabels(&h.Factories, h.DBFactory, h.NewID(), map[string]string{ - "environment": "test", - }) Expect(err).NotTo(HaveOccurred()) + Expect(availableResp.StatusCode()).To(Equal(http.StatusOK)) + availableList := availableResp.JSON200 + Expect(availableList).NotTo(BeNil()) + Expect(availableList.Total).To(BeNumerically(">=", 2)) + + // Should contain readyCluster and notReadyCluster (both have Available=True) + foundReadyInAvailable := false + foundNotReadyInAvailable := false + for _, item := range availableList.Items { + if *item.Id == readyCluster.ID { + foundReadyInAvailable = true + } + if *item.Id == notReadyCluster.ID { + foundNotReadyInAvailable = true + } + // Should not contain notAvailableCluster + Expect(*item.Id).NotTo(Equal(notAvailableCluster.ID)) + } + Expect(foundReadyInAvailable).To(BeTrue(), "Expected to find ready cluster in Available=True search") + Expect(foundNotReadyInAvailable).To(BeTrue(), "Expected to find notReady cluster in Available=True search") +} - searchLabelsStr := "labels.environment='test'" - searchLabels := openapi.SearchParams(searchLabelsStr) - labelsParams := &openapi.GetNodePoolsParams{ - Search: &searchLabels, +// TestSearchStatusConditionsCombinedWithLabels verifies that condition queries +// can be combined with label queries using AND +func TestSearchStatusConditionsCombinedWithLabels(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create cluster with Ready=True and region=us-east + matchCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + true, // isAvailable + true, // isReady + map[string]string{"region": "us-east"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with Ready=True but wrong region + wrongRegionCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + true, // isAvailable + true, // isReady + map[string]string{"region": "us-west"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with correct region but Ready=False + wrongStatusCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + true, // isAvailable + false, // isReady + map[string]string{"region": "us-east"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Search for Ready=True AND region=us-east + searchStr := "status.conditions.Ready='True' AND labels.region='us-east'" + search := openapi.SearchParams(searchStr) + params := &openapi.GetClustersParams{ + Search: &search, } - labelsResp, labelsErr := client.GetNodePoolsWithResponse(ctx, labelsParams, test.WithAuthToken(ctx)) + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - Expect(labelsErr).NotTo(HaveOccurred()) - Expect(labelsResp.StatusCode()).To(Equal(http.StatusOK)) - labelsList := labelsResp.JSON200 - Expect(labelsList).NotTo(BeNil()) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusOK)) + list := resp.JSON200 + Expect(list).NotTo(BeNil()) + Expect(list.Total).To(BeNumerically(">=", 1)) - foundLabeled := false - for _, item := range labelsList.Items { - if *item.Id == npWithLabels.ID { - foundLabeled = true + // Should only find matchCluster + foundMatch := false + for _, item := range list.Items { + if *item.Id == matchCluster.ID { + foundMatch = true } + // Should not contain wrongRegionCluster or wrongStatusCluster + Expect(*item.Id).NotTo(Equal(wrongRegionCluster.ID)) + Expect(*item.Id).NotTo(Equal(wrongStatusCluster.ID)) } - Expect(foundLabeled).To(BeTrue(), "Expected to find the labeled node pool") + Expect(foundMatch).To(BeTrue(), "Expected to find the matching cluster") +} + +// TestSearchStatusConditionsInvalidValues verifies that invalid condition values +// are rejected with 400 Bad Request +func TestSearchStatusConditionsInvalidValues(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Test invalid condition status + searchStr := "status.conditions.Ready='Invalid'" + search := openapi.SearchParams(searchStr) + params := &openapi.GetClustersParams{ + Search: &search, + } + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) + + // Test invalid condition type (lowercase) + searchInvalidType := "status.conditions.ready='True'" + searchInvalidTypeParam := openapi.SearchParams(searchInvalidType) + invalidTypeParams := &openapi.GetClustersParams{ + Search: &searchInvalidTypeParam, + } + invalidTypeResp, err := client.GetClustersWithResponse(ctx, invalidTypeParams, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + Expect(invalidTypeResp.StatusCode()).To(Equal(http.StatusBadRequest)) } From db6bb14f022d2ad8b961010df7c3cff2a1d44596 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Sun, 25 Jan 2026 11:07:06 +0100 Subject: [PATCH 11/13] HYPERFLEET-386 - docs: update documentation for conditions Documentation updates: - Updated API examples with condition-based status - Updated search syntax documentation - Documented condition semantics (Ready vs Available) - Updated agent configuration docs --- AGENTS.md | 16 +++-- docs/api-resources.md | 156 +++++++++++++++++++++++++++++++----------- 2 files changed, 128 insertions(+), 44 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dfbade4..00674de 100755 --- a/AGENTS.md +++ b/AGENTS.md @@ -254,13 +254,19 @@ The API calculates aggregate status from adapter-specific conditions: ``` **Aggregation Logic**: -- Phase is `Ready` if all adapters report `Ready=True` -- Phase is `Failed` if any adapter reports `Ready=False` -- Phase is `NotReady` otherwise (progressing, unknown, or missing conditions) -- `observed_generation` tracks which spec version the adapter has seen +The API synthesizes two top-level conditions from adapter reports: + +- **Available** condition: + - `True` if all required adapters report `Available=True` at any generation + - `observed_generation` is the minimum across all adapters + - Indicates the resource is running at some known good configuration + +- **Ready** condition: + - `True` if all required adapters report `Available=True` AND their `observed_generation` matches the current resource generation + - Indicates the resource is fully reconciled to the current spec **Why This Pattern**: -Kubernetes-style conditions allow multiple independent adapters to report status without coordination. The API simply aggregates these into a summary phase for client convenience. +Kubernetes-style conditions allow multiple independent adapters to report status without coordination. The API synthesizes `Available` and `Ready` conditions for clients to easily determine resource state. ## API Resources diff --git a/docs/api-resources.md b/docs/api-resources.md index fbf068d..5255391 100644 --- a/docs/api-resources.md +++ b/docs/api-resources.md @@ -19,6 +19,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses **POST** `/api/hyperfleet/v1/clusters` **Request Body:** + ```json { "kind": "Cluster", @@ -31,6 +32,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses ``` **Response (201 Created):** + ```json { "kind": "Cluster", @@ -47,22 +49,40 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses "created_by": "user@example.com", "updated_by": "user@example.com", "status": { - "phase": "NotReady", - "observed_generation": 0, - "last_transition_time": "2025-01-01T00:00:00Z", - "last_updated_time": "2025-01-01T00:00:00Z", - "conditions": [] + "conditions": [ + { + "type": "Available", + "status": "False", + "reason": "AwaitingAdapters", + "message": "Waiting for adapters to report status", + "observed_generation": 0, + "created_time": "2025-01-01T00:00:00Z", + "last_updated_time": "2025-01-01T00:00:00Z", + "last_transition_time": "2025-01-01T00:00:00Z" + }, + { + "type": "Ready", + "status": "False", + "reason": "AwaitingAdapters", + "message": "Waiting for adapters to report status", + "observed_generation": 0, + "created_time": "2025-01-01T00:00:00Z", + "last_updated_time": "2025-01-01T00:00:00Z", + "last_transition_time": "2025-01-01T00:00:00Z" + } + ] } } ``` -**Note**: Status is initially `NotReady` with empty conditions until adapters report status. +**Note**: Status initially has `Available=False` and `Ready=False` conditions until adapters report status. ### Get Cluster **GET** `/api/hyperfleet/v1/clusters/{cluster_id}` **Response (200 OK):** + ```json { "kind": "Cluster", @@ -79,26 +99,22 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses "created_by": "user@example.com", "updated_by": "user@example.com", "status": { - "phase": "Ready", - "observed_generation": 1, - "last_transition_time": "2025-01-01T00:00:00Z", - "last_updated_time": "2025-01-01T00:00:00Z", "conditions": [ { - "type": "ValidationSuccessful", + "type": "Available", "status": "True", - "reason": "AllValidationsPassed", - "message": "All validations passed", + "reason": "ResourceAvailable", + "message": "Cluster is accessible", "observed_generation": 1, "created_time": "2025-01-01T00:00:00Z", "last_updated_time": "2025-01-01T00:00:00Z", "last_transition_time": "2025-01-01T00:00:00Z" }, { - "type": "DNSSuccessful", + "type": "Ready", "status": "True", - "reason": "DNSProvisioned", - "message": "DNS successfully configured", + "reason": "ResourceReady", + "message": "All adapters report ready at current generation", "observed_generation": 1, "created_time": "2025-01-01T00:00:00Z", "last_updated_time": "2025-01-01T00:00:00Z", @@ -114,6 +130,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses **GET** `/api/hyperfleet/v1/clusters?page=1&pageSize=10` **Response (200 OK):** + ```json { "kind": "ClusterList", @@ -138,6 +155,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses Adapters use this endpoint to report their status. **Request Body:** + ```json { "adapter": "validator", @@ -171,6 +189,7 @@ Adapters use this endpoint to report their status. ``` **Response (201 Created):** + ```json { "adapter": "validator", @@ -209,11 +228,26 @@ Adapters use this endpoint to report their status. **Note**: The API automatically sets `created_time`, `last_report_time`, and `last_transition_time` fields. -### Status Phases +### Status Conditions + +The status uses Kubernetes-style conditions instead of a single phase field: + +- **Ready** - Whether all adapters report successfully at the current generation + - `True`: All required adapters report `Available=True` at current spec generation + - `False`: One or more adapters report Available=False at current generation + - After every spec change, `Ready` becomes `False` since adapters take some time to report at current spec generation + - Default value when creating the cluster, when no adapters have reported yet any value + +- **Available** - Aggregated adapter result for a common `observed_generation` + - `True`: All required adapters report Available=True for the same observed_generation + - `False`: At least one adapter reports Available=False when all adapters report the same observed_generation + - Default value when creating the cluster, when no adapters have reported yet any value -- `NotReady` - Cluster is being provisioned or has failing conditions -- `Ready` - All adapter conditions report success -- `Failed` - Cluster provisioning or operation failed +`Available` keeps its value unchanged in case adapters report from a different `observed_generation` or there is already a mix of `observed_generation` statuses + +- e.g. `Available=True` for `observed_generation==1` + - One adapter reports `Available=False` for `observed_generation=1` `Available` transitions to `False` + - One adapter reports `Available=False` for `observed_generation=2` `Available` keeps its `True` status ## NodePool Management @@ -233,6 +267,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses **POST** `/api/hyperfleet/v1/clusters/{cluster_id}/nodepools` **Request Body:** + ```json { "kind": "NodePool", @@ -245,6 +280,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses ``` **Response (201 Created):** + ```json { "kind": "NodePool", @@ -265,11 +301,28 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses "created_by": "user@example.com", "updated_by": "user@example.com", "status": { - "phase": "NotReady", - "observed_generation": 0, - "last_transition_time": "2025-01-01T00:00:00Z", - "last_updated_time": "2025-01-01T00:00:00Z", - "conditions": [] + "conditions": [ + { + "type": "Available", + "status": "False", + "reason": "AwaitingAdapters", + "message": "Waiting for adapters to report status", + "observed_generation": 0, + "created_time": "2025-01-01T00:00:00Z", + "last_updated_time": "2025-01-01T00:00:00Z", + "last_transition_time": "2025-01-01T00:00:00Z" + }, + { + "type": "Ready", + "status": "False", + "reason": "AwaitingAdapters", + "message": "Waiting for adapters to report status", + "observed_generation": 0, + "created_time": "2025-01-01T00:00:00Z", + "last_updated_time": "2025-01-01T00:00:00Z", + "last_transition_time": "2025-01-01T00:00:00Z" + } + ] } } ``` @@ -279,6 +332,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses **GET** `/api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}` **Response (200 OK):** + ```json { "kind": "NodePool", @@ -299,11 +353,22 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses "created_by": "user@example.com", "updated_by": "user@example.com", "status": { - "phase": "Ready", - "observed_generation": 1, - "last_transition_time": "2025-01-01T00:00:00Z", - "last_updated_time": "2025-01-01T00:00:00Z", - "conditions": [...] + "conditions": [ + { + "type": "Available", + "status": "True", + "reason": "ResourceAvailable", + "message": "NodePool is accessible", + "observed_generation": 1 + }, + { + "type": "Ready", + "status": "True", + "reason": "ResourceReady", + "message": "All adapters report ready at current generation", + "observed_generation": 1 + } + ] } } ``` @@ -325,10 +390,12 @@ GET /api/hyperfleet/v1/clusters?page=1&pageSize=10 ``` **Parameters:** + - `page` - Page number (default: 1) - `pageSize` - Items per page (default: 100) **Response:** + ```json { "kind": "ClusterList", @@ -348,21 +415,29 @@ Search using TSL (Tree Search Language) query syntax: curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ --data-urlencode "search=name='my-cluster'" -# AND query +# AND query with condition-based status curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ - --data-urlencode "search=status.phase='Ready' and labels.environment='production'" + --data-urlencode "search=status.conditions.Ready='True' and labels.environment='production'" # OR query curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ --data-urlencode "search=labels.environment='dev' or labels.environment='staging'" + +# Query for available resources +curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=status.conditions.Available='True'" ``` **Supported fields:** + - `name` - Resource name -- `status.phase` - Status phase (NotReady, Ready, Failed) +- `status.conditions.` - Condition status (True, False). Examples: + - `status.conditions.Ready='True'` - Resources that are ready + - `status.conditions.Available='True'` - Resources that are available - `labels.` - Label values **Supported operators:** + - `=` - Equality - `in` - In list - `and` - Logical AND @@ -386,21 +461,24 @@ curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ ### Status Fields -- `phase` - Current resource phase (NotReady, Ready, Failed) -- `observed_generation` - Last spec generation processed (min across all adapters) -- `last_transition_time` - When phase last changed -- `last_updated_time` - Min of all adapter last_report_time (detects stale adapters) -- `conditions` - Array of resource conditions from adapters +The status object contains synthesized conditions computed from adapter reports: + +- `conditions` - Array of resource conditions, including: + - **Available** - Whether resource is running at any known good configuration + - **Ready** - Whether all adapters have processed current spec generation + - Additional conditions from adapters (with `observed_generation`, timestamps) ### Condition Fields **In AdapterStatus POST request (ConditionRequest):** + - `type` - Condition type (Available, Applied, Health) -- `status` - Condition status (True, False, Unknown) +- `status` - Condition status (True, False) - `reason` - Machine-readable reason code - `message` - Human-readable message **In Cluster/NodePool status (ResourceCondition):** + - All above fields plus: - `observed_generation` - Generation this condition reflects - `created_time` - When condition was first created (API-managed) From fb4cd46627a30c49798f2fb463ffefc4af9e1c05 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Wed, 28 Jan 2026 14:20:15 +0100 Subject: [PATCH 12/13] HYPERFLEET-386 - doc: change openapi examples --- openapi/openapi.yaml | 82 ++++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 81624ba..46f47e7 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -521,23 +521,23 @@ components: AdapterStatus represents the complete status report from an adapter Contains multiple conditions, job metadata, and adapter-specific data example: - adapter: validator + adapter: adapter1 observed_generation: 1 conditions: - type: Available status: 'True' - reason: All validations passed - message: All 30 validation tests passed + reason: This adapter1 is available + message: This adapter1 is available last_transition_time: '2021-01-01T10:00:00Z' - type: Applied status: 'True' reason: Validation job applied - message: Validation job applied successfully + message: Adapter1 validation job applied successfully last_transition_time: '2021-01-01T10:00:00Z' - type: Health status: 'True' - reason: All adapter operations completed successfully - message: All adapter runtime operations completed successfully + reason: All adapter1 operations completed successfully + message: All adapter1 runtime operations completed successfully last_transition_time: '2021-01-01T10:00:00Z' metadata: job_name: validator-job-abc123 @@ -609,16 +609,16 @@ components: conditions: - type: Available status: 'True' - reason: All validations passed - message: All 30 validation tests passed + reason: This adapter1 is available + message: This adapter1 is available - type: Applied status: 'True' reason: Validation job applied - message: Validation job applied successfully + message: Adapter1 validation job applied successfully - type: Health status: 'True' - reason: All adapter operations completed successfully - message: All adapter runtime operations completed successfully + reason: All adapter1 operations completed successfully + message: All adapter1 runtime operations completed successfully metadata: job_name: validator-job-abc123 job_namespace: hyperfleet-system @@ -662,26 +662,26 @@ components: size: 2 total: 2 items: - - adapter: validator + - adapter: adapter1 observed_generation: 1 conditions: - type: Available status: 'True' - reason: All validations passed - message: All 30 validation tests passed + reason: This adapter1 is available + message: This adapter1 is available last_transition_time: '2021-01-01T10:00:00Z' metadata: job_name: validator-job-abc123 duration: 2m created_time: '2021-01-01T10:00:00Z' last_report_time: '2021-01-01T10:02:00Z' - - adapter: dns + - adapter: adapter2 observed_generation: 1 conditions: - type: Available status: 'True' - reason: DNS configured - message: DNS records created + reason: This adapter2 is available + message: This adapter2 is available last_transition_time: '2021-01-01T10:01:00Z' created_time: '2021-01-01T10:01:00Z' last_report_time: '2021-01-01T10:01:30Z' @@ -768,32 +768,32 @@ components: conditions: - type: Ready status: 'True' - reason: ResourceReady - message: All conditions successful for current spec generation + reason: All adapters reported Ready True for the current generation + message: All adapters reported Ready True for the current generation observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - type: Available status: 'True' - reason: ResourceAvailable - message: All conditions successful for observed_generation + reason: All adapters reported Available True for the same generation + message: All adapters reported Available True for the same generation observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - - type: ValidationSuccessful + - type: Adapter1Successful status: 'True' - reason: All validations passed - message: All 30 validation tests passed + reason: This adapter1 is available + message: This adapter1 is available observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - - type: DNSSuccessful + - type: Adapter2Successful status: 'True' - reason: DNS configured - message: DNS records created for custom.domain.com + reason: This adapter2 is available + message: This adapter2 is available observed_generation: 1 created_time: '2021-01-01T10:01:00Z' last_updated_time: '2021-01-01T10:01:00Z' @@ -879,8 +879,8 @@ components: List of status conditions for the cluster. **Mandatory conditions**: - - `type: "Available"`: indicates if the cluster is accessible and functional. - - `type: "Ready"`: indicates if the cluster has reached its desired state. + - `type: "Ready"`: Whether all adapters report successfully at the current generation. + - `type: "Available"`: Aggregated adapter result for a common observed_generation. These conditions are present immediately upon resource creation. description: |- @@ -1033,32 +1033,32 @@ components: conditions: - type: Ready status: 'True' - reason: ResourceReady - message: All conditions successful for observed_generation + reason: All adapters reported Ready True for the current generation + message: All adapters reported Ready True for the current generation observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - type: Available status: 'True' - reason: ResourceAvailable - message: All conditions successful for current spec generation + reason: All adapters reported Available True for the same generation + message: All adapters reported Available True for the same generation observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - - type: ValidationSuccessful + - type: Adapter1Successful status: 'True' - reason: All validations passed - message: NodePool validation passed + reason: This adapter1 is available + message: This adapter1 is available observed_generation: 1 created_time: '2021-01-01T10:00:00Z' last_updated_time: '2021-01-01T10:00:00Z' last_transition_time: '2021-01-01T10:00:00Z' - - type: NodePoolSuccessful + - type: Adapter2Successful status: 'True' - reason: NodePool provisioned successfully - message: NodePool has been scaled to desired count + reason: This adapter2 is available + message: This adapter2 is available observed_generation: 1 created_time: '2021-01-01T10:01:00Z' last_updated_time: '2021-01-01T10:01:00Z' @@ -1197,8 +1197,8 @@ components: List of status conditions for the nodepool. **Mandatory conditions**: - - `type: "Available"`: indicates if the nodepool is accessible and functional. - - `type: "Ready"`: indicates if the nodepool has reached its desired state. + - `type: "Ready"`: Whether all adapters report successfully at the current generation. + - `type: "Available"`: Aggregated adapter result for a common observed_generation. These conditions are present immediately upon resource creation. description: |- From 2e310669c48446384fae13929f92804cdd3afe15 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Wed, 28 Jan 2026 14:23:59 +0100 Subject: [PATCH 13/13] HYPERFLEET-386 - doc: change comment --- pkg/services/node_pool_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index 24e0527..21d6e6c 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -324,11 +324,11 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { nonAvailableConds := []api.AdapterCondition{{Type: "Health", Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}} nonAvailableJSON, _ := json.Marshal(nonAvailableConds) nonAvailableStatus := &api.AdapterStatus{ - ResourceType: "NodePool", - ResourceID: nodePoolID, - Adapter: "hypershift", + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "hypershift", ObservedGeneration: 2, - Conditions: nonAvailableJSON, + Conditions: nonAvailableJSON, } result, svcErr := service.ProcessAdapterStatus(ctx, nodePoolID, nonAvailableStatus) Expect(svcErr).To(BeNil()) @@ -419,7 +419,7 @@ func TestNodePoolStaleAdapterStatusUpdatePolicy(t *testing.T) { Expect(available.Status).To(Equal(api.ConditionTrue)) Expect(available.ObservedGeneration).To(Equal(int32(2))) - // Stale False is more restrictive and should override. + // Stale False is more restrictive and should override but we do not override newer generation responses upsert("validation", api.AdapterConditionFalse, 1) available = getAvailable() Expect(available.Status).To(Equal(api.ConditionTrue))