Add Conditions in HNCConfiguration.Status to indicate error conditions.

This PR adds Conditions in HNCConfiguration.Status to indicate error scenarios. Condition specifies error code and message for a specific error. Currently we support following error code:
    - "CRIT_SINGLETON_NAME_INVALID": the specified singleton name is invalid.
    - "OBJECT_RECONCILER_CREATION_FAILED": an error exists when creating the object reconciler for a specific type.

This PR includes changes introduced in kubernetes-retired#453

Tested: GKE cluster; unit tests.

Design doc: http://bit.ly/hnc-type-configuration
Issue: kubernetes-retired#411

incubator/hnc/api/v1alpha1/hnc_config.go

@@ -42,6 +42,13 @@ const (
 	Remove SynchronizationMode = "remove"
 )
 
+// HNCConfigurationCondition codes. *All* codes must also be documented in the
+// comment to HNCConfigurationCondition.Code.
+const (
+	CritSingletonNameInvalid       HNCConfigurationCode = "CRIT_SINGLETON_NAME_INVALID"
+	ObjectReconcilerCreationFailed HNCConfigurationCode = "OBJECT_RECONCILER_CREATION_FAILED"
+)
+
 // TypeSynchronizationSpec defines the desired synchronization state of a specific kind.
 type TypeSynchronizationSpec struct {
 	// API version of the kind defined below. This is used to unambiguously identifies the kind.
@@ -88,6 +95,9 @@ type HNCConfigurationSpec struct {
 type HNCConfigurationStatus struct {
 	// Types indicates the observed synchronization states of kinds, if any.
 	Types []TypeSynchronizationStatus `json:"types,omitempty"`
+
+	// Conditions describes the errors, if any.
+	Conditions []HNCConfigurationCondition `json:"conditions,omitempty"`
 }
 
 // +kubebuilder:object:root=true
@@ -99,6 +109,35 @@ type HNCConfigurationList struct {
 	Items           []HNCConfiguration `json:"items"`
 }
 
+// HNCConfigurationCode is the machine-readable, enum-like type of `HNCConfigurationCondition.Code`.
+// See that field for more information.
+type HNCConfigurationCode string
+
+// HNCConfigurationCondition specifies the code and the description of an error condition.
+type HNCConfigurationCondition struct {
+	// Describes the condition in a machine-readable string value. The currently valid values are
+	// shown below, but new values may be added over time. This field is always present in a
+	// condition.
+	//
+	// All codes that begin with the prefix `CRIT_` indicate that reconciliation has
+	// been paused for this configuration. Future changes of the configuration will be
+	// ignored by HNC until the condition has been resolved. Non-critical conditions
+	// typically indicate some kinds of error that HNC itself can ignore. However,
+	// the behaviors of some types might be out-of-sync with the users' expectations.
+	//
+	// Currently, the supported values are:
+	//
+	// - "CRIT_SINGLETON_NAME_INVALID": the specified singleton name is invalid.
+	//
+	// - "OBJECT_RECONCILER_CREATION_FAILED": an error exists when creating the object
+	// reconciler for a specific type.
+	Code HNCConfigurationCode `json:"code"`
+
+	// A human-readable description of the condition, if the `code` field is not
+	// sufficiently clear on their own.
+	Msg string `json:"msg,omitempty"`
+}
+
 func init() {
 	SchemeBuilder.Register(&HNCConfiguration{}, &HNCConfigurationList{})
 }

incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml

@@ -58,6 +58,35 @@ spec:
         status:
           description: HNCConfigurationStatus defines the observed state of HNC configuration.
           properties:
+            conditions:
+              description: Conditions describes the errors, if any.
+              items:
+                description: HNCConfigurationCondition specifies the code and the
+                  description of an error condition.
+                properties:
+                  code:
+                    description: "Describes the condition in a machine-readable string
+                      value. The currently valid values are shown below, but new values
+                      may be added over time. This field is always present in a condition.
+                      \n All codes that begin with the prefix `CRIT_` indicate that
+                      reconciliation has been paused for this configuration. Future
+                      changes of the configuration will be ignored by HNC until the
+                      condition has been resolved. Non-critical conditions typically
+                      indicate some kinds of error that HNC itself can ignore. However,
+                      the behaviors of some types might be out-of-sync with the users'
+                      expectations. \n Currently, the supported values are: \n - \"CRIT_SINGLETON_NAME_INVALID\":
+                      the specified singleton name is invalid. \n - \"OBJECT_RECONCILER_CREATION_FAILED\":
+                      an error exists when creating the object reconciler for a specific
+                      type."
+                    type: string
+                  msg:
+                    description: A human-readable description of the condition, if
+                      the `code` field is not sufficiently clear on their own.
+                    type: string
+                required:
+                - code
+                type: object
+              type: array
             types:
               description: Types indicates the observed synchronization states of
                 kinds, if any.

incubator/hnc/go.sum

@@ -175,14 +175,17 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v0.9.0 h1:tXuTFVHC03mW0D+Ua1Q2d1EAVqLTuggX50V0VLICCzY=
 github.com/prometheus/client_golang v0.9.0/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw=
+github.com/prometheus/client_golang v0.9.2 h1:awm861/B8OKDd2I/6o1dy3ra4BamzKhYOiGItCeZ740=
 github.com/prometheus/client_golang v0.9.2/go.mod h1:OsXs2jCmiKlQ1lTBmv21f2mNfw4xf/QclQDMrYNZzcM=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910 h1:idejC8f05m9MGOsuEi1ATq9shN03HrxNkD/luQvxCv8=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
 github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e h1:n/3MEhJQjQxrOUCzh1Y3Re6aJUUWRp2M9+Oc3eVn/54=
 github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e/go.mod h1:daVV7qP5qjZbuso7PdcryaAu0sAZbrN9i7WWcTMWvro=
+github.com/prometheus/common v0.0.0-20181126121408-4724e9255275 h1:PnBWHBf+6L0jOqq0gIVUe6Yk0/QMZ640k6NvkxcBf+8=
 github.com/prometheus/common v0.0.0-20181126121408-4724e9255275/go.mod h1:daVV7qP5qjZbuso7PdcryaAu0sAZbrN9i7WWcTMWvro=
 github.com/prometheus/procfs v0.0.0-20180725123919-05ee40e3a273 h1:agujYaXJSxSo18YNX3jzl+4G6Bstwt+kqv47GS12uL0=
 github.com/prometheus/procfs v0.0.0-20180725123919-05ee40e3a273/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
+github.com/prometheus/procfs v0.0.0-20181204211112-1dc9a6cbc91a h1:9a8MnZMP0X2nLJdBg+pBmGgkJlSaKC2KaQmTCk1XDtE=
 github.com/prometheus/procfs v0.0.0-20181204211112-1dc9a6cbc91a/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
 github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g=

incubator/hnc/pkg/reconcilers/hnc_config.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -29,6 +31,11 @@ type ConfigReconciler struct {
 	// Forest is the in-memory data structure that is shared with all other reconcilers.
 	Forest *forest.Forest
 
+	// Igniter is a channel of event.GenericEvent (see "Watching Channels" in
+	// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html)
+	// that is used to enqueue the singleton for initial reconciliation.
+	Igniter chan event.GenericEvent
+
 	// HierarchyConfigUpdates is a channel of events used to update hierarchy configuration changes performed by
 	// ObjectReconcilers. It is passed on to ObjectReconcilers for the updates. The ConfigReconciler itself does
 	// not use it.
@@ -38,33 +45,50 @@ type ConfigReconciler struct {
 // Reconcile sets up some basic variable and logs the Spec.
 // TODO: Updates the comment above when adding more logic to the Reconcile method.
 func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
-	// TODO: Surface the error more prominently and add a validating admission controller to prevent
-	// the problem in the first place.
+	ctx := context.Background()
+
+	// Validate the singleton name.
+	// TODO: Add a validating admission controller to prevent the problem in the first place.
 	if req.NamespacedName.Name != api.HNCConfigSingleton {
-		r.Log.Error(nil, "Singleton name is wrong. It should be 'config'.")
+		nnm := types.NamespacedName{Name: req.NamespacedName.Name}
+		inst := &api.HNCConfiguration{}
+		if err := r.Get(ctx, nnm, inst); err != nil {
+			r.Log.Error(err, "Couldn't read the singleton.")
+			return ctrl.Result{}, err
+		}
+
+		condition := api.HNCConfigurationCondition{
+			Code: api.CritSingletonNameInvalid,
+			Msg:  "Singleton name is wrong. It should be 'config'.",
+		}
+		inst.Status.Conditions = nil
+		inst.Status.Conditions = append(inst.Status.Conditions, condition)
+
+		if err := r.writeSingleton(ctx, inst); err != nil {
+			r.Log.Error(err, "Couldn't write singleton.")
+			return ctrl.Result{}, err
+		}
 		return ctrl.Result{}, nil
 	}
 
-	ctx := context.Background()
 	inst, err := r.getSingleton(ctx)
 	if err != nil {
 		r.Log.Error(err, "Couldn't read singleton.")
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, err
 	}
 
 	// TODO: Modify this and other reconcilers (e.g., hierarchy and object reconcilers) to
 	// achieve the reconciliation.
 	r.Log.Info("Reconciling cluster-wide HNC configuration.")
 
-	// Create cooresponding ObjectReconcilers for newly added types, if needed.
-	// TODO: Returning an error here will make controller-runtime to requeue the object to be reconciled again.
-	// A better way to handle this is to display the error as part of the HNCConfig.status field to suggest the error is a
-	// permanent problem and to stop controller-runtime from retrying.
+	// Clear the existing conditions because we will reconstruct the latest conditions.
+	inst.Status.Conditions = nil
+
+	// Create corresponding ObjectReconcilers for newly added types, if needed.
 	// TODO: Rename the method syncObjectReconcilers because we might need more than creating ObjectReconcilers in future.
 	// For example, we might need to delete an ObjectReconciler if its corresponding type is deleted in the HNCConfiguration.
-	if err := r.createObjectReconcilers(inst); err != nil {
-		r.Log.Error(err, "Couldn't create the new object reconciler.")
-		return ctrl.Result{}, nil
+	if hasError := r.createObjectReconcilers(inst); hasError {
+		r.Log.Info("Object reconciler creation has an error.")
 	}
 
 	// Write back to the apiserver.
@@ -117,8 +141,8 @@ func (r *ConfigReconciler) writeSingleton(ctx context.Context, inst *api.HNCConf
 
 // createObjectReconcilers creates corresponding ObjectReconcilers for the newly added types in the
 // HNC configuration, if there is any. It also informs the in-memory forest about the newly created
-// ObjectReconcilers.
-func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) error {
+// ObjectReconcilers. If a type is misconfigured, the corresponding object reconciler will not be created.
+func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) bool {
 	// This method is guarded by the forest mutex. The mutex is guarding two actions: creating ObjectReconcilers for
 	// the newly added types and adding the created ObjectReconcilers to the types list in the Forest (r.Forest.types).
 	//
@@ -133,17 +157,25 @@ func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) e
 	// using the latest forest structure.
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
+
+	hasError := false
 	for _, t := range inst.Spec.Types {
 		gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind)
 		if r.Forest.HasTypeSyncer(gvk) {
 			continue
 		}
 		if err := r.createObjectReconciler(gvk); err != nil {
-			return fmt.Errorf("Error while trying to create ObjectReconciler: %s", err)
+			msg := fmt.Sprintf("Error while trying to create ObjectReconciler for type %s: %s", gvk, err)
+			condition := api.HNCConfigurationCondition{
+				Code: api.ObjectReconcilerCreationFailed,
+				Msg:  msg,
+			}
+			inst.Status.Conditions = append(inst.Status.Conditions, condition)
+			hasError = true
 		}
 	}
 
-	return nil
+	return hasError
 }
 
 // createObjectReconciler creates an ObjectReconciler for the given GVK and informs forest about the reconciler.
@@ -175,5 +207,21 @@ func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind) e
 func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&api.HNCConfiguration{}).
+		Watches(&source.Channel{Source: r.Igniter}, &handler.EnqueueRequestForObject{}).
 		Complete(r)
 }
+
+// forceInitialReconcile forces reconciliation to start after setting up the
+// controller with the manager. This is used to create a default singleton if
+// there is no singleton in the cluster. This occurs in a goroutine so the
+// caller doesn't block; since the reconciler is never garbage-collected,
+// this is safe.
+func (r *ConfigReconciler) forceInitialReconcile(log logr.Logger, reason string) {
+	go func() {
+		log.Info("Enqueuing for reconciliation", "reason", reason)
+		// The watch handler doesn't care about anything except the metadata.
+		inst := &api.HNCConfiguration{}
+		inst.ObjectMeta.Name = api.HNCConfigSingleton
+		r.Igniter <- event.GenericEvent{Meta: inst}
+	}()
+}