From 1fc574dc6e0400134bc19e602654d3b0fd96ab4e Mon Sep 17 00:00:00 2001
From: mblos <marcel.gute@sap.com>
Date: Fri, 8 May 2026 15:08:45 +0200
Subject: [PATCH] fix: cr crd update on concurrency

---
 .../commitments/api/change_commitments.go     | 36 ++++++++++++-------
 .../api/change_commitments_test.go            | 19 +++-------
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/internal/scheduling/reservations/commitments/api/change_commitments.go b/internal/scheduling/reservations/commitments/api/change_commitments.go
index baee3086c..5d2dcf318 100644
--- a/internal/scheduling/reservations/commitments/api/change_commitments.go
+++ b/internal/scheduling/reservations/commitments/api/change_commitments.go
@@ -118,6 +118,13 @@ func (api *HTTPAPI) HandleChangeCommitments(w http.ResponseWriter, r *http.Reque
 		return
 	}
 
+	if req.AZ == "" {
+		statusCode = http.StatusBadRequest
+		http.Error(w, "availability zone is required", statusCode)
+		api.recordMetrics(req, resp, statusCode, startTime)
+		return
+	}
+
 	if err := api.processCommitmentChanges(ctx, w, logger, req, &resp); err != nil {
 		if strings.Contains(err.Error(), "version mismatch") {
 			statusCode = http.StatusConflict
@@ -255,18 +262,23 @@ ProcessLoop:
 					break ProcessLoop
 				}
 
-				cr := &v1alpha1.CommittedResource{}
-				cr.Name = crName
-				if _, err := controllerutil.CreateOrUpdate(ctx, api.client, cr, func() error {
-					if cr.Spec.AvailabilityZone != "" && cr.Spec.AvailabilityZone != stateDesired.AvailabilityZone {
-						return fmt.Errorf("cannot change availability zone of commitment %s: current=%q requested=%q",
-							commitment.UUID, cr.Spec.AvailabilityZone, stateDesired.AvailabilityZone)
-					}
-					applyCRSpec(cr, stateDesired, allowRejection)
-					if cr.Annotations == nil {
-						cr.Annotations = make(map[string]string)
+				// RetryOnConflict handles the race where the CommittedResource controller reconciles
+				// the CRD (bumping resourceVersion) between the Get and Update inside CreateOrUpdate.
+				var crGeneration int64
+				if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+					cr := &v1alpha1.CommittedResource{}
+					cr.Name = crName
+					if _, err := controllerutil.CreateOrUpdate(ctx, api.client, cr, func() error {
+						applyCRSpec(cr, stateDesired, allowRejection)
+						if cr.Annotations == nil {
+							cr.Annotations = make(map[string]string)
+						}
+						cr.Annotations[v1alpha1.AnnotationCreatorRequestID] = reservations.GlobalRequestIDFromContext(ctx)
+						return nil
+					}); err != nil {
+						return err
 					}
-					cr.Annotations[v1alpha1.AnnotationCreatorRequestID] = reservations.GlobalRequestIDFromContext(ctx)
+					crGeneration = cr.Generation
 					return nil
 				}); err != nil {
 					failedReason = fmt.Sprintf("commitment %s: failed to write CommittedResource CRD: %v", commitment.UUID, err)
@@ -274,7 +286,7 @@ ProcessLoop:
 					break ProcessLoop
 				}
 
-				toWatch = append(toWatch, crWatch{name: crName, generation: cr.Generation})
+				toWatch = append(toWatch, crWatch{name: crName, generation: crGeneration})
 				snapshots = append(snapshots, snap)
 				logger.V(1).Info("upserted CommittedResource CRD", "name", crName)
 			}
diff --git a/internal/scheduling/reservations/commitments/api/change_commitments_test.go b/internal/scheduling/reservations/commitments/api/change_commitments_test.go
index e150a7fa5..c843751ec 100644
--- a/internal/scheduling/reservations/commitments/api/change_commitments_test.go
+++ b/internal/scheduling/reservations/commitments/api/change_commitments_test.go
@@ -130,22 +130,13 @@ func TestHandleChangeCommitments(t *testing.T) {
 			ExpectedAPIResponse: newAPIResponse("uuid-b: not sufficient capacity"),
 			ExpectedDeletedCRs:  []string{"commitment-uuid-a", "commitment-uuid-b"},
 		},
-		// --- AZ immutability ---
+		// --- AZ validation ---
 		{
-			// AZ is immutable once set on a CommittedResource. Attempting to change it via
-			// change-commitments must be rejected immediately, before any polling or controller
-			// interaction, and the CR must remain at its original spec.
-			Name:    "AZ change on existing CR: must be rejected",
+			Name:    "Empty AZ: rejected with 400",
 			Flavors: []*TestFlavor{m1Small},
-			ExistingCRs: []*TestCR{
-				{CommitmentUUID: "uuid-az-stale", State: v1alpha1.CommitmentStatusConfirmed,
-					AmountMiB: 1024, ProjectID: "project-A", AZ: "az-old", ReadyCondition: true},
-			},
-			CommitmentRequest: newCommitmentRequest("az-new", false, 1234,
-				createCommitment("hw_version_hana_1_ram", "project-A", "uuid-az-stale", "confirmed", 2)),
-			ExpectedAPIResponse: newAPIResponse("cannot change availability zone"),
-			// CR spec must not have changed.
-			ExpectedCRSpecs: map[string]int64{"commitment-uuid-az-stale": 1024 * 1024 * 1024},
+			CommitmentRequest: newCommitmentRequest("", false, 1234,
+				createCommitment("hw_version_hana_1_ram", "project-A", "uuid-noaz", "confirmed", 2)),
+			ExpectedAPIResponse: APIResponseExpectation{StatusCode: http.StatusBadRequest},
 		},
 		// --- Timeout ---
 		{