Reconcile parent instances when a child instance is done (#1568)

Summary:
after updating the [flink-demo](kudobuilder/operators#279) to use dependencies a few fixes were needed:
- instance controller now properly reconciles (parent) instances owning child instances
- `InClusterResolver` ignores operators `appVersion` if it wasn't set
- child instance names are generated differently:
  - if the `instanceName` was set, it is taken without modifications
  - otherwise, `parentInstance-childOperator` is taken e.g. `kafka-zookeeper`. in this case we also use `-` instead of `.` separator since instance names are often used as service names (DNS-1035 label) which only allow alphanumeric characters or `-` in them.

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

pkg/controller/instance/instance_controller.go

@@ -96,8 +96,15 @@ func (r *Reconciler) SetupWithManager(
 		})
 
 	return ctrl.NewControllerManagedBy(mgr).
+		// Owns(&kudov1beta1.Instance{}) is equivalent to Watches(&source.Kind{Type: <ForType-apiType>},
+		// &handler.EnqueueRequestForOwner{OwnerType: apiType, IsController: true}) and is responsible for reconciliation
+		// when k8s resources owned by an Instance change.
+		// Watches((&source.Kind{Type: &kudov1beta1.Instance{}}...) is almost the same as Owns(), but with IsController: false
+		// for reconciliation of (parent) instances owning other (child) instances e.g. when a child instance is complete
+		// and parent instance can move on with the plan execution.
 		For(&kudov1beta1.Instance{}).
 		Owns(&kudov1beta1.Instance{}).
+		Watches(&source.Kind{Type: &kudov1beta1.Instance{}}, &handler.EnqueueRequestForOwner{OwnerType: &kudov1beta1.Instance{}, IsController: false}).
 		Owns(&appsv1.Deployment{}).
 		Owns(&corev1.Service{}).
 		Owns(&batchv1.Job{}).

pkg/controller/instance/resolver_incluster.go

@@ -25,11 +25,6 @@ func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersi
 		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)

pkg/engine/task/task_kudo_operator.go

@@ -70,16 +70,18 @@ func (kt KudoOperatorTask) Run(ctx Context) (bool, error) {
 	return true, nil
 }
 
-// dependencyInstanceName returns a name for the child instance in an operator with dependencies looking like
-// <parent-instance.<child-instance> if a child instance name is provided e.g. `kafka-instance.custom-name` or
-// <parent-instance.<child-operator> if not e.g. `kafka-instance.zookeeper`. This way we always have a valid child
-// instance name and user can install the same operator multiple times in the same namespace, because the instance
-// names will be unique thanks to the top-level instance name prefix.
+// dependencyInstanceName returns a name for the child instance. If the name was provided by the user as part of the
+// KudoOperator task definition, it is simply applied. If the name wasn't provided it is generated in the form of
+// <parentInstance-<childOperator> e.g. `kafka-zookeeper`. This way the generated name is always valid and the same
+// operator can be installed multiple times in the same namespace, because the instance names will be unique thanks to
+// the top-level instance name prefix. For self-picked names, it is the responsibility of the user, to pick a unique one.
+// Note: since instance names are often used as service names (DNS-1035 label), we generate instance names using lower case
+// alphanumeric characters or '-', (e.g. 'my-name',  or 'abc-123')
 func dependencyInstanceName(parentInstanceName, instanceName, operatorName string) string {
 	if instanceName != "" {
-		return fmt.Sprintf("%s.%s", parentInstanceName, instanceName)
+		return instanceName
 	}
-	return fmt.Sprintf("%s.%s", parentInstanceName, operatorName)
+	return fmt.Sprintf("%s-%s", parentInstanceName, operatorName)
 }
 
 // instanceParameters method takes templated parameter file and a map of parameters and then renders passed template using kudo engine.

pkg/kudoctl/resources/install/operator.go

@@ -80,10 +80,10 @@ func installDependencies(client *kudo.Client, ov *v1beta1.OperatorVersion, resol
 	// 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 upgrade as well as in
-	// all of its new dependencies.
+	// dependency and the 'OperatorVersion' with the corresponding version.
+	// This has to be done for the operator to upgrade as well as in  all of its new dependencies.
 
-	updateKudoOperatorTaskPackageNames(dependencies, ov)
+	updateKudoOperatorTasks(dependencies, ov)
 
 	for _, dependency := range dependencies {
 		dependency.Operator.SetNamespace(ov.Namespace)
@@ -110,7 +110,7 @@ func installDependencies(client *kudo.Client, ov *v1beta1.OperatorVersion, resol
 		}
 
 		if !funk.ContainsString(installed, dependency.OperatorVersion.Spec.Version) {
-			updateKudoOperatorTaskPackageNames(dependencies, dependency.OperatorVersion)
+			updateKudoOperatorTasks(dependencies, dependency.OperatorVersion)
 
 			if _, err := client.InstallOperatorVersionObjToCluster(
 				dependency.OperatorVersion,
@@ -131,10 +131,10 @@ func installDependencies(client *kudo.Client, ov *v1beta1.OperatorVersion, resol
 	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(
+// updateKudoOperatorTasks method updates all 'KudoOperatorTasks' of an OperatorVersion by setting their 'Package' and
+// 'OperatorVersion' fields to the already resolved packages. This is done for the KUDO controller to be able to grab
+// the right 'OperatorVersion' resources from the cluster when the corresponding task is executed.
+func updateKudoOperatorTasks(
 	dependencies []deps.Dependency, operatorVersion *v1beta1.OperatorVersion) {
 	tasks := operatorVersion.Spec.Tasks
 
@@ -143,6 +143,7 @@ func updateKudoOperatorTaskPackageNames(
 			for _, dependency := range dependencies {
 				if tasks[i].Spec.KudoOperatorTaskSpec.Package == dependency.PackageName {
 					tasks[i].Spec.KudoOperatorTaskSpec.Package = dependency.Operator.Name
+					tasks[i].Spec.KudoOperatorTaskSpec.OperatorVersion = dependency.OperatorVersion.Spec.Version
 					break
 				}
 			}

test/integration/operator-with-dependencies/01-assert.yaml

@@ -22,7 +22,7 @@ status:
 apiVersion: kudo.dev/v1beta1
 kind: Instance
 metadata:
-  name: dummy-instance.child
+  name: dummy-instance-child
 status:
   planStatus:
     deploy: