From 7ce9b160eb23d869fda5756f91ed223eba528876 Mon Sep 17 00:00:00 2001 From: Angel Marin Date: Wed, 11 Mar 2026 17:31:02 +0100 Subject: [PATCH] HYPERFLEET-774 - feat: update aggregation logic --- docs/api-resources.md | 105 ++- pkg/dao/adapter_status.go | 58 +- pkg/services/CLAUDE.md | 6 +- pkg/services/adapter_status.go | 4 +- pkg/services/cluster.go | 182 ++--- pkg/services/cluster_test.go | 122 +++- pkg/services/node_pool.go | 175 ++--- pkg/services/node_pool_test.go | 132 +++- pkg/services/process_adapter_status.go | 154 +++++ pkg/services/status_aggregation.go | 585 ++++++++++++---- pkg/services/status_aggregation_test.go | 847 ++++++++++++++++++++++++ test/integration/adapter_status_test.go | 77 +-- 12 files changed, 1932 insertions(+), 515 deletions(-) create mode 100644 pkg/services/process_adapter_status.go diff --git a/docs/api-resources.md b/docs/api-resources.md index fa83ebf..4ab3b99 100644 --- a/docs/api-resources.md +++ b/docs/api-resources.md @@ -55,7 +55,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses "status": "False", "reason": "AwaitingAdapters", "message": "Waiting for adapters to report status", - "observed_generation": 0, + "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" @@ -65,7 +65,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/statuses "status": "False", "reason": "AwaitingAdapters", "message": "Waiting for adapters to report status", - "observed_generation": 0, + "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" @@ -249,6 +249,101 @@ The status uses Kubernetes-style conditions instead of a single phase field: - 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 +### Aggregation logic + +Description of the aggregation logic for the resource status conditions + +- An API that stores resources entities (clusters, nodepools) +- A sentinel that polls the API for changes and triggers messages +- Instances of "adapters": + - Read the messages + - Reconcile the state with the world + - Report back to the API, using statuses "conditions" + +Resources keep track of its status, which is affected by the reports from adapters + +- Each resource keeps a `generation` property that gets increased on every change +- Adapters associated with a resource, report their state as an array of adapter conditions + - Three of these conditions are always mandatory : `Available`, `Applied`, `Health` + - If one of the mandatory conditions is missing, the report is discarded + - A `observed_generation` field indicating the generation associated with the report + - `observed_time` for when the adapter work was done + - If the reported `observed_generation` is lower than the already stored `observed_generation` for that adapter, the report is discarded +- Each resource has a list of associated "adapters" used to compute the aggregated status.conditions +- Each resource "status.conditions" is array property composed of: + - The `Available` condition of each adapter, named as `Successful` + - 2 aggregated conditions: `Ready` and `Available` computed from the array of `Available` resource statuses conditions + - Only `Available` condition from adapters is used to compute aggregated conditions + +The whole API spec is at: + +The aggregation logic for a resource (cluster/nodepool) works as follows. + +**Notation:** + +- `X` = report's `observed_generation` +- `G` = resource's current `generation` +- `statuses[]` = all stored adapter condition reports +- `lut` = adapter's `last_report_time` +- `ltt` = `last_transition_time` +- `obs_gen` = `observed_generation` +- `obs_time` = report's `observed_time` +- `—` = no change + +--- + +#### Discard / Reject Rules + +Checked before any aggregation. A discarded or rejected report causes no state change. + +| Rule | Condition | Outcome | +|---|---|---| +| `obs_gen` too high | report `observed_generation` > resource `generation` | Discarded | +| Stale adapter report | report `observed_generation` < adapter's stored `observed_generation` | Discarded | +| Missing mandatory conditions | Missing any of `Available`, `Applied`, `Health`, or value not in `{True, False, Unknown}` | Discarded | +| Available=Unknown | Report is valid but `Available=Unknown` | Discarded | + +--- + +#### Lifecycle Events + +| Event | Condition | Target | → status | → obs_gen | → lut | → ltt | +|---|---|---|---|---|---|---| +| Creation | — | `Ready` | `False` | `1` | `now` | `now` | +| Creation | — | `Available` | `False` | `1` | `now` | `now` | +| Change (→G) | Was `Ready=True` | `Ready` | `False` | `G` | `now` | `now` | +| Change (→G) | Was `Ready=False` | `Ready` | `False` | `G` | `now` | `—` | +| Change (→G) | — | `Available` | unchanged | unchanged | `—` | `—` | + +--- + +#### Adapter Report Aggregation Matrix + +The **Ready** check and **Available** check are independent — both can apply to the same incoming report. + +##### Report `Available=True` (obs_gen = X) + +| Target | Current State | Required Condition | → status | → lut | → ltt | → obs_gen | +|---|---|---|---|---|---|---| +| `Ready` | `Ready=True` | `X==G` AND all `statuses[].obs_gen==G` AND all `statuses[].status==True` | unchanged | `min(adapter last_report_time)` | `—` | `—` | +| `Ready` | `Ready=False` | `X==G` AND all `statuses[].obs_gen==G` AND all `statuses[].status==True` | **`True`** | `min(adapter last_report_time)` | `obs_time` | `—` | +| `Ready` | `Ready=False` | Any required adapter has no stored status | `—` | `now` | `—` | `—` | +| `Ready` | any | Conditions above not met | `—` | `—` | `—` | `—` | +| `Available` | `Available=False` | all `statuses[].obs_gen==X` | **`True`** | `min(adapter last_report_time)` | `obs_time` | `X` | +| `Available` | `Available=True` | all `statuses[].obs_gen==X` | unchanged | `min(adapter last_report_time)` | `—` | `X` | +| `Available` | any | Conditions above not met | `—` | `—` | `—` | `—` | + +##### Report `Available=False` (obs_gen = X) + +| Target | Current State | Required Condition | → status | → lut | → ltt | → obs_gen | +|---|---|---|---|---|---|---| +| `Ready` | `Ready=False` | `X==G` | unchanged | `min(adapter last_report_time)` | `—` | `—` | +| `Ready` | `Ready=True` | `X==G` | **`False`** | `obs_time` | `obs_time` | `—` | +| `Ready` | any | Conditions above not met | `—` | `—` | `—` | `—` | +| `Available` | `Available=False` | all `statuses[].obs_gen==X` | unchanged | `min(adapter last_report_time)` | `—` | `X` | +| `Available` | `Available=True` | all `statuses[].obs_gen==X` | **`False`** | `obs_time` | `obs_time` | `X` | +| `Available` | any | Conditions above not met | `—` | `—` | `—` | `—` | + ## NodePool Management ### Endpoints @@ -307,7 +402,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses "status": "False", "reason": "AwaitingAdapters", "message": "Waiting for adapters to report status", - "observed_generation": 0, + "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" @@ -317,7 +412,7 @@ POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses "status": "False", "reason": "AwaitingAdapters", "message": "Waiting for adapters to report status", - "observed_generation": 0, + "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" @@ -456,7 +551,7 @@ The status object contains synthesized conditions computed from adapter reports: - All above fields plus: - `observed_generation` - Generation this condition reflects - `created_time` - When condition was first created (API-managed) -- `last_updated_time` - When adapter last reported (API-managed, from AdapterStatus.last_report_time) +- `last_updated_time` - When this condition was last refreshed (API-managed). For **Available**, always the evaluation time. For **Ready**: when Ready=True, the minimum of `last_report_time` across all required adapters that report Available=True at the current generation; when Ready=False, the evaluation time (so consumers can detect staleness). - `last_transition_time` - When status last changed (API-managed) ## Parameter Restrictions diff --git a/pkg/dao/adapter_status.go b/pkg/dao/adapter_status.go index cdb56f3..6cbb85a 100644 --- a/pkg/dao/adapter_status.go +++ b/pkg/dao/adapter_status.go @@ -19,7 +19,10 @@ type AdapterStatusDao interface { Get(ctx context.Context, id string) (*api.AdapterStatus, error) Create(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) Replace(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) - Upsert(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, error) + // Upsert creates or replaces an adapter status. The second return value reports whether the + // write was actually applied: false means the incoming observed_generation was stale (either + // detected immediately or lost a race to a concurrent write with a higher generation). + Upsert(ctx context.Context, adapterStatus *api.AdapterStatus) (*api.AdapterStatus, bool, error) Delete(ctx context.Context, id string) error FindByResource(ctx context.Context, resourceType, resourceID string) (api.AdapterStatusList, error) FindByResourcePaginated( @@ -72,49 +75,66 @@ func (d *sqlAdapterStatusDao) Replace( return adapterStatus, nil } -// Upsert creates or updates an adapter status based on resource_type, resource_id, and adapter -// This implements the upsert semantic required by the new API spec +// Upsert creates or updates an adapter status based on resource_type, resource_id, and adapter. +// The UPDATE path includes a WHERE predicate on observed_generation so that a stale write can +// never overwrite a newer generation, even under concurrent requests. +// Returns (status, true, nil) when the write was applied, or (existing, false, nil) when the +// incoming observed_generation was stale (fast-path check) or lost a race to a concurrent write. func (d *sqlAdapterStatusDao) Upsert( ctx context.Context, adapterStatus *api.AdapterStatus, -) (*api.AdapterStatus, error) { +) (*api.AdapterStatus, bool, error) { g2 := (*d.sessionFactory).New(ctx) - // Try to find existing adapter status + // Try to find existing adapter status. existing, err := d.FindByResourceAndAdapter( ctx, adapterStatus.ResourceType, adapterStatus.ResourceID, adapterStatus.Adapter, ) - if err != nil { - // If not found, create new if errors.Is(err, gorm.ErrRecordNotFound) { - return d.Create(ctx, adapterStatus) + created, createErr := d.Create(ctx, adapterStatus) + if createErr != nil { + return nil, false, createErr + } + return created, true, nil } - // Other errors db.MarkForRollback(ctx, err) - return nil, err + return nil, false, err + } + + // Fast-path stale check: if the stored generation is already strictly newer, skip the write. + if existing.ObservedGeneration > adapterStatus.ObservedGeneration { + return existing, false, nil } - // Update existing record - // Keep the original ID and CreatedTime + // Prepare the update: keep original ID and CreatedTime. adapterStatus.ID = existing.ID if existing.CreatedTime != nil { adapterStatus.CreatedTime = existing.CreatedTime } - // Update LastReportTime to now + // Update LastReportTime to now. now := time.Now() adapterStatus.LastReportTime = &now - // Preserve LastTransitionTime for conditions whose status hasn't changed + // Preserve LastTransitionTime for conditions whose status hasn't changed. adapterStatus.Conditions = preserveLastTransitionTime(existing.Conditions, adapterStatus.Conditions) - // Save (update) the record - if err := g2.Omit(clause.Associations).Save(adapterStatus).Error; err != nil { - db.MarkForRollback(ctx, err) - return nil, err + // Atomic conditional UPDATE: only applies when the stored observed_generation is still <= + // the incoming one. A concurrent request that wrote a higher generation will cause + // RowsAffected to be 0, signalling a no-op. + result := g2.Omit(clause.Associations). + Where("observed_generation <= ?", adapterStatus.ObservedGeneration). + Save(adapterStatus) + if result.Error != nil { + db.MarkForRollback(ctx, result.Error) + return nil, false, result.Error + } + if result.RowsAffected == 0 { + // A concurrent write with a higher generation won the race. + return existing, false, nil } - return adapterStatus, nil + return adapterStatus, true, nil } func (d *sqlAdapterStatusDao) Delete(ctx context.Context, id string) error { diff --git a/pkg/services/CLAUDE.md b/pkg/services/CLAUDE.md index 4d52c13..bfa2c96 100644 --- a/pkg/services/CLAUDE.md +++ b/pkg/services/CLAUDE.md @@ -22,14 +22,18 @@ func NewClusterService(dao, adapterStatusDao, config) ClusterService ## Status Aggregation `UpdateClusterStatusFromAdapters()` in `cluster.go` synthesizes two top-level conditions: -- **Available**: True if all required adapters report `Available=True` (any generation) + +- **Available**: True when all required adapters report `Available=True` at the same `observed_generation` (not necessarily the current resource generation). When adapters are at different generations, Available preserves its previous value (last-known-good semantics). `ObservedGeneration` = the common adapter generation when consistent; preserved from existing state otherwise. - **Ready**: True if all adapters report `Available=True` AND `observed_generation` matches current generation +Ready's `LastUpdatedTime` is computed in `status_aggregation.computeReadyLastUpdated`: when Ready=False it is the minimum of `LastReportTime` across all required adapters (falls back to `now` if any required adapter has no stored status yet); when Ready=True it is the minimum of `LastReportTime` across required adapters that have Available=True at the current generation. True→False transitions override this with the triggering adapter's `observedTime`. + `ProcessAdapterStatus()` validates mandatory conditions (`Available`, `Applied`, `Health`) before persisting. Rejects `Available=Unknown` on subsequent reports (only allowed on first report). ## GenericService `generic.go` provides `List()` with pagination, search, and ordering. + - `ListArguments` has Page, Size, Search, OrderBy, Fields, Preloads - Search validation: `SearchDisallowedFields` map blocks searching certain fields per resource type - Default ordering: `created_time desc` diff --git a/pkg/services/adapter_status.go b/pkg/services/adapter_status.go index f566909..f965949 100644 --- a/pkg/services/adapter_status.go +++ b/pkg/services/adapter_status.go @@ -71,11 +71,11 @@ func (s *sqlAdapterStatusService) Replace( func (s *sqlAdapterStatusService) Upsert( ctx context.Context, adapterStatus *api.AdapterStatus, ) (*api.AdapterStatus, *errors.ServiceError) { - adapterStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) + result, _, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) if err != nil { return nil, handleCreateError("AdapterStatus", err) } - return adapterStatus, nil + return result, nil } func (s *sqlAdapterStatusService) Delete(ctx context.Context, id string) *errors.ServiceError { diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index e37f958..d231dfc 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -3,9 +3,6 @@ package services import ( "context" "encoding/json" - stderrors "errors" - "fmt" - "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -13,7 +10,6 @@ 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 @@ -96,8 +92,12 @@ func (s *sqlClusterService) Replace(ctx context.Context, cluster *api.Cluster) ( return nil, handleUpdateError("Cluster", err) } - // REMOVED: Event creation - no event-driven components - return cluster, nil + updatedCluster, svcErr := s.UpdateClusterStatusFromAdapters(ctx, cluster.ID) + if svcErr != nil { + return nil, svcErr + } + + return updatedCluster, nil } func (s *sqlClusterService) Delete(ctx context.Context, id string) *errors.ServiceError { @@ -131,21 +131,33 @@ func (s *sqlClusterService) OnUpsert(ctx context.Context, id string) error { return err } - ctx = logger.WithClusterID(ctx, cluster.ID) - logger.Info(ctx, "Perform idempotent operations on cluster") + logger.With(ctx, "cluster_id", cluster.ID). + Info("Perform idempotent operations on cluster") return nil } func (s *sqlClusterService) OnDelete(ctx context.Context, id string) error { - ctx = logger.WithClusterID(ctx, id) - logger.Info(ctx, "Cluster has been deleted") + logger.With(ctx, "cluster_id", id).Info("Cluster has been deleted") return nil } -// UpdateClusterStatusFromAdapters aggregates adapter statuses into cluster status +// UpdateClusterStatusFromAdapters aggregates adapter statuses into cluster status. +// Uses time.Now() as the observed time (for generation-change recomputations). +// Called from Create/Replace, so isLifecycleChange=true (Available frozen, Ready resets). func (s *sqlClusterService) UpdateClusterStatusFromAdapters( ctx context.Context, clusterID string, +) (*api.Cluster, *errors.ServiceError) { + return s.updateClusterStatusFromAdapters(ctx, clusterID, time.Now(), true) +} + +// updateClusterStatusFromAdapters is the internal implementation. +// observedTime is the triggering adapter's observed_time (its LastReportTime) and is used +// for transition timestamps in the synthetic conditions. +// isLifecycleChange=true freezes Available and resets Ready.lut=now (Create/Replace path). +// isLifecycleChange=false uses the normal adapter-report aggregation path. +func (s *sqlClusterService) updateClusterStatusFromAdapters( + ctx context.Context, clusterID string, observedTime time.Time, isLifecycleChange bool, ) (*api.Cluster, *errors.ServiceError) { // Get the cluster cluster, err := s.clusterDao.Get(ctx, clusterID) @@ -162,59 +174,18 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( now := time.Now() // Build the list of adapter ResourceConditions - adapterConditions := []api.ResourceCondition{} - - for _, adapterStatus := range adapterStatuses { - // Unmarshal Conditions from JSONB - var conditions []api.AdapterCondition - if unmarshalErr := json.Unmarshal(adapterStatus.Conditions, &conditions); unmarshalErr != nil { - continue // Skip if can't unmarshal - } - - // Find the "Available" condition - var availableCondition *api.AdapterCondition - for i := range conditions { - if conditions[i].Type == "Available" { - availableCondition = &conditions[i] - break - } - } - - if availableCondition == nil { - // No Available condition, skip this adapter - continue - } - - // Convert to ResourceCondition - condResource := api.ResourceCondition{ - Type: MapAdapterToConditionType(adapterStatus.Adapter), - Status: api.ResourceConditionStatus(availableCondition.Status), - Reason: availableCondition.Reason, - Message: availableCondition.Message, - ObservedGeneration: adapterStatus.ObservedGeneration, - LastTransitionTime: availableCondition.LastTransitionTime, - } - - // Set CreatedTime with nil check - if adapterStatus.CreatedTime != nil { - condResource.CreatedTime = *adapterStatus.CreatedTime - } - - // Set LastUpdatedTime with nil check - if adapterStatus.LastReportTime != nil { - condResource.LastUpdatedTime = *adapterStatus.LastReportTime - } - - adapterConditions = append(adapterConditions, condResource) - } + adapterConditions := buildAdapterResourceConditions(adapterStatuses) // Compute synthetic Available and Ready conditions availableCondition, readyCondition := BuildSyntheticConditions( + ctx, cluster.StatusConditions, adapterStatuses, s.adapterConfig.RequiredClusterAdapters(), cluster.Generation, now, + observedTime, + isLifecycleChange, ) // Combine synthetic conditions with adapter conditions @@ -238,84 +209,31 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( return cluster, nil } -// ProcessAdapterStatus handles the business logic for adapter status: -// - Validates that all mandatory conditions (Available, Applied, Health) are present -// - Rejects duplicate condition types -// - For first status report: accepts Unknown Available condition to avoid data loss -// - For subsequent reports: rejects Unknown Available condition to preserve existing valid state -// - Uses complete replacement semantics: each update replaces all conditions for this adapter -// - Returns (nil, nil) for discarded updates +// ProcessAdapterStatus handles the business logic for adapter status. +// Pre-processing rules applied in order (spec §2): +// - Stale: discards if observed_generation < existing adapter generation +// - P1: discards if observed_generation > resource generation (report ahead of resource) +// - P2: rejects if mandatory conditions (Available, Applied, Health) are missing or have invalid status +// - P3: discards if Available == Unknown (not processed per spec) +// +// Otherwise: upserts the status and triggers aggregation. +// Returns (nil, nil) for discarded/rejected updates. 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) - } - } - - // Validate mandatory conditions and check for duplicates - if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { - ctx = logger.WithClusterID(ctx, clusterID) - logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: %s condition %s", - adapterStatus.Adapter, errorType, conditionName)) - return nil, nil - } - - // Check Available condition for Unknown status - triggerAggregation := false - for _, cond := range conditions { - if cond.Type != api.ConditionTypeAvailable { - continue - } - - triggerAggregation = true - if cond.Status == api.AdapterConditionUnknown { - if existingStatus != nil { - // Non-first report && Available=Unknown → reject - ctx = logger.WithClusterID(ctx, clusterID) - logger.Info(ctx, fmt.Sprintf("Discarding adapter status update from %s: subsequent Unknown Available", - adapterStatus.Adapter)) - return nil, nil + return processAdapterStatus(ctx, "Cluster", clusterID, adapterStatus, s.adapterStatusDao, + func(ctx context.Context) (int32, *errors.ServiceError) { + cluster, err := s.clusterDao.Get(ctx, clusterID) + if err != nil { + return 0, handleGetError("Cluster", "id", clusterID, err) } - // First report from this adapter: allow storing even with Available=Unknown - // but skip aggregation since Unknown should not affect cluster-level conditions - triggerAggregation = false - } - break - } - - // Upsert the adapter status (complete replacement) - upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) - if err != nil { - return nil, handleCreateError("AdapterStatus", err) - } - - // Only trigger aggregation when triggerAggregation is true - if triggerAggregation { - 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 + return cluster.Generation, nil + }, + func(ctx context.Context, observedTime time.Time) { + if _, err := s.updateClusterStatusFromAdapters(ctx, clusterID, observedTime, false); err != nil { + logger.With(ctx, "resource_type", "Cluster", "resource_id", clusterID). + WithError(err).Warn("Failed to aggregate cluster status") + } + }, + ) } diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index f4ea939..2a9db63 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -109,11 +109,18 @@ func (d *mockAdapterStatusDao) Replace(ctx context.Context, status *api.AdapterS return status, nil } -func (d *mockAdapterStatusDao) Upsert(ctx context.Context, status *api.AdapterStatus) (*api.AdapterStatus, error) { +func (d *mockAdapterStatusDao) Upsert( + ctx context.Context, status *api.AdapterStatus, +) (*api.AdapterStatus, bool, error) { key := status.ResourceType + ":" + status.ResourceID + ":" + status.Adapter + if existing, ok := d.statuses[key]; ok { + if existing.ObservedGeneration > status.ObservedGeneration { + return existing, false, nil + } + } status.ID = key d.statuses[key] = status - return status, nil + return status, true, nil } func (d *mockAdapterStatusDao) Delete(ctx context.Context, id string) error { @@ -165,7 +172,8 @@ func (d *mockAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, var _ dao.AdapterStatusDao = &mockAdapterStatusDao{} -// TestProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored +// TestProcessAdapterStatus_FirstUnknownCondition tests that Available=Unknown is discarded +// per spec §2 P3, regardless of whether it is the first or a subsequent report. func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { RegisterTestingT(t) @@ -202,14 +210,60 @@ func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) - // First report with Unknown should be accepted + // Per spec §2 P3: Available=Unknown is always discarded. Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") - Expect(result.Adapter).To(Equal("test-adapter")) + Expect(result).To(BeNil(), "Available=Unknown must be discarded (spec §2 P3)") - // Verify the status was stored + // Verify the status was NOT stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") + Expect(len(storedStatuses)).To(Equal(0), "Unknown status must not be stored") +} + +// TestProcessAdapterStatus_FutureObservedGeneration tests that a report with +// observed_generation > resource.Generation is discarded per spec §2 P1. +func TestProcessAdapterStatus_FutureObservedGeneration(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Create cluster at generation 1 + cluster := &api.Cluster{Generation: 1} + cluster.ID = clusterID + _, svcErr := service.Create(ctx, cluster) + Expect(svcErr).To(BeNil()) + + // Send a report claiming observed_generation=2, but resource is at gen=1 + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, 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, + ObservedGeneration: 2, // ahead of resource gen=1 + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + // Per spec §2 P1: observed_generation > G → Discard. + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Report with observed_generation > resource.Generation must be discarded (spec §2 P1)") + + // Verify the status was NOT stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) + Expect(len(storedStatuses)).To(Equal(0), "Future-generation report must not be stored") } // TestProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown Available conditions are discarded @@ -241,7 +295,7 @@ func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { Conditions: conditionsJSON, CreatedTime: &now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _, _ = adapterStatusDao.Upsert(ctx, existingStatus) // Now send another Unknown status report newAdapterStatus := &api.AdapterStatus{ @@ -379,8 +433,8 @@ func TestProcessAdapterStatus_FalseCondition(t *testing.T) { Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for False condition") } -// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that first reports with -// Available=Unknown are accepted even when other conditions are present +// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that Available=Unknown +// is discarded per spec §2 P3, even when other conditions are present and it is the first report. func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) @@ -435,12 +489,13 @@ func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testin result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + // Per spec §2 P3: Available=Unknown is always discarded, even on first report. Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") + Expect(result).To(BeNil(), "Available=Unknown must be discarded (spec §2 P3)") - // Verify the status was stored + // Verify the status was NOT stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(1), "First status with Available=Unknown should be stored") + Expect(len(storedStatuses)).To(Equal(0), "Unknown status must not be stored") } // TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent reports @@ -473,7 +528,7 @@ func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *t Conditions: existingConditionsJSON, CreatedTime: &now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _, _ = adapterStatusDao.Upsert(ctx, existingStatus) // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ @@ -596,24 +651,19 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { 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). + // One adapter updates to gen=2 while the other stays at gen=1 => all_at_X=false. + // Per spec §5.2: all_at_X=false → no change → Available stays True@gen1. 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. + // Stale gen=1 report from validation is rejected (we don't allow downgrading observed generations). + // DAO state remains: validation@gen2=True, dns@gen1=True => Available still True@gen1. upsert("validation", api.AdapterConditionFalse, 1) avail, ready = getSynth() - Expect(avail.Status).To(Equal(api.ConditionTrue)) // <-- this is the edge case + Expect(avail.Status).To(Equal(api.ConditionTrue)) Expect(avail.ObservedGeneration).To(Equal(int32(1))) Expect(ready.Status).To(Equal(api.ConditionFalse)) @@ -628,7 +678,7 @@ func TestClusterAvailableReadyTransitions(t *testing.T) { upsert("dns", api.AdapterConditionFalse, 2) avail, ready = getSynth() Expect(avail.Status).To(Equal(api.ConditionFalse)) - Expect(avail.ObservedGeneration).To(Equal(int32(0))) + Expect(avail.ObservedGeneration).To(Equal(int32(2))) Expect(ready.Status).To(Equal(api.ConditionFalse)) // Available=Unknown is a no-op (does not store, does not overwrite cluster conditions). @@ -722,7 +772,7 @@ func TestClusterStaleAdapterStatusUpdatePolicy(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 from gen=1 is discarded by the stale check (validation is already at gen=2). upsert("validation", api.AdapterConditionFalse, 1) available = getAvailable() Expect(available.Status).To(Equal(api.ConditionTrue)) @@ -769,7 +819,9 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { StatusConditions: initialConditionsJSON, } cluster.ID = clusterID + beforeCreate := time.Now() created, svcErr := service.Create(ctx, cluster) + afterCreate := time.Now() Expect(svcErr).To(BeNil()) var createdConds []api.ResourceCondition @@ -789,12 +841,17 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { Expect(createdReady).ToNot(BeNil()) Expect(createdAvailable.CreatedTime).To(Equal(fixedNow)) Expect(createdAvailable.LastTransitionTime).To(Equal(fixedNow)) + // No adapters reported → all_at_X=false → spec §5.2 no change → Available preserved from initialConditions. Expect(createdAvailable.LastUpdatedTime).To(Equal(fixedNow)) Expect(createdReady.CreatedTime).To(Equal(fixedNow)) Expect(createdReady.LastTransitionTime).To(Equal(fixedNow)) - Expect(createdReady.LastUpdatedTime).To(Equal(fixedNow)) + // Ready.LastUpdatedTime is refreshed to the evaluation time when isReady=false; assert it lies in the Create() window. + Expect(createdReady.LastUpdatedTime).To(BeTemporally(">=", beforeCreate)) + Expect(createdReady.LastUpdatedTime).To(BeTemporally("<=", afterCreate)) + beforeUpdate := time.Now() updated, err := service.UpdateClusterStatusFromAdapters(ctx, clusterID) + afterUpdate := time.Now() Expect(err).To(BeNil()) var updatedConds []api.ResourceCondition @@ -814,10 +871,14 @@ func TestClusterSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { Expect(updatedReady).ToNot(BeNil()) Expect(updatedAvailable.CreatedTime).To(Equal(fixedNow)) Expect(updatedAvailable.LastTransitionTime).To(Equal(fixedNow)) + // No adapters reported → all_at_X=false → Available still preserved from 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)) + // Ready.LastUpdatedTime is refreshed to the evaluation time when isReady=false; + // assert it lies in the UpdateClusterStatusFromAdapters() window. + Expect(updatedReady.LastUpdatedTime).To(BeTemporally(">=", beforeUpdate)) + Expect(updatedReady.LastUpdatedTime).To(BeTemporally("<=", afterUpdate)) } // TestProcessAdapterStatus_MissingMandatoryCondition_Available tests that updates missing Available are rejected @@ -1004,6 +1065,9 @@ func TestProcessAdapterStatus_CustomConditionRemoval(t *testing.T) { Expect(unmarshalErr).To(BeNil()) Expect(len(storedConditions1)).To(Equal(4)) + // Bump cluster generation to 2 so the second update (ObservedGeneration=2) is not blocked by P1. + clusterDao.clusters[clusterID].Generation = 2 + // Second update: remove custom condition (only send mandatory conditions) conditions2 := []api.AdapterCondition{ {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index 89b2198..a190951 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -3,9 +3,6 @@ package services import ( "context" "encoding/json" - stderrors "errors" - "fmt" - "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" @@ -13,7 +10,6 @@ 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 @@ -98,8 +94,12 @@ func (s *sqlNodePoolService) Replace( return nil, handleUpdateError("NodePool", err) } - // REMOVED: Event creation - no event-driven components - return nodePool, nil + updatedNodePool, svcErr := s.UpdateNodePoolStatusFromAdapters(ctx, nodePool.ID) + if svcErr != nil { + return nil, svcErr + } + + return updatedNodePool, nil } func (s *sqlNodePoolService) Delete(ctx context.Context, id string) *errors.ServiceError { @@ -144,9 +144,22 @@ func (s *sqlNodePoolService) OnDelete(ctx context.Context, id string) error { return nil } -// UpdateNodePoolStatusFromAdapters aggregates adapter statuses into nodepool status +// UpdateNodePoolStatusFromAdapters aggregates adapter statuses into nodepool status. +// Uses time.Now() as the observed time (for generation-change recomputations). +// Called from Create/Replace, so isLifecycleChange=true (Available frozen, Ready resets). func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( ctx context.Context, nodePoolID string, +) (*api.NodePool, *errors.ServiceError) { + return s.updateNodePoolStatusFromAdapters(ctx, nodePoolID, time.Now(), true) +} + +// updateNodePoolStatusFromAdapters is the internal implementation. +// observedTime is the triggering adapter's observed_time (its LastReportTime) and is used +// for transition timestamps in the synthetic conditions. +// isLifecycleChange=true freezes Available and resets Ready.lut=now (Create/Replace path). +// isLifecycleChange=false uses the normal adapter-report aggregation path. +func (s *sqlNodePoolService) updateNodePoolStatusFromAdapters( + ctx context.Context, nodePoolID string, observedTime time.Time, isLifecycleChange bool, ) (*api.NodePool, *errors.ServiceError) { // Get the nodepool nodePool, err := s.nodePoolDao.Get(ctx, nodePoolID) @@ -163,59 +176,18 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( now := time.Now() // Build the list of adapter ResourceConditions - adapterConditions := []api.ResourceCondition{} - - for _, adapterStatus := range adapterStatuses { - // Unmarshal Conditions from JSONB - var conditions []api.AdapterCondition - if unmarshalErr := json.Unmarshal(adapterStatus.Conditions, &conditions); unmarshalErr != nil { - continue // Skip if can't unmarshal - } - - // Find the "Available" condition - var availableCondition *api.AdapterCondition - for i := range conditions { - if conditions[i].Type == api.ConditionTypeAvailable { - availableCondition = &conditions[i] - break - } - } - - if availableCondition == nil { - // No Available condition, skip this adapter - continue - } - - // Convert to ResourceCondition - condResource := api.ResourceCondition{ - Type: MapAdapterToConditionType(adapterStatus.Adapter), - Status: api.ResourceConditionStatus(availableCondition.Status), - Reason: availableCondition.Reason, - Message: availableCondition.Message, - ObservedGeneration: adapterStatus.ObservedGeneration, - LastTransitionTime: availableCondition.LastTransitionTime, - } - - // Set CreatedTime with nil check - if adapterStatus.CreatedTime != nil { - condResource.CreatedTime = *adapterStatus.CreatedTime - } - - // Set LastUpdatedTime with nil check - if adapterStatus.LastReportTime != nil { - condResource.LastUpdatedTime = *adapterStatus.LastReportTime - } - - adapterConditions = append(adapterConditions, condResource) - } + adapterConditions := buildAdapterResourceConditions(adapterStatuses) // Compute synthetic Available and Ready conditions availableCondition, readyCondition := BuildSyntheticConditions( + ctx, nodePool.StatusConditions, adapterStatuses, s.adapterConfig.RequiredNodePoolAdapters(), nodePool.Generation, now, + observedTime, + isLifecycleChange, ) // Combine synthetic conditions with adapter conditions @@ -239,84 +211,31 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( return nodePool, nil } -// ProcessAdapterStatus handles the business logic for adapter status: -// - Validates that all mandatory conditions (Available, Applied, Health) are present -// - Rejects duplicate condition types -// - For first status report: accepts Unknown Available condition to avoid data loss -// - For subsequent reports: rejects Unknown Available condition to preserve existing valid state -// - Uses complete replacement semantics: each update replaces all conditions for this adapter -// - Returns (nil, nil) for discarded updates +// ProcessAdapterStatus handles the business logic for adapter status. +// Pre-processing rules applied in order (spec §2): +// - Stale: discards if observed_generation < existing adapter generation +// - P1: discards if observed_generation > resource generation (report ahead of resource) +// - P2: rejects if mandatory conditions (Available, Applied, Health) are missing or have invalid status +// - P3: discards if Available == Unknown (not processed per spec) +// +// Otherwise: upserts the status and triggers aggregation. +// Returns (nil, nil) for discarded/rejected updates. 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) - } - } - - // Validate mandatory conditions and check for duplicates - if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { - logger.With(ctx, logger.FieldNodePoolID, nodePoolID). - Info(fmt.Sprintf("Discarding adapter status update from %s: %s condition %s", - adapterStatus.Adapter, errorType, conditionName)) - return nil, nil - } - - // Check Available condition for Unknown status - triggerAggregation := false - for _, cond := range conditions { - if cond.Type != api.ConditionTypeAvailable { - continue - } - - triggerAggregation = true - if cond.Status == api.AdapterConditionUnknown { - if existingStatus != nil { - // Non-first report && Available=Unknown → reject - logger.With(ctx, logger.FieldNodePoolID, nodePoolID). - Info(fmt.Sprintf("Discarding adapter status update from %s: subsequent Unknown Available", - adapterStatus.Adapter)) - return nil, nil + return processAdapterStatus(ctx, "NodePool", nodePoolID, adapterStatus, s.adapterStatusDao, + func(ctx context.Context) (int32, *errors.ServiceError) { + nodePool, err := s.nodePoolDao.Get(ctx, nodePoolID) + if err != nil { + return 0, handleGetError("NodePool", "id", nodePoolID, err) } - // First report from this adapter: allow storing even with Available=Unknown - // but skip aggregation since Unknown should not affect nodepool-level conditions - triggerAggregation = false - } - break - } - - // Upsert the adapter status (complete replacement) - upsertedStatus, err := s.adapterStatusDao.Upsert(ctx, adapterStatus) - if err != nil { - return nil, handleCreateError("AdapterStatus", err) - } - - // Only trigger aggregation when triggerAggregation is true - if triggerAggregation { - 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 + return nodePool.Generation, nil + }, + func(ctx context.Context, observedTime time.Time) { + if _, err := s.updateNodePoolStatusFromAdapters(ctx, nodePoolID, observedTime, false); err != nil { + logger.With(ctx, "resource_type", "NodePool", "resource_id", nodePoolID). + WithError(err).Warn("Failed to aggregate nodepool status") + } + }, + ) } diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index c38193b..34e2806 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -82,7 +82,8 @@ func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { var _ dao.NodePoolDao = &mockNodePoolDao{} -// TestNodePoolProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored +// TestNodePoolProcessAdapterStatus_FirstUnknownCondition tests that Available=Unknown is discarded +// per spec §2 P3, regardless of whether it is the first or a subsequent report. func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { RegisterTestingT(t) @@ -95,7 +96,13 @@ func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { ctx := context.Background() nodePoolID := testNodePoolID - // Create first adapter status with all mandatory conditions but Available=Unknown + // Create nodepool first (needed for P1 check) + nodePool := &api.NodePool{Generation: 1} + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + // Send first status with Available=Unknown conditions := []api.AdapterCondition{ { Type: api.ConditionTypeAvailable, @@ -117,22 +124,71 @@ func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { now := time.Now() adapterStatus := &api.AdapterStatus{ - ResourceType: "NodePool", - ResourceID: nodePoolID, - Adapter: "test-adapter", - Conditions: conditionsJSON, - CreatedTime: &now, + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + // Per spec §2 P3: Available=Unknown is always discarded. Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") - Expect(result.Adapter).To(Equal("test-adapter")) + Expect(result).To(BeNil(), "Available=Unknown must be discarded (spec §2 P3)") - // Verify the status was stored + // Verify the status was NOT stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) - Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") + Expect(len(storedStatuses)).To(Equal(0), "Unknown status must not be stored") +} + +// TestNodePoolProcessAdapterStatus_FutureObservedGeneration tests that a report with +// observed_generation > resource.Generation is discarded per spec §2 P1. +func TestNodePoolProcessAdapterStatus_FutureObservedGeneration(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := testNodePoolAdapterConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := testNodePoolID + + // Create nodepool at generation 1 + nodePool := &api.NodePool{Generation: 1} + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + + // Send a report claiming observed_generation=2, but resource is at gen=1 + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, 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, + ObservedGeneration: 2, // ahead of resource gen=1 + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + + // Per spec §2 P1: observed_generation > G → Discard. + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Report with observed_generation > resource.Generation must be discarded (spec §2 P1)") + + // Verify the status was NOT stored + storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) + Expect(len(storedStatuses)).To(Equal(0), "Future-generation report must not be stored") } // TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown conditions are discarded @@ -164,7 +220,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { Conditions: conditionsJSON, CreatedTime: &now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _, _ = adapterStatusDao.Upsert(ctx, existingStatus) // Now send another Unknown status report newAdapterStatus := &api.AdapterStatus{ @@ -242,8 +298,8 @@ func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for True condition") } -// TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that first reports -// with Available=Unknown are accepted even when other conditions are present +// TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that Available=Unknown +// is discarded per spec §2 P3, even when other conditions are present and it is the first report. func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) @@ -256,6 +312,12 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t ctx := context.Background() nodePoolID := testNodePoolID + // Create nodepool first (needed for P1 check) + nodePool := &api.NodePool{Generation: 1} + nodePool.ID = nodePoolID + _, svcErr := service.Create(ctx, nodePool) + Expect(svcErr).To(BeNil()) + // Create first adapter status with all mandatory conditions but Available=Unknown conditions := []api.AdapterCondition{ { @@ -283,21 +345,23 @@ func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t now := time.Now() adapterStatus := &api.AdapterStatus{ - ResourceType: "NodePool", - ResourceID: nodePoolID, - Adapter: "test-adapter", - Conditions: conditionsJSON, - CreatedTime: &now, + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + ObservedGeneration: 1, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + // Per spec §2 P3: Available=Unknown is always discarded, even on first report. Expect(err).To(BeNil()) - Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be accepted") + Expect(result).To(BeNil(), "Available=Unknown must be discarded (spec §2 P3)") - // Verify the status was stored + // Verify the status was NOT stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) - Expect(len(storedStatuses)).To(Equal(1), "First status with Available=Unknown should be stored") + Expect(len(storedStatuses)).To(Equal(0), "Unknown status must not be stored") } // TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent @@ -330,7 +394,7 @@ func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnkn Conditions: existingConditionsJSON, CreatedTime: &now, } - _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + _, _, _ = adapterStatusDao.Upsert(ctx, existingStatus) // Now send another report with multiple conditions including Available=Unknown conditions := []api.AdapterCondition{ @@ -451,7 +515,8 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { 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). + // One adapter updates to gen=2 while the other stays at gen=1 => all_at_X=false. + // Per spec §5.2: all_at_X=false → no change → Available stays True@gen1. upsert("validation", api.AdapterConditionTrue, 2) avail, ready = getSynth() Expect(avail.Status).To(Equal(api.ConditionTrue)) @@ -469,7 +534,7 @@ func TestNodePoolAvailableReadyTransitions(t *testing.T) { upsert("hypershift", api.AdapterConditionFalse, 2) avail, ready = getSynth() Expect(avail.Status).To(Equal(api.ConditionFalse)) - Expect(avail.ObservedGeneration).To(Equal(int32(0))) + Expect(avail.ObservedGeneration).To(Equal(int32(2))) Expect(ready.Status).To(Equal(api.ConditionFalse)) // Adapter status missing mandatory conditions should be rejected and not overwrite synthetic conditions. @@ -582,7 +647,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 but we do not override newer generation responses + // Stale False from gen=1 is discarded by the stale check (validation is already at gen=2). upsert("validation", api.AdapterConditionFalse, 1) available = getAvailable() Expect(available.Status).To(Equal(api.ConditionTrue)) @@ -629,7 +694,9 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { StatusConditions: initialConditionsJSON, } nodePool.ID = nodePoolID + beforeCreate := time.Now() created, svcErr := service.Create(ctx, nodePool) + afterCreate := time.Now() Expect(svcErr).To(BeNil()) var createdConds []api.ResourceCondition @@ -649,12 +716,17 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { Expect(createdReady).ToNot(BeNil()) Expect(createdAvailable.CreatedTime).To(Equal(fixedNow)) Expect(createdAvailable.LastTransitionTime).To(Equal(fixedNow)) + // No adapters reported → all_at_X=false → spec §5.2 no change → Available preserved from initialConditions. Expect(createdAvailable.LastUpdatedTime).To(Equal(fixedNow)) Expect(createdReady.CreatedTime).To(Equal(fixedNow)) Expect(createdReady.LastTransitionTime).To(Equal(fixedNow)) - Expect(createdReady.LastUpdatedTime).To(Equal(fixedNow)) + // Ready.LastUpdatedTime is refreshed to the evaluation time when isReady=false; assert it lies in the Create() window. + Expect(createdReady.LastUpdatedTime).To(BeTemporally(">=", beforeCreate)) + Expect(createdReady.LastUpdatedTime).To(BeTemporally("<=", afterCreate)) + beforeUpdate := time.Now() updated, err := service.UpdateNodePoolStatusFromAdapters(ctx, nodePoolID) + afterUpdate := time.Now() Expect(err).To(BeNil()) var updatedConds []api.ResourceCondition @@ -674,8 +746,12 @@ func TestNodePoolSyntheticTimestampsStableWithoutAdapterStatus(t *testing.T) { Expect(updatedReady).ToNot(BeNil()) Expect(updatedAvailable.CreatedTime).To(Equal(fixedNow)) Expect(updatedAvailable.LastTransitionTime).To(Equal(fixedNow)) + // No adapters reported → all_at_X=false → Available still preserved from 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)) + // Ready.LastUpdatedTime is refreshed to the evaluation time when isReady=false; + // assert it lies in the UpdateNodePoolStatusFromAdapters() window. + Expect(updatedReady.LastUpdatedTime).To(BeTemporally(">=", beforeUpdate)) + Expect(updatedReady.LastUpdatedTime).To(BeTemporally("<=", afterUpdate)) } diff --git a/pkg/services/process_adapter_status.go b/pkg/services/process_adapter_status.go new file mode 100644 index 0000000..5ffddf4 --- /dev/null +++ b/pkg/services/process_adapter_status.go @@ -0,0 +1,154 @@ +package services + +import ( + "context" + "encoding/json" + stderrors "errors" + "fmt" + "strings" + "time" + + "gorm.io/gorm" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" +) + +// processAdapterStatus contains the shared pre-processing pipeline for adapter status updates. +// resourceType is the resource kind (e.g. "Cluster", "NodePool"); resourceID is the resource's ID. +// getGeneration fetches the current resource generation (used for P1 check). +// triggerAggregation is called fire-and-forget after a successful upsert; callers handle errors. +func processAdapterStatus( + ctx context.Context, + resourceType, resourceID string, + adapterStatus *api.AdapterStatus, + adapterStatusDao dao.AdapterStatusDao, + getGeneration func(ctx context.Context) (int32, *errors.ServiceError), + triggerAggregation func(ctx context.Context, observedTime time.Time), +) (*api.AdapterStatus, *errors.ServiceError) { + existingStatus, findErr := adapterStatusDao.FindByResourceAndAdapter( + ctx, resourceType, resourceID, 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) + } + } + // Stale check: discard if older than the adapter's last recorded generation. + if existingStatus != nil && adapterStatus.ObservedGeneration < existingStatus.ObservedGeneration { + return nil, nil + } + + // Parse conditions from the adapter status (needed for P2 and P3 before resource fetch). + 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) + } + } + + // P2: validate mandatory conditions (presence and valid status values). + if errorType, conditionName := ValidateMandatoryConditions(conditions); errorType != "" { + logger.With(ctx, "resource_type", resourceType, "resource_id", resourceID). + Info(fmt.Sprintf("Discarding adapter status update from %s: %s condition %s", + adapterStatus.Adapter, errorType, conditionName)) + return nil, nil + } + + // P3: discard if Available == Unknown (spec §2, all reports). + for _, cond := range conditions { + if cond.Type == api.ConditionTypeAvailable && cond.Status == api.AdapterConditionUnknown { + logger.With(ctx, "resource_type", resourceType, "resource_id", resourceID). + Info(fmt.Sprintf("Discarding adapter status update from %s: Available=Unknown reports are not processed", + adapterStatus.Adapter)) + return nil, nil + } + } + + // P1: discard if observed_generation is ahead of the current resource generation. + // Checked after P2/P3 to avoid unnecessary resource fetches for invalid/Unknown reports. + generation, svcErr := getGeneration(ctx) + if svcErr != nil { + return nil, svcErr + } + if adapterStatus.ObservedGeneration > generation { + logger.With(ctx, "resource_type", resourceType, "resource_id", resourceID). + Info(fmt.Sprintf( + "Discarding adapter status update from %s: observed_generation %d > resource generation %d", + adapterStatus.Adapter, adapterStatus.ObservedGeneration, generation)) + return nil, nil + } + + // Upsert the adapter status (complete replacement). + upsertedStatus, applied, upsertErr := adapterStatusDao.Upsert(ctx, adapterStatus) + if upsertErr != nil { + return nil, handleCreateError("AdapterStatus", upsertErr) + } + if !applied { + // A concurrent write with a higher generation was already stored; discard this update. + return nil, nil + } + + // Trigger aggregation using the adapter's observed_time for transition timestamps. + observedTime := time.Now() + if upsertedStatus.LastReportTime != nil { + observedTime = *upsertedStatus.LastReportTime + } + triggerAggregation(ctx, observedTime) + + return upsertedStatus, nil +} + +// buildAdapterResourceConditions builds a []api.ResourceCondition from adapter statuses, +// using each adapter's Available condition as the reported status. +func buildAdapterResourceConditions(adapterStatuses api.AdapterStatusList) []api.ResourceCondition { + adapterConditions := []api.ResourceCondition{} + + for _, adapterStatus := range adapterStatuses { + // Unmarshal Conditions from JSONB + var conditions []api.AdapterCondition + if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err != nil { + continue // Skip if can't unmarshal + } + + // Find the "Available" condition + var availableCondition *api.AdapterCondition + for i := range conditions { + if conditions[i].Type == api.ConditionTypeAvailable { + availableCondition = &conditions[i] + break + } + } + + if availableCondition == nil { + // No Available condition, skip this adapter + continue + } + + // Convert to ResourceCondition + condResource := api.ResourceCondition{ + Type: MapAdapterToConditionType(adapterStatus.Adapter), + Status: api.ResourceConditionStatus(availableCondition.Status), + Reason: availableCondition.Reason, + Message: availableCondition.Message, + ObservedGeneration: adapterStatus.ObservedGeneration, + LastTransitionTime: availableCondition.LastTransitionTime, + } + + // Set CreatedTime with nil check + if adapterStatus.CreatedTime != nil { + condResource.CreatedTime = *adapterStatus.CreatedTime + } + + // Set LastUpdatedTime with nil check + if adapterStatus.LastReportTime != nil { + condResource.LastUpdatedTime = *adapterStatus.LastReportTime + } + + adapterConditions = append(adapterConditions, condResource) + } + + return adapterConditions +} diff --git a/pkg/services/status_aggregation.go b/pkg/services/status_aggregation.go index 9afa275..2fbc5d5 100644 --- a/pkg/services/status_aggregation.go +++ b/pkg/services/status_aggregation.go @@ -1,13 +1,14 @@ 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" ) // Mandatory condition types that must be present in all adapter status updates @@ -15,8 +16,9 @@ var mandatoryConditionTypes = []string{api.ConditionTypeAvailable, api.Condition // Condition validation error types const ( - ConditionValidationErrorDuplicate = "duplicate" - ConditionValidationErrorMissing = "missing" + ConditionValidationErrorDuplicate = "duplicate" + ConditionValidationErrorMissing = "missing" + ConditionValidationErrorInvalidStatus = "invalid_status" ) // Required adapter lists configured via pkg/config/adapter.go (see AdapterRequirementsConfig) @@ -30,30 +32,43 @@ var adapterConditionSuffixMap = map[string]string{ // Add custom mappings here when needed } -// ValidateMandatoryConditions checks if all mandatory conditions are present and rejects duplicate condition types. +// validAdapterConditionStatuses holds the set of allowed status values for adapter conditions. +var validAdapterConditionStatuses = map[api.AdapterConditionStatus]bool{ + api.AdapterConditionTrue: true, + api.AdapterConditionFalse: true, + api.AdapterConditionUnknown: true, +} + +// ValidateMandatoryConditions checks that all mandatory conditions are present, have no +// duplicate types, and carry a valid status value (True, False, or Unknown). // Returns (errorType, conditionName) where: // - If duplicate found: (ConditionValidationErrorDuplicate, duplicateConditionType) // - If missing condition: (ConditionValidationErrorMissing, missingConditionType) +// - If invalid status: (ConditionValidationErrorInvalidStatus, conditionType) // - If all valid: ("", "") func ValidateMandatoryConditions(conditions []api.AdapterCondition) (errorType, conditionName string) { - // Check for duplicate condition types - seen := make(map[string]bool) + // Check for duplicate condition types and track status values. + seen := make(map[string]api.AdapterConditionStatus) for _, cond := range conditions { // Reject empty condition types if cond.Type == "" { return ConditionValidationErrorMissing, "" } - if seen[cond.Type] { + if _, exists := seen[cond.Type]; exists { return ConditionValidationErrorDuplicate, cond.Type } - seen[cond.Type] = true + seen[cond.Type] = cond.Status } - // Check that all mandatory conditions are present + // Check that all mandatory conditions are present and have valid status values. for _, mandatoryType := range mandatoryConditionTypes { - if !seen[mandatoryType] { + status, exists := seen[mandatoryType] + if !exists { return ConditionValidationErrorMissing, mandatoryType } + if !validAdapterConditionStatuses[status] { + return ConditionValidationErrorInvalidStatus, mandatoryType + } } return "", "" @@ -96,150 +111,404 @@ func MapAdapterToConditionType(adapterName string) string { return result.String() } -// 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 +// adapterAvailabilitySnapshot is the result of scanning all required adapters to determine +// whether they are at a consistent generation and whether they all report Available=True. +type adapterAvailabilitySnapshot struct { + // consistent is true when every required adapter has reported at the same observed generation. + // When false, the existing Available condition should be left unchanged. + consistent bool + // available is true when consistent=true and every required adapter has Available=True. + available bool + // generation is the common observed generation across required adapters (only valid when consistent=true). + generation int32 + // minLastReportTime is the earliest LastReportTime across all required adapters (nil when none available). + minLastReportTime *time.Time +} + +// scanAdapterAvailability inspects the stored adapter statuses for the required adapters and +// returns a snapshot describing whether they are consistent and available. +// +// Available=True requires all required adapters to have reported at the same observed generation +// and all to have Available=True. If any required adapter is missing or at a different generation, +// consistent=false is returned and the caller should leave the current Available condition unchanged. +func scanAdapterAvailability( + ctx context.Context, + adapterStatuses api.AdapterStatusList, + requiredAdapters []string, +) adapterAvailabilitySnapshot { + if len(requiredAdapters) == 0 { + return adapterAvailabilitySnapshot{} } - // Build a map of adapter name -> (available status, observed generation) - adapterMap := make(map[string]struct { + type adapterInfo struct { available string observedGeneration int32 - }) + lastReportTime *time.Time + } + adapterMap := make(map[string]adapterInfo, len(adapterStatuses)) - for _, adapterStatus := range adapterStatuses { - // Unmarshal conditions to find "Available" + for _, s := range adapterStatuses { 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 == api.ConditionTypeAvailable { - adapterMap[adapterStatus.Adapter] = struct { - available string - observedGeneration int32 - }{ - available: cond.Status, - observedGeneration: adapterStatus.ObservedGeneration, - } - break - } + if len(s.Conditions) == 0 { + continue + } + if err := json.Unmarshal(s.Conditions, &conditions); err != nil { + logger.With(ctx, "adapter", s.Adapter).WithError(err).Warn( + "Failed to parse adapter conditions") + continue + } + for _, cond := range conditions { + if cond.Type == api.ConditionTypeAvailable { + adapterMap[s.Adapter] = adapterInfo{ + available: cond.Status, + observedGeneration: s.ObservedGeneration, + lastReportTime: s.LastReportTime, } + break } } } - // Count available adapters and track min observed generation - numAvailable := 0 - minObservedGeneration := int32(math.MaxInt32) - - for _, adapterName := range requiredAdapters { - adapterInfo, exists := adapterMap[adapterName] - + // All required adapters must have reported at the same observed generation. + var commonGen *int32 + for _, name := range requiredAdapters { + info, exists := adapterMap[name] if !exists { - // Required adapter not found - not available - continue + return adapterAvailabilitySnapshot{} + } + if commonGen == nil { + g := info.observedGeneration + commonGen = &g + } else if info.observedGeneration != *commonGen { + return adapterAvailabilitySnapshot{} } + } + + if commonGen == nil { + return adapterAvailabilitySnapshot{} + } - // 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 + // Consistent generation: determine availability and earliest report time. + allAvailable := true + var minLRT *time.Time + for _, name := range requiredAdapters { + info := adapterMap[name] + if info.available != string(api.AdapterConditionTrue) { + allAvailable = false + } + if info.lastReportTime != nil { + if minLRT == nil || info.lastReportTime.Before(*minLRT) { + t := *info.lastReportTime + minLRT = &t } } } - // Available when all required adapters have Available=True (at any generation) - numRequired := len(requiredAdapters) - if numAvailable == numRequired { - return true, minObservedGeneration + return adapterAvailabilitySnapshot{ + consistent: true, + available: allAvailable, + generation: *commonGen, + minLastReportTime: minLRT, + } +} + +// buildAvailableCondition computes the Available ResourceCondition from the current adapter +// availability snapshot, the previous condition (may be nil), and the evaluation time. +// +// observedTime is the triggering adapter's observed_time (its LastReportTime). It is used for +// LastTransitionTime on status changes and LastUpdatedTime on True→False transitions. For all +// other cases LastUpdatedTime is the earliest report time across required adapters. +// +// When adapters are not at a consistent generation, the existing condition is preserved unchanged. +func buildAvailableCondition( + snapshot adapterAvailabilitySnapshot, + existing *api.ResourceCondition, + resourceGeneration int32, + now time.Time, + observedTime time.Time, +) api.ResourceCondition { + if !snapshot.consistent { + if existing != nil { + return *existing + } + return api.ResourceCondition{ + Type: api.ConditionTypeAvailable, + Status: api.ConditionFalse, + ObservedGeneration: resourceGeneration, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + } + } + + newStatus := api.ConditionFalse + if snapshot.available { + newStatus = api.ConditionTrue + } + + prevStatus := api.ConditionFalse + if existing != nil { + prevStatus = existing.Status + } + + // True→False: use the triggering adapter's observed time (when the failure was first seen). + // All other cases (stays True, stays False, False→True): use earliest adapter report time. + var lut time.Time + switch { + case prevStatus == api.ConditionTrue && newStatus == api.ConditionFalse: + lut = observedTime + case snapshot.minLastReportTime != nil: + lut = *snapshot.minLastReportTime + default: + lut = now } - // Return 0 for minObservedGeneration when not available - return false, 0 + // LastTransitionTime advances on status change using the triggering adapter's observed time. + ltt := observedTime + if existing != nil && existing.Status == newStatus && !existing.LastTransitionTime.IsZero() { + ltt = existing.LastTransitionTime + } + + createdTime := now + if existing != nil && !existing.CreatedTime.IsZero() { + createdTime = existing.CreatedTime + } + + cond := api.ResourceCondition{ + Type: api.ConditionTypeAvailable, + Status: newStatus, + ObservedGeneration: snapshot.generation, + LastTransitionTime: ltt, + CreatedTime: createdTime, + LastUpdatedTime: lut, + } + + // Carry over Reason/Message when status is unchanged. + if existing != nil && prevStatus == newStatus { + if cond.Reason == nil { + cond.Reason = existing.Reason + } + if cond.Message == nil { + cond.Message = existing.Message + } + } + + return cond } -// ComputeReadyCondition checks if all required adapters have Available=True at the CURRENT generation. -// "Ready" means the system is running at the latest spec generation. +// ComputeReadyCondition reports whether all required adapters have Available=True at the current +// resource generation. Unlike Available, Ready requires adapters to be caught up to the latest +// generation — it does not accept reports from older generations. func ComputeReadyCondition( - adapterStatuses api.AdapterStatusList, requiredAdapters []string, resourceGeneration int32, + ctx context.Context, 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 { + type adapterInfo struct { available string observedGeneration int32 - }) + } + adapterMap := make(map[string]adapterInfo, len(adapterStatuses)) - for _, adapterStatus := range adapterStatuses { - // Unmarshal conditions to find "Available" + for _, s := range adapterStatuses { 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 == api.ConditionTypeAvailable { - adapterMap[adapterStatus.Adapter] = struct { - available string - observedGeneration int32 - }{ - available: cond.Status, - observedGeneration: adapterStatus.ObservedGeneration, - } - break - } + if len(s.Conditions) == 0 { + continue + } + if err := json.Unmarshal(s.Conditions, &conditions); err != nil { + logger.With(ctx, "adapter", s.Adapter).WithError(err).Warn( + "Failed to parse adapter conditions") + continue + } + for _, cond := range conditions { + if cond.Type == api.ConditionTypeAvailable { + adapterMap[s.Adapter] = adapterInfo{ + available: cond.Status, + observedGeneration: s.ObservedGeneration, } + break } } } - // Count ready adapters (Available=True at current generation) numReady := 0 + for _, name := range requiredAdapters { + info, exists := adapterMap[name] + if !exists || info.observedGeneration != resourceGeneration { + continue + } + if info.available == string(api.AdapterConditionTrue) { + numReady++ + } + } - for _, adapterName := range requiredAdapters { - adapterInfo, exists := adapterMap[adapterName] + return numReady == len(requiredAdapters) +} - if !exists { - // Required adapter not found - not ready - continue +// findAdapterStatus returns the first adapter status in the list with the given adapter name, or (nil, false). +func findAdapterStatus(adapterStatuses api.AdapterStatusList, adapterName string) (*api.AdapterStatus, bool) { + for _, s := range adapterStatuses { + if s.Adapter == adapterName { + return s, true } + } + return nil, false +} - // For Ready condition, we require generation matching - if resourceGeneration > 0 && adapterInfo.observedGeneration != resourceGeneration { - // Adapter is processing old generation (stale) - not ready - continue +// adapterConditionsHasAvailableTrue returns true if the adapter conditions JSON +// contains a condition with type Available and status True. +func adapterConditionsHasAvailableTrue(ctx context.Context, adapterName string, conditions []byte) bool { + if len(conditions) == 0 { + return false + } + var conds []struct { + Type string `json:"type"` + Status string `json:"status"` + } + if err := json.Unmarshal(conditions, &conds); err != nil { + logger.With(ctx, "adapter", adapterName).WithError(err).Warn( + "Failed to parse adapter conditions") + return false + } + for _, c := range conds { + if c.Type == api.ConditionTypeAvailable && c.Status == string(api.AdapterConditionTrue) { + return true } + } + return false +} - // Check available status - if adapterInfo.available == "True" { - numReady++ +// computeReadyLastUpdated returns the timestamp to use for the Ready condition's LastUpdatedTime. +// +// When isReady is false, it returns the minimum LastReportTime across all required adapters +// (spec: last_update_time=min(resource.statuses[].last_update_time) when Ready stays False). +// Falls back to now if any required adapter has no stored status or no LRT yet. +// +// When isReady is true, it returns the minimum LastReportTime across all required adapters +// that have Available=True at the current generation. Falls back to now if none qualify. +// +// Note: True→False transitions override this value with observedTime in buildReadyCondition. +func computeReadyLastUpdated( + ctx context.Context, + adapterStatuses api.AdapterStatusList, + requiredAdapters []string, + resourceGeneration int32, + now time.Time, + isReady bool, +) time.Time { + if !isReady { + // Use min(LRTs) across all required adapters. + // Fall back to now if a required adapter has no stored status or no LRT. + var minTime *time.Time + for _, adapterName := range requiredAdapters { + status, ok := findAdapterStatus(adapterStatuses, adapterName) + if !ok || status.LastReportTime == nil { + return now // safety: required adapter missing or has no timestamp + } + if minTime == nil || status.LastReportTime.Before(*minTime) { + t := *status.LastReportTime + minTime = &t + } + } + if minTime == nil { + return now + } + return *minTime + } + + var minTime *time.Time + for _, adapterName := range requiredAdapters { + status, ok := findAdapterStatus(adapterStatuses, adapterName) + if !ok { + return now // safety: required adapter missing + } + if status.LastReportTime == nil { + return now // safety: no timestamp + } + if status.ObservedGeneration != resourceGeneration { + continue // not at current gen, skip + } + if !adapterConditionsHasAvailableTrue(ctx, adapterName, status.Conditions) { + continue } + if minTime == nil || status.LastReportTime.Before(*minTime) { + t := *status.LastReportTime + minTime = &t + } + } + + if minTime == nil { + return now // safety fallback + } + return *minTime +} + +// buildReadyCondition computes the Ready ResourceCondition from the current adapter statuses, +// the previous condition (may be nil), and the evaluation time. +// +// observedTime is the triggering adapter's observed_time. It is used for LastTransitionTime +// on status changes and LastUpdatedTime on True→False transitions. +// +// Ready=True requires all required adapters to have Available=True at the current resource +// generation. LastUpdatedTime is the evaluation time when False (so callers can apply a +// freshness threshold), or the earliest adapter report time when True. +func buildReadyCondition( + ctx context.Context, + adapterStatuses api.AdapterStatusList, + requiredAdapters []string, + resourceGeneration int32, + existing *api.ResourceCondition, + now time.Time, + observedTime time.Time, +) api.ResourceCondition { + isReady := ComputeReadyCondition(ctx, adapterStatuses, requiredAdapters, resourceGeneration) + status := api.ConditionFalse + if isReady { + status = api.ConditionTrue + } + + cond := api.ResourceCondition{ + Type: api.ConditionTypeReady, + Status: status, + ObservedGeneration: resourceGeneration, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + } + + lut := computeReadyLastUpdated(ctx, adapterStatuses, requiredAdapters, resourceGeneration, now, isReady) + + // True→False: use the triggering adapter's observed time (when the failure was first seen). + prevStatus := api.ConditionFalse + if existing != nil { + prevStatus = existing.Status + } + if prevStatus == api.ConditionTrue && status == api.ConditionFalse { + lut = observedTime } - // Ready when all required adapters have Available=True at current generation - numRequired := len(requiredAdapters) - return numReady == numRequired + applyConditionHistory(&cond, existing, observedTime, lut) + + return cond } func BuildSyntheticConditions( + ctx context.Context, existingConditionsJSON []byte, adapterStatuses api.AdapterStatusList, requiredAdapters []string, resourceGeneration int32, now time.Time, + observedTime time.Time, + isLifecycleChange bool, ) (api.ResourceCondition, api.ResourceCondition) { var existingAvailable *api.ResourceCondition var existingReady *api.ResourceCondition @@ -258,66 +527,124 @@ func BuildSyntheticConditions( } } - isAvailable, minObservedGeneration := ComputeAvailableCondition(adapterStatuses, requiredAdapters) - availableStatus := api.ConditionFalse - if isAvailable { - availableStatus = api.ConditionTrue + // Zero adapters: trivially satisfied — both conditions are True. + if len(requiredAdapters) == 0 { + available := buildTrueCondition(api.ConditionTypeAvailable, existingAvailable, resourceGeneration, now) + ready := buildTrueCondition(api.ConditionTypeReady, existingReady, resourceGeneration, now) + return available, ready } - availableCondition := api.ResourceCondition{ - Type: api.ConditionTypeAvailable, - Status: availableStatus, - ObservedGeneration: minObservedGeneration, + + // Lifecycle change (Create / Replace): Available is frozen; Ready resets with lut=now. + if isLifecycleChange { + var available api.ResourceCondition + if existingAvailable != nil { + available = *existingAvailable + } else { + available = api.ResourceCondition{ + Type: api.ConditionTypeAvailable, + Status: api.ConditionFalse, + ObservedGeneration: resourceGeneration, + LastTransitionTime: now, + CreatedTime: now, + LastUpdatedTime: now, + } + } + ready := buildReadyConditionLifecycle(existingReady, resourceGeneration, now, observedTime) + return available, ready + } + + // Normal adapter-report path. + snapshot := scanAdapterAvailability(ctx, adapterStatuses, requiredAdapters) + available := buildAvailableCondition(snapshot, existingAvailable, resourceGeneration, now, observedTime) + ready := buildReadyCondition( + ctx, adapterStatuses, requiredAdapters, resourceGeneration, existingReady, now, observedTime) + + return available, ready +} + +// buildTrueCondition produces a True ResourceCondition, preserving history from existing. +// CreatedTime and LastTransitionTime are preserved when the existing condition was also True. +func buildTrueCondition( + condType string, + existing *api.ResourceCondition, + resourceGeneration int32, + now time.Time, +) api.ResourceCondition { + cond := api.ResourceCondition{ + Type: condType, + Status: api.ConditionTrue, + ObservedGeneration: resourceGeneration, LastTransitionTime: now, CreatedTime: now, LastUpdatedTime: now, } - preserveSyntheticCondition(&availableCondition, existingAvailable, now) - - isReady := ComputeReadyCondition(adapterStatuses, requiredAdapters, resourceGeneration) - readyStatus := api.ConditionFalse - if isReady { - readyStatus = api.ConditionTrue + if existing != nil { + if !existing.CreatedTime.IsZero() { + cond.CreatedTime = existing.CreatedTime + } + if existing.Status == api.ConditionTrue && !existing.LastTransitionTime.IsZero() { + cond.LastTransitionTime = existing.LastTransitionTime + } } - readyCondition := api.ResourceCondition{ + return cond +} + +// buildReadyConditionLifecycle produces Ready=False at the new generation with lut=now. +// History (CreatedTime, LastTransitionTime) is preserved via applyConditionHistory. +func buildReadyConditionLifecycle( + existing *api.ResourceCondition, + resourceGeneration int32, + now time.Time, + observedTime time.Time, +) api.ResourceCondition { + cond := api.ResourceCondition{ Type: api.ConditionTypeReady, - Status: readyStatus, + Status: api.ConditionFalse, ObservedGeneration: resourceGeneration, LastTransitionTime: now, CreatedTime: now, LastUpdatedTime: now, } - preserveSyntheticCondition(&readyCondition, existingReady, now) - - return availableCondition, readyCondition + applyConditionHistory(&cond, existing, observedTime, now) + return cond } -func preserveSyntheticCondition(target *api.ResourceCondition, existing *api.ResourceCondition, now time.Time) { +// applyConditionHistory copies stable timestamps and metadata from an existing condition. +// transitionTime is used for LastTransitionTime on status changes — callers pass the +// triggering adapter's observed_time so the timestamp reflects when the change was observed. +// lastUpdatedTime is used unconditionally for LastUpdatedTime — the caller is responsible +// for computing the correct value (e.g. now, computeReadyLastUpdated(...)). +func applyConditionHistory( + target *api.ResourceCondition, + existing *api.ResourceCondition, + transitionTime time.Time, + lastUpdatedTime time.Time, +) { if existing == nil { + target.LastUpdatedTime = lastUpdatedTime return } + if !existing.CreatedTime.IsZero() { + target.CreatedTime = existing.CreatedTime + } + + // LastTransitionTime only advances when the status value (True/False) changes. + // A change in ObservedGeneration alone does not constitute a transition. + if existing.Status == target.Status && !existing.LastTransitionTime.IsZero() { + target.LastTransitionTime = existing.LastTransitionTime + } else { + target.LastTransitionTime = transitionTime + } + + target.LastUpdatedTime = lastUpdatedTime + 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 } - target.LastTransitionTime = now - target.LastUpdatedTime = now } diff --git a/pkg/services/status_aggregation_test.go b/pkg/services/status_aggregation_test.go index 81f8e1b..a1a019f 100644 --- a/pkg/services/status_aggregation_test.go +++ b/pkg/services/status_aggregation_test.go @@ -1,12 +1,464 @@ package services import ( + "context" + "encoding/json" "testing" "time" + "gorm.io/datatypes" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" ) +// makeConditionsJSON marshals a slice of {Type, Status} pairs into datatypes.JSON. +func makeConditionsJSON(t *testing.T, conditions []struct{ Type, Status string }) datatypes.JSON { + t.Helper() + b, err := json.Marshal(conditions) + if err != nil { + t.Fatalf("failed to marshal conditions: %v", err) + } + return datatypes.JSON(b) +} + +// makeAdapterStatus builds an AdapterStatus with the given fields. +func makeAdapterStatus( + adapter string, gen int32, lastReportTime *time.Time, conditionsJSON datatypes.JSON, +) *api.AdapterStatus { + return &api.AdapterStatus{ + Adapter: adapter, + ObservedGeneration: gen, + LastReportTime: lastReportTime, + Conditions: conditionsJSON, + } +} + +func ptr(t time.Time) *time.Time { return &t } + +func TestComputeReadyLastUpdated_NotReady_NoAdapters(t *testing.T) { + now := time.Now() + // When isReady=false and a required adapter has no stored status, fall back to now. + result := computeReadyLastUpdated(context.Background(), nil, []string{"dns"}, 1, now, false) + if !result.Equal(now) { + t.Errorf("expected now (missing adapter fallback), got %v", result) + } +} + +func TestComputeReadyLastUpdated_NotReady_WithAdapters(t *testing.T) { + now := time.Now() + older := now.Add(-30 * time.Second) + newer := now.Add(-10 * time.Second) + + // Both required adapters present; one is False → isReady=false. + // LUT must be min(LRTs) = older, not now. + statuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(older), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + makeAdapterStatus("validator", 1, ptr(newer), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns", "validator"}, 1, now, false) + if !result.Equal(older) { + t.Errorf("expected min(LRTs)=%v, got %v", older, result) + } +} + +func TestComputeReadyLastUpdated_MissingAdapter(t *testing.T) { + now := time.Now() + statuses := api.AdapterStatusList{ + makeAdapterStatus("validator", 1, ptr(now.Add(-5*time.Second)), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + // "dns" is required but not in the list → safety fallback to now. + result := computeReadyLastUpdated(context.Background(), statuses, []string{"validator", "dns"}, 1, now, true) + if !result.Equal(now) { + t.Errorf("expected now (missing adapter), got %v", result) + } +} + +func TestComputeReadyLastUpdated_NilLastReportTime(t *testing.T) { + now := time.Now() + statuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, nil, makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns"}, 1, now, true) + if !result.Equal(now) { + t.Errorf("expected now (nil LastReportTime), got %v", result) + } +} + +func TestComputeReadyLastUpdated_WrongGeneration(t *testing.T) { + now := time.Now() + reportTime := now.Add(-10 * time.Second) + statuses := api.AdapterStatusList{ + // ObservedGeneration=1 but resourceGeneration=2 — skipped. + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + // All adapters skipped → minTime is nil → fallback to now. + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns"}, 2, now, true) + if !result.Equal(now) { + t.Errorf("expected now (wrong generation), got %v", result) + } +} + +func TestComputeReadyLastUpdated_AvailableFalse(t *testing.T) { + now := time.Now() + reportTime := now.Add(-10 * time.Second) + statuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + } + // Available=False → skipped → fallback to now. + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns"}, 1, now, true) + if !result.Equal(now) { + t.Errorf("expected now (Available=False), got %v", result) + } +} + +func TestComputeReadyLastUpdated_SingleQualifyingAdapter(t *testing.T) { + now := time.Now() + reportTime := now.Add(-30 * time.Second) + statuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns"}, 1, now, true) + if !result.Equal(reportTime) { + t.Errorf("expected %v, got %v", reportTime, result) + } +} + +func TestComputeReadyLastUpdated_MultipleAdapters_ReturnsMinimum(t *testing.T) { + now := time.Now() + older := now.Add(-60 * time.Second) + newer := now.Add(-10 * time.Second) + + statuses := api.AdapterStatusList{ + makeAdapterStatus("validator", 2, ptr(newer), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + makeAdapterStatus("dns", 2, ptr(older), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + result := computeReadyLastUpdated(context.Background(), statuses, []string{"validator", "dns"}, 2, now, true) + if !result.Equal(older) { + t.Errorf("expected minimum timestamp %v, got %v", older, result) + } +} + +// TestBuildSyntheticConditions_ReadyLastUpdatedThreaded verifies the full chain: +// when Ready=True, Ready.LastUpdatedTime equals the adapter's LastReportTime, +// not the evaluation time. +func TestBuildSyntheticConditions_ReadyLastUpdatedThreaded(t *testing.T) { + now := time.Now() + reportTime := now.Add(-30 * time.Second) + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + requiredAdapters := []string{"dns"} + resourceGeneration := int32(1) + + _, readyCondition := BuildSyntheticConditions( + context.Background(), []byte("[]"), adapterStatuses, requiredAdapters, resourceGeneration, now, now, false, + ) + + if !readyCondition.LastUpdatedTime.Equal(reportTime) { + t.Errorf("Ready.LastUpdatedTime = %v, want reportTime %v", + readyCondition.LastUpdatedTime, reportTime) + } +} + +// TestBuildSyntheticConditions_AvailableLastUpdatedTime_Stable verifies that +// Available's LastUpdatedTime is updated to min_lut (the adapter's LastReportTime) +// when all_at_X=true and the status stays True. Per spec §5.2, lut=min_lut for +// all cases except True→False transitions. +func TestBuildSyntheticConditions_AvailableLastUpdatedTime_Stable(t *testing.T) { + originalLastUpdated := time.Now().Add(-5 * time.Minute) + now := time.Now() + reportTime := now.Add(-10 * time.Second) + + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + requiredAdapters := []string{"dns"} + resourceGeneration := int32(1) + + // Simulate an existing Available condition. + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: originalLastUpdated, + LastTransitionTime: originalLastUpdated, + CreatedTime: originalLastUpdated, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, requiredAdapters, resourceGeneration, now, now, false) + + // Per spec §5.2: when all_at_X=true and status stays True, lut=min_lut=adapter's LastReportTime. + if !availableCondition.LastUpdatedTime.Equal(reportTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (min_lut from adapter's LastReportTime)", + availableCondition.LastUpdatedTime, reportTime) + } +} + +// TestBuildSyntheticConditions_AvailableLastUpdatedTime_UpdatesOnChange verifies that +// on a True→False transition, Available's LastUpdatedTime is set to the triggering +// adapter's observed_time (not now), per spec. +func TestBuildSyntheticConditions_AvailableLastUpdatedTime_UpdatesOnChange(t *testing.T) { + originalLastUpdated := time.Now().Add(-5 * time.Minute) + now := time.Now() + adapterReportTime := now.Add(-10 * time.Second) + + // Adapter now reports Available=False (changed from True). + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(adapterReportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + } + requiredAdapters := []string{"dns"} + resourceGeneration := int32(1) + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, // was True, now False + ObservedGeneration: 1, + LastUpdatedTime: originalLastUpdated, + LastTransitionTime: originalLastUpdated, + CreatedTime: originalLastUpdated, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + // observedTime = adapter's LastReportTime (the triggering adapter's observed_time). + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, requiredAdapters, + resourceGeneration, now, adapterReportTime, false) + + // Per spec: True→False transition → lut=observed_time (adapter's report time). + if !availableCondition.LastUpdatedTime.Equal(adapterReportTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (observed_time on True→False transition)", + availableCondition.LastUpdatedTime, adapterReportTime) + } +} + +// TestBuildSyntheticConditions_Available_MixedGenerations verifies that Available stays False +// when required adapters report at different observed generations (all_at_X=false per spec §5.2). +// With no existing Available state and mixed adapter generations, the condition defaults to False. +func TestBuildSyntheticConditions_Available_MixedGenerations(t *testing.T) { + now := time.Now() + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(now.Add(-10*time.Second)), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + makeAdapterStatus("validator", 2, ptr(now.Add(-5*time.Second)), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + // No existing Available condition; adapters at different generations → all_at_X=false. + availableCondition, _ := BuildSyntheticConditions( + context.Background(), []byte("[]"), adapterStatuses, []string{"dns", "validator"}, 2, now, now, false) + + // Per spec §5.2: all_at_X=false → no change. With no existing state, defaults to False. + if availableCondition.Status != api.ConditionFalse { + t.Errorf("Available.Status = %v, want False (all_at_X=false with mixed generations, no existing state)", + availableCondition.Status) + } + if availableCondition.ObservedGeneration != 2 { + t.Errorf("Available.ObservedGeneration = %v, want 2 (resource generation when all_at_X=false)", + availableCondition.ObservedGeneration) + } +} + +// TestBuildSyntheticConditions_AvailableLastUpdatedTime_UpdatesOnGenerationChange verifies that +// Available's LastUpdatedTime is set to min_lut (the adapter's LastReportTime) when the observed +// generation advances and status stays True. Per spec §5.2, lut=min_lut when all_at_X=true and +// the status stays True. +func TestBuildSyntheticConditions_AvailableLastUpdatedTime_UpdatesOnGenerationChange(t *testing.T) { + originalLastUpdated := time.Now().Add(-5 * time.Minute) + now := time.Now() + reportTime := now.Add(-10 * time.Second) + + // Adapter advances from gen1 to gen2; status stays True. + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 2, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, // was gen1, now gen2 + LastUpdatedTime: originalLastUpdated, + LastTransitionTime: originalLastUpdated, + CreatedTime: originalLastUpdated, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns"}, 2, now, now, false) + + if availableCondition.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True", availableCondition.Status) + } + // Per spec §5.2: all_at_X=true, stays True → lut=min_lut=adapter's LastReportTime. + if !availableCondition.LastUpdatedTime.Equal(reportTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (min_lut from adapter's LastReportTime when generation advances)", + availableCondition.LastUpdatedTime, reportTime) + } +} + +func TestValidateMandatoryConditions_InvalidStatus(t *testing.T) { + // P2: Available status not in {True, False, Unknown} → reject. + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: "InvalidValue", LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + if errorType != ConditionValidationErrorInvalidStatus { + t.Errorf("Expected errorType ConditionValidationErrorInvalidStatus, got: %s", errorType) + } + if conditionName != api.ConditionTypeAvailable { + t.Errorf("Expected conditionName %s, got: %s", api.ConditionTypeAvailable, conditionName) + } +} + +func TestValidateMandatoryConditions_InvalidStatusApplied(t *testing.T) { + // P2: Applied status not in {True, False, Unknown} → reject. + conditions := []api.AdapterCondition{ + {Type: api.ConditionTypeAvailable, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeApplied, Status: "bad-value", LastTransitionTime: time.Now()}, + {Type: api.ConditionTypeHealth, Status: api.AdapterConditionTrue, LastTransitionTime: time.Now()}, + } + + errorType, conditionName := ValidateMandatoryConditions(conditions) + + if errorType != ConditionValidationErrorInvalidStatus { + t.Errorf("Expected errorType ConditionValidationErrorInvalidStatus, got: %s", errorType) + } + if conditionName != api.ConditionTypeApplied { + t.Errorf("Expected conditionName %s, got: %s", api.ConditionTypeApplied, conditionName) + } +} + +// TestBuildSyntheticConditions_Available_AllAtX_True verifies that Available becomes True +// when all required adapters are at the same generation X and all report True (spec §5.2 T6). +func TestBuildSyntheticConditions_Available_AllAtX_True(t *testing.T) { + now := time.Now() + reportTime1 := now.Add(-20 * time.Second) + reportTime2 := now.Add(-10 * time.Second) + + // Both adapters at gen=1 (same X), both True. + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime1), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + makeAdapterStatus("validator", 1, ptr(reportTime2), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), []byte("[]"), adapterStatuses, []string{"dns", "validator"}, 2, now, now, false) + + // Per spec §5.2: all_at_X=true for X=1, all True → Available=True@1, lut=min_lut. + if availableCondition.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True (all_at_X=true, all True)", availableCondition.Status) + } + if availableCondition.ObservedGeneration != 1 { + t.Errorf("Available.ObservedGeneration = %v, want 1 (the common generation X)", availableCondition.ObservedGeneration) + } + // min_lut = min(reportTime1, reportTime2) = reportTime1 (earlier). + if !availableCondition.LastUpdatedTime.Equal(reportTime1) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (min_lut)", availableCondition.LastUpdatedTime, reportTime1) + } +} + +// TestBuildSyntheticConditions_Available_PreservesExistingTrueOnMixedGens verifies that +// when adapters are at different generations (all_at_X=false), an existing Available=True +// is preserved unchanged (spec §5.2 "True | false → no change"). +func TestBuildSyntheticConditions_Available_PreservesExistingTrueOnMixedGens(t *testing.T) { + originalLastUpdated := time.Now().Add(-2 * time.Minute) + now := time.Now() + + // dns at gen=1, validator at gen=2 → all_at_X=false. + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(now.Add(-30*time.Second)), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + makeAdapterStatus("validator", 2, ptr(now.Add(-10*time.Second)), + makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: originalLastUpdated, + LastTransitionTime: originalLastUpdated, + CreatedTime: originalLastUpdated, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns", "validator"}, 2, now, now, false) + + // Per spec §5.2: all_at_X=false → no change → preserve existing True@gen1. + if availableCondition.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True (preserved, all_at_X=false)", availableCondition.Status) + } + if availableCondition.ObservedGeneration != 1 { + t.Errorf("Available.ObservedGeneration = %v, want 1 (preserved)", availableCondition.ObservedGeneration) + } + if !availableCondition.LastUpdatedTime.Equal(originalLastUpdated) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (preserved when all_at_X=false)", + availableCondition.LastUpdatedTime, originalLastUpdated) + } +} + func TestMapAdapterToConditionType(t *testing.T) { tests := []struct { adapter string @@ -202,3 +654,398 @@ func TestValidateMandatoryConditions_EmptyConditionType(t *testing.T) { t.Errorf("Expected conditionName '', got: %s", conditionName) } } + +// TestBuildSyntheticConditions_ZeroAdapters_BothTrue verifies that when no required adapters +// are configured, both Available and Ready are trivially True (spec: zero required → satisfied). +func TestBuildSyntheticConditions_ZeroAdapters_BothTrue(t *testing.T) { + now := time.Now() + + available, ready := BuildSyntheticConditions( + context.Background(), nil, nil, nil, 1, now, now, false) + + if available.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True (zero required adapters → trivially satisfied)", available.Status) + } + if ready.Status != api.ConditionTrue { + t.Errorf("Ready.Status = %v, want True (zero required adapters → trivially satisfied)", ready.Status) + } + if available.ObservedGeneration != 1 { + t.Errorf("Available.ObservedGeneration = %v, want 1", available.ObservedGeneration) + } + if ready.ObservedGeneration != 1 { + t.Errorf("Ready.ObservedGeneration = %v, want 1", ready.ObservedGeneration) + } +} + +// TestBuildSyntheticConditions_LifecycleChange_AvailableFrozen verifies that on a lifecycle +// change (Create/Replace), Available is completely frozen and Ready resets with lut=now. +// False→False transition: Ready.ltt is preserved from existing. +func TestBuildSyntheticConditions_LifecycleChange_AvailableFrozen(t *testing.T) { + fixedTime := time.Now().Add(-5 * time.Minute) + now := time.Now() + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastUpdatedTime: fixedTime, + LastTransitionTime: fixedTime, + CreatedTime: fixedTime, + }, + { + Type: api.ConditionTypeReady, + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastUpdatedTime: fixedTime, + LastTransitionTime: fixedTime, + CreatedTime: fixedTime, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + // Adapter reports (should be ignored for Available on lifecycle change). + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 2, ptr(now.Add(-1*time.Second)), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + available, ready := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns"}, 2, now, now, true) + + // Available must be completely frozen (unchanged from existing). + if available.Status != api.ConditionFalse { + t.Errorf("Available.Status = %v, want False (frozen on lifecycle change)", available.Status) + } + if available.ObservedGeneration != 1 { + t.Errorf("Available.ObservedGeneration = %v, want 1 (frozen)", available.ObservedGeneration) + } + if !available.LastUpdatedTime.Equal(fixedTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (frozen)", available.LastUpdatedTime, fixedTime) + } + if !available.LastTransitionTime.Equal(fixedTime) { + t.Errorf("Available.LastTransitionTime = %v, want %v (frozen)", available.LastTransitionTime, fixedTime) + } + + // Ready must reset: status=False at new generation, lut=now. + if ready.Status != api.ConditionFalse { + t.Errorf("Ready.Status = %v, want False", ready.Status) + } + if ready.ObservedGeneration != 2 { + t.Errorf("Ready.ObservedGeneration = %v, want 2 (new generation)", ready.ObservedGeneration) + } + if !ready.LastUpdatedTime.Equal(now) { + t.Errorf("Ready.LastUpdatedTime = %v, want now=%v (lifecycle reset)", ready.LastUpdatedTime, now) + } + // False→False: ltt preserved from existing. + if !ready.LastTransitionTime.Equal(fixedTime) { + t.Errorf("Ready.LastTransitionTime = %v, want %v (preserved on False→False)", ready.LastTransitionTime, fixedTime) + } + // CreatedTime preserved from existing. + if !ready.CreatedTime.Equal(fixedTime) { + t.Errorf("Ready.CreatedTime = %v, want %v (preserved)", ready.CreatedTime, fixedTime) + } +} + +// TestBuildSyntheticConditions_LifecycleChange_ReadyTrueToFalse verifies that on a lifecycle +// change when Ready was True, Ready resets to False with lut=now and ltt=observedTime (now). +func TestBuildSyntheticConditions_LifecycleChange_ReadyTrueToFalse(t *testing.T) { + fixedTime := time.Now().Add(-5 * time.Minute) + now := time.Now() + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: fixedTime, + LastTransitionTime: fixedTime, + CreatedTime: fixedTime, + }, + { + Type: api.ConditionTypeReady, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: fixedTime, + LastTransitionTime: fixedTime, + CreatedTime: fixedTime, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + available, ready := BuildSyntheticConditions( + context.Background(), existingJSON, nil, []string{"dns"}, 2, now, now, true) + + // Available frozen at True@1. + if available.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True (frozen)", available.Status) + } + if available.ObservedGeneration != 1 { + t.Errorf("Available.ObservedGeneration = %v, want 1 (frozen)", available.ObservedGeneration) + } + + // Ready: True→False, lut=now, ltt=observedTime=now. + if ready.Status != api.ConditionFalse { + t.Errorf("Ready.Status = %v, want False", ready.Status) + } + if ready.ObservedGeneration != 2 { + t.Errorf("Ready.ObservedGeneration = %v, want 2", ready.ObservedGeneration) + } + if !ready.LastUpdatedTime.Equal(now) { + t.Errorf("Ready.LastUpdatedTime = %v, want now=%v", ready.LastUpdatedTime, now) + } + // True→False: ltt=observedTime=now. + if !ready.LastTransitionTime.Equal(now) { + t.Errorf("Ready.LastTransitionTime = %v, want now=%v (True→False transition)", ready.LastTransitionTime, now) + } + // CreatedTime preserved from existing. + if !ready.CreatedTime.Equal(fixedTime) { + t.Errorf("Ready.CreatedTime = %v, want %v (preserved)", ready.CreatedTime, fixedTime) + } +} + +// TestBuildSyntheticConditions_Available_TrueToFalse_Ltt verifies that on a True→False +// transition, Available's LastTransitionTime is set to observedTime (spec: ltt=obs_time). +// Complements TestBuildSyntheticConditions_AvailableLastUpdatedTime_UpdatesOnChange which +// only checked LastUpdatedTime. +func TestBuildSyntheticConditions_Available_TrueToFalse_Ltt(t *testing.T) { + fixedLtt := time.Now().Add(-5 * time.Minute) + now := time.Now() + adapterReportTime := now.Add(-10 * time.Second) + + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(adapterReportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: fixedLtt, + LastTransitionTime: fixedLtt, + CreatedTime: fixedLtt, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns"}, 1, now, adapterReportTime, false) + + if availableCondition.Status != api.ConditionFalse { + t.Errorf("Available.Status = %v, want False", availableCondition.Status) + } + // True→False: ltt=observedTime=adapterReportTime. + if !availableCondition.LastTransitionTime.Equal(adapterReportTime) { + t.Errorf("Available.LastTransitionTime = %v, want %v (observedTime on True→False)", + availableCondition.LastTransitionTime, adapterReportTime) + } + // True→False: lut=observedTime=adapterReportTime. + if !availableCondition.LastUpdatedTime.Equal(adapterReportTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (observedTime on True→False)", + availableCondition.LastUpdatedTime, adapterReportTime) + } +} + +// TestBuildSyntheticConditions_Available_FalseToTrue_Ltt verifies that on a False→True +// transition, Available's LastTransitionTime is set to observedTime and LastUpdatedTime +// is set to min(LRTs) (spec: ltt=obs_time, lut=min_lut on False→True). +func TestBuildSyntheticConditions_Available_FalseToTrue_Ltt(t *testing.T) { + fixedLtt := time.Now().Add(-5 * time.Minute) + now := time.Now() + reportTime1 := now.Add(-20 * time.Second) // earlier — will be min(LRTs) + reportTime2 := now.Add(-10 * time.Second) + observedTime := now.Add(-5 * time.Second) // distinct from both LRTs + + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime1), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + makeAdapterStatus("validator", 1, ptr(reportTime2), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastUpdatedTime: fixedLtt, + LastTransitionTime: fixedLtt, + CreatedTime: fixedLtt, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns", "validator"}, 1, now, observedTime, false) + + if availableCondition.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True (all at same gen, all True)", availableCondition.Status) + } + // False→True: ltt=observedTime (status transition). + if !availableCondition.LastTransitionTime.Equal(observedTime) { + t.Errorf("Available.LastTransitionTime = %v, want %v (observedTime on False→True)", + availableCondition.LastTransitionTime, observedTime) + } + // False→True: lut=min(LRTs)=reportTime1. + if !availableCondition.LastUpdatedTime.Equal(reportTime1) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (min_lut on False→True)", + availableCondition.LastUpdatedTime, reportTime1) + } +} + +// TestBuildSyntheticConditions_Available_TrueToTrue_LttPreserved verifies that when +// Available stays True, LastTransitionTime is preserved from the existing condition +// (spec: ltt=— on True→True, no change). +func TestBuildSyntheticConditions_Available_TrueToTrue_LttPreserved(t *testing.T) { + fixedLtt := time.Now().Add(-5 * time.Minute) + now := time.Now() + reportTime := now.Add(-10 * time.Second) + + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionTrue, + ObservedGeneration: 1, + LastUpdatedTime: fixedLtt, + LastTransitionTime: fixedLtt, + CreatedTime: fixedLtt, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns"}, 1, now, now, false) + + if availableCondition.Status != api.ConditionTrue { + t.Errorf("Available.Status = %v, want True", availableCondition.Status) + } + // True→True: ltt preserved from existing. + if !availableCondition.LastTransitionTime.Equal(fixedLtt) { + t.Errorf("Available.LastTransitionTime = %v, want %v (preserved on True→True)", + availableCondition.LastTransitionTime, fixedLtt) + } + // True→True: lut=min(LRTs)=reportTime. + if !availableCondition.LastUpdatedTime.Equal(reportTime) { + t.Errorf("Available.LastUpdatedTime = %v, want %v (min_lut on True→True)", + availableCondition.LastUpdatedTime, reportTime) + } +} + +// TestBuildSyntheticConditions_Available_FalseToFalse_LttPreserved verifies that when +// Available stays False with all_at_X=true (consistent gen, adapter reports False), +// LastTransitionTime is preserved from the existing condition (spec: ltt=— on False→False). +// This is distinct from the all_at_X=false path (TestBuildSyntheticConditions_Available_MixedGenerations) +// where buildAvailableCondition is never reached. +func TestBuildSyntheticConditions_Available_FalseToFalse_LttPreserved(t *testing.T) { + fixedLtt := time.Now().Add(-5 * time.Minute) + now := time.Now() + reportTime := now.Add(-10 * time.Second) + + // Single adapter at gen=1 reports False → all_at_X=true, consistent=true, newStatus=False. + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(reportTime), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + } + + existingConditions := []api.ResourceCondition{ + { + Type: api.ConditionTypeAvailable, + Status: api.ConditionFalse, + ObservedGeneration: 1, + LastUpdatedTime: fixedLtt, + LastTransitionTime: fixedLtt, + CreatedTime: fixedLtt, + }, + } + existingJSON, err := json.Marshal(existingConditions) + if err != nil { + t.Fatalf("failed to marshal existing conditions: %v", err) + } + + availableCondition, _ := BuildSyntheticConditions( + context.Background(), existingJSON, adapterStatuses, []string{"dns"}, 1, now, now, false) + + if availableCondition.Status != api.ConditionFalse { + t.Errorf("Available.Status = %v, want False", availableCondition.Status) + } + // False→False: ltt preserved from existing (no status transition occurred). + if !availableCondition.LastTransitionTime.Equal(fixedLtt) { + t.Errorf("Available.LastTransitionTime = %v, want %v (preserved on False→False, all_at_X=true path)", + availableCondition.LastTransitionTime, fixedLtt) + } +} + +// TestComputeReadyLastUpdated_NotReady_NilLastReportTime verifies that when isReady=false +// and a required adapter has nil LastReportTime, the function falls back to now. +// Complements TestComputeReadyLastUpdated_NilLastReportTime which only tests isReady=true. +func TestComputeReadyLastUpdated_NotReady_NilLastReportTime(t *testing.T) { + now := time.Now() + statuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, nil, makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + } + result := computeReadyLastUpdated(context.Background(), statuses, []string{"dns"}, 1, now, false) + if !result.Equal(now) { + t.Errorf("expected now (nil LastReportTime, isReady=false), got %v", result) + } +} + +// TestBuildSyntheticConditions_ReadyStaysFalse_LutIsMinLRT verifies that when Ready stays +// False and all required adapters have real LRTs, Ready.LastUpdatedTime equals min(LRTs) +// rather than now (spec: lut=min(LRTs) when Ready stays False with real adapter LRTs). +func TestBuildSyntheticConditions_ReadyStaysFalse_LutIsMinLRT(t *testing.T) { + now := time.Now() + dnsLRT := now.Add(-30 * time.Second) // earlier — will be min(LRTs) + validatorLRT := now.Add(-10 * time.Second) // later + + // dns is Available=False → Ready cannot be True. + adapterStatuses := api.AdapterStatusList{ + makeAdapterStatus("dns", 1, ptr(dnsLRT), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionFalse)}, + })), + makeAdapterStatus("validator", 1, ptr(validatorLRT), makeConditionsJSON(t, []struct{ Type, Status string }{ + {api.ConditionTypeAvailable, string(api.AdapterConditionTrue)}, + })), + } + + _, readyCondition := BuildSyntheticConditions( + context.Background(), []byte("[]"), adapterStatuses, []string{"dns", "validator"}, 1, now, now, false) + + if readyCondition.Status != api.ConditionFalse { + t.Errorf("Ready.Status = %v, want False (dns is Available=False)", readyCondition.Status) + } + // Ready stays False: lut=min(all LRTs)=dnsLRT. + if !readyCondition.LastUpdatedTime.Equal(dnsLRT) { + t.Errorf("Ready.LastUpdatedTime = %v, want %v (min(LRTs) when Ready stays False)", + readyCondition.LastUpdatedTime, dnsLRT) + } +} diff --git a/test/integration/adapter_status_test.go b/test/integration/adapter_status_test.go index 2b2a15c..a18296e 100644 --- a/test/integration/adapter_status_test.go +++ b/test/integration/adapter_status_test.go @@ -432,9 +432,9 @@ func TestAdapterStatusIdempotency(t *testing.T) { To(Equal(openapi.AdapterConditionStatusTrue), "Conditions should be updated to latest") } -// TestClusterStatusPost_FirstUnknownAccepted tests that first status reports with Unknown -// Available condition are accepted, subsequent ones are rejected (HYPERFLEET-657) -func TestClusterStatusPost_FirstUnknownAccepted(t *testing.T) { +// TestClusterStatusPost_UnknownAvailableAlwaysDiscarded tests that status reports with +// Available=Unknown are always discarded (P3 rule), regardless of whether it's the first report. +func TestClusterStatusPost_UnknownAvailableAlwaysDiscarded(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -468,30 +468,26 @@ func TestClusterStatusPost_FirstUnknownAccepted(t *testing.T) { nil, ) - // First report with Unknown Available condition: should be accepted + // First report with Unknown Available condition: discarded per P3 (204 No Content) 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.StatusCreated), "Expected 201 Created for first status with Unknown Available condition") + To(Equal(http.StatusNoContent), "Expected 204 No Content: Available=Unknown is always discarded") - // Verify status was stored + // Verify status was NOT stored listResp, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) Expect(err).NotTo(HaveOccurred()) Expect(listResp.JSON200).NotTo(BeNil()) - found := false for _, s := range listResp.JSON200.Items { - if s.Adapter == "test-adapter-unknown" { - found = true - break - } + Expect(s.Adapter).NotTo(Equal("test-adapter-unknown"), + "Status with Available=Unknown must not be stored") } - Expect(found).To(BeTrue(), "First status with Unknown Available condition should be stored") - // Subsequent report with same adapter: should be rejected (204 No Content) + // Subsequent report with same adapter: also discarded (204 No Content) resp2, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -501,9 +497,9 @@ func TestClusterStatusPost_FirstUnknownAccepted(t *testing.T) { To(Equal(http.StatusNoContent), "Expected 204 No Content for subsequent Unknown status report") } -// TestNodePoolStatusPost_FirstUnknownAccepted tests that first status reports with Unknown -// Available condition are accepted, subsequent ones are rejected (HYPERFLEET-657) -func TestNodePoolStatusPost_FirstUnknownAccepted(t *testing.T) { +// TestNodePoolStatusPost_UnknownAvailableAlwaysDiscarded tests that status reports with +// Available=Unknown are always discarded (P3 rule), regardless of whether it's the first report. +func TestNodePoolStatusPost_UnknownAvailableAlwaysDiscarded(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -537,32 +533,28 @@ func TestNodePoolStatusPost_FirstUnknownAccepted(t *testing.T) { nil, ) - // First report with Unknown Available condition: should be accepted + // First report with Unknown Available condition: discarded per P3 (204 No Content) 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.StatusCreated), "Expected 201 Created for first status with Unknown Available condition") + To(Equal(http.StatusNoContent), "Expected 204 No Content: Available=Unknown is always discarded") - // Verify status was stored + // Verify 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()) - found := false for _, s := range listResp.JSON200.Items { - if s.Adapter == "test-nodepool-adapter-unknown" { - found = true - break - } + Expect(s.Adapter).NotTo(Equal("test-nodepool-adapter-unknown"), + "Status with Available=Unknown must not be stored") } - Expect(found).To(BeTrue(), "First status with Unknown Available condition should be stored") - // Subsequent report with same adapter: should be rejected (204 No Content) + // Subsequent report with same adapter: also discarded (204 No Content) resp2, err := client.PostNodePoolStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -572,8 +564,8 @@ func TestNodePoolStatusPost_FirstUnknownAccepted(t *testing.T) { To(Equal(http.StatusNoContent), "Expected 204 No Content for subsequent Unknown status report") } -// TestClusterStatusPost_MultipleConditionsWithUnknownAvailable tests that -// first report with Unknown Available is accepted, subsequent ones rejected (HYPERFLEET-657) +// TestClusterStatusPost_MultipleConditionsWithUnknownAvailable tests that reports with +// Available=Unknown are discarded (P3 rule) even when other conditions are present. func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) { h, client := test.RegisterIntegration(t) @@ -616,16 +608,16 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) nil, ) - // First report with Unknown Available condition: should be accepted + // First report with Available=Unknown among multiple conditions: discarded per P3 (204 No Content) 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.StatusCreated), - "Expected 201 Created for first report with Available=Unknown among multiple conditions") + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), + "Expected 204 No Content: Available=Unknown is always discarded regardless of other conditions") - // Subsequent report: should be rejected (204 No Content) + // Subsequent report: also discarded (204 No Content) resp2, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), @@ -883,9 +875,9 @@ func TestClusterStatusPost_MissingMandatoryConditionsRejected(t *testing.T) { Expect(storedConditionTypes["Health2"]).To(BeFalse(), "Health2 should not be present") } -// TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected tests that first status with -// Unknown Available is accepted, subsequent ones are rejected -func TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected(t *testing.T) { +// TestClusterStatusPost_UnknownAvailableNeverStored tests that Available=Unknown reports +// are never stored, regardless of first or subsequent attempts. +func TestClusterStatusPost_UnknownAvailableNeverStored(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -895,7 +887,7 @@ func TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected(t *testing.T) cluster, err := h.Factories.NewClusters(h.NewID()) Expect(err).NotTo(HaveOccurred()) - // Send first status with all mandatory conditions but Available=Unknown + // Send status with all mandatory conditions but Available=Unknown statusWithUnknown := newAdapterStatusRequest( "adapter1", cluster.Generation, @@ -922,29 +914,30 @@ func TestClusterStatusPost_FirstUnknownAcceptedSubsequentRejected(t *testing.T) nil, ) - // First report: should be accepted + // First report: discarded per P3 (204 No Content) resp, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusWithUnknown), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusCreated), "First status with Unknown Available should be accepted") + Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), + "Available=Unknown status must always be discarded") - // Verify status was stored + // Verify status was NOT stored respGet, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) Expect(err).NotTo(HaveOccurred()) Expect(respGet.StatusCode()).To(Equal(http.StatusOK)) Expect(respGet.JSON200).ToNot(BeNil()) - Expect(len(respGet.JSON200.Items)).To(Equal(1), "First status with Unknown Available should be stored") + Expect(len(respGet.JSON200.Items)).To(Equal(0), "Available=Unknown status must not be stored") - // Subsequent report: should be rejected (204 No Content) + // Subsequent report: also discarded (204 No Content) resp2, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusWithUnknown), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred()) Expect(resp2.StatusCode()).To(Equal(http.StatusNoContent), - "Subsequent status with Unknown Available should be rejected") + "Subsequent Available=Unknown status must also be discarded") } // TestClusterStatusPost_DuplicateConditionsRejected tests that adapter status updates