From 81be7058edc0ecb2b713e482a04fd5212860fa58 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Mon, 29 Jun 2020 10:14:24 +0200 Subject: [PATCH] Reconcile parent instances when a child instance is done (#1568) Summary: after updating the [flink-demo](https://github.com/kudobuilder/operators/pull/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 --- pkg/controller/instance/instance_controller.go | 7 +++++++ pkg/controller/instance/resolver_incluster.go | 5 ----- pkg/engine/task/task_kudo_operator.go | 16 +++++++++------- pkg/kudoctl/resources/install/operator.go | 17 +++++++++-------- .../operator-with-dependencies/01-assert.yaml | 2 +- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index 5979d6fb0..28ad6b73c 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/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: }, + // &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{}). diff --git a/pkg/controller/instance/resolver_incluster.go b/pkg/controller/instance/resolver_incluster.go index fbf2c7856..b07192dc2 100644 --- a/pkg/controller/instance/resolver_incluster.go +++ b/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) diff --git a/pkg/engine/task/task_kudo_operator.go b/pkg/engine/task/task_kudo_operator.go index 08b442778..8b19cf0b2 100644 --- a/pkg/engine/task/task_kudo_operator.go +++ b/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 -// if a child instance name is provided e.g. `kafka-instance.custom-name` or -// 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 +// 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. diff --git a/pkg/kudoctl/resources/install/operator.go b/pkg/kudoctl/resources/install/operator.go index 0c6f4c523..27aeb094e 100644 --- a/pkg/kudoctl/resources/install/operator.go +++ b/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 } } diff --git a/test/integration/operator-with-dependencies/01-assert.yaml b/test/integration/operator-with-dependencies/01-assert.yaml index a6226f292..61219e41d 100644 --- a/test/integration/operator-with-dependencies/01-assert.yaml +++ b/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: