KEP-29: Handle changed dependencies when upgrading operators (#1558)

When upgrading operators with dependencies, its dependencies could have changed. In this case the Operator and OperatorVersion resources of the new dependencies are installed. Outdated OperatorVersion resources aren't removed.

The code was refactored to decouple dependency handling from package installation and have separate packages to install and upgrade KUDO resources.

Signed-off-by: Jan Schlicht <jan@d2iq.com>
Co-authored-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

pkg/controller/instance/instance_controller.go

@@ -50,8 +50,7 @@ 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/kudoctl/resources/dependencies"
 	"github.com/kudobuilder/kudo/pkg/util/convert"
 )
 
@@ -199,15 +198,13 @@ 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
-		}
+	// check if all the dependencies can be resolved (if necessary)
+	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 ----------
@@ -255,10 +252,13 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 }
 
 func (r *Reconciler) resolveDependencies(i *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion) error {
+	// no need to check the dependencies if this is a child-level instance, as the top-level instance will take care of that
+	if i.IsChildInstance() {
+		return nil
+	}
 	resolver := &InClusterResolver{ns: i.Namespace, c: r.Client}
-	root := packages.Resources{OperatorVersion: ov}
 
-	_, err := install.ResolveDependencies(root, resolver)
+	_, err := dependencies.Resolve(ov, resolver)
 	if err != nil {
 		return engine.ExecutionError{Err: fmt.Errorf("%w%v", engine.ErrFatalExecution, err), EventName: "CircularDependency"}
 	}

pkg/kudoctl/cmd/upgrade.go

@@ -11,7 +11,7 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/params"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
 	pkgresolver "github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
-	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/resources/upgrade"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
 )
 
@@ -109,5 +109,7 @@ func runUpgrade(args []string, options *options, fs afero.Fs, settings *env.Sett
 
 	resources := pkg.Resources
 
-	return kudo.UpgradeOperatorVersion(kc, resources.OperatorVersion, options.InstanceName, settings.Namespace, options.Parameters)
+	resources.OperatorVersion.SetNamespace(settings.Namespace)
+
+	return upgrade.OperatorVersion(kc, resources.OperatorVersion, options.InstanceName, options.Parameters, resolver)
 }

pkg/kudoctl/packages/install/package.go

@@ -5,10 +5,10 @@ import (
 	"time"
 
 	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
-	engtask "github.com/kudobuilder/kudo/pkg/engine/task"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/resources/install"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 )
 
@@ -60,48 +60,21 @@ func Package(
 		}
 	}
 
