Vtop: Refactor Conditions to List for Kubectl Wait (#136)

* Refactored conditions to be a list, and have methods for map like access.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* Oops.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* Update shard conditions per new list design for Wait support.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* List size needs to be 0 to use append.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* Refactored so public GetCondition returns deep copy per review suggestion.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* Remove left over fragment I missed.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

* Reverted changes to VitessShardStatus and related methods per review feedback.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

deploy/crds/planetscale.com_vitesskeyspaces.yaml

@@ -869,7 +869,7 @@ spec:
         status:
           properties:
             conditions:
-              additionalProperties:
+              items:
                 properties:
                   lastTransitionTime:
                     format: date-time
@@ -884,10 +884,13 @@ spec:
                     - "False"
                     - Unknown
                     type: string
+                  type:
+                    type: string
                 required:
                 - status
+                - type
                 type: object
-              type: object
+              type: array
             idle:
               type: string
             observedGeneration:

docs/api/index.html

@@ -4541,6 +4541,19 @@ <h3 id="planetscale.com/v2.VitessKeyspaceCondition">VitessKeyspaceCondition
 <tbody>
 <tr>
 <td>
+<code>type</code></br>
+<em>
+<a href="#planetscale.com/v2.VitessKeyspaceConditionType">
+VitessKeyspaceConditionType
+</a>
+</em>
+</td>
+<td>
+<p>Type is the type of the condition.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>status</code></br>
 <em>
 <a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.13/#conditionstatus-v1-core">
@@ -4596,6 +4609,10 @@ <h3 id="planetscale.com/v2.VitessKeyspaceCondition">VitessKeyspaceCondition
 <h3 id="planetscale.com/v2.VitessKeyspaceConditionType">VitessKeyspaceConditionType
 (<code>string</code> alias)</p></h3>
 <p>
+(<em>Appears on:</em>
+<a href="#planetscale.com/v2.VitessKeyspaceCondition">VitessKeyspaceCondition</a>)
+</p>
+<p>
 <p>VitessKeyspaceConditionType is a valid value for the key of a VitessKeyspaceCondition map where the key is a
 VitessKeyspaceConditionType and the value is a VitessKeyspaceCondition.</p>
 </p>
@@ -5294,13 +5311,12 @@ <h3 id="planetscale.com/v2.VitessKeyspaceStatus">VitessKeyspaceStatus
 <code>conditions</code></br>
 <em>
 <a href="#planetscale.com/v2.VitessKeyspaceCondition">
-map[planetscale.dev/vitess-operator/pkg/apis/planetscale/v2.VitessKeyspaceConditionType]planetscale.dev/vitess-operator/pkg/apis/planetscale/v2.VitessKeyspaceCondition
+[]VitessKeyspaceCondition
 </a>
 </em>
 </td>
 <td>
-<p>FIXME: Convert this to a list for kubectl wait compatibility before merging the feature branch to master.
-Conditions is a map of all VitessKeyspace specific conditions we want to set and monitor.
+<p>Conditions is a list of all VitessKeyspace specific conditions we want to set and monitor.
 It&rsquo;s ok for multiple controllers to add conditions here, and those conditions will be preserved.</p>
 </td>
 </tr>

pkg/apis/planetscale/v2/vitesskeyspace_methods.go

@@ -167,11 +167,7 @@ func (p *VitessKeyspacePartitioning) TabletPools() []VitessShardTabletPool {
 // newStatus, then we do not update LastTransitionTime. However, if newStatus is different from current status, then
 // we update the status and update the transition time.
 func (s *VitessKeyspaceStatus) SetConditionStatus(condType VitessKeyspaceConditionType, newStatus corev1.ConditionStatus, reason, message string) {
-	if s.Conditions == nil {
-		s.Conditions = make(map[VitessKeyspaceConditionType]VitessKeyspaceCondition)
-	}
-
-	cond, ok := s.Conditions[condType]
+	cond, ok := s.getCondition(condType)
 	if !ok {
 		cond = NewVitessKeyspaceCondition()
 	}
@@ -186,13 +182,13 @@ func (s *VitessKeyspaceStatus) SetConditionStatus(condType VitessKeyspaceConditi
 		cond.LastTransitionTime = &now
 	}
 
-	s.Conditions[condType] = cond
+	s.setCondition(cond)
 }
 
 // NewVitessKeyspaceCondition returns an init VitessKeyspaceCondition object.
-func NewVitessKeyspaceCondition() VitessKeyspaceCondition {
+func NewVitessKeyspaceCondition() *VitessKeyspaceCondition {
 	now := metav1.NewTime(time.Now())
-	return VitessKeyspaceCondition{
+	return &VitessKeyspaceCondition{
 		Status:             corev1.ConditionUnknown,
 		LastTransitionTime: &now,
 		Reason:             "",
@@ -212,15 +208,50 @@ func (c *VitessKeyspaceCondition) StatusDuration() time.Duration {
 }
 
 // DeepCopyConditions deep copies the conditions map for VitessKeyspaceStatus.
-func (s *VitessKeyspaceStatus) DeepCopyConditions() map[VitessKeyspaceConditionType]VitessKeyspaceCondition {
-	if s == nil {
-		return nil
-	}
-	out := make(map[VitessKeyspaceConditionType]VitessKeyspaceCondition, len(s.Conditions))
+func (s *VitessKeyspaceStatus) DeepCopyConditions() []VitessKeyspaceCondition {
+	out := make([]VitessKeyspaceCondition, 0, len(s.Conditions))
 
-	for key, val := range s.Conditions {
-		out[key] = *val.DeepCopy()
+	for _, condition := range s.Conditions {
+		out = append(out, *condition.DeepCopy())
 	}
 
 	return out
 }
+
+// GetCondition provides map style access to retrieve a condition from the conditions list by it's type
+// If the condition doesn't exist, we return false for the exists named return value.
+func (s *VitessKeyspaceStatus) GetCondition(ty VitessKeyspaceConditionType) (value VitessKeyspaceCondition, exists bool) {
+	cond, exists := s.getCondition(ty)
+	if !exists {
+		return VitessKeyspaceCondition{}, false
+	}
+	return *cond.DeepCopy(), true
+}
+
+// getCondition is used internally for map style access, and returns a pointer to reduce unnecessary copying.
+func (s *VitessKeyspaceStatus) getCondition(ty VitessKeyspaceConditionType) (value *VitessKeyspaceCondition, exists bool) {
+	for i := range s.Conditions {
+		condition := &s.Conditions[i]
+		if condition.Type == ty {
+			return condition, true
+		}
+	}
+	return nil, false
+}
+
+// setCondition is used internally to provide map style setting of conditions, and will ensure uniqueness by using
+
+// setCondition is used internally to provide map style setting of conditions, and will ensure uniqueness by using
+// upsert semantics.
+func (s *VitessKeyspaceStatus) setCondition(newCondition *VitessKeyspaceCondition) {
+	for i := range s.Conditions {
+		condition := &s.Conditions[i]
+		if condition.Type == newCondition.Type {
+			s.Conditions[i] = *newCondition
+			return
+		}
+	}
+
+	// We got here so we didn't return early by finding the condition already existing. We'll just append to the end.
+	s.Conditions = append(s.Conditions, *newCondition)
+}

pkg/apis/planetscale/v2/vitesskeyspace_types.go

@@ -300,10 +300,9 @@ type VitessKeyspaceStatus struct {
 	// This field is only present if the ReshardingActive condition is True. If that condition is Unknown,
 	// it means the operator was unable to query resharding status from Vitess.
 	Resharding *ReshardingStatus `json:"resharding,omitempty"`
-	// FIXME: Convert this to a list for kubectl wait compatibility before merging the feature branch to master.
-	// Conditions is a map of all VitessKeyspace specific conditions we want to set and monitor.
+	// Conditions is a list of all VitessKeyspace specific conditions we want to set and monitor.
 	// It's ok for multiple controllers to add conditions here, and those conditions will be preserved.
-	Conditions map[VitessKeyspaceConditionType]VitessKeyspaceCondition `json:"conditions,omitempty"`
+	Conditions []VitessKeyspaceCondition `json:"conditions,omitempty"`
 }
 
 // ReshardingStatus defines some of the workflow related status information.
@@ -409,6 +408,8 @@ func NewVitessKeyspacePartitioningStatus(partitioning *VitessKeyspacePartitionin
 
 // VitessKeyspaceCondition contains details for the current condition of this VitessKeyspace.
 type VitessKeyspaceCondition struct {
+	// Type is the type of the condition.
+	Type VitessKeyspaceConditionType `json:"type"`
 	// Status is the status of the condition.
 	// Can be True, False, Unknown.
 	// +kubebuilder:validation:Enum=True;False;Unknown

pkg/controller/vitesskeyspace/vitesskeyspace_controller.go

@@ -210,11 +210,6 @@ func (r *ReconcileVitessKeyspace) NewReconcileHandler(ctx context.Context, reque
 	vtk.Status = planetscalev2.NewVitessKeyspaceStatus()
 	vtk.Status.Conditions = oldStatus.DeepCopyConditions()
 
-	// Add empty condition map if condition map isn't there already for safety.
-	if vtk.Status.Conditions == nil {
-		vtk.Status.Conditions = make(map[planetscalev2.VitessKeyspaceConditionType]planetscalev2.VitessKeyspaceCondition)
-	}
-
 	untouchedConditions := make(map[planetscalev2.VitessKeyspaceConditionType]bool, len(keyspaceConditions))
 	for condition := range keyspaceConditions {
 		untouchedConditions[condition] = true