KEP-29: Introducing operator dependency cycle detection to the instan…

…ce controller (#1559) (#1567)

Summary:
Operator dependencies cycle detection is now part of the instance controller reconciliation. We execute the check for the top-level instances only since the lower-level instances are already take care of.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Fixes #1513

config/crds/kudo.dev_operatorversions.yaml

@@ -183,17 +183,15 @@ spec:
                         type: boolean
                       instanceName:
                         type: string
-                      operatorName:
-                        description: name of installed operator. this field is set
-                          by the CLI and should not be set directly by the user
-                        type: string
                       operatorVersion:
                         description: a specific operator version in the official repo,
                           defaults to the most recent one
                         type: string
                       package:
                         description: either repo package name, local package folder
-                          or an URL to package tarball
+                          or an URL to package tarball. during operator installation,
+                          kudoctl will resolve the package and override this field
+                          with the resolved operator name.
                         type: string
                       parameter:
                         type: string

pkg/apis/kudo/v1beta1/instance_types.go

@@ -16,7 +16,6 @@ limitations under the License.
 package v1beta1
 
 import (
-	"fmt"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -222,14 +221,3 @@ type InstanceList struct {
 func init() {
 	SchemeBuilder.Register(&Instance{}, &InstanceList{})
 }
-
-// InstanceError indicates error on that can also emit a kubernetes warn event
-// +k8s:deepcopy-gen=false
-type InstanceError struct {
-	Err       error
-	EventName *string // nil if no warn event should be created
-}
-
-func (e *InstanceError) Error() string {
-	return fmt.Sprintf("Error during execution: %v", e.Err)
-}

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -15,18 +15,13 @@ const (
 	instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
 )
 
-// ScheduledPlan returns plan status of currently active plan or nil if no plan is running. In most cases this method
-// will return the same plan status as the [GetPlanInProgress](pkg/apis/kudo/v1beta1/instance_types_helpers.go:25) below.
-// However, there is a small window where both might return different results:
-// 1. GetScheduledPlan reads the plan from i.Spec.PlanExecution.PlanName which is set and reset by the instance admission
-//   webhook
-// 2. GetPlanInProgress goes through i.Spec.PlanStatus map and returns the first found plan that is running
-//
-// In (1), i.Spec.PlanExecution.PlanName is set directly when the user updates the instance and reset **after** the plan
-// is terminal
-// In (2) i.Spec.PlanStatus is updated **AFTER** the instance controller is done with the reconciliation call
-func (i *Instance) GetScheduledPlan() *PlanStatus {
-	return i.PlanStatus(i.Spec.PlanExecution.PlanName)
+func GetInstance(namespacedName types.NamespacedName, c client.Client) (i *Instance, err error) {
+	i = &Instance{}
+	err = c.Get(context.TODO(), namespacedName, i)
+	if err != nil {
+		return nil, err
+	}
+	return i, nil
 }
 
 // GetPlanInProgress returns plan status of currently active plan or nil if no plan is running
@@ -186,18 +181,19 @@ func (i *Instance) GetOperatorVersion(c client.Reader) (ov *OperatorVersion, err
 	return GetOperatorVersionByName(i.Spec.OperatorVersion.Name, i.OperatorVersionNamespace(), c)
 }
 
-func GetOperatorVersionByName(ovn, ns string, c client.Reader) (ov *OperatorVersion, err error) {
-	ov = &OperatorVersion{}
-	err = c.Get(context.TODO(),
-		types.NamespacedName{
-			Name:      ovn,
-			Namespace: ns,
-		},
-		ov)
-	if err != nil {
-		return nil, err
+// IsChildInstance method return true if this instance is owned by another instance (as a dependency) and false otherwise.
+// If there is any owner with the same kind 'Instance' then this Instance is owned by another one.
+func (i *Instance) IsChildInstance() bool {
+	for _, or := range i.GetOwnerReferences() {
+		if or.Kind == i.Kind {
+			return true
+		}
 	}
-	return ov, nil
+	return false
+}
+
+func (i *Instance) IsTopLevelInstance() bool {
+	return !i.IsChildInstance()
 }
 
 // wasRunAfter returns true if p1 was run after p2

pkg/apis/kudo/v1beta1/operator_types_helpers.go

@@ -0,0 +1,17 @@
+package v1beta1
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func GetOperator(name, ns string, c client.Client) (*Operator, error) {
+	o := &Operator{}
+	err := c.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: ns}, o)
+	if err != nil {
+		return nil, err
+	}
+	return o, nil
+}

pkg/apis/kudo/v1beta1/operatorversion_types.go

@@ -199,12 +199,10 @@ type PipeSpec struct {
 
 // KudoOperatorSpec specifies how a KUDO operator is installed
 type KudoOperatorTaskSpec struct {
-	// either repo package name, local package folder or an URL to package tarball
+	// either repo package name, local package folder or an URL to package tarball. during operator installation,
+	// kudoctl will resolve the package and override this field with the resolved operator name.
 	// +optional
 	Package string `json:"package,omitempty"`
-	// name of installed operator. this field is set by the CLI and should not be set directly by the user
-	// +optional
-	OperatorName string `json:"operatorName,omitempty"`
 	// +optional
 	InstanceName string `json:"instanceName,omitempty"`
 	// a specific app version in the official repo, defaults to the most recent

pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go

@@ -1,7 +1,11 @@
 package v1beta1
 
 import (
+	"context"
 	"fmt"
+
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func OperatorInstanceName(operatorName string) string {
@@ -19,3 +23,12 @@ func (ov *OperatorVersion) FullyQualifiedName() string {
 func (ov *OperatorVersion) EqualOperatorVersion(other *OperatorVersion) bool {
 	return ov.FullyQualifiedName() == other.FullyQualifiedName()
 }
+
+func GetOperatorVersionByName(name, ns string, c client.Reader) (ov *OperatorVersion, err error) {
+	ov = &OperatorVersion{}
+	err = c.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: ns}, ov)
+	if err != nil {
+		return nil, err
+	}
+	return ov, nil
+}

pkg/controller/instance/instance_controller.go

@@ -50,6 +50,8 @@ import (
 	"github.com/kudobuilder/kudo/pkg/engine/renderer"
 	"github.com/kudobuilder/kudo/pkg/engine/task"
 	"github.com/kudobuilder/kudo/pkg/engine/workflow"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/install"
 	"github.com/kudobuilder/kudo/pkg/util/convert"
 )
 
@@ -197,6 +199,17 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", plan))
 	}
 
+	// if this is a top-level instance, check for dependency cycles
+	if instance.IsTopLevelInstance() {
+		err := r.resolveDependencies(instance, ov)
+		if err != nil {
+			planStatus.SetWithMessage(kudov1beta1.ExecutionFatalError, err.Error())
+			instance.UpdateInstanceStatus(planStatus, &metav1.Time{Time: time.Now()})
+			err = r.handleError(err, instance, oldInstance)
+			return reconcile.Result{}, err
+		}
+	}
+
 	// ---------- 3. Execute the scheduled plan ----------
 
 	metadata := &engine.Metadata{
@@ -241,8 +254,18 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 	return reconcile.Result{}, nil
 }
 
-func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error {
+func (r *Reconciler) resolveDependencies(i *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion) error {
+	resolver := &InClusterResolver{ns: i.Namespace, c: r.Client}
+	root := packages.Resources{OperatorVersion: ov}
 
+	_, err := install.ResolveDependencies(root, resolver)
+	if err != nil {
+		return engine.ExecutionError{Err: fmt.Errorf("%w%v", engine.ErrFatalExecution, err), EventName: "CircularDependency"}
+	}
+	return nil
+}
+
+func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error {
 	// The order of both updates below is important: *first* the instance Spec and Metadata and *then* the Status.
 	// If Status is updated first, a new reconcile request will be scheduled and might fetch the *WRONG* instance
 	// Spec.PlanExecution. This request will then try to execute an already finished plan (again).
@@ -331,22 +354,13 @@ func (r *Reconciler) handleError(err error, instance *kudov1beta1.Instance, oldI
 		}
 	}
 
-	// for code being processed on instance, we need to handle these errors as well
-	var iError *kudov1beta1.InstanceError
-	if errors.As(err, &iError) {
-		if iError.EventName != nil {
-			r.Recorder.Event(instance, "Warning", convert.StringValue(iError.EventName), err.Error())
-		}
-	}
 	return err
 }
 
 // getInstance retrieves the instance by namespaced name
 func (r *Reconciler) getInstance(request ctrl.Request) (instance *kudov1beta1.Instance, err error) {
-	instance = &kudov1beta1.Instance{}
-	err = r.Get(context.TODO(), request.NamespacedName, instance)
+	instance, err = kudov1beta1.GetInstance(request.NamespacedName, r.Client)
 	if err != nil {
-		// Error reading the object - requeue the request.
 		log.Printf("InstanceController: Error getting instance %v: %v",
 			request.NamespacedName,
 			err)

pkg/controller/instance/resolver_incluster.go

@@ -0,0 +1,46 @@
+package instance
+
+import (
+	"fmt"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+)
+
+// InClusterResolver resolves packages that are already installed in the cluster. Note, that unlike other resolvers, the
+// resulting 'packages.Package' struct does not contain package 'packages.Files' (we don't have the original files) and
+// doesn't have an Instance resource because multiple Instances of the same Operator/OperatorVersion can exist
+type InClusterResolver struct {
+	c  client.Client
+	ns string
+}
+
+func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersion string) (*packages.Package, error) {
+	ovn := v1beta1.OperatorVersionName(name, operatorVersion)
+
+	ov, err := v1beta1.GetOperatorVersionByName(ovn, r.ns, r.c)
+	if err != nil {
+		return nil, fmt.Errorf("failed to resolve operator version %s/%s:%s", r.ns, ovn, appVersion)
+	}
+
+	// sanity check, as there is an explicit 1:1 relationship between an operator and app version
+	if ov.Spec.AppVersion != appVersion {
+		return nil, fmt.Errorf("found operator version %s/%s but found appVersion %s is not equal to the requested %s", r.ns, ovn, ov.Spec.AppVersion, appVersion)
+	}
+
+	o, err := v1beta1.GetOperator(name, r.ns, r.c)
+	if err != nil {
+		return nil, fmt.Errorf("failed to resolve operator %s/%s", r.ns, name)
+	}
+
+	return &packages.Package{
+		Resources: &packages.Resources{
+			Operator:        o,
+			OperatorVersion: ov,
+			Instance:        nil,
+		},
+		Files: nil,
+	}, nil
+}

pkg/engine/task/task.go

@@ -189,8 +189,8 @@ func fatalExecutionError(cause error, eventName string, meta renderer.Metadata)
 
 func newKudoOperator(task *v1beta1.Task) (Tasker, error) {
 	// validate KudoOperatorTask
-	if task.Spec.KudoOperatorTaskSpec.OperatorName == "" {
-		return nil, fmt.Errorf("task validation error: kudo operator task '%s' has an empty operator name", task.Name)
+	if task.Spec.KudoOperatorTaskSpec.Package == "" {
+		return nil, fmt.Errorf("task validation error: kudo operator task '%s' has an empty package name", task.Name)
 	}
 
 	if task.Spec.KudoOperatorTaskSpec.OperatorVersion == "" {
@@ -199,7 +199,7 @@ func newKudoOperator(task *v1beta1.Task) (Tasker, error) {
 
 	return KudoOperatorTask{
 		Name:            task.Name,
-		OperatorName:    task.Spec.KudoOperatorTaskSpec.OperatorName,
+		OperatorName:    task.Spec.KudoOperatorTaskSpec.Package,
 		InstanceName:    task.Spec.KudoOperatorTaskSpec.InstanceName,
 		AppVersion:      task.Spec.KudoOperatorTaskSpec.AppVersion,
 		OperatorVersion: task.Spec.KudoOperatorTaskSpec.OperatorVersion,

pkg/engine/task/task_test.go

@@ -169,7 +169,7 @@ spec:
 name: deploy-zk
 kind: KudoOperator
 spec: 
-    operatorName: zookeeper
+    package: zookeeper
     appVersion: 0.0.3
     operatorVersion: 0.0.4
     instanceName: zk`,

pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden

@@ -257,17 +257,15 @@ spec:
                         type: boolean
                       instanceName:
                         type: string
-                      operatorName:
-                        description: name of installed operator. this field is set
-                          by the CLI and should not be set directly by the user
-                        type: string
                       operatorVersion:
                         description: a specific operator version in the official repo,
                           defaults to the most recent one
                         type: string
                       package:
                         description: either repo package name, local package folder
-                          or an URL to package tarball
+                          or an URL to package tarball. during operator installation,
+                          kudoctl will resolve the package and override this field
+                          with the resolved operator name.
                         type: string
                       parameter:
                         type: string

pkg/kudoctl/cmd/testdata/deploy-kudo-sa.yaml.golden

@@ -257,17 +257,15 @@ spec:
                         type: boolean
                       instanceName:
                         type: string
-                      operatorName:
-                        description: name of installed operator. this field is set
-                          by the CLI and should not be set directly by the user
-                        type: string
                       operatorVersion:
                         description: a specific operator version in the official repo,
                           defaults to the most recent one
                         type: string
                       package:
                         description: either repo package name, local package folder
-                          or an URL to package tarball
+                          or an URL to package tarball. during operator installation,
+                          kudoctl will resolve the package and override this field
+                          with the resolved operator name.
                         type: string
                       parameter:
                         type: string

pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden

@@ -257,17 +257,15 @@ spec:
                         type: boolean
                       instanceName:
                         type: string
-                      operatorName:
-                        description: name of installed operator. this field is set
-                          by the CLI and should not be set directly by the user
-                        type: string
                       operatorVersion:
                         description: a specific operator version in the official repo,
                           defaults to the most recent one
                         type: string
                       package:
                         description: either repo package name, local package folder
-                          or an URL to package tarball
+                          or an URL to package tarball. during operator installation,
+                          kudoctl will resolve the package and override this field
+                          with the resolved operator name.
                         type: string
                       parameter:
                         type: string