-	dependencies, err := ResolveDependencies(resources, resolver)
-	if err != nil {
-		return err
-	}
-
-	// The KUDO controller will create Instances for the dependencies. For this
-	// it needs to resolve the dependencies again from 'KudoOperatorTaskSpec'.
-	// But it cannot resolve packages like the CLI, because it may
-	// not have access to the referenced local files or URLs.
-	// It can however resolve the OperatorVersion from the name of the operator
-	// dependency. For this, we overwrite the 'Package' field describing
-	// dependencies in 'KudoOperatorTaskSpec' with the operator name of the
-	// dependency. This has to be done for the operator to install as well as in
-	// all of its dependencies.
-
-	updateKudoOperatorTaskPackageNames(dependencies, resources.OperatorVersion)
-
-	for _, dependency := range dependencies {
-		dependency.Operator.SetNamespace(namespace)
-		dependency.OperatorVersion.SetNamespace(namespace)
-
-		updateKudoOperatorTaskPackageNames(dependencies, dependency.OperatorVersion)
-
-		if err := installOperatorAndOperatorVersion(client, dependency.Resources); err != nil {
-			return err
-		}
-	}
-
-	if err := installOperatorAndOperatorVersion(client, resources); err != nil {
+	if err := install.OperatorAndOperatorVersion(
+		client, resources.Operator, resources.OperatorVersion, resolver); err != nil {
 		return err
 	}
 
 	if options.SkipInstance {
 		return nil
 	}
 
-	if err := installInstance(client, resources.Instance); err != nil {
+	if err := install.Instance(client, resources.Instance); err != nil {
 		return err
 	}
 
 	if options.Wait != nil {
-		if err := waitForInstance(client, resources.Instance, *options.Wait); err != nil {
+		if err := install.WaitForInstance(client, resources.Instance, *options.Wait); err != nil {
 			return err
 		}
 	}
@@ -154,23 +127,3 @@ func validateParameters(instance v1beta1.Instance, parameters []v1beta1.Paramete
 
 	return nil
 }
-
-// updateKudoOperatorTaskPackageNames sets the 'Package' and 'OperatorName'
-// fields of the 'KudoOperatorTaskSpec' of an 'OperatorVersion' to the operator name
-// initially referenced in the 'Package' field.
-func updateKudoOperatorTaskPackageNames(pkgs []Dependency, operatorVersion *v1beta1.OperatorVersion) {
-	tasks := operatorVersion.Spec.Tasks
-
-	for i := range tasks {
-		if tasks[i].Kind == engtask.KudoOperatorTaskKind {
-			for _, pkg := range pkgs {
-				if tasks[i].Spec.KudoOperatorTaskSpec.Package == pkg.PackageName {
-					tasks[i].Spec.KudoOperatorTaskSpec.Package = pkg.Operator.Name
-					break
-				}
-			}
-		}
-	}
-
-	operatorVersion.Spec.Tasks = tasks
-}

pkg/kudoctl/resources/dependencies/doc.go

@@ -0,0 +1,3 @@
+// Package dependencies provides functions to resolve operator dependencies of
+// operator packages.
+package dependencies

pkg/kudoctl/packages/install/dependencies.go → pkg/kudoctl/resources/dependencies/resolve.go

@@ -1,4 +1,4 @@
-package install
+package dependencies
 
 import (
 	"fmt"
@@ -49,10 +49,14 @@ type Dependency struct {
 	PackageName string
 }
 
-// ResolveDependencies resolved all dependencies of a package.
+// Resolve resolves all dependencies of an OperatorVersion.
 // Dependencies are resolved recursively.
 // Cyclic dependencies are detected and result in an error.
-func ResolveDependencies(root packages.Resources, resolver pkgresolver.Resolver) ([]Dependency, error) {
+func Resolve(operatorVersion *v1beta1.OperatorVersion, resolver pkgresolver.Resolver) ([]Dependency, error) {
+	root := packages.Resources{
+		OperatorVersion: operatorVersion,
+	}
+
 	dependencies := []Dependency{
 		{Resources: root},
 	}

pkg/kudoctl/packages/install/dependencies_test.go → pkg/kudoctl/resources/dependencies/resolve_test.go

@@ -1,4 +1,4 @@
-package install
+package dependencies
 
 import (
 	"fmt"
@@ -188,7 +188,7 @@ func TestResolve(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			resolver := nameResolver{tt.pkgs}
-			got, err := ResolveDependencies(*tt.pkgs[0].Resources, resolver)
+			got, err := Resolve(tt.pkgs[0].Resources.OperatorVersion, resolver)
 
 			assert.Equal(t, err == nil, tt.wantErr == "")
 

pkg/kudoctl/packages/install/instance.go → pkg/kudoctl/resources/install/instance.go

@@ -12,7 +12,9 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 )
 
-func installInstance(client *kudo.Client, instance *v1beta1.Instance) error {
+// Instance installs a KUDO instance to a cluster.
+// It returns an error if the namespace already contains an instance with the same name.
+func Instance(client *kudo.Client, instance *v1beta1.Instance) error {
 	existingInstance, err := client.GetInstance(instance.Name, instance.Namespace)
 	if err != nil {
 		return fmt.Errorf("failed to verify existing instance: %v", err)
@@ -37,7 +39,8 @@ func installInstance(client *kudo.Client, instance *v1beta1.Instance) error {
 	return nil
 }
 
-func waitForInstance(client *kudo.Client, instance *v1beta1.Instance, timeout time.Duration) error {
+// WaitForInstance waits for an amount of time for an instance to be "complete".
+func WaitForInstance(client *kudo.Client, instance *v1beta1.Instance, timeout time.Duration) error {
 	err := client.WaitForInstance(instance.Name, instance.Namespace, nil, timeout)
 	if errors.Is(err, pollwait.ErrWaitTimeout) {
 		clog.Printf("timeout waiting for instance %s/%s", instance.Namespace, instance.Name)