From eb6c8fe6df3a093ef18d624ffaad878330413848 Mon Sep 17 00:00:00 2001 From: mblos Date: Thu, 12 Mar 2026 16:13:42 +0100 Subject: [PATCH 1/8] commitment integration test --- Makefile | 11 +- api/v1alpha1/reservation_types.go | 4 + .../files/crds/cortex.cloud_reservations.yaml | 4 + .../commitments/api_change_commitments.go | 123 +- .../api_change_commitments_test.go | 1668 +++++++++++++++-- .../commitments/reservation_manager.go | 33 +- .../reservations/commitments/state.go | 2 +- 7 files changed, 1638 insertions(+), 207 deletions(-) diff --git a/Makefile b/Makefile index b63e2e267..148130f0f 100644 --- a/Makefile +++ b/Makefile @@ -25,8 +25,8 @@ lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes "$(GOLANGCI_LINT)" run --fix .PHONY: test -test: ## Run all tests. - go test ./... +test: gotestsum ## Run all tests with summary. + $(GOTESTSUM) --format testname ./... .PHONY: generate generate: deepcopy crds ## Regenerate CRDs and DeepCopy after API type changes. @@ -45,9 +45,11 @@ $(LOCALBIN): CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen GOLANGCI_LINT = $(LOCALBIN)/golangci-lint +GOTESTSUM = $(LOCALBIN)/gotestsum CONTROLLER_TOOLS_VERSION ?= v0.20.0 GOLANGCI_LINT_VERSION ?= v2.9.0 +GOTESTSUM_VERSION ?= v1.13.0 .PHONY: controller-gen controller-gen: $(CONTROLLER_GEN) ## Download controller-gen locally if necessary. @@ -59,6 +61,11 @@ golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary. $(GOLANGCI_LINT): $(LOCALBIN) $(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/v2/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION)) +.PHONY: gotestsum +gotestsum: $(GOTESTSUM) ## Download gotestsum locally if necessary. +$(GOTESTSUM): $(LOCALBIN) + $(call go-install-tool,$(GOTESTSUM),gotest.tools/gotestsum,$(GOTESTSUM_VERSION)) + # go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist # $1 - target path with name of binary # $2 - package url which can be installed diff --git a/api/v1alpha1/reservation_types.go b/api/v1alpha1/reservation_types.go index 913a93a8f..5e6a30b01 100644 --- a/api/v1alpha1/reservation_types.go +++ b/api/v1alpha1/reservation_types.go @@ -54,6 +54,10 @@ type CommittedResourceReservationSpec struct { // +kubebuilder:validation:Optional ResourceName string `json:"resourceName,omitempty"` + // CommitmentUUID is the UUID of the commitment that this reservation corresponds to. + // +kubebuilder:validation:Optional + CommitmentUUID string `json:"commitmentUUID,omitempty"` + // ResourceGroup is the group/category of the resource (e.g., flavor group for Nova) // +kubebuilder:validation:Optional ResourceGroup string `json:"resourceGroup,omitempty"` diff --git a/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml b/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml index 915e5677e..d9256e5db 100644 --- a/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml +++ b/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml @@ -87,6 +87,10 @@ spec: Key: Workload UUID (VM UUID for Nova, Pod UID for Pods, Machine UID for IronCore, etc.) Value: allocation state and metadata type: object + commitmentUUID: + description: CommitmentUUID is the UUID of the commitment that + this reservation corresponds to. + type: string creator: description: |- Creator identifies the system or component that created this reservation. diff --git a/internal/scheduling/reservations/commitments/api_change_commitments.go b/internal/scheduling/reservations/commitments/api_change_commitments.go index 3134b3b9d..88fe5732a 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments.go @@ -9,6 +9,8 @@ import ( "errors" "fmt" "net/http" + "sort" + "strings" "time" "github.com/cobaltcore-dev/cortex/api/v1alpha1" @@ -23,12 +25,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// sortedKeys returns map keys sorted alphabetically for deterministic iteration. +func sortedKeys[K ~string, V any](m map[K]V) []K { + keys := make([]K, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Slice(keys, func(i, j int) bool { + return string(keys[i]) < string(keys[j]) + }) + return keys +} + const ( // watchTimeout is how long to wait for all reservations to become ready - watchTimeout = 20 * time.Second + watchTimeout = 2 * time.Second // pollInterval is how frequently to poll reservation status - pollInterval = 1 * time.Second + pollInterval = 100 * time.Millisecond ) // implements POST /v1/change-commitments from Limes LIQUID API: @@ -99,6 +113,7 @@ func (api *HTTPAPI) processCommitmentChanges(w http.ResponseWriter, log logr.Log ctx := context.Background() manager := NewReservationManager(api.client) requireRollback := false + failedCommitments := make(map[string]string) // commitmentUUID to reason for failure, for better response messages in case of rollback log.Info("processing commitment change request", "availabilityZone", req.AZ, "dryRun", req.DryRun, "affectedProjects", len(req.ByProject)) knowledge := &reservations.FlavorGroupKnowledgeClient{Client: api.client} @@ -135,8 +150,10 @@ func (api *HTTPAPI) processCommitmentChanges(w http.ResponseWriter, log logr.Log } ProcessLoop: - for projectID, projectChanges := range req.ByProject { - for resourceName, resourceChanges := range projectChanges.ByResource { + for _, projectID := range sortedKeys(req.ByProject) { + projectChanges := req.ByProject[projectID] + for _, resourceName := range sortedKeys(projectChanges.ByResource) { + resourceChanges := projectChanges.ByResource[resourceName] // Validate resource name pattern (instances_group_*) flavorGroupName, err := getFlavorGroupNameFromResource(string(resourceName)) if err != nil { @@ -157,6 +174,7 @@ ProcessLoop: // Additional per-commitment validation if needed log.Info("processing commitment change", "commitmentUUID", commitment.UUID, "projectID", projectID, "resourceName", resourceName, "oldStatus", commitment.OldStatus.UnwrapOr("none"), "newStatus", commitment.NewStatus.UnwrapOr("none")) + // TODO add configurable upper limit validation for commitment size (number of instances) to prevent excessive reservation creation // TODO add domain // List all committed resource reservations, then filter by name prefix @@ -164,7 +182,8 @@ ProcessLoop: if err := api.client.List(ctx, &all_reservations, client.MatchingLabels{ v1alpha1.LabelReservationType: v1alpha1.ReservationTypeLabelCommittedResource, }); err != nil { - resp.RejectionReason = fmt.Sprintf("failed to list reservations for commitment %s: %v", commitment.UUID, err) + failedCommitments[string(commitment.UUID)] = "failed to list reservations" + log.Info(fmt.Sprintf("failed to list reservations for commitment %s: %v", commitment.UUID, err)) requireRollback = true break ProcessLoop } @@ -189,7 +208,8 @@ ProcessLoop: } else { stateBefore, err = FromReservations(existing_reservations.Items) if err != nil { - resp.RejectionReason = fmt.Sprintf("failed to get existing state for commitment %s: %v", commitment.UUID, err) + failedCommitments[string(commitment.UUID)] = "failed to parse existing commitment reservations" + log.Info(fmt.Sprintf("failed to get existing state for commitment %s: %v", commitment.UUID, err)) requireRollback = true break ProcessLoop } @@ -199,7 +219,8 @@ ProcessLoop: // get desired state stateDesired, err := FromChangeCommitmentTargetState(commitment, string(projectID), flavorGroupName, flavorGroup, string(req.AZ)) if err != nil { - resp.RejectionReason = fmt.Sprintf("failed to get desired state for commitment %s: %v", commitment.UUID, err) + failedCommitments[string(commitment.UUID)] = "failed to determine desired commitment state" + log.Info(fmt.Sprintf("failed to get desired state for commitment %s: %v", commitment.UUID, err)) requireRollback = true break ProcessLoop } @@ -208,7 +229,8 @@ ProcessLoop: touchedReservations, deletedReservations, err := manager.ApplyCommitmentState(ctx, log, stateDesired, flavorGroups, "changeCommitmentsApi") if err != nil { - resp.RejectionReason = fmt.Sprintf("failed to apply commitment state for commitment %s: %v", commitment.UUID, err) + failedCommitments[string(commitment.UUID)] = "failed to apply commitment state" + log.Info(fmt.Sprintf("failed to apply commitment state for commitment %s: %v", commitment.UUID, err)) requireRollback = true break ProcessLoop } @@ -224,10 +246,14 @@ ProcessLoop: time_start := time.Now() - if err := watchReservationsUntilReady(ctx, log, api.client, reservationsToWatch, watchTimeout); err != nil { + if failedReservations, errors := watchReservationsUntilReady(ctx, log, api.client, reservationsToWatch, watchTimeout); len(failedReservations) > 0 || len(errors) > 0 { log.Info("reservations failed to become ready, initiating rollback", - "reason", err.Error()) - resp.RejectionReason = fmt.Sprintf("Not all reservations can be fulfilled: %v", err) + "failedReservations", len(failedReservations), + "errors", errors) + + for _, res := range failedReservations { + failedCommitments[res.Spec.CommittedResourceReservation.CommitmentUUID] = "not sufficient capacity" + } requireRollback = true } @@ -235,6 +261,16 @@ ProcessLoop: } if requireRollback { + // Build rejection reason from failed commitments + if len(failedCommitments) > 0 { + var reasonBuilder strings.Builder + reasonBuilder.WriteString(fmt.Sprintf("%d commitment(s) failed to apply: ", len(failedCommitments))) + for commitmentUUID, reason := range failedCommitments { + reasonBuilder.WriteString(fmt.Sprintf("\n- commitment %s: %s", commitmentUUID, reason)) + } + resp.RejectionReason = reasonBuilder.String() + } + log.Info("rollback of commitment changes") for commitmentUUID, state := range statesBefore { // Rollback to statesBefore for this commitment @@ -247,16 +283,10 @@ ProcessLoop: } log.Info("finished applying rollbacks for commitment changes", "reasonOfRollback", resp.RejectionReason) - - // TODO improve human-readable reasoning based on actual failure, i.e. polish resp.RejectionReason return nil } log.Info("commitment changes accepted") - if resp.RejectionReason != "" { - log.Info("unexpected non-empty rejection reason without rollback", "reason", resp.RejectionReason) - resp.RejectionReason = "" - } return nil } @@ -267,23 +297,27 @@ func watchReservationsUntilReady( k8sClient client.Client, reservations []v1alpha1.Reservation, timeout time.Duration, -) error { +) (failedReservations []v1alpha1.Reservation, errors []error) { if len(reservations) == 0 { - return nil + return failedReservations, nil } deadline := time.Now().Add(timeout) + reservationsToWatch := make([]v1alpha1.Reservation, len(reservations)) + copy(reservationsToWatch, reservations) + for { if time.Now().After(deadline) { - return fmt.Errorf("timeout after %v waiting for reservations to become ready", timeout) + errors = append(errors, fmt.Errorf("timeout after %v waiting for reservations to become ready", timeout)) + return failedReservations, errors } - allReady := true - var notReadyReasons []string + allChecked := true - for _, res := range reservations { + check: + for i, res := range reservationsToWatch { // Fetch current state var current v1alpha1.Reservation nn := types.NamespacedName{ @@ -292,12 +326,16 @@ func watchReservationsUntilReady( } if err := k8sClient.Get(ctx, nn, ¤t); err != nil { + allChecked = false if apierrors.IsNotFound(err) { - // Reservation is still in process of being created - allReady = false + // Reservation is still in process of being created, continue waiting for it continue } - return fmt.Errorf("failed to get reservation %s: %w", res.Name, err) + // remove reservation from waiting + failedReservations = append(failedReservations, res) + reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) + errors = append(errors, fmt.Errorf("failed to get reservation %s: %w", res.Name, err)) + break check // break because iterating list was modified } // Check Ready condition @@ -308,37 +346,36 @@ func watchReservationsUntilReady( if readyCond == nil { // Condition not set yet, keep waiting - allReady = false - notReadyReasons = append(notReadyReasons, - res.Name+": condition not set") + allChecked = false continue } switch readyCond.Status { case metav1.ConditionTrue: // This reservation is ready - continue + // TODO use more than readyCondition + allChecked = false + reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) + break check // break because iterating list was modified case metav1.ConditionFalse: - // Explicit failure - stop immediately - return fmt.Errorf("reservation %s failed: %s (reason: %s)", - res.Name, readyCond.Message, readyCond.Reason) + allChecked = false + failedReservations = append(failedReservations, res) + reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) + break check // break because iterating list was modified case metav1.ConditionUnknown: - // Still processing - allReady = false - notReadyReasons = append(notReadyReasons, - fmt.Sprintf("%s: %s", res.Name, readyCond.Message)) + allChecked = false } } - if allReady { - log.Info("all reservations are ready", - "count", len(reservations)) - return nil + if allChecked { + log.Info("all reservations checked", + "failed", len(failedReservations)) + return failedReservations, errors } // Log progress log.Info("waiting for reservations to become ready", - "notReady", len(notReadyReasons), + "notReady", len(reservationsToWatch), "total", len(reservations), "timeRemaining", time.Until(deadline).Round(time.Second)) @@ -347,7 +384,7 @@ func watchReservationsUntilReady( case <-time.After(pollInterval): // Continue polling case <-ctx.Done(): - return fmt.Errorf("context cancelled while waiting for reservations: %w", ctx.Err()) + return failedReservations, append(errors, fmt.Errorf("context cancelled while waiting for reservations: %w", ctx.Err())) } } } diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index c4703c4a1..4a4b9476f 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -1,246 +1,1618 @@ // Copyright SAP SE // SPDX-License-Identifier: Apache-2.0 +//nolint:unparam,unused // test helper functions have fixed parameters for simplicity package commitments import ( "bytes" + "context" "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" + "os" + "sort" + "strconv" + "strings" "testing" "time" "github.com/cobaltcore-dev/cortex/api/v1alpha1" "github.com/cobaltcore-dev/cortex/internal/knowledge/extractor/plugins/compute" + hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" "github.com/sapcc/go-api-declarations/liquid" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" ) -// TODO refactor with proper integration tests +// ============================================================================ +// Integration Tests +// ============================================================================ -func TestHandleChangeCommitments_VersionMismatch(t *testing.T) { - // Create a fake Kubernetes client with a Knowledge CRD - scheme := runtime.NewScheme() - if err := v1alpha1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add scheme: %v", err) +func TestCommitmentChangeIntegration(t *testing.T) { + m1Small := &TestFlavor{Name: "m1.small", Group: "hana_1", MemoryMB: 1024, VCPUs: 4} + m1Large := &TestFlavor{Name: "m1.large", Group: "hana_1", MemoryMB: 4096, VCPUs: 16} + m1XL := &TestFlavor{Name: "m1.xl", Group: "hana_1", MemoryMB: 8192, VCPUs: 32} + + testCases := []CommitmentChangeTestCase{ + { + Name: "Shrinking CR - unused reservations removed, used reservations untouched", + VMs: []*TestVM{{UUID: "vm-a1", Flavor: m1Large, ProjectID: "project-A", Host: "host-1", AZ: "az-a"}}, + Flavors: []*TestFlavor{m1Small, m1Large}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", VMs: []string{"vm-a1"}}, + {CommitmentID: "uuid-123", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-123", Host: "host-3", Flavor: m1Small, ProjectID: "project-A"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, createCommitment("ram_hana_1", "project-A", "uuid-123", "confirmed", 2)), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", VMs: []string{"vm-a1"}}, + {CommitmentID: "uuid-123", Host: "host-3", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Insufficient capacity when increasing CR", + VMs: []*TestVM{}, + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{{CommitmentID: "uuid-456", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, createCommitment("ram_hana_1", "project-A", "uuid-456", "confirmed", 3)), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 1024, "host-2": 0}}, + ExpectedReservations: []*TestReservation{{CommitmentID: "uuid-456", Host: "", Flavor: m1Small, ProjectID: "project-A"}}, + ExpectedAPIResponse: newAPIResponse("uuid-456", "not sufficient capacity"), + }, + { + Name: "Swap capacity between CRs - order dependent - delete-first succeeds", + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-456", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-456", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-456", "confirmed", 0), + createCommitment("ram_hana_1", "project-B", "uuid-123", "confirmed", 2), + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 0, "host-2": 0}}, + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-123", Host: "host-2", Flavor: m1Small, ProjectID: "project-B"}}, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Swap capacity between CRs - order dependent - create-first fails", + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-123", Host: "host-2", Flavor: m1Small, ProjectID: "project-B"}}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-456", "confirmed", 2), + createCommitment("ram_hana_1", "project-B", "uuid-123", "confirmed", 0), + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 0, "host-2": 0}}, + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-123", Host: "host-2", Flavor: m1Small, ProjectID: "project-B"}}, + ExpectedAPIResponse: newAPIResponse("uuid-456", "not sufficient capacity"), + }, + { + Name: "Flavor bin-packing - mixed sizes when largest doesn't fit", + // Greedy selection: 10GB request with 8/4/1GB flavors → picks 1×8GB + 2×1GB + Flavors: []*TestFlavor{m1XL, m1Large, m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-binpack", "confirmed", 10), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-binpack", Flavor: m1XL, ProjectID: "project-A"}, + {CommitmentID: "uuid-binpack", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-binpack", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Version mismatch - request rejected with 409 Conflict", + // InfoVersion validation prevents stale requests (1233 vs 1234) + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1233, + createCommitment("ram_hana_1", "project-A", "uuid-version", "confirmed", 2), + ), + EnvInfoVersion: 1234, + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: APIResponseExpectation{StatusCode: 409}, + }, + { + Name: "Multi-project rollback - one failure rolls back all", + // Transactional: project-B fails (insufficient capacity) → both projects rollback + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-project-a", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-project-a", "confirmed", 2), + createCommitment("ram_hana_1", "project-B", "uuid-project-b", "confirmed", 2), + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 1024, "host-2": 0}}, + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-project-a", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse("uuid-project-b", "not sufficient capacity"), + }, + { + Name: "Rollback with VMs allocated - limitation: VM allocations not rolled back", + // Controller will eventually clean up and repair inconsistent state + VMs: []*TestVM{{UUID: "vm-rollback", Flavor: m1Small, ProjectID: "project-A", Host: "host-1", AZ: "az-a"}}, + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "commitment-A", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", VMs: []string{"vm-rollback"}}, + {CommitmentID: "commitment-A", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "commitment-A", "confirmed", 0), + createCommitment("ram_hana_1", "project-B", "commitment-B", "confirmed", 6), + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 0}}, + ExpectedReservations: []*TestReservation{ + // Rollback creates unscheduled reservations (empty Host accepts any in matching) + {CommitmentID: "commitment-A", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "commitment-A", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse("commitment-B", "not sufficient capacity"), + }, + { + Name: "New commitment creation - from zero to N reservations", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-new", "confirmed", 3), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-new", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-new", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-new", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "With reservations of custom size - total unchanged", + // Preserves custom-sized reservations when total matches (2×2GB = 4GB) + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Host: "host-2", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-custom", "confirmed", 4), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Host: "host-2", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "With reservations of custom size - increase total", + // 4GB (2×2GB custom) → 6GB: preserves custom sizes, adds standard-sized reservations + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Host: "host-2", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-custom", "confirmed", 6), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Host: "host-2", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-custom", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "With reservations of custom size - decrease total", + // 4GB (2×2GB custom) → 3GB: removes 1×2GB custom, adds 1×1GB standard + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Host: "host-2", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-custom", "confirmed", 3), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-custom", Flavor: m1Small, ProjectID: "project-A", MemoryMB: 2048}, + {CommitmentID: "uuid-custom", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Complete commitment deletion - N to zero reservations", + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-delete", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-delete", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-delete", Host: "host-3", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-b-1", Host: "host-3", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-a-1", Host: "host-3", Flavor: m1Small, ProjectID: "project-A"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-delete", "confirmed", 0), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-b-1", Host: "host-3", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-a-1", Host: "host-3", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "VM allocation preservation - keep VMs during growth", + VMs: []*TestVM{{UUID: "vm-existing", Flavor: m1Small, ProjectID: "project-A", Host: "host-1", AZ: "az-a"}}, + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-growth", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", VMs: []string{"vm-existing"}}, + {CommitmentID: "uuid-growth", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-growth", "confirmed", 3), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-growth", Host: "host-1", Flavor: m1Small, ProjectID: "project-A", VMs: []string{"vm-existing"}}, + {CommitmentID: "uuid-growth", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-growth", Flavor: m1Small, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Multi-project success - both projects succeed", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-a", "confirmed", 2), + createCommitment("ram_hana_1", "project-B", "uuid-b", "confirmed", 2), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-a", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-a", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-b", Flavor: m1Small, ProjectID: "project-B"}, + {CommitmentID: "uuid-b", Flavor: m1Small, ProjectID: "project-B"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Multiple flavor groups - ram_hana_1 and ram_hana_2", + // Amount in multiples of smallest flavor: hana_1 (2×1GB), hana_2 (2×2GB) + Flavors: []*TestFlavor{ + m1Small, + {Name: "m2.small", Group: "hana_2", MemoryMB: 2048, VCPUs: 8}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-hana1", "confirmed", 2), + createCommitment("ram_hana_2", "project-A", "uuid-hana2", "confirmed", 2), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-hana1", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-hana1", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-hana2", Flavor: &TestFlavor{Name: "m2.small", Group: "hana_2", MemoryMB: 2048, VCPUs: 8}, ProjectID: "project-A"}, + {CommitmentID: "uuid-hana2", Flavor: &TestFlavor{Name: "m2.small", Group: "hana_2", MemoryMB: 2048, VCPUs: 8}, ProjectID: "project-A"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Unknown flavor group - clear rejection message", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_nonexistent", "project-A", "uuid-unknown", "confirmed", 2), + ), + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: newAPIResponse("flavor group not found"), + }, + { + Name: "Three-way capacity swap - complex reallocation", + // A:2→0, B:1→0, C:0→3 in single transaction + Flavors: []*TestFlavor{m1Small}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-a", Host: "host-1", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-a", Host: "host-2", Flavor: m1Small, ProjectID: "project-A"}, + {CommitmentID: "uuid-b", Host: "host-3", Flavor: m1Small, ProjectID: "project-B"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-a", "confirmed", 0), + createCommitment("ram_hana_1", "project-B", "uuid-b", "confirmed", 0), + createCommitment("ram_hana_1", "project-C", "uuid-c", "confirmed", 3), + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 0, "host-2": 0, "host-3": 0}}, + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-c", Host: "host-1", Flavor: m1Small, ProjectID: "project-C"}, + {CommitmentID: "uuid-c", Host: "host-2", Flavor: m1Small, ProjectID: "project-C"}, + {CommitmentID: "uuid-c", Host: "host-3", Flavor: m1Small, ProjectID: "project-C"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Reservation repair - existing reservations with wrong metadata", + Flavors: []*TestFlavor{m1Small, m1Large}, + ExistingReservations: []*TestReservation{ + {CommitmentID: "uuid-repair", Host: "host-preserved", Flavor: m1Small, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Host: "host-1", Flavor: m1Small, ProjectID: "wrong-project", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Host: "host-2", Flavor: &TestFlavor{Name: "m1.small", Group: "hana_13", MemoryMB: 1024, VCPUs: 4}, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Host: "host-4", Flavor: m1Small, ProjectID: "project-A", AZ: "wrong-az"}, + }, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-repair", "confirmed", 8, "az-a"), + ), + ExpectedReservations: []*TestReservation{ + {CommitmentID: "uuid-repair", Host: "host-preserved", Flavor: m1Small, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Flavor: m1Small, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Flavor: m1Small, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Flavor: m1Small, ProjectID: "project-A", AZ: "az-a"}, + {CommitmentID: "uuid-repair", Flavor: m1Large, ProjectID: "project-A", AZ: "az-a"}, + }, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Empty request - no commitment changes", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234), + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: newAPIResponse(), + }, + { + Name: "Dry run request - feature not yet implemented", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", true, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-dryrun", "confirmed", 2), + ), + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: newAPIResponse("Dry run not supported"), + }, + { + Name: "Knowledge not ready - clear rejection with RetryAt", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-knowledge", "confirmed", 2), + ), + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: newAPIResponse("caches not ready"), + EnvInfoVersion: -1, // Skip Knowledge CRD creation + }, } - // Create a Knowledge CRD with a specific version timestamp and flavor groups - knowledgeTimestamp := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) - flavorGroup := createTestFlavorGroup() + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + runCommitmentChangeTest(t, tc) + }) + } +} - // Box the features using the Knowledge API - rawExt, err := v1alpha1.BoxFeatureList([]compute.FlavorGroupFeature{flavorGroup}) - if err != nil { - t.Fatalf("failed to box feature list: %v", err) +// runCommitmentChangeTest executes a single commitment change integration test case. +func runCommitmentChangeTest(t *testing.T, tc CommitmentChangeTestCase) { + t.Helper() + + // Convert test types to actual types + var vms []VM + for _, testVM := range tc.VMs { + vms = append(vms, testVM.ToVM()) } - knowledge := &v1alpha1.Knowledge{ - ObjectMeta: metav1.ObjectMeta{ - Name: "flavor-groups", + var flavorInGroups []compute.FlavorInGroup + for _, testFlavor := range tc.Flavors { + flavorInGroups = append(flavorInGroups, testFlavor.ToFlavorInGroup()) + } + + // Use EnvInfoVersion if specified (non-zero), otherwise default to CommitmentRequest.InfoVersion + envInfoVersion := tc.CommitmentRequest.InfoVersion + if tc.EnvInfoVersion != 0 { + envInfoVersion = tc.EnvInfoVersion + } + + flavorGroups := TestFlavorGroup{ + infoVersion: envInfoVersion, + flavors: flavorInGroups, + }.ToFlavorGroupsKnowledge() + + // Convert existing reservations with auto-numbering per commitment + var existingReservations []*v1alpha1.Reservation + numberCounters := make(map[string]int) + for _, testRes := range tc.ExistingReservations { + number := numberCounters[testRes.CommitmentID] + numberCounters[testRes.CommitmentID]++ + existingReservations = append(existingReservations, testRes.toReservation(number)) + } + + // Create test environment with available resources + env := newCommitmentTestEnv(t, vms, nil, existingReservations, flavorGroups, tc.AvailableResources) + defer env.Close() + + t.Log("Initial state:") + env.LogStateSummary() + + // Call commitment change API + reqJSON := buildRequestJSON(tc.CommitmentRequest) + resp, statusCode := env.CallChangeCommitmentsAPI(reqJSON) + + t.Log("After API call:") + env.LogStateSummary() + + // Verify API response + env.VerifyAPIResponse(tc.ExpectedAPIResponse, resp, statusCode) + + // Verify reservations using content-based matching + env.VerifyReservationsMatch(tc.ExpectedReservations) + + // Log final test result + if t.Failed() { + t.Log("❌ Test FAILED") + } else { + t.Log("✅ Test PASSED") + } +} + +// ============================================================================ +// Test Types & Constants +// ============================================================================ + +const ( + defaultFlavorDiskGB = 40 + flavorGroupsKnowledgeName = "flavor-groups" + knowledgeRecencyDuration = 60 * time.Second + defaultCommitmentExpiryYears = 1 +) + +type CommitmentChangeTestCase struct { + Name string + VMs []*TestVM + Flavors []*TestFlavor + ExistingReservations []*TestReservation + CommitmentRequest CommitmentChangeRequest + ExpectedReservations []*TestReservation + ExpectedAPIResponse APIResponseExpectation + AvailableResources *AvailableResources // If nil, all reservations accepted without checks + EnvInfoVersion int64 // Override InfoVersion for version mismatch tests +} + +// AvailableResources defines available memory per host (MB). +// Scheduler uses first-come-first-serve. CPU is ignored. +type AvailableResources struct { + PerHost map[string]int64 // host -> available memory MB +} + +type TestFlavorGroup struct { + infoVersion int64 + flavors []compute.FlavorInGroup +} + +func (tfg TestFlavorGroup) ToFlavorGroupsKnowledge() FlavorGroupsKnowledge { + groupMap := make(map[string][]compute.FlavorInGroup) + + for _, flavor := range tfg.flavors { + groupName := flavor.ExtraSpecs["quota:hw_version"] + if groupName == "" { + panic("Flavor " + flavor.Name + " is missing quota:hw_version in extra specs") + } + groupMap[groupName] = append(groupMap[groupName], flavor) + } + + var groups []compute.FlavorGroupFeature + for groupName, groupFlavors := range groupMap { + if len(groupFlavors) == 0 { + continue + } + + // Sort descending: required by reservation manager's flavor selection + sort.Slice(groupFlavors, func(i, j int) bool { + return groupFlavors[i].MemoryMB > groupFlavors[j].MemoryMB + }) + + smallest := groupFlavors[len(groupFlavors)-1] + largest := groupFlavors[0] + + groups = append(groups, compute.FlavorGroupFeature{ + Name: groupName, + Flavors: groupFlavors, + SmallestFlavor: smallest, + LargestFlavor: largest, + }) + } + + return FlavorGroupsKnowledge{ + InfoVersion: tfg.infoVersion, + Groups: groups, + } +} + +type FlavorGroupsKnowledge struct { + InfoVersion int64 + Groups []compute.FlavorGroupFeature +} + +type CommitmentChangeRequest struct { + AZ string + DryRun bool + InfoVersion int64 + Commitments []TestCommitment +} + +type TestCommitment struct { + ResourceName liquid.ResourceName + ProjectID string + ConfirmationID string + State string + Amount uint64 +} + +type APIResponseExpectation struct { + StatusCode int + RejectReasonSubstrings []string + RetryAtPresent bool +} + +type ReservationVerification struct { + Host string + Allocations map[string]string +} + +type VM struct { + UUID string + FlavorName string + ProjectID string + CurrentHypervisor string + AvailabilityZone string + Resources map[string]int64 + FlavorExtraSpecs map[string]string +} + +type TestFlavor struct { + Name string + Group string + MemoryMB int64 + VCPUs int64 + DiskGB uint64 +} + +func (f *TestFlavor) ToFlavorInGroup() compute.FlavorInGroup { + diskGB := f.DiskGB + if diskGB == 0 { + diskGB = defaultFlavorDiskGB + } + return compute.FlavorInGroup{ + Name: f.Name, + MemoryMB: uint64(f.MemoryMB), //nolint:gosec // test values are always positive + VCPUs: uint64(f.VCPUs), //nolint:gosec // test values are always positive + DiskGB: diskGB, + ExtraSpecs: map[string]string{ + "quota:hw_version": f.Group, }, - Spec: v1alpha1.KnowledgeSpec{ - SchedulingDomain: v1alpha1.SchedulingDomainNova, - Extractor: v1alpha1.KnowledgeExtractorSpec{ - Name: "flavor-groups", + } +} + +type TestVM struct { + UUID string + Flavor *TestFlavor + ProjectID string + Host string + AZ string +} + +func (vm *TestVM) ToVM() VM { + return VM{ + UUID: vm.UUID, + FlavorName: vm.Flavor.Name, + ProjectID: vm.ProjectID, + CurrentHypervisor: vm.Host, + AvailabilityZone: vm.AZ, + Resources: map[string]int64{ + "memory": vm.Flavor.MemoryMB, + "vcpus": vm.Flavor.VCPUs, + }, + FlavorExtraSpecs: map[string]string{ + "quota:hw_version": vm.Flavor.Group, + }, + } +} + +type TestReservation struct { + CommitmentID string + Host string // Empty = any host accepted in matching + Flavor *TestFlavor + ProjectID string + VMs []string // VM UUIDs + MemoryMB int64 // If 0, uses Flavor.MemoryMB; else custom size + AZ string +} + +func (tr *TestReservation) toReservation(number int) *v1alpha1.Reservation { + name := fmt.Sprintf("commitment-%s-%d", tr.CommitmentID, number) + + memoryMB := tr.MemoryMB + if memoryMB == 0 { + memoryMB = tr.Flavor.MemoryMB + } + + specAllocations := make(map[string]v1alpha1.CommittedResourceAllocation) + statusAllocations := make(map[string]string) + for _, vmUUID := range tr.VMs { + specAllocations[vmUUID] = v1alpha1.CommittedResourceAllocation{ + CreationTimestamp: metav1.Now(), + Resources: map[hv1.ResourceName]resource.Quantity{ + "memory": resource.MustParse(strconv.FormatInt(memoryMB, 10) + "Mi"), + "cpu": resource.MustParse(strconv.FormatInt(tr.Flavor.VCPUs, 10)), }, + } + statusAllocations[vmUUID] = tr.Host + } + + spec := v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: tr.Host, + Resources: map[hv1.ResourceName]resource.Quantity{ + "memory": resource.MustParse(strconv.FormatInt(memoryMB, 10) + "Mi"), + "cpu": resource.MustParse(strconv.FormatInt(tr.Flavor.VCPUs, 10)), }, - Status: v1alpha1.KnowledgeStatus{ - LastContentChange: metav1.Time{Time: knowledgeTimestamp}, - Raw: rawExt, - RawLength: 1, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ + CommitmentUUID: tr.CommitmentID, + ProjectID: tr.ProjectID, + ResourceName: tr.Flavor.Name, + ResourceGroup: tr.Flavor.Group, + Allocations: specAllocations, + }, + } + + if tr.AZ != "" { + spec.AvailabilityZone = tr.AZ + } + + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + v1alpha1.LabelReservationType: v1alpha1.ReservationTypeLabelCommittedResource, + }, + }, + Spec: spec, + Status: v1alpha1.ReservationStatus{ Conditions: []metav1.Condition{ { - Type: v1alpha1.KnowledgeConditionReady, + Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionTrue, - Reason: "Ready", + Reason: "ReservationActive", }, }, + Host: tr.Host, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationStatus{ + Allocations: statusAllocations, + }, }, } +} - k8sClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(knowledge). - WithStatusSubresource(knowledge). - Build() +// ============================================================================ +// Test Environment +// ============================================================================ - api := &HTTPAPI{ - client: k8sClient, - } +type CommitmentTestEnv struct { + T *testing.T + Scheme *runtime.Scheme + K8sClient client.Client + VMSource *MockVMSource + FlavorGroups FlavorGroupsKnowledge + HTTPServer *httptest.Server + API *HTTPAPI + availableResources map[string]int64 // host -> available memory MB + processedReserv map[string]bool // track processed reservations +} - // Create request JSON with mismatched version - requestJSON := `{ - "az": "az-a", - "dryRun": false, - "infoVersion": 12345, - "byProject": {} - }` +// FakeReservationController simulates synchronous reservation controller. +type FakeReservationController struct { + env *CommitmentTestEnv +} - req := httptest.NewRequest(http.MethodPost, "/v1/change-commitments", bytes.NewReader([]byte(requestJSON))) - req.Header.Set("Content-Type", "application/json") +func (c *FakeReservationController) OnReservationCreated(res *v1alpha1.Reservation) { + c.env.processNewReservation(res) +} - w := httptest.NewRecorder() +func (c *FakeReservationController) OnReservationDeleted(res *v1alpha1.Reservation) { + if c.env.availableResources != nil && res.Status.Host != "" { + memoryQuantity := res.Spec.Resources["memory"] + memoryBytes := memoryQuantity.Value() + memoryMB := memoryBytes / (1024 * 1024) - // Call the handler - api.HandleChangeCommitments(w, req) + if _, exists := c.env.availableResources[res.Status.Host]; exists { + c.env.availableResources[res.Status.Host] += memoryMB + c.env.T.Logf("↩ Returned %d MB to %s (now %d MB available) before deleting reservation %s", + memoryMB, res.Status.Host, c.env.availableResources[res.Status.Host], res.Name) + } + } +} - // Check response - resp := w.Result() - defer resp.Body.Close() +// operationInterceptorClient routes reservation events to FakeReservationController. +type operationInterceptorClient struct { + client.Client + controller *FakeReservationController +} - // Verify HTTP 409 Conflict status - if resp.StatusCode != http.StatusConflict { - t.Errorf("expected status code %d (Conflict), got %d", http.StatusConflict, resp.StatusCode) +func (d *operationInterceptorClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + err := d.Client.Create(ctx, obj, opts...) + if err != nil { + return err } - // Verify Content-Type is text/plain (set by http.Error) - contentType := resp.Header.Get("Content-Type") - if contentType != "text/plain; charset=utf-8" { - t.Errorf("expected Content-Type 'text/plain; charset=utf-8', got %q", contentType) + if res, ok := obj.(*v1alpha1.Reservation); ok { + d.controller.OnReservationCreated(res) } - // Verify error message contains version information - var responseBody bytes.Buffer - if _, err = responseBody.ReadFrom(resp.Body); err != nil { - t.Fatalf("failed to read response body: %v", err) - } + return nil +} - bodyStr := responseBody.String() - if !bytes.Contains([]byte(bodyStr), []byte("Version mismatch")) { - t.Errorf("expected response to contain 'Version mismatch', got: %s", bodyStr) +func (d *operationInterceptorClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + if res, ok := obj.(*v1alpha1.Reservation); ok { + d.controller.OnReservationDeleted(res) } - if !bytes.Contains([]byte(bodyStr), []byte("12345")) { - t.Errorf("expected response to contain request version '12345', got: %s", bodyStr) + + return d.Client.Delete(ctx, obj, opts...) +} + +func (env *CommitmentTestEnv) Close() { + if env.HTTPServer != nil { + env.HTTPServer.Close() } } -func TestHandleChangeCommitments_DryRun(t *testing.T) { + +func newCommitmentTestEnv( + t *testing.T, + vms []VM, + hypervisors []*hv1.Hypervisor, + reservations []*v1alpha1.Reservation, + flavorGroups FlavorGroupsKnowledge, + resources *AvailableResources, +) *CommitmentTestEnv { + + t.Helper() + + log.SetLogger(zap.New(zap.WriteTo(os.Stderr), zap.UseDevMode(true))) + + objects := make([]client.Object, 0, len(hypervisors)+len(reservations)) + for _, hv := range hypervisors { + objects = append(objects, hv) + } + for _, res := range reservations { + objects = append(objects, res) + } + scheme := runtime.NewScheme() if err := v1alpha1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add scheme: %v", err) + t.Fatalf("Failed to add v1alpha1 scheme: %v", err) + } + if err := hv1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add hv1 scheme: %v", err) + } + + // InfoVersion of -1 skips Knowledge CRD creation (tests "not ready" scenario) + if flavorGroups.InfoVersion != -1 { + knowledgeCRD := createKnowledgeCRD(flavorGroups) + objects = append(objects, knowledgeCRD) } - k8sClient := fake.NewClientBuilder(). + baseK8sClient := fake.NewClientBuilder(). WithScheme(scheme). + WithObjects(objects...). + WithStatusSubresource(&v1alpha1.Reservation{}). + WithStatusSubresource(&v1alpha1.Knowledge{}). + WithIndex(&v1alpha1.Reservation{}, "spec.type", func(obj client.Object) []string { + res := obj.(*v1alpha1.Reservation) + return []string{string(res.Spec.Type)} + }). Build() - api := &HTTPAPI{ - client: k8sClient, + var availableResources map[string]int64 + if resources != nil && resources.PerHost != nil { + availableResources = make(map[string]int64) + for host, memMB := range resources.PerHost { + availableResources[host] = memMB + } } - // Create dry run request JSON - requestJSON := `{ - "az": "az-a", - "dryRun": true, - "infoVersion": 12345, - "byProject": {} - }` + env := &CommitmentTestEnv{ + T: t, + Scheme: scheme, + K8sClient: nil, // Will be set below + VMSource: NewMockVMSource(vms), + FlavorGroups: flavorGroups, + HTTPServer: nil, // Will be set below + API: nil, // Will be set below + availableResources: availableResources, + processedReserv: make(map[string]bool), + } + + controller := &FakeReservationController{env: env} + wrappedClient := &operationInterceptorClient{ + Client: baseK8sClient, + controller: controller, + } + env.K8sClient = wrappedClient - req := httptest.NewRequest(http.MethodPost, "/v1/change-commitments", bytes.NewReader([]byte(requestJSON))) - req.Header.Set("Content-Type", "application/json") - w := httptest.NewRecorder() + api := NewAPI(wrappedClient) + mux := http.NewServeMux() + api.Init(mux) + httpServer := httptest.NewServer(mux) - api.HandleChangeCommitments(w, req) + env.HTTPServer = httpServer + env.API = api - resp := w.Result() - defer resp.Body.Close() + return env +} + +// ============================================================================ +// Environment Helper Methods +// ============================================================================ - // Dry run should return 200 OK with rejection reason - if resp.StatusCode != http.StatusOK { - t.Errorf("expected status code %d (OK), got %d", http.StatusOK, resp.StatusCode) +// ListVMs returns all VMs from the VMSource. +func (env *CommitmentTestEnv) ListVMs() []VM { + vms, err := env.VMSource.ListVMs(context.Background()) + if err != nil { + env.T.Fatalf("Failed to list VMs: %v", err) } + return vms +} - // Verify response is JSON - contentType := resp.Header.Get("Content-Type") - if contentType != "application/json" { - t.Errorf("expected Content-Type 'application/json', got %q", contentType) +// ListReservations returns all reservations. +func (env *CommitmentTestEnv) ListReservations() []v1alpha1.Reservation { + var list v1alpha1.ReservationList + if err := env.K8sClient.List(context.Background(), &list); err != nil { + env.T.Fatalf("Failed to list reservations: %v", err) } + return list.Items +} - // Parse response - var response liquid.CommitmentChangeResponse - if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { - t.Fatalf("failed to decode response: %v", err) +// ListHypervisors returns all hypervisors. +func (env *CommitmentTestEnv) ListHypervisors() []hv1.Hypervisor { + var list hv1.HypervisorList + if err := env.K8sClient.List(context.Background(), &list); err != nil { + env.T.Fatalf("Failed to list hypervisors: %v", err) } + return list.Items +} + +// LogStateSummary logs a summary of the current state. +func (env *CommitmentTestEnv) LogStateSummary() { + env.T.Helper() + + hypervisors := env.ListHypervisors() + vms := env.ListVMs() + reservations := env.ListReservations() + + env.T.Log("=== State Summary ===") + env.T.Logf("Hypervisors: %d", len(hypervisors)) + env.T.Logf("VMs: %d", len(vms)) + env.T.Logf("Reservations: %d", len(reservations)) - if response.RejectionReason != "Dry run not supported yet" { - t.Errorf("expected rejection reason 'Dry run not supported yet', got %q", response.RejectionReason) + for _, res := range reservations { + allocCount := 0 + if res.Status.CommittedResourceReservation != nil { + allocCount = len(res.Status.CommittedResourceReservation.Allocations) + } + env.T.Logf(" - %s (host: %s, allocations: %d)", res.Name, res.Status.Host, allocCount) } + env.T.Log("=====================") } -func TestProcessCommitmentChanges_KnowledgeNotReady(t *testing.T) { - // Test when flavor groups knowledge is not available - scheme := runtime.NewScheme() - if err := v1alpha1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add scheme: %v", err) +// CallChangeCommitmentsAPI calls the change commitments API endpoint with JSON. +// It uses a hybrid approach: fast polling during API execution + synchronous final pass. +func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liquid.CommitmentChangeResponse, statusCode int) { + env.T.Helper() + + // Start fast polling in background to handle reservations during API execution + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + + go func() { + ticker := time.NewTicker(5 * time.Millisecond) // Very fast - 5ms + defer ticker.Stop() + defer close(done) + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + env.processReservations() + } + } + }() + + // Make HTTP request + url := env.HTTPServer.URL + "/v1/change-commitments" + httpResp, err := http.Post(url, "application/json", bytes.NewReader([]byte(reqJSON))) //nolint:gosec,noctx // test server URL, not user input + if err != nil { + cancel() + <-done + env.T.Fatalf("Failed to make HTTP request: %v", err) } + defer httpResp.Body.Close() - // No Knowledge CRD created - simulates knowledge not ready - k8sClient := fake.NewClientBuilder(). - WithScheme(scheme). - Build() + // Read response body + respBytes, err := io.ReadAll(httpResp.Body) + if err != nil { + cancel() + <-done + env.T.Fatalf("Failed to read response body: %v", err) + } - api := &HTTPAPI{ - client: k8sClient, + // Parse response - only for 200 OK responses + // Non-200 responses (like 409 Conflict for version mismatch) use plain text via http.Error() + if httpResp.StatusCode == http.StatusOK { + if err := json.Unmarshal(respBytes, &resp); err != nil { + cancel() + <-done + env.T.Fatalf("Failed to unmarshal response: %v", err) + } } - requestJSON := `{ - "az": "az-a", - "dryRun": false, - "infoVersion": 12345, - "byProject": {} - }` + // Stop background polling + cancel() + <-done - req := httptest.NewRequest(http.MethodPost, "/v1/change-commitments", bytes.NewReader([]byte(requestJSON))) - req.Header.Set("Content-Type", "application/json") - w := httptest.NewRecorder() + // Final synchronous pass to ensure all reservations are processed + // This eliminates any race conditions + env.processReservations() - api.HandleChangeCommitments(w, req) + statusCode = httpResp.StatusCode + return resp, statusCode +} + +// processReservations handles all reservation lifecycle events synchronously. +// This includes marking reservations as Ready/Failed and removing finalizers from deleted reservations. +func (env *CommitmentTestEnv) processReservations() { + ctx := context.Background() + reservations := env.ListReservations() + + for _, res := range reservations { + // Handle deletion - return memory to host and remove finalizers + if !res.DeletionTimestamp.IsZero() { + env.T.Logf("Processing deletion for reservation %s (host: %s)", res.Name, res.Status.Host) + + // Return memory to host if resource tracking is enabled + if env.availableResources != nil { + env.T.Logf("Resource tracking enabled, returning memory for %s", res.Name) + memoryQuantity := res.Spec.Resources["memory"] + memoryBytes := memoryQuantity.Value() + memoryMB := memoryBytes / (1024 * 1024) + + env.T.Logf("Reservation %s has host=%s, memory=%d MB", res.Name, res.Status.Host, memoryMB) + + // Check if host exists in our tracking + if _, exists := env.availableResources[res.Status.Host]; !exists { + env.T.Fatalf("Host %s not found in available resources for reservation %s - this indicates an inconsistency", + res.Status.Host, res.Name) + } - resp := w.Result() - defer resp.Body.Close() + // Return memory to host + env.availableResources[res.Status.Host] += memoryMB + env.T.Logf("↩ Returned %d MB to %s (now %d MB available) from deleted reservation %s", + memoryMB, res.Status.Host, env.availableResources[res.Status.Host], res.Name) + } else { + env.T.Logf("Resource tracking NOT enabled for %s", res.Name) + } + + // Remove finalizers to allow deletion + if len(res.Finalizers) > 0 { + res.Finalizers = []string{} + if err := env.K8sClient.Update(ctx, &res); err != nil { + // Ignore errors - might be already deleted + continue + } + } + continue + } + + // Skip if already processed (has a condition set) + if env.hasCondition(&res) { + continue + } + + // Skip if already tracked as processed + if env.processedReserv[res.Name] { + continue + } + + // Process new reservation with resource-based scheduling + env.processNewReservation(&res) + } +} - // Should return 200 OK with rejection reason - if resp.StatusCode != http.StatusOK { - t.Errorf("expected status code %d (OK), got %d", http.StatusOK, resp.StatusCode) +// hasCondition checks if a reservation has any Ready condition set. +func (env *CommitmentTestEnv) hasCondition(res *v1alpha1.Reservation) bool { + for _, cond := range res.Status.Conditions { + if cond.Type == v1alpha1.ReservationConditionReady { + return true + } } + return false +} + +// processNewReservation implements first-come-first-serve scheduling based on available resources. +// It tries to find a host with enough memory capacity and assigns the reservation to that host. +func (env *CommitmentTestEnv) processNewReservation(res *v1alpha1.Reservation) { + env.processedReserv[res.Name] = true - var response liquid.CommitmentChangeResponse - if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { - t.Fatalf("failed to decode response: %v", err) + // If no available resources configured, accept all reservations without host assignment + if env.availableResources == nil { + env.markReservationReady(res) + return } - if response.RejectionReason != "caches not ready" { - t.Errorf("expected rejection reason 'caches not ready', got %q", response.RejectionReason) + // Get required memory from reservation spec + memoryQuantity := res.Spec.Resources["memory"] + memoryBytes := memoryQuantity.Value() + memoryMB := memoryBytes / (1024 * 1024) + + // First-come-first-serve: find first host with enough capacity + var selectedHost string + for host, availableMB := range env.availableResources { + if availableMB >= memoryMB { + selectedHost = host + break + } } - if response.RetryAt.IsNone() { - t.Error("expected RetryAt to be set") + if selectedHost != "" { + // SUCCESS: Schedule on this host + env.availableResources[selectedHost] -= memoryMB + + // Update reservation with selected host + ctx := context.Background() + + // Update spec (TargetHost) + res.Spec.TargetHost = selectedHost + if err := env.K8sClient.Update(ctx, res); err != nil { + env.T.Logf("Warning: Failed to update reservation spec: %v", err) + } + + // Update status (Host) - requires Status().Update + res.Status.Host = selectedHost + if err := env.K8sClient.Status().Update(ctx, res); err != nil { + env.T.Logf("Warning: Failed to update reservation status host: %v", err) + } + + env.markReservationReady(res) + env.T.Logf("✓ Scheduled reservation %s on %s (%d MB used, %d MB remaining)", + res.Name, selectedHost, memoryMB, env.availableResources[selectedHost]) + } else { + // FAILURE: No host has enough capacity + env.markReservationFailed(res, "Insufficient capacity on all hosts") + env.T.Logf("✗ Failed to schedule reservation %s (needs %d MB, no host has capacity)", + res.Name, memoryMB) } } -// Helper function to create a minimal flavor group for testing -func createTestFlavorGroup() compute.FlavorGroupFeature { - return compute.FlavorGroupFeature{ - Name: "test_group", - Flavors: []compute.FlavorInGroup{ - { - Name: "test.small", - MemoryMB: 8192, - VCPUs: 2, - DiskGB: 40, - ExtraSpecs: map[string]string{ - "quota:separate": "true", - }, +// markReservationReady updates a reservation to have Ready=True status. +func (env *CommitmentTestEnv) markReservationReady(res *v1alpha1.Reservation) { + res.Status.Conditions = []metav1.Condition{ + { + Type: v1alpha1.ReservationConditionReady, + Status: metav1.ConditionTrue, + Reason: "ReservationActive", + Message: "Reservation is ready (set by test controller)", + LastTransitionTime: metav1.Now(), + }, + } + + if err := env.K8sClient.Status().Update(context.Background(), res); err != nil { + // Ignore errors - might be deleted during update + return + } +} + +// markReservationFailed updates a reservation to have Ready=False status (scheduling failed). +func (env *CommitmentTestEnv) markReservationFailed(res *v1alpha1.Reservation, reason string) { + res.Status.Conditions = []metav1.Condition{ + { + Type: v1alpha1.ReservationConditionReady, + Status: metav1.ConditionFalse, + Reason: "SchedulingFailed", + Message: reason, + LastTransitionTime: metav1.Now(), + }, + } + + if err := env.K8sClient.Status().Update(context.Background(), res); err != nil { + // Ignore errors - might be deleted during update + return + } +} + +// VerifyAPIResponse verifies the API response matches expectations. +// For rejection reasons, it checks if ALL expected substrings are present in the actual rejection reason. +func (env *CommitmentTestEnv) VerifyAPIResponse(expected APIResponseExpectation, actual liquid.CommitmentChangeResponse, statusCode int) { + env.T.Helper() + + if statusCode != expected.StatusCode { + env.T.Errorf("Expected status code %d, got %d", expected.StatusCode, statusCode) + } + + if len(expected.RejectReasonSubstrings) > 0 { + if actual.RejectionReason == "" { + env.T.Errorf("Expected rejection reason containing substrings %v, got none", expected.RejectReasonSubstrings) + } else { + // Check that ALL expected substrings are present + for _, substring := range expected.RejectReasonSubstrings { + if !strings.Contains(actual.RejectionReason, substring) { + env.T.Errorf("Expected rejection reason to contain %q, but got %q", substring, actual.RejectionReason) + } + } + } + } else { + if actual.RejectionReason != "" { + env.T.Errorf("Expected no rejection reason, got %q", actual.RejectionReason) + } + } +} + +// VerifyReservationsMatch verifies that actual reservations match expected reservations by content. +func (env *CommitmentTestEnv) VerifyReservationsMatch(expected []*TestReservation) { + env.T.Helper() + + actualReservations := env.ListReservations() + + // Make copies of both lists so we can remove matched items + expectedCopy := make([]*TestReservation, len(expected)) + copy(expectedCopy, expected) + + actualCopy := make([]v1alpha1.Reservation, len(actualReservations)) + copy(actualCopy, actualReservations) + + // Track unmatched items for detailed reporting + var unmatchedExpected []*TestReservation + var unmatchedActual []v1alpha1.Reservation + + // Greedy matching: while there are expected items, find matches and remove + for len(expectedCopy) > 0 { + exp := expectedCopy[0] + found := false + + // Find first actual that matches this expected + for i, actual := range actualCopy { + if env.reservationMatches(exp, &actual) { + expectedCopy = expectedCopy[1:] + actualCopy = append(actualCopy[:i], actualCopy[i+1:]...) + found = true + break + } + } + + if !found { + unmatchedExpected = append(unmatchedExpected, exp) + expectedCopy = expectedCopy[1:] + } + } + + unmatchedActual = actualCopy + + // If there are any mismatches, print detailed comparison + if len(unmatchedExpected) > 0 || len(unmatchedActual) > 0 { + env.T.Error("❌ Reservation mismatch detected!") + env.T.Log("") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.T.Log("EXPECTED RESERVATIONS:") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.printExpectedReservations(expected, unmatchedExpected) + + env.T.Log("") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.T.Log("ACTUAL RESERVATIONS:") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.printActualReservations(actualReservations, unmatchedActual) + + env.T.Log("") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.T.Log("DIFF SUMMARY:") + env.T.Log("═══════════════════════════════════════════════════════════════") + env.printDiffSummary(unmatchedExpected, unmatchedActual) + env.T.Log("═══════════════════════════════════════════════════════════════") + } +} + +// String returns a compact string representation of a TestReservation. +func (tr *TestReservation) String() string { + flavorName := "" + flavorGroup := "" + if tr.Flavor != nil { + flavorName = tr.Flavor.Name + flavorGroup = tr.Flavor.Group + } + + host := tr.Host + if host == "" { + host = "" + } + + az := tr.AZ + if az == "" { + az = "" + } + + vmInfo := "" + if len(tr.VMs) > 0 { + vmInfo = fmt.Sprintf(" VMs=%v", tr.VMs) + } + + return fmt.Sprintf("%s/%s/%s(%s)/%s/az=%s%s", tr.CommitmentID, tr.ProjectID, flavorName, flavorGroup, host, az, vmInfo) +} + +// compactReservationString returns a compact string representation of an actual Reservation. +func compactReservationString(res *v1alpha1.Reservation) string { + commitmentID := "" + projectID := "" + flavorName := "" + flavorGroup := "" + vmCount := 0 + + if res.Spec.CommittedResourceReservation != nil { + commitmentID = res.Spec.CommittedResourceReservation.CommitmentUUID + projectID = res.Spec.CommittedResourceReservation.ProjectID + flavorName = res.Spec.CommittedResourceReservation.ResourceName + flavorGroup = res.Spec.CommittedResourceReservation.ResourceGroup + if res.Status.CommittedResourceReservation != nil { + vmCount = len(res.Status.CommittedResourceReservation.Allocations) + } + } + + host := res.Status.Host + if host == "" { + host = "" + } + + az := res.Spec.AvailabilityZone + if az == "" { + az = "" + } + + vmInfo := "" + if vmCount > 0 { + vmInfo = fmt.Sprintf(" VMs=%d", vmCount) + } + + return fmt.Sprintf("%s/%s/%s(%s)/%s/az=%s%s", commitmentID, projectID, flavorName, flavorGroup, host, az, vmInfo) +} + +// printExpectedReservations prints all expected reservations with markers for unmatched ones. +func (env *CommitmentTestEnv) printExpectedReservations(all, unmatched []*TestReservation) { + env.T.Helper() + + unmatchedMap := make(map[*TestReservation]bool) + for _, res := range unmatched { + unmatchedMap[res] = true + } + + if len(all) == 0 { + env.T.Log(" (none)") + return + } + + for i, res := range all { + marker := "✓" + if unmatchedMap[res] { + marker = "✗" + } + env.T.Logf(" %s [%d] %s", marker, i+1, res.String()) + } + + env.T.Logf(" Total: %d (%d matched, %d missing)", + len(all), len(all)-len(unmatched), len(unmatched)) +} + +// printActualReservations prints all actual reservations with markers for unmatched ones. +func (env *CommitmentTestEnv) printActualReservations(all, unmatched []v1alpha1.Reservation) { + env.T.Helper() + + unmatchedMap := make(map[string]bool) + for _, res := range unmatched { + unmatchedMap[res.Name] = true + } + + if len(all) == 0 { + env.T.Log(" (none)") + return + } + + for i, res := range all { + marker := "✓" + if unmatchedMap[res.Name] { + marker = "⊕" + } + env.T.Logf(" %s [%d] %s", marker, i+1, compactReservationString(&res)) + } + + env.T.Logf(" Total: %d (%d matched, %d unexpected)", + len(all), len(all)-len(unmatched), len(unmatched)) +} + +// printDiffSummary prints a summary of differences between expected and actual. +func (env *CommitmentTestEnv) printDiffSummary(unmatchedExpected []*TestReservation, unmatchedActual []v1alpha1.Reservation) { + env.T.Helper() + + if len(unmatchedExpected) > 0 { + env.T.Logf(" MISSING (%d expected, not found):", len(unmatchedExpected)) + for _, res := range unmatchedExpected { + env.T.Logf(" • %s", res.String()) + } + } + + if len(unmatchedActual) > 0 { + env.T.Logf(" UNEXPECTED (%d found, not expected):", len(unmatchedActual)) + for _, res := range unmatchedActual { + env.T.Logf(" • %s", compactReservationString(&res)) + } + } + + if len(unmatchedExpected) == 0 && len(unmatchedActual) == 0 { + env.T.Log(" ✓ All match!") + } +} + +// reservationMatches checks if an actual reservation matches an expected one. +// All fields are checked comprehensively for complete validation. +func (env *CommitmentTestEnv) reservationMatches(expected *TestReservation, actual *v1alpha1.Reservation) bool { + // Check CommitmentID (from reservation name prefix) + if !strings.HasPrefix(actual.Name, "commitment-"+expected.CommitmentID+"-") { + return false + } + + // Check that CommittedResourceReservation spec exists + if actual.Spec.CommittedResourceReservation == nil { + return false + } + + // Check CommitmentUUID in spec matches + if actual.Spec.CommittedResourceReservation.CommitmentUUID != expected.CommitmentID { + return false + } + + // Check ProjectID + if actual.Spec.CommittedResourceReservation.ProjectID != expected.ProjectID { + return false + } + + // Check ResourceName (flavor name) + if expected.Flavor != nil { + if actual.Spec.CommittedResourceReservation.ResourceName != expected.Flavor.Name { + return false + } + } + + // Check ResourceGroup (flavor group) + if expected.Flavor != nil { + if actual.Spec.CommittedResourceReservation.ResourceGroup != expected.Flavor.Group { + return false + } + } + + // Check Host (if specified in expected) + if expected.Host != "" && actual.Status.Host != expected.Host { + return false + } + + // Check AZ (if specified in expected) + if expected.AZ != "" && actual.Spec.AvailabilityZone != expected.AZ { + return false + } + + // Check Memory (use custom MemoryMB if non-zero, otherwise use flavor size) + expectedMemoryMB := expected.MemoryMB + if expectedMemoryMB == 0 && expected.Flavor != nil { + expectedMemoryMB = expected.Flavor.MemoryMB + } + memoryQuantity := actual.Spec.Resources["memory"] + actualMemoryBytes := memoryQuantity.Value() + actualMemoryMB := actualMemoryBytes / (1024 * 1024) + if actualMemoryMB != expectedMemoryMB { + return false + } + + // Check CPU (from flavor if available) + if expected.Flavor != nil { + cpuQuantity := actual.Spec.Resources["cpu"] + actualCPU := cpuQuantity.Value() + if actualCPU != expected.Flavor.VCPUs { + return false + } + } + + // Check VM allocations (set comparison - order doesn't matter) + if !env.vmAllocationsMatch(expected.VMs, actual) { + return false + } + + // Check reservation type + if actual.Spec.Type != v1alpha1.ReservationTypeCommittedResource { + return false + } + + return true +} + +// vmAllocationsMatch checks if VM allocations match (set comparison). +func (env *CommitmentTestEnv) vmAllocationsMatch(expectedVMs []string, actual *v1alpha1.Reservation) bool { + if actual.Status.CommittedResourceReservation == nil { + return len(expectedVMs) == 0 + } + + actualVMs := make(map[string]bool) + for vmUUID := range actual.Status.CommittedResourceReservation.Allocations { + actualVMs[vmUUID] = true + } + + // Check counts match + if len(expectedVMs) != len(actualVMs) { + return false + } + + // Check all expected VMs are in actual + for _, vmUUID := range expectedVMs { + if !actualVMs[vmUUID] { + return false + } + } + + return true +} + +// ============================================================================ +// Mock VM Source +// ============================================================================ + +// MockVMSource implements VMSource for testing. +type MockVMSource struct { + VMs []VM +} + +// NewMockVMSource creates a new MockVMSource with the given VMs. +func NewMockVMSource(vms []VM) *MockVMSource { + return &MockVMSource{VMs: vms} +} + +// ListVMs returns the configured VMs. +func (s *MockVMSource) ListVMs(_ context.Context) ([]VM, error) { + return s.VMs, nil +} + +// ============================================================================ +// Helper Functions +// ============================================================================ + +// newHypervisorWithAZ creates a Hypervisor CRD with the given parameters including availability zone. +func newHypervisorWithAZ(name string, cpuCap, memoryGi, cpuAlloc, memoryGiAlloc int, instances []hv1.Instance, traits []string, az string) *hv1.Hypervisor { + labels := make(map[string]string) + if az != "" { + labels[corev1.LabelTopologyZone] = az + } + return &hv1.Hypervisor{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: labels, + }, + Status: hv1.HypervisorStatus{ + Capacity: map[hv1.ResourceName]resource.Quantity{ + "cpu": resource.MustParse(strconv.Itoa(cpuCap)), + "memory": resource.MustParse(strconv.Itoa(memoryGi) + "Gi"), + }, + Allocation: map[hv1.ResourceName]resource.Quantity{ + "cpu": resource.MustParse(strconv.Itoa(cpuAlloc)), + "memory": resource.MustParse(strconv.Itoa(memoryGiAlloc) + "Gi"), + }, + NumInstances: len(instances), + Instances: instances, + Traits: traits, + }, + } +} + +// createCommitment creates a TestCommitment for use in test cases. +// The az parameter is optional - if empty string, no AZ constraint is set. +func createCommitment(resourceName, projectID, confirmationID, state string, amount uint64, az ...string) TestCommitment { + return TestCommitment{ + ResourceName: liquid.ResourceName(resourceName), + ProjectID: projectID, + ConfirmationID: confirmationID, + State: state, + Amount: amount, + } +} + +// newCommitmentRequest creates a CommitmentChangeRequest with the given commitments. +func newCommitmentRequest(az string, dryRun bool, infoVersion int64, commitments ...TestCommitment) CommitmentChangeRequest { + return CommitmentChangeRequest{ + AZ: az, + DryRun: dryRun, + InfoVersion: infoVersion, + Commitments: commitments, + } +} + +// newAPIResponse creates an APIResponseExpectation with 200 OK status. +func newAPIResponse(rejectReasonSubstrings ...string) APIResponseExpectation { + return APIResponseExpectation{ + StatusCode: 200, + RejectReasonSubstrings: rejectReasonSubstrings, + } +} + +// buildRequestJSON converts a test CommitmentChangeRequest to JSON string. +// Builds the nested JSON structure directly for simplicity. +func buildRequestJSON(req CommitmentChangeRequest) string { + // Group commitments by project and resource for nested structure + type projectResources map[liquid.ResourceName][]TestCommitment + byProject := make(map[string]projectResources) + + for _, commit := range req.Commitments { + if byProject[commit.ProjectID] == nil { + byProject[commit.ProjectID] = make(projectResources) + } + byProject[commit.ProjectID][commit.ResourceName] = append( + byProject[commit.ProjectID][commit.ResourceName], + commit, + ) + } + + // Build nested JSON structure + var projectParts []string + for projectID, resources := range byProject { + var resourceParts []string + for resourceName, commits := range resources { + var commitParts []string + for _, c := range commits { + expiryTime := time.Now().Add(time.Duration(defaultCommitmentExpiryYears) * 365 * 24 * time.Hour) + commitParts = append(commitParts, fmt.Sprintf(`{"uuid":"%s","newStatus":"%s","amount":%d,"expiresAt":"%s"}`, + c.ConfirmationID, c.State, c.Amount, expiryTime.Format(time.RFC3339))) + } + resourceParts = append(resourceParts, fmt.Sprintf(`"%s":{"commitments":[%s]}`, + resourceName, strings.Join(commitParts, ","))) + } + projectParts = append(projectParts, fmt.Sprintf(`"%s":{"byResource":{%s}}`, + projectID, strings.Join(resourceParts, ","))) + } + + return fmt.Sprintf(`{"az":"%s","dryRun":%t,"infoVersion":%d,"byProject":{%s}}`, + req.AZ, req.DryRun, req.InfoVersion, strings.Join(projectParts, ",")) +} + +// createKnowledgeCRD creates a Knowledge CRD populated with flavor groups. +func createKnowledgeCRD(flavorGroups FlavorGroupsKnowledge) *v1alpha1.Knowledge { + rawExt, err := v1alpha1.BoxFeatureList(flavorGroups.Groups) + if err != nil { + panic("Failed to box flavor groups: " + err.Error()) + } + + lastContentChange := time.Unix(flavorGroups.InfoVersion, 0) + + return &v1alpha1.Knowledge{ + ObjectMeta: metav1.ObjectMeta{ + Name: flavorGroupsKnowledgeName, + }, + Spec: v1alpha1.KnowledgeSpec{ + SchedulingDomain: v1alpha1.SchedulingDomainNova, + Extractor: v1alpha1.KnowledgeExtractorSpec{ + Name: flavorGroupsKnowledgeName, }, + Recency: metav1.Duration{Duration: knowledgeRecencyDuration}, }, - SmallestFlavor: compute.FlavorInGroup{ - Name: "test.small", - MemoryMB: 8192, - VCPUs: 2, - DiskGB: 40, + Status: v1alpha1.KnowledgeStatus{ + LastExtracted: metav1.Time{Time: lastContentChange}, + LastContentChange: metav1.Time{Time: lastContentChange}, + Raw: rawExt, + RawLength: len(flavorGroups.Groups), + Conditions: []metav1.Condition{ + { + Type: v1alpha1.KnowledgeConditionReady, + Status: metav1.ConditionTrue, + Reason: "KnowledgeReady", + Message: "Flavor groups knowledge is ready", + LastTransitionTime: metav1.Time{Time: lastContentChange}, + }, + }, }, } } diff --git a/internal/scheduling/reservations/commitments/reservation_manager.go b/internal/scheduling/reservations/commitments/reservation_manager.go index 13856d992..21ee1fee1 100644 --- a/internal/scheduling/reservations/commitments/reservation_manager.go +++ b/internal/scheduling/reservations/commitments/reservation_manager.go @@ -136,7 +136,7 @@ func (m *ReservationManager) ApplyCommitmentState( memValue := reservationToDelete.Spec.Resources[hv1.ResourceMemory] deltaMemoryBytes += memValue.Value() - log.Info("deleting reservation", + log.Info("deleting reservation (capacity decrease)", "commitmentUUID", desiredState.CommitmentUUID, "deltaMemoryBytes", deltaMemoryBytes, "name", reservationToDelete.Name, @@ -205,19 +205,25 @@ func (m *ReservationManager) syncReservationMetadata( state *CommitmentState, ) (*v1alpha1.Reservation, error) { - // if any of AZ, StarTime, EndTime differ from desired state, need to patch - if (state.AvailabilityZone != "" && reservation.Spec.AvailabilityZone != state.AvailabilityZone) || + // if any of CommitmentUUID, AZ, StarTime, EndTime differ from desired state, need to patch + if (state.CommitmentUUID != "" && reservation.Spec.CommittedResourceReservation.CommitmentUUID != state.CommitmentUUID) || + (state.AvailabilityZone != "" && reservation.Spec.AvailabilityZone != state.AvailabilityZone) || (state.StartTime != nil && (reservation.Spec.StartTime == nil || !reservation.Spec.StartTime.Time.Equal(*state.StartTime))) || (state.EndTime != nil && (reservation.Spec.EndTime == nil || !reservation.Spec.EndTime.Time.Equal(*state.EndTime))) { // Apply patch log.Info("syncing reservation metadata", - "reservation", reservation.Name, - "availabilityZone", state.AvailabilityZone, - "startTime", state.StartTime, - "endTime", state.EndTime) + "reservation", reservation, + "desired commitmentUUID", state.CommitmentUUID, + "desired availabilityZone", state.AvailabilityZone, + "desired startTime", state.StartTime, + "desired endTime", state.EndTime) patch := client.MergeFrom(reservation.DeepCopy()) + if state.CommitmentUUID != "" { + reservation.Spec.CommittedResourceReservation.CommitmentUUID = state.CommitmentUUID + } + if state.AvailabilityZone != "" { reservation.Spec.AvailabilityZone = state.AvailabilityZone } @@ -277,12 +283,13 @@ func (m *ReservationManager) newReservation( ), }, CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ - ProjectID: state.ProjectID, - DomainID: state.DomainID, - ResourceGroup: state.FlavorGroupName, - ResourceName: flavorInGroup.Name, - Creator: creator, - Allocations: nil, + ProjectID: state.ProjectID, + CommitmentUUID: state.CommitmentUUID, + DomainID: state.DomainID, + ResourceGroup: state.FlavorGroupName, + ResourceName: flavorInGroup.Name, + Creator: creator, + Allocations: nil, }, } diff --git a/internal/scheduling/reservations/commitments/state.go b/internal/scheduling/reservations/commitments/state.go index 996efff8e..50108beef 100644 --- a/internal/scheduling/reservations/commitments/state.go +++ b/internal/scheduling/reservations/commitments/state.go @@ -29,7 +29,7 @@ func getFlavorGroupNameFromResource(resourceName string) (string, error) { // CommitmentState represents desired or current commitment resource allocation. type CommitmentState struct { - // CommitmentUUID uniquely identifies this commitment + // CommitmentUUID is the UUID of the commitment this state corresponds to. CommitmentUUID string // ProjectID is the OpenStack project this commitment belongs to ProjectID string From f0407427c5553ddf04f72aa3d0668aa4312f1ee2 Mon Sep 17 00:00:00 2001 From: mblos Date: Tue, 17 Mar 2026 15:46:23 +0100 Subject: [PATCH 2/8] make target testsum --- Makefile | 15 +++++++++++++-- docs/develop.md | 15 +++++++++++++++ .../commitments/api_change_commitments_test.go | 18 ++++++++++++++++-- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 148130f0f..3d90f6161 100644 --- a/Makefile +++ b/Makefile @@ -25,8 +25,19 @@ lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes "$(GOLANGCI_LINT)" run --fix .PHONY: test -test: gotestsum ## Run all tests with summary. - $(GOTESTSUM) --format testname ./... +test: ## Run all tests. + go test ./... + +.PHONY: testsum +testsum: gotestsum ## Run all tests (clean output for passing, verbose for failing). Options: WATCH=1, RUN=, PACKAGE=, FORMAT= (e.g., standard-verbose for all output) + $(GOTESTSUM) \ + $(if $(WATCH),--watch) \ + --format $(if $(FORMAT),$(FORMAT),testname) \ + --hide-summary=all \ + -- \ + $(if $(VERBOSE),-v) \ + $(if $(RUN),-run $(RUN)) \ + $(if $(PACKAGE),$(PACKAGE),./...) .PHONY: generate generate: deepcopy crds ## Regenerate CRDs and DeepCopy after API type changes. diff --git a/docs/develop.md b/docs/develop.md index 5b090c889..c39cbd61a 100644 --- a/docs/develop.md +++ b/docs/develop.md @@ -34,6 +34,21 @@ Cortex is developed using the Go programming language. To get started with the d Run `make` in your terminal from the cortex root directory to perform linting and testing tasks. +### Working on Tests + +```bash +# Watch mode for continuous testing; print logs for failed tests only +make testsum WATCH=1 +``` + +The `testsum` target provides cleaner output by showing only full verbose output for failing tests. + +**Available options:** +- `WATCH=1` - Automatically re-run tests when files change +- `RUN=` - Run specific tests matching the pattern +- `PACKAGE=` - Test specific package(s) +- `FORMAT=` - Change output format (e.g., `standard-verbose` for verbose output on all tests) + ## Helm Charts Helm charts bundle the application into a package, containing all the [Kubernetes](https://kubernetes.io/docs/tutorials/hello-minikube/) resources needed to run the application. The configuration for the application is specified in the [Helm `values.yaml`](cortex.secrets.example.yaml). diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index 4a4b9476f..730ec7a44 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -38,6 +38,7 @@ import ( // ============================================================================ func TestCommitmentChangeIntegration(t *testing.T) { + m1Tiny := &TestFlavor{Name: "m1.tiny", Group: "gp_1", MemoryMB: 256, VCPUs: 1} m1Small := &TestFlavor{Name: "m1.small", Group: "hana_1", MemoryMB: 1024, VCPUs: 4} m1Large := &TestFlavor{Name: "m1.large", Group: "hana_1", MemoryMB: 4096, VCPUs: 16} m1XL := &TestFlavor{Name: "m1.xl", Group: "hana_1", MemoryMB: 8192, VCPUs: 32} @@ -67,7 +68,7 @@ func TestCommitmentChangeIntegration(t *testing.T) { CommitmentRequest: newCommitmentRequest("az-a", false, 1234, createCommitment("ram_hana_1", "project-A", "uuid-456", "confirmed", 3)), AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 1024, "host-2": 0}}, ExpectedReservations: []*TestReservation{{CommitmentID: "uuid-456", Host: "", Flavor: m1Small, ProjectID: "project-A"}}, - ExpectedAPIResponse: newAPIResponse("uuid-456", "not sufficient capacity"), + ExpectedAPIResponse: newAPIResponse("1 commitment(s) failed", "commitment uuid-456: not sufficient capacity"), }, { Name: "Swap capacity between CRs - order dependent - delete-first succeeds", @@ -99,7 +100,7 @@ func TestCommitmentChangeIntegration(t *testing.T) { ExpectedReservations: []*TestReservation{ {CommitmentID: "uuid-123", Host: "host-1", Flavor: m1Small, ProjectID: "project-B"}, {CommitmentID: "uuid-123", Host: "host-2", Flavor: m1Small, ProjectID: "project-B"}}, - ExpectedAPIResponse: newAPIResponse("uuid-456", "not sufficient capacity"), + ExpectedAPIResponse: newAPIResponse("1 commitment(s) failed", "commitment uuid-456: not sufficient capacity"), }, { Name: "Flavor bin-packing - mixed sizes when largest doesn't fit", @@ -379,6 +380,19 @@ func TestCommitmentChangeIntegration(t *testing.T) { ExpectedAPIResponse: newAPIResponse("caches not ready"), EnvInfoVersion: -1, // Skip Knowledge CRD creation }, + { + Name: "Multiple commitments insufficient capacity - all listed in error", + // Tests that multiple failed commitments are all mentioned in the rejection reason + Flavors: []*TestFlavor{m1Small, m1Tiny}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-multi-fail-1", "confirmed", 3), + createCommitment("ram_hana_1", "project-B", "uuid-multi-fail-2", "confirmed", 3), + createCommitment("ram_gp_1", "project-C", "uuid-would-not-fail", "confirmed", 1), // would be rolled back, but not part of the reject reason + ), + AvailableResources: &AvailableResources{PerHost: map[string]int64{"host-1": 256}}, + ExpectedReservations: []*TestReservation{}, + ExpectedAPIResponse: newAPIResponse("2 commitment(s) failed", "commitment uuid-multi-fail-1: not sufficient capacity", "commitment uuid-multi-fail-2: not sufficient capacity"), + }, } for _, tc := range testCases { From 090a47c318b5a07e0cb854b07855c113ac308cf6 Mon Sep 17 00:00:00 2001 From: mblos Date: Tue, 17 Mar 2026 16:27:03 +0100 Subject: [PATCH 3/8] validate retry at --- .../api_change_commitments_test.go | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index 730ec7a44..b77678294 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -377,8 +377,12 @@ func TestCommitmentChangeIntegration(t *testing.T) { createCommitment("ram_hana_1", "project-A", "uuid-knowledge", "confirmed", 2), ), ExpectedReservations: []*TestReservation{}, - ExpectedAPIResponse: newAPIResponse("caches not ready"), - EnvInfoVersion: -1, // Skip Knowledge CRD creation + ExpectedAPIResponse: APIResponseExpectation{ + StatusCode: 200, + RejectReasonSubstrings: []string{"caches not ready"}, + RetryAtPresent: true, + }, + EnvInfoVersion: -1, // Skip Knowledge CRD creation }, { Name: "Multiple commitments insufficient capacity - all listed in error", @@ -446,13 +450,13 @@ func runCommitmentChangeTest(t *testing.T, tc CommitmentChangeTestCase) { // Call commitment change API reqJSON := buildRequestJSON(tc.CommitmentRequest) - resp, statusCode := env.CallChangeCommitmentsAPI(reqJSON) + resp, respJSON, statusCode := env.CallChangeCommitmentsAPI(reqJSON) t.Log("After API call:") env.LogStateSummary() // Verify API response - env.VerifyAPIResponse(tc.ExpectedAPIResponse, resp, statusCode) + env.VerifyAPIResponse(tc.ExpectedAPIResponse, resp, respJSON, statusCode) // Verify reservations using content-based matching env.VerifyReservationsMatch(tc.ExpectedReservations) @@ -915,7 +919,7 @@ func (env *CommitmentTestEnv) LogStateSummary() { // CallChangeCommitmentsAPI calls the change commitments API endpoint with JSON. // It uses a hybrid approach: fast polling during API execution + synchronous final pass. -func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liquid.CommitmentChangeResponse, statusCode int) { +func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liquid.CommitmentChangeResponse, respJSON string, statusCode int) { env.T.Helper() // Start fast polling in background to handle reservations during API execution @@ -955,6 +959,8 @@ func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liq env.T.Fatalf("Failed to read response body: %v", err) } + respJSON = string(respBytes) + // Parse response - only for 200 OK responses // Non-200 responses (like 409 Conflict for version mismatch) use plain text via http.Error() if httpResp.StatusCode == http.StatusOK { @@ -974,7 +980,7 @@ func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liq env.processReservations() statusCode = httpResp.StatusCode - return resp, statusCode + return resp, respJSON, statusCode } // processReservations handles all reservation lifecycle events synchronously. @@ -1140,7 +1146,7 @@ func (env *CommitmentTestEnv) markReservationFailed(res *v1alpha1.Reservation, r // VerifyAPIResponse verifies the API response matches expectations. // For rejection reasons, it checks if ALL expected substrings are present in the actual rejection reason. -func (env *CommitmentTestEnv) VerifyAPIResponse(expected APIResponseExpectation, actual liquid.CommitmentChangeResponse, statusCode int) { +func (env *CommitmentTestEnv) VerifyAPIResponse(expected APIResponseExpectation, actual liquid.CommitmentChangeResponse, respJSON string, statusCode int) { env.T.Helper() if statusCode != expected.StatusCode { @@ -1163,6 +1169,18 @@ func (env *CommitmentTestEnv) VerifyAPIResponse(expected APIResponseExpectation, env.T.Errorf("Expected no rejection reason, got %q", actual.RejectionReason) } } + + // Check RetryAt field presence in JSON (avoids dealing with option.Option type) + retryAtPresent := strings.Contains(respJSON, `"retryAt"`) + if expected.RetryAtPresent { + if !retryAtPresent { + env.T.Error("Expected retryAt field to be present in JSON response, but it was not found") + } + } else { + if retryAtPresent { + env.T.Error("Expected retryAt field to be absent from JSON response, but it was found") + } + } } // VerifyReservationsMatch verifies that actual reservations match expected reservations by content. From 19b9f328b7c9c5fada01e8c5ff7d23559e3c3680 Mon Sep 17 00:00:00 2001 From: mblos Date: Tue, 17 Mar 2026 16:32:37 +0100 Subject: [PATCH 4/8] fix reservation watch --- .../commitments/api_change_commitments.go | 17 ++++++-------- .../api_change_commitments_test.go | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/internal/scheduling/reservations/commitments/api_change_commitments.go b/internal/scheduling/reservations/commitments/api_change_commitments.go index 88fe5732a..6da316bd8 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments.go @@ -309,6 +309,7 @@ func watchReservationsUntilReady( copy(reservationsToWatch, reservations) for { + var stillWaiting []v1alpha1.Reservation if time.Now().After(deadline) { errors = append(errors, fmt.Errorf("timeout after %v waiting for reservations to become ready", timeout)) return failedReservations, errors @@ -316,8 +317,7 @@ func watchReservationsUntilReady( allChecked := true - check: - for i, res := range reservationsToWatch { + for _, res := range reservationsToWatch { // Fetch current state var current v1alpha1.Reservation nn := types.NamespacedName{ @@ -329,13 +329,13 @@ func watchReservationsUntilReady( allChecked = false if apierrors.IsNotFound(err) { // Reservation is still in process of being created, continue waiting for it + stillWaiting = append(stillWaiting, res) continue } // remove reservation from waiting failedReservations = append(failedReservations, res) - reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) errors = append(errors, fmt.Errorf("failed to get reservation %s: %w", res.Name, err)) - break check // break because iterating list was modified + continue } // Check Ready condition @@ -347,23 +347,19 @@ func watchReservationsUntilReady( if readyCond == nil { // Condition not set yet, keep waiting allChecked = false + stillWaiting = append(stillWaiting, res) continue } switch readyCond.Status { case metav1.ConditionTrue: - // This reservation is ready // TODO use more than readyCondition - allChecked = false - reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) - break check // break because iterating list was modified case metav1.ConditionFalse: allChecked = false failedReservations = append(failedReservations, res) - reservationsToWatch = append(reservationsToWatch[:i], reservationsToWatch[i+1:]...) - break check // break because iterating list was modified case metav1.ConditionUnknown: allChecked = false + stillWaiting = append(stillWaiting, res) } } @@ -373,6 +369,7 @@ func watchReservationsUntilReady( return failedReservations, errors } + reservationsToWatch = stillWaiting // Log progress log.Info("waiting for reservations to become ready", "notReady", len(reservationsToWatch), diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index b77678294..61b855682 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -178,6 +178,25 @@ func TestCommitmentChangeIntegration(t *testing.T) { }, ExpectedAPIResponse: newAPIResponse(), }, + { + Name: "New commitment creation - large batch", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-new", "confirmed", 200), + ), + ExpectedReservations: func() []*TestReservation { + var reservations []*TestReservation + for range 200 { + reservations = append(reservations, &TestReservation{ + CommitmentID: "uuid-new", + Flavor: m1Small, + ProjectID: "project-A", + }) + } + return reservations + }(), + ExpectedAPIResponse: newAPIResponse(), + }, { Name: "With reservations of custom size - total unchanged", // Preserves custom-sized reservations when total matches (2×2GB = 4GB) @@ -744,6 +763,9 @@ func (c *FakeReservationController) OnReservationDeleted(res *v1alpha1.Reservati memoryMB, res.Status.Host, c.env.availableResources[res.Status.Host], res.Name) } } + + // Clear tracking so recreated reservations with same name are processed + delete(c.env.processedReserv, res.Name) } // operationInterceptorClient routes reservation events to FakeReservationController. From d72414d072742cd66bc37994393fb480348eff2c Mon Sep 17 00:00:00 2001 From: mblos Date: Tue, 17 Mar 2026 17:16:09 +0100 Subject: [PATCH 5/8] fix timeout issue --- .../reservations/commitments/api.go | 6 ++++ .../commitments/api_change_commitments.go | 14 ++++------ .../api_change_commitments_test.go | 28 +++++++++++++++++-- .../reservations/commitments/config.go | 22 +++++++++++++++ 4 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 internal/scheduling/reservations/commitments/config.go diff --git a/internal/scheduling/reservations/commitments/api.go b/internal/scheduling/reservations/commitments/api.go index ba83e2ab8..fe9de856b 100644 --- a/internal/scheduling/reservations/commitments/api.go +++ b/internal/scheduling/reservations/commitments/api.go @@ -14,13 +14,19 @@ import ( // HTTPAPI implements Limes LIQUID commitment validation endpoints. type HTTPAPI struct { client client.Client + config Config // Mutex to serialize change-commitments requests changeMutex sync.Mutex } func NewAPI(client client.Client) *HTTPAPI { + return NewAPIWithConfig(client, DefaultConfig()) +} + +func NewAPIWithConfig(client client.Client, config Config) *HTTPAPI { return &HTTPAPI{ client: client, + config: config, } } diff --git a/internal/scheduling/reservations/commitments/api_change_commitments.go b/internal/scheduling/reservations/commitments/api_change_commitments.go index 6da316bd8..e967943fe 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments.go @@ -37,14 +37,6 @@ func sortedKeys[K ~string, V any](m map[K]V) []K { return keys } -const ( - // watchTimeout is how long to wait for all reservations to become ready - watchTimeout = 2 * time.Second - - // pollInterval is how frequently to poll reservation status - pollInterval = 100 * time.Millisecond -) - // implements POST /v1/change-commitments from Limes LIQUID API: // See: https://github.com/sapcc/go-api-declarations/blob/main/liquid/commitment.go // See: https://pkg.go.dev/github.com/sapcc/go-api-declarations/liquid @@ -246,7 +238,7 @@ ProcessLoop: time_start := time.Now() - if failedReservations, errors := watchReservationsUntilReady(ctx, log, api.client, reservationsToWatch, watchTimeout); len(failedReservations) > 0 || len(errors) > 0 { + if failedReservations, errors := watchReservationsUntilReady(ctx, log, api.client, reservationsToWatch, api.config.ChangeAPIWatchReservationsTimeout, api.config.ChangeAPIWatchReservationsPollInterval); len(failedReservations) > 0 || len(errors) > 0 { log.Info("reservations failed to become ready, initiating rollback", "failedReservations", len(failedReservations), "errors", errors) @@ -254,6 +246,9 @@ ProcessLoop: for _, res := range failedReservations { failedCommitments[res.Spec.CommittedResourceReservation.CommitmentUUID] = "not sufficient capacity" } + if len(failedReservations) == 0 { + resp.RejectionReason += "timeout reached while processing commitment changes" + } requireRollback = true } @@ -297,6 +292,7 @@ func watchReservationsUntilReady( k8sClient client.Client, reservations []v1alpha1.Reservation, timeout time.Duration, + pollInterval time.Duration, ) (failedReservations []v1alpha1.Reservation, errors []error) { if len(reservations) == 0 { diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index 61b855682..d67b229b8 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -416,6 +416,20 @@ func TestCommitmentChangeIntegration(t *testing.T) { ExpectedReservations: []*TestReservation{}, ExpectedAPIResponse: newAPIResponse("2 commitment(s) failed", "commitment uuid-multi-fail-1: not sufficient capacity", "commitment uuid-multi-fail-2: not sufficient capacity"), }, + { + Name: "Watch timeout with custom config - triggers rollback with timeout error", + Flavors: []*TestFlavor{m1Small}, + CommitmentRequest: newCommitmentRequest("az-a", false, 1234, + createCommitment("ram_hana_1", "project-A", "uuid-timeout", "confirmed", 2), + ), + // With 0ms timeout, the watch will timeout immediately before reservations become ready + CustomConfig: &Config{ + ChangeAPIWatchReservationsTimeout: 0 * time.Millisecond, + ChangeAPIWatchReservationsPollInterval: 100 * time.Millisecond, + }, + ExpectedReservations: []*TestReservation{}, // Rollback removes all reservations + ExpectedAPIResponse: newAPIResponse("timeout reached while processing commitment changes"), + }, } for _, tc := range testCases { @@ -460,8 +474,8 @@ func runCommitmentChangeTest(t *testing.T, tc CommitmentChangeTestCase) { existingReservations = append(existingReservations, testRes.toReservation(number)) } - // Create test environment with available resources - env := newCommitmentTestEnv(t, vms, nil, existingReservations, flavorGroups, tc.AvailableResources) + // Create test environment with available resources and custom config if provided + env := newCommitmentTestEnv(t, vms, nil, existingReservations, flavorGroups, tc.AvailableResources, tc.CustomConfig) defer env.Close() t.Log("Initial state:") @@ -509,6 +523,7 @@ type CommitmentChangeTestCase struct { ExpectedAPIResponse APIResponseExpectation AvailableResources *AvailableResources // If nil, all reservations accepted without checks EnvInfoVersion int64 // Override InfoVersion for version mismatch tests + CustomConfig *Config // Override default config for testing timeout behavior } // AvailableResources defines available memory per host (MB). @@ -808,6 +823,7 @@ func newCommitmentTestEnv( reservations []*v1alpha1.Reservation, flavorGroups FlavorGroupsKnowledge, resources *AvailableResources, + customConfig *Config, ) *CommitmentTestEnv { t.Helper() @@ -874,7 +890,13 @@ func newCommitmentTestEnv( } env.K8sClient = wrappedClient - api := NewAPI(wrappedClient) + // Use custom config if provided, otherwise use default + var api *HTTPAPI + if customConfig != nil { + api = NewAPIWithConfig(wrappedClient, *customConfig) + } else { + api = NewAPI(wrappedClient) + } mux := http.NewServeMux() api.Init(mux) httpServer := httptest.NewServer(mux) diff --git a/internal/scheduling/reservations/commitments/config.go b/internal/scheduling/reservations/commitments/config.go new file mode 100644 index 000000000..95dc904d8 --- /dev/null +++ b/internal/scheduling/reservations/commitments/config.go @@ -0,0 +1,22 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package commitments + +import "time" + +// Config defines the configuration for the commitments HTTP API. +type Config struct { + // how long to wait for reservations to become ready before timing out and rolling back. + ChangeAPIWatchReservationsTimeout time.Duration `json:"changeAPIWatchReservationsTimeout"` + + // how frequently to poll reservation status during watch. + ChangeAPIWatchReservationsPollInterval time.Duration `json:"changeAPIWatchReservationsPollInterval"` +} + +func DefaultConfig() Config { + return Config{ + ChangeAPIWatchReservationsTimeout: 2 * time.Second, + ChangeAPIWatchReservationsPollInterval: 100 * time.Millisecond, + } +} From d51400108abc53e2768d896f258163909b726926 Mon Sep 17 00:00:00 2001 From: mblos Date: Wed, 18 Mar 2026 08:18:37 +0100 Subject: [PATCH 6/8] fix test case --- .../commitments/api_change_commitments.go | 14 +++----- .../api_change_commitments_test.go | 34 ++++++++++++++++--- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/internal/scheduling/reservations/commitments/api_change_commitments.go b/internal/scheduling/reservations/commitments/api_change_commitments.go index e967943fe..1c6276ade 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments.go @@ -18,7 +18,6 @@ import ( "github.com/go-logr/logr" . "github.com/majewsky/gg/option" "github.com/sapcc/go-api-declarations/liquid" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -323,14 +322,9 @@ func watchReservationsUntilReady( if err := k8sClient.Get(ctx, nn, ¤t); err != nil { allChecked = false - if apierrors.IsNotFound(err) { - // Reservation is still in process of being created, continue waiting for it - stillWaiting = append(stillWaiting, res) - continue - } - // remove reservation from waiting - failedReservations = append(failedReservations, res) - errors = append(errors, fmt.Errorf("failed to get reservation %s: %w", res.Name, err)) + // Reservation is still in process of being created, or there is a transient error, continue waiting for it + log.V(1).Info("transient error getting reservation, will retry", "reservation", res.Name, "error", err) + stillWaiting = append(stillWaiting, res) continue } @@ -359,7 +353,7 @@ func watchReservationsUntilReady( } } - if allChecked { + if allChecked || len(stillWaiting) == 0 { log.Info("all reservations checked", "failed", len(failedReservations)) return failedReservations, errors diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index d67b229b8..6cf49d540 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -16,6 +16,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" @@ -755,6 +756,7 @@ type CommitmentTestEnv struct { API *HTTPAPI availableResources map[string]int64 // host -> available memory MB processedReserv map[string]bool // track processed reservations + mu sync.Mutex // protects availableResources and processedReserv } // FakeReservationController simulates synchronous reservation controller. @@ -767,6 +769,10 @@ func (c *FakeReservationController) OnReservationCreated(res *v1alpha1.Reservati } func (c *FakeReservationController) OnReservationDeleted(res *v1alpha1.Reservation) { + c.env.mu.Lock() + defer c.env.mu.Unlock() + + // Return memory when Delete() is called directly (before deletion timestamp is set) if c.env.availableResources != nil && res.Status.Host != "" { memoryQuantity := res.Spec.Resources["memory"] memoryBytes := memoryQuantity.Value() @@ -774,7 +780,7 @@ func (c *FakeReservationController) OnReservationDeleted(res *v1alpha1.Reservati if _, exists := c.env.availableResources[res.Status.Host]; exists { c.env.availableResources[res.Status.Host] += memoryMB - c.env.T.Logf("↩ Returned %d MB to %s (now %d MB available) before deleting reservation %s", + c.env.T.Logf("↩ Returned %d MB to %s (now %d MB available) via OnReservationDeleted for %s", memoryMB, res.Status.Host, c.env.availableResources[res.Status.Host], res.Name) } } @@ -1038,6 +1044,7 @@ func (env *CommitmentTestEnv) processReservations() { if !res.DeletionTimestamp.IsZero() { env.T.Logf("Processing deletion for reservation %s (host: %s)", res.Name, res.Status.Host) + env.mu.Lock() // Return memory to host if resource tracking is enabled if env.availableResources != nil { env.T.Logf("Resource tracking enabled, returning memory for %s", res.Name) @@ -1049,6 +1056,7 @@ func (env *CommitmentTestEnv) processReservations() { // Check if host exists in our tracking if _, exists := env.availableResources[res.Status.Host]; !exists { + env.mu.Unlock() env.T.Fatalf("Host %s not found in available resources for reservation %s - this indicates an inconsistency", res.Status.Host, res.Name) } @@ -1061,6 +1069,10 @@ func (env *CommitmentTestEnv) processReservations() { env.T.Logf("Resource tracking NOT enabled for %s", res.Name) } + // Clear tracking so recreated reservations with same name are processed + delete(env.processedReserv, res.Name) + env.mu.Unlock() + // Remove finalizers to allow deletion if len(res.Finalizers) > 0 { res.Finalizers = []string{} @@ -1077,8 +1089,12 @@ func (env *CommitmentTestEnv) processReservations() { continue } + env.mu.Lock() + alreadyProcessed := env.processedReserv[res.Name] + env.mu.Unlock() + // Skip if already tracked as processed - if env.processedReserv[res.Name] { + if alreadyProcessed { continue } @@ -1100,6 +1116,9 @@ func (env *CommitmentTestEnv) hasCondition(res *v1alpha1.Reservation) bool { // processNewReservation implements first-come-first-serve scheduling based on available resources. // It tries to find a host with enough memory capacity and assigns the reservation to that host. func (env *CommitmentTestEnv) processNewReservation(res *v1alpha1.Reservation) { + env.mu.Lock() + defer env.mu.Unlock() + env.processedReserv[res.Name] = true // If no available resources configured, accept all reservations without host assignment @@ -1114,9 +1133,16 @@ func (env *CommitmentTestEnv) processNewReservation(res *v1alpha1.Reservation) { memoryMB := memoryBytes / (1024 * 1024) // First-come-first-serve: find first host with enough capacity + // Sort hosts to ensure deterministic behavior (Go map iteration is random) + hosts := make([]string, 0, len(env.availableResources)) + for host := range env.availableResources { + hosts = append(hosts, host) + } + sort.Strings(hosts) + var selectedHost string - for host, availableMB := range env.availableResources { - if availableMB >= memoryMB { + for _, host := range hosts { + if env.availableResources[host] >= memoryMB { selectedHost = host break } From b8175ab1d678d3d94d28806090cdb42e7f28e627 Mon Sep 17 00:00:00 2001 From: mblos Date: Wed, 18 Mar 2026 08:22:06 +0100 Subject: [PATCH 7/8] change http endpoint --- internal/scheduling/reservations/commitments/api.go | 4 ++-- .../reservations/commitments/api_change_commitments_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/scheduling/reservations/commitments/api.go b/internal/scheduling/reservations/commitments/api.go index fe9de856b..9d8fd5944 100644 --- a/internal/scheduling/reservations/commitments/api.go +++ b/internal/scheduling/reservations/commitments/api.go @@ -31,9 +31,9 @@ func NewAPIWithConfig(client client.Client, config Config) *HTTPAPI { } func (api *HTTPAPI) Init(mux *http.ServeMux) { - mux.HandleFunc("/v1/change-commitments", api.HandleChangeCommitments) + mux.HandleFunc("/v1/commitments/change-commitments", api.HandleChangeCommitments) // mux.HandleFunc("/v1/report-capacity", api.HandleReportCapacity) - mux.HandleFunc("/v1/info", api.HandleInfo) + mux.HandleFunc("/v1/commitments/info", api.HandleInfo) } var commitmentApiLog = ctrl.Log.WithName("commitment_api") diff --git a/internal/scheduling/reservations/commitments/api_change_commitments_test.go b/internal/scheduling/reservations/commitments/api_change_commitments_test.go index 6cf49d540..871e72b54 100644 --- a/internal/scheduling/reservations/commitments/api_change_commitments_test.go +++ b/internal/scheduling/reservations/commitments/api_change_commitments_test.go @@ -992,7 +992,7 @@ func (env *CommitmentTestEnv) CallChangeCommitmentsAPI(reqJSON string) (resp liq }() // Make HTTP request - url := env.HTTPServer.URL + "/v1/change-commitments" + url := env.HTTPServer.URL + "/v1/commitments/change-commitments" httpResp, err := http.Post(url, "application/json", bytes.NewReader([]byte(reqJSON))) //nolint:gosec,noctx // test server URL, not user input if err != nil { cancel() From cb997851348510f8eed2b883eb78820d6c4774bf Mon Sep 17 00:00:00 2001 From: mblos Date: Wed, 18 Mar 2026 08:27:12 +0100 Subject: [PATCH 8/8] fix hypervisor crd --- Tiltfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tiltfile b/Tiltfile index 2de697c32..a42fe43f4 100644 --- a/Tiltfile +++ b/Tiltfile @@ -75,7 +75,7 @@ local('kubectl wait --namespace cert-manager --for=condition=available deploymen ########### Dependency CRDs # Make sure the local cluster is running if you are running into startup issues here. -url = 'https://raw.githubusercontent.com/cobaltcore-dev/openstack-hypervisor-operator/refs/heads/main/charts/openstack-hypervisor-operator/crds/hypervisor-crd.yaml' +url = 'https://raw.githubusercontent.com/cobaltcore-dev/openstack-hypervisor-operator/refs/heads/main/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml' local('curl -L ' + url + ' | kubectl apply -f -') ########### Cortex Operator & CRDs