Improve upgradeOrder behavior more intuitive and safer (#1547)

* Improve the roleset upgradeOrder behavior
* Add roleset upgrade integration test
* Sync crds changes
* Address code review feedback
---------

Signed-off-by: Jiaxin Shan <seedjeffwan@gmail.com>

Author: Jeffwan

api/orchestration/v1alpha1/roleset_types.go

@@ -67,11 +67,10 @@ type RoleSpec struct {
 	Replicas *int32 `json:"replicas,omitempty"`
 
 	// UpgradeOrder specifies the order in which this role should be upgraded.
-	// Lower values are upgraded first. If not specified, defaults to 0.
+	// Lower values are upgraded first. If not specified, roles upgrade after all explicitly ordered roles.
 	// +optional
-	// +kubebuilder:validation:Minimum=0
+	// +kubebuilder:validation:Minimum=1
 	// +kubebuilder:validation:Type=integer
-	// +kubebuilder:default:=0
 	UpgradeOrder *int32 `json:"upgradeOrder,omitempty"`
 
 	// PodGroupSize is the number of pods to form a minimum role instance.

config/crd/orchestration/orchestration.aibrix.ai_rolesets.yaml

@@ -3690,9 +3690,8 @@ spec:
                           x-kubernetes-int-or-string: true
                       type: object
                     upgradeOrder:
-                      default: 0
                       format: int32
-                      minimum: 0
+                      minimum: 1
                       type: integer
                   type: object
                 type: array

config/crd/orchestration/orchestration.aibrix.ai_stormservices.yaml

@@ -3757,9 +3757,8 @@ spec:
                                   x-kubernetes-int-or-string: true
                               type: object
                             upgradeOrder:
-                              default: 0
                               format: int32
-                              minimum: 0
+                              minimum: 1
                               type: integer
                           type: object
                         type: array

pkg/controller/roleset/rolling.go

@@ -58,11 +58,13 @@ func (m *RollingManagerSequential) Next(ctx context.Context, roleSet *orchestrat
 	sortedRoles := sortRolesByUpgradeOrder(roleSet.Spec.Roles)
 	var sequenceLines []string
 	for i, role := range sortedRoles {
-		order := int32(0)
+		var orderStr string
 		if role.UpgradeOrder != nil {
-			order = *role.UpgradeOrder
+			orderStr = fmt.Sprintf("%d", *role.UpgradeOrder)
+		} else {
+			orderStr = "nil"
 		}
-		sequenceLines = append(sequenceLines, fmt.Sprintf("[%d] %s (Order=%d)", i+1, role.Name, order))
+		sequenceLines = append(sequenceLines, fmt.Sprintf("[%d] %s (Order=%s)", i+1, role.Name, orderStr))
 	}
 
 	klog.Infof("[RollingManagerSequential.Next] Upgrade sequence for %s/%s:\n%s",

pkg/controller/roleset/utils.go

@@ -396,15 +396,19 @@ func sortRolesByUpgradeOrder(roles []orchestrationv1alpha1.RoleSpec) []orchestra
 	sortedRoles := make([]orchestrationv1alpha1.RoleSpec, len(roles))
 	copy(sortedRoles, roles)
 	sort.SliceStable(sortedRoles, func(i, j int) bool {
-		orderI := int32(0)
-		if sortedRoles[i].UpgradeOrder != nil {
-			orderI = *sortedRoles[i].UpgradeOrder
+		iOrder := sortedRoles[i].UpgradeOrder
+		jOrder := sortedRoles[j].UpgradeOrder
+		if iOrder == nil {
+			// i is nil. If j is also nil, stable sort. If j is not nil, i comes after.
+			// In both cases, i is not "less than" j.
+			return false
 		}
-		orderJ := int32(0)
-		if sortedRoles[j].UpgradeOrder != nil {
-			orderJ = *sortedRoles[j].UpgradeOrder
+		if jOrder == nil {
+			// i is not nil, but j is. i comes before.
+			return true
 		}
-		return orderI < orderJ
+		// Both have explicit orders, sort by value.
+		return *iOrder < *jOrder
 	})
 	return sortedRoles
 }

pkg/controller/roleset/utils_test.go

@@ -341,9 +341,9 @@ func TestSortRolesByUpgradeOrder(t *testing.T) {
 				{Name: "role2", UpgradeOrder: int32Ptr(1)},
 			},
 			expected: []orchestrationv1alpha1.RoleSpec{
-				{Name: "role1", UpgradeOrder: nil},
 				{Name: "role2", UpgradeOrder: int32Ptr(1)},
 				{Name: "role3", UpgradeOrder: int32Ptr(2)},
+				{Name: "role1", UpgradeOrder: nil},
 			},
 		},
 		{
@@ -368,10 +368,25 @@ func TestSortRolesByUpgradeOrder(t *testing.T) {
 				{Name: "role3", UpgradeOrder: int32Ptr(1)},
 			},
 			expected: []orchestrationv1alpha1.RoleSpec{
-				{Name: "role1", UpgradeOrder: nil},
-				{Name: "role2", UpgradeOrder: nil},
 				{Name: "role3", UpgradeOrder: int32Ptr(1)},
 				{Name: "role4", UpgradeOrder: int32Ptr(2)},
+				{Name: "role1", UpgradeOrder: nil},
+				{Name: "role2", UpgradeOrder: nil},
+			},
+		},
+		{
+			name: "multiple roles with explicit order and one missing (real-world scenario)",
+			roles: []orchestrationv1alpha1.RoleSpec{
+				{Name: "api-server", UpgradeOrder: int32Ptr(2)},
+				{Name: "database", UpgradeOrder: nil}, // Missing - should upgrade LAST
+				{Name: "cache", UpgradeOrder: int32Ptr(1)},
+				{Name: "monitoring", UpgradeOrder: int32Ptr(3)},
+			},
+			expected: []orchestrationv1alpha1.RoleSpec{
+				{Name: "cache", UpgradeOrder: int32Ptr(1)},      // First
+				{Name: "api-server", UpgradeOrder: int32Ptr(2)}, // Second
+				{Name: "monitoring", UpgradeOrder: int32Ptr(3)}, // Third
+				{Name: "database", UpgradeOrder: nil},           // Last (safest)
 			},
 		},
 	}