diff --git a/pkg/services/cluster.go b/pkg/services/cluster.go index 9d85135..9a5fd5c 100644 --- a/pkg/services/cluster.go +++ b/pkg/services/cluster.go @@ -237,7 +237,8 @@ func (s *sqlClusterService) UpdateClusterStatusFromAdapters( } // ProcessAdapterStatus handles the business logic for adapter status: -// - If Available condition is "Unknown": returns (nil, nil) indicating no-op +// - If Available is "Unknown" and a status already exists: returns (nil, nil) as no-op +// - If Available is "Unknown" and no status exists (first report): upserts the status // - Otherwise: upserts the status and triggers aggregation func (s *sqlClusterService) ProcessAdapterStatus( ctx context.Context, clusterID string, adapterStatus *api.AdapterStatus, @@ -272,8 +273,13 @@ func (s *sqlClusterService) ProcessAdapterStatus( hasAvailableCondition = true if cond.Status == api.AdapterConditionUnknown { - // Available condition is "Unknown", return nil to indicate no-op - return nil, nil + if existingStatus != nil { + // Available condition is "Unknown" and a status already exists, return nil to indicate no-op + return nil, nil + } + // First report from this adapter: allow storing even with Available=Unknown + // but skip aggregation since Unknown should not affect cluster-level conditions + hasAvailableCondition = false } } diff --git a/pkg/services/cluster_test.go b/pkg/services/cluster_test.go index 8d972d0..a66cfb4 100644 --- a/pkg/services/cluster_test.go +++ b/pkg/services/cluster_test.go @@ -163,8 +163,8 @@ func (d *mockAdapterStatusDao) All(ctx context.Context) (api.AdapterStatusList, var _ dao.AdapterStatusDao = &mockAdapterStatusDao{} -// TestProcessAdapterStatus_UnknownCondition tests that Unknown Available condition returns nil (no-op) -func TestProcessAdapterStatus_UnknownCondition(t *testing.T) { +// TestProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored +func TestProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { RegisterTestingT(t) clusterDao := newMockClusterDao() @@ -186,21 +186,72 @@ func TestProcessAdapterStatus_UnknownCondition(t *testing.T) { } conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() adapterStatus := &api.AdapterStatus{ ResourceType: "Cluster", ResourceID: clusterID, Adapter: "test-adapter", Conditions: conditionsJSON, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil for Unknown status") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") + Expect(result.Adapter).To(Equal("test-adapter")) - // Verify nothing was stored + // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") + Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") +} + +// TestProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown Available conditions are discarded +func TestProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Pre-populate an existing adapter status to simulate a previously stored report + conditions := []api.AdapterCondition{ + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + existingStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + + // Now send another Unknown status report + newAdapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, newAdapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Subsequent Unknown status should be discarded") } // TestProcessAdapterStatus_TrueCondition tests that True Available condition upserts and aggregates @@ -383,8 +434,9 @@ func TestProcessAdapterStatus_NoAvailableCondition(t *testing.T) { "Cluster status conditions should not be overwritten when adapter status lacks Available") } -// TestProcessAdapterStatus_MultipleConditions_AvailableUnknown tests multiple conditions with Available=Unknown -func TestProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *testing.T) { +// TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that the first report with +// multiple conditions including Available=Unknown is stored +func TestProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) clusterDao := newMockClusterDao() @@ -416,21 +468,90 @@ func TestProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *testing.T) } conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() adapterStatus := &api.AdapterStatus{ ResourceType: "Cluster", ResourceID: clusterID, Adapter: "test-adapter", Conditions: conditionsJSON, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil when Available=Unknown") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") - // Verify nothing was stored + // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "Cluster", clusterID) - Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") + Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") +} + +// TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent reports +// with multiple conditions including Available=Unknown are discarded +func TestProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *testing.T) { + RegisterTestingT(t) + + clusterDao := newMockClusterDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := testAdapterConfig() + service := NewClusterService(clusterDao, adapterStatusDao, config) + + ctx := context.Background() + clusterID := testClusterID + + // Pre-populate an existing adapter status + existingConditions := []api.AdapterCondition{ + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + existingConditionsJSON, _ := json.Marshal(existingConditions) + + now := time.Now() + existingStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: existingConditionsJSON, + CreatedTime: &now, + } + _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + + // Now send another report with multiple conditions including Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: "Ready", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + { + Type: "Progressing", + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "Cluster", + ResourceID: clusterID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, clusterID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Subsequent Available=Unknown should be discarded") } func TestClusterAvailableReadyTransitions(t *testing.T) { diff --git a/pkg/services/node_pool.go b/pkg/services/node_pool.go index 30dee7a..c341de4 100644 --- a/pkg/services/node_pool.go +++ b/pkg/services/node_pool.go @@ -238,7 +238,8 @@ func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters( } // ProcessAdapterStatus handles the business logic for adapter status: -// - If Available condition is "Unknown": returns (nil, nil) indicating no-op +// - If Available is "Unknown" and a status already exists: returns (nil, nil) as no-op +// - If Available is "Unknown" and no status exists (first report): upserts the status // - Otherwise: upserts the status and triggers aggregation func (s *sqlNodePoolService) ProcessAdapterStatus( ctx context.Context, nodePoolID string, adapterStatus *api.AdapterStatus, @@ -273,8 +274,13 @@ func (s *sqlNodePoolService) ProcessAdapterStatus( hasAvailableCondition = true if cond.Status == api.AdapterConditionUnknown { - // Available condition is "Unknown", return nil to indicate no-op - return nil, nil + if existingStatus != nil { + // Available condition is "Unknown" and a status already exists, return nil to indicate no-op + return nil, nil + } + // First report from this adapter: allow storing even with Available=Unknown + // but skip aggregation since Unknown should not affect nodepool-level conditions + hasAvailableCondition = false } } diff --git a/pkg/services/node_pool_test.go b/pkg/services/node_pool_test.go index b72c3aa..9c5f39e 100644 --- a/pkg/services/node_pool_test.go +++ b/pkg/services/node_pool_test.go @@ -80,8 +80,8 @@ func (d *mockNodePoolDao) All(ctx context.Context) (api.NodePoolList, error) { var _ dao.NodePoolDao = &mockNodePoolDao{} -// TestNodePoolProcessAdapterStatus_UnknownCondition tests that Unknown Available condition returns nil (no-op) -func TestNodePoolProcessAdapterStatus_UnknownCondition(t *testing.T) { +// TestNodePoolProcessAdapterStatus_FirstUnknownCondition tests that the first Unknown Available condition is stored +func TestNodePoolProcessAdapterStatus_FirstUnknownCondition(t *testing.T) { RegisterTestingT(t) nodePoolDao := newMockNodePoolDao() @@ -103,21 +103,72 @@ func TestNodePoolProcessAdapterStatus_UnknownCondition(t *testing.T) { } conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() adapterStatus := &api.AdapterStatus{ ResourceType: "NodePool", ResourceID: nodePoolID, Adapter: "test-adapter", Conditions: conditionsJSON, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil for Unknown status") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") + Expect(result.Adapter).To(Equal("test-adapter")) - // Verify nothing was stored + // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) - Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") + Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") +} + +// TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition tests that subsequent Unknown conditions are discarded +func TestNodePoolProcessAdapterStatus_SubsequentUnknownCondition(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := testNodePoolAdapterConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := testNodePoolID + + // Pre-populate an existing adapter status + conditions := []api.AdapterCondition{ + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + now := time.Now() + existingStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + + // Now send another Unknown status report + newAdapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + CreatedTime: &now, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, newAdapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Subsequent Unknown status should be discarded") } // TestNodePoolProcessAdapterStatus_TrueCondition tests that True Available condition upserts and aggregates @@ -171,8 +222,9 @@ func TestNodePoolProcessAdapterStatus_TrueCondition(t *testing.T) { Expect(len(storedStatuses)).To(Equal(1), "Status should be stored for True condition") } -// TestNodePoolProcessAdapterStatus_MultipleConditions_AvailableUnknown tests multiple conditions with Available=Unknown -func TestNodePoolProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *testing.T) { +// TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown tests that the first report +// with multiple conditions including Available=Unknown is stored +func TestNodePoolProcessAdapterStatus_FirstMultipleConditions_AvailableUnknown(t *testing.T) { RegisterTestingT(t) nodePoolDao := newMockNodePoolDao() @@ -199,21 +251,85 @@ func TestNodePoolProcessAdapterStatus_MultipleConditions_AvailableUnknown(t *tes } conditionsJSON, _ := json.Marshal(conditions) + now := time.Now() adapterStatus := &api.AdapterStatus{ ResourceType: "NodePool", ResourceID: nodePoolID, Adapter: "test-adapter", Conditions: conditionsJSON, + CreatedTime: &now, } result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) Expect(err).To(BeNil()) - Expect(result).To(BeNil(), "ProcessAdapterStatus should return nil when Available=Unknown") + Expect(result).ToNot(BeNil(), "First report with Available=Unknown should be stored") - // Verify nothing was stored + // Verify the status was stored storedStatuses, _ := adapterStatusDao.FindByResource(ctx, "NodePool", nodePoolID) - Expect(len(storedStatuses)).To(Equal(0), "No status should be stored for Unknown") + Expect(len(storedStatuses)).To(Equal(1), "First Unknown status should be stored") +} + +// TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown tests that subsequent +// reports with multiple conditions including Available=Unknown are discarded +func TestNodePoolProcessAdapterStatus_SubsequentMultipleConditions_AvailableUnknown(t *testing.T) { + RegisterTestingT(t) + + nodePoolDao := newMockNodePoolDao() + adapterStatusDao := newMockAdapterStatusDao() + + config := testNodePoolAdapterConfig() + service := NewNodePoolService(nodePoolDao, adapterStatusDao, config) + + ctx := context.Background() + nodePoolID := testNodePoolID + + // Pre-populate an existing adapter status + existingConditions := []api.AdapterCondition{ + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + existingConditionsJSON, _ := json.Marshal(existingConditions) + + now := time.Now() + existingStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: existingConditionsJSON, + CreatedTime: &now, + } + _, _ = adapterStatusDao.Upsert(ctx, existingStatus) + + // Now send another report with multiple conditions including Available=Unknown + conditions := []api.AdapterCondition{ + { + Type: conditionTypeReady, + Status: api.AdapterConditionTrue, + LastTransitionTime: time.Now(), + }, + { + Type: conditionTypeAvailable, + Status: api.AdapterConditionUnknown, + LastTransitionTime: time.Now(), + }, + } + conditionsJSON, _ := json.Marshal(conditions) + + adapterStatus := &api.AdapterStatus{ + ResourceType: "NodePool", + ResourceID: nodePoolID, + Adapter: "test-adapter", + Conditions: conditionsJSON, + } + + result, err := service.ProcessAdapterStatus(ctx, nodePoolID, adapterStatus) + + Expect(err).To(BeNil()) + Expect(result).To(BeNil(), "Subsequent Available=Unknown should be discarded") } func TestNodePoolAvailableReadyTransitions(t *testing.T) { diff --git a/test/integration/adapter_status_test.go b/test/integration/adapter_status_test.go index 6b2a114..9351510 100644 --- a/test/integration/adapter_status_test.go +++ b/test/integration/adapter_status_test.go @@ -319,8 +319,9 @@ func TestAdapterStatusIdempotency(t *testing.T) { To(Equal(openapi.AdapterConditionStatusTrue), "Conditions should be updated to latest") } -// TestClusterStatusPost_UnknownReturns204 tests that posting Unknown Available status returns 204 No Content -func TestClusterStatusPost_UnknownReturns204(t *testing.T) { +// TestClusterStatusPost_UnknownReturns201ThenSubsequent204 tests that the first Unknown Available +// status report is stored (201), while subsequent Unknown reports return 204 No Content. +func TestClusterStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -344,27 +345,42 @@ func TestClusterStatusPost_UnknownReturns204(t *testing.T) { nil, ) + // First report: should be stored (201 Created) resp, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) Expect(resp.StatusCode()). - To(Equal(http.StatusNoContent), "Expected 204 No Content for Unknown status") + To(Equal(http.StatusCreated), "Expected 201 Created for first Unknown status report") - // Verify the status was NOT stored + // Verify the status was stored listResp, err := client.GetClusterStatusesWithResponse(ctx, cluster.ID, nil, test.WithAuthToken(ctx)) Expect(err).NotTo(HaveOccurred()) Expect(listResp.JSON200).NotTo(BeNil()) - // Check that no adapter status with "test-adapter-unknown" exists + found := false for _, s := range listResp.JSON200.Items { - Expect(s.Adapter).NotTo(Equal("test-adapter-unknown"), "Unknown status should not be stored") + if s.Adapter == "test-adapter-unknown" { + found = true + break + } } + Expect(found).To(BeTrue(), "First Unknown status report should be stored") + + // Subsequent report with same adapter: should be no-op (204 No Content) + resp2, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) + Expect(resp2.StatusCode()). + To(Equal(http.StatusNoContent), "Expected 204 No Content for subsequent Unknown status report") } -// TestNodePoolStatusPost_UnknownReturns204 tests that posting Unknown Available status returns 204 No Content -func TestNodePoolStatusPost_UnknownReturns204(t *testing.T) { +// TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204 tests that the first Unknown Available +// status report is stored (201), while subsequent Unknown reports return 204 No Content. +func TestNodePoolStatusPost_UnknownReturns201ThenSubsequent204(t *testing.T) { h, client := test.RegisterIntegration(t) account := h.NewRandAccount() @@ -388,26 +404,39 @@ func TestNodePoolStatusPost_UnknownReturns204(t *testing.T) { nil, ) + // First report: should be stored (201 Created) resp, err := client.PostNodePoolStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred(), "Error posting nodepool status: %v", err) Expect(resp.StatusCode()). - To(Equal(http.StatusNoContent), "Expected 204 No Content for Unknown status") + To(Equal(http.StatusCreated), "Expected 201 Created for first Unknown status report") - // Verify the status was NOT stored + // Verify the status was stored listResp, err := client.GetNodePoolsStatusesWithResponse( ctx, nodePool.OwnerID, nodePool.ID, nil, test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred()) Expect(listResp.JSON200).NotTo(BeNil()) - // Check that no adapter status with "test-nodepool-adapter-unknown" exists + found := false for _, s := range listResp.JSON200.Items { - Expect(s.Adapter).NotTo(Equal("test-nodepool-adapter-unknown"), - "Unknown status should not be stored") + if s.Adapter == "test-nodepool-adapter-unknown" { + found = true + break + } } + Expect(found).To(BeTrue(), "First Unknown status report should be stored") + + // Subsequent report with same adapter: should be no-op (204 No Content) + resp2, 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(resp2.StatusCode()). + To(Equal(http.StatusNoContent), "Expected 204 No Content for subsequent Unknown status report") } // TestClusterStatusPost_MultipleConditionsWithUnknownAvailable tests that @@ -444,13 +473,23 @@ func TestClusterStatusPost_MultipleConditionsWithUnknownAvailable(t *testing.T) nil, ) + // First report: should be stored (201 Created) even with Available=Unknown resp, err := client.PostClusterStatusesWithResponse( ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), ) Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) - Expect(resp.StatusCode()).To(Equal(http.StatusNoContent), - "Expected 204 No Content when Available=Unknown among multiple conditions") + Expect(resp.StatusCode()).To(Equal(http.StatusCreated), + "Expected 201 Created for first report with Available=Unknown among multiple conditions") + + // Subsequent report: should be no-op (204 No Content) + resp2, err := client.PostClusterStatusesWithResponse( + ctx, cluster.ID, + openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx), + ) + Expect(err).NotTo(HaveOccurred(), "Error posting cluster status: %v", err) + Expect(resp2.StatusCode()).To(Equal(http.StatusNoContent), + "Expected 204 No Content for subsequent report with Available=Unknown among multiple conditions") } // TestAdapterStatusPagingEdgeCases tests edge cases in pagination