make status.condition patchable (kubernetes-sigs#319)

* make `status.condition` patchable

* Proposed tweaks to pr 319

* test: shorten the reconcile duration

* Reintroduce abnormal conditions

* cleanup; klog error if `statusConditions` is not used.

---------

Co-authored-by: Justin SB <justinsb@google.com>

Author: yuwenma

pkg/patterns/addon/pkg/apis/v1alpha1/common_types.go

@@ -42,17 +42,11 @@ type CommonSpec struct {
 	Channel string `json:"channel,omitempty"`
 }
 
-//go:generate go run ../../../../../../vendor/k8s.io/code-generator/cmd/deepcopy-gen/main.go -O zz_generated.deepcopy -i ./... -h ../../../../../../hack/boilerplate.go.txt
-// +k8s:deepcopy-gen=true
-
 // CommonStatus is a set of status attributes that must be exposed on all addons.
 type CommonStatus struct {
 	Healthy bool     `json:"healthy"`
 	Errors  []string `json:"errors,omitempty"`
 	Phase   string   `json:"phase,omitempty"`
-	// Conditions follows the API specification "Conditions" properties.
-	// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties
-	Conditions []metav1.Condition `json:"conditions,omitempty"`
 }
 
 // Patchable is a trait for addon CRDs that expose a raw set of Patches to be

pkg/patterns/addon/pkg/apis/v1alpha1/conditions.go

@@ -0,0 +1,28 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// StatusConditions defines the fields to support status.conditions.
+type StatusConditions struct {
+	// Conditions follows the API specification "Conditions" properties.
+	// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
+}

pkg/patterns/addon/pkg/apis/v1alpha1/zz_generated.deepcopy.go

@@ -0,0 +1,55 @@
+package status
+
+import (
+	"fmt"
+	"reflect"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog/v2"
+)
+
+// GetConditions pulls out the `status.conditions` field from runtime.Object
+func GetConditions(instance runtime.Object) ([]metav1.Condition, error) {
+	statusVal := reflect.ValueOf(instance).Elem().FieldByName("Status")
+	if !statusVal.IsValid() {
+		return nil, fmt.Errorf("status field not found")
+	}
+	conditionsVal := statusVal.FieldByName("Conditions")
+	if !conditionsVal.IsValid() {
+		klog.Errorf("unable to find `status.condition` in %T", instance)
+		return nil, nil
+	}
+
+	v := conditionsVal.Interface()
+	conditions, ok := v.([]metav1.Condition)
+	if !ok {
+		return nil, fmt.Errorf("unexpecetd type for status.conditions; got %T, want []metav1.Condition", v)
+	}
+	return conditions, nil
+}
+
+// SetConditions sets the newConditions to runtime.Object `status.conditions` field.
+func SetConditions(instance runtime.Object, newConditions []metav1.Condition) error {
+	statusVal := reflect.ValueOf(instance).Elem().FieldByName("Status")
+	if !statusVal.IsValid() {
+		// Status not ready.
+		return fmt.Errorf("status field not found")
+	}
+	conditionsVal := statusVal.FieldByName("Conditions")
+	if !conditionsVal.IsValid() {
+		klog.Errorf("unable to find `status.condition` in %T", instance)
+		return nil
+	}
+
+	newConditionsVal := reflect.ValueOf(newConditions)
+	if !conditionsVal.CanSet() {
+		return fmt.Errorf("cannot set status.conditions field")
+	}
+	if !newConditionsVal.CanConvert(conditionsVal.Type()) {
+		return fmt.Errorf("cannot set type %v to status.conditions type %v", newConditionsVal.Type(), conditionsVal.Type())
+	}
+	conditionsVal.Set(newConditionsVal)
+
+	return nil
+}

pkg/patterns/addon/pkg/status/accessors.go

@@ -3,10 +3,8 @@ package status
 import (
 	"strings"
 
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/cli-utils/pkg/kstatus/status"
-	addonsv1alpha1 "sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/addon/pkg/apis/v1alpha1"
 )
 
 const (
@@ -15,27 +13,7 @@ const (
 	ReadyType      = "Ready"
 )
 
-// SetInProgress set the present condition to a single condition with type "Ready" and status "false". This means
-// the current resources is still reconciling. If any deployment manifests are abnormal, their abnormal status condition will
-// be recorded in the `message` field.
-func SetInProgress(commonStatus *addonsv1alpha1.CommonStatus, abnormalTrueConditions []status.Condition) {
-	setCondition(metav1.ConditionFalse, commonStatus, abnormalTrueConditions)
-}
-
-// SetReady set the present condition to a single condition with type "Ready" and status "true". This means
-// all the deployment manifests are reconciled.
-func SetReady(commonStatus *addonsv1alpha1.CommonStatus, abnormalTrueConditions []status.Condition) {
-	setCondition(metav1.ConditionTrue, commonStatus, abnormalTrueConditions)
-}
-
-func setCondition(status metav1.ConditionStatus, commonStatus *addonsv1alpha1.CommonStatus, abnormalTrueConditions []status.Condition) {
-	newCondition := new(abnormalTrueConditions)
-	newCondition.Status = status
-	newCondition.Type = ReadyType
-	meta.SetStatusCondition(&commonStatus.Conditions, newCondition)
-}
-
-// new returns a Condition object with human-readable message and reason.
+// buildReadyCondition returns a Condition object with human-readable message and reason.
 // The "reason" should be "Normal" if no deployment manifests have abnormal conditions, or "ManifestsNotReady"
 // as long as one deployment manifest has an abnormal condition.
 // The "message" contains each abnormal condition's "reason" and "message".
@@ -46,22 +24,31 @@ func setCondition(status metav1.ConditionStatus, commonStatus *addonsv1alpha1.Co
 //	   message: |-
 //		    apps/v1, Kind=Deployment/argocd/argocd-repo-server:Deployment does not have minimum availability.
 //		    apps/v1, Kind=Deployment/argocd/argocd-server:Deployment does not have minimum availability.
-func new(conditions []status.Condition) metav1.Condition {
-	if len(conditions) == 0 {
-		return metav1.Condition{
-			Reason:  NormalReason,
-			Message: "all manifests are reconciled.",
-		}
+func buildReadyCondition(isReady bool, abnormalConditions []status.Condition) metav1.Condition {
+	var readyCondition metav1.Condition
+	readyCondition.Type = ReadyType
+
+	if isReady {
+		readyCondition.Status = metav1.ConditionTrue
+	} else {
+		readyCondition.Status = metav1.ConditionFalse
 	}
-	abnormalMessage := ""
-	for _, cond := range conditions {
-		if cond.Message != "" {
-			abnormalMessage += cond.Message + "\n"
+
+	if len(abnormalConditions) == 0 {
+		readyCondition.Reason = NormalReason
+		readyCondition.Message = "all manifests are reconciled."
+	} else {
+		var messages []string
+		for _, cond := range abnormalConditions {
+			if cond.Message != "" {
+				messages = append(messages, cond.Message)
+			}
 		}
-	}
 
-	return metav1.Condition{
-		Reason:  AbnormalReason,
-		Message: strings.TrimSuffix(abnormalMessage, "\n"),
+		readyCondition.Reason = AbnormalReason
+
+		readyCondition.Message = strings.Join(messages, "\n")
 	}
+
+	return readyCondition
 }

pkg/patterns/addon/pkg/status/conditions.go

@@ -3,6 +3,7 @@ package status
 import (
 	"context"
 
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/cli-utils/pkg/kstatus/status"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -27,6 +28,11 @@ func (k *kstatusAggregator) BuildStatus(ctx context.Context, info *declarative.S
 		log.Error(err, "error retrieving status")
 		return err
 	}
+	conditions, err := GetConditions(info.Subject)
+	if err != nil {
+		log.Error(err, "error retrieving status.conditions")
+		return err
+	}
 
 	shouldComputeHealthFromObjects := info.Manifest != nil && info.LiveObjects != nil
 	if info.Err != nil {
@@ -78,17 +84,20 @@ func (k *kstatusAggregator) BuildStatus(ctx context.Context, info *declarative.S
 		}
 
 		// Summarize all the deployment manifests statuses to a single results.
-		aggregatedPhase := aggregateStatus(statusMap)
 		// Update the Conditions for the declarativeObject status.
-		if aggregatedPhase == status.CurrentStatus {
-			SetReady(&currentStatus, abnormalConditions)
-		} else {
-			SetInProgress(&currentStatus, abnormalConditions)
-		}
+		aggregatedPhase := aggregateStatus(statusMap)
+		isReady := aggregatedPhase == status.CurrentStatus
+		readyCondition := buildReadyCondition(isReady, abnormalConditions)
+
+		meta.SetStatusCondition(&conditions, readyCondition)
+
 		currentStatus.Phase = string(aggregatedPhase)
+		if err := SetConditions(info.Subject, conditions); err != nil {
+			return err
+		}
 	}
 	currentStatus.Healthy = currentStatus.Phase == string(status.CurrentStatus)
-	if err := utils.SetCommonStatus(info.Subject, currentStatus); err != nil {
+	if err = utils.SetCommonStatus(info.Subject, currentStatus); err != nil {
 		return err
 	}
 	return nil

pkg/patterns/addon/pkg/status/kstatus.go

@@ -138,7 +138,7 @@ func testSimpleReconciler(h *testharness.Harness, testdir string, applier applie
 
 	mgrContext, mgrStop := context.WithCancel(ctx)
 	go func() {
-		time.Sleep(2 * time.Second)
+		time.Sleep(1 * time.Second)
 		// time.Sleep(200 * time.Second)
 		mgrStop()
 	}()

pkg/test/testreconciler/simpletest/controller_test.go

@@ -29,7 +29,8 @@ type SimpleTestSpec struct {
 
 // SimpleTestStatus defines the observed state of Guestbook
 type SimpleTestStatus struct {
-	addonv1alpha1.CommonStatus `json:",inline"`
+	addonv1alpha1.CommonStatus     `json:",inline"`
+	addonv1alpha1.StatusConditions `json:",inline"`
 }
 
 //+kubebuilder:object:root=true