From e204320c00ad4b5f756f7471b8ce7aa6bf0f6f9f Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Wed, 28 Aug 2019 00:40:43 +0200 Subject: [PATCH 01/18] minio objectmatcher + some refactors --- .../clusterinstallation/create_resources.go | 40 +++++++--- .../clusterinstallation/mattermost.go | 36 +++------ pkg/controller/clusterinstallation/minio.go | 80 ++++++++++--------- pkg/controller/clusterinstallation/mysql.go | 4 +- 4 files changed, 81 insertions(+), 79 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 072089aa6..493b8d492 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -22,31 +22,47 @@ type Object interface { v1.Object } -// createResource creates the provided resource and sets the owner -func (r *ReconcileClusterInstallation) createResource(owner v1.Object, resource Object, reqLogger logr.Logger) error { +// Create creates the provided resource and sets the owner +func (r *ReconcileClusterInstallation) Create(owner v1.Object, desired Object, reqLogger logr.Logger) error { // adding the last applied annotation to use the object matcher later // see: https://github.com/banzaicloud/k8s-objectmatcher - err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(resource) + err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) if err != nil { reqLogger.Error(err, "Error applying the annotation in the resource") return err } - err = r.client.Create(context.TODO(), resource) + err = r.client.Create(context.TODO(), desired) if err != nil { reqLogger.Error(err, "Error creating resource") return err } + return controllerutil.SetControllerReference(owner, desired, r.scheme) +} - return controllerutil.SetControllerReference(owner, resource, r.scheme) +func (r *ReconcileClusterInstallation) Update(current, desired Object, reqLogger logr.Logger) error { + patchResult, err := objectMatcher.DefaultPatchMaker.Calculate(current, desired) + if err != nil { + reqLogger.Error(err, "error checking the difference in the deployment") + return err + } + if !patchResult.IsEmpty() { + err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) + if err != nil { + reqLogger.Error(err, "error applying the annotation in the deployment") + return err + } + return r.client.Update(context.TODO(), desired) + } + return nil } func (r *ReconcileClusterInstallation) createServiceAccountIfNotExists(owner v1.Object, serviceAccount *corev1.ServiceAccount, reqLogger logr.Logger) error { foundServiceAccount := &corev1.ServiceAccount{} err := r.client.Get(context.TODO(), types.NamespacedName{Name: serviceAccount.Name, Namespace: serviceAccount.Namespace}, foundServiceAccount) - if err != nil && errors.IsNotFound(err) { + if err != nil && kerrors.IsNotFound(err) { reqLogger.Info("Creating service account", "name", serviceAccount.Name) - return r.createResource(owner, serviceAccount, reqLogger) + return r.Create(owner, serviceAccount, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if service account exists") return err @@ -60,7 +76,7 @@ func (r *ReconcileClusterInstallation) createRoleBindingIfNotExists(owner v1.Obj err := r.client.Get(context.TODO(), types.NamespacedName{Name: roleBinding.Name, Namespace: roleBinding.Namespace}, foundRoleBinding) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating role binding", "name", roleBinding.Name) - return r.createResource(owner, roleBinding, reqLogger) + return r.Create(owner, roleBinding, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if role binding exists") return err @@ -74,7 +90,7 @@ func (r *ReconcileClusterInstallation) createServiceIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: service.Name, Namespace: service.Namespace}, foundService) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating service", "name", service.Name) - return r.createResource(owner, service, reqLogger) + return r.Create(owner, service, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if service exists") return err @@ -88,7 +104,7 @@ func (r *ReconcileClusterInstallation) createIngressIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: ingress.Name, Namespace: ingress.Namespace}, foundIngress) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating ingress", "name", ingress.Name) - return r.createResource(owner, ingress, reqLogger) + return r.Create(owner, ingress, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if ingress exists") return err @@ -102,7 +118,7 @@ func (r *ReconcileClusterInstallation) createDeploymentIfNotExists(owner v1.Obje err := r.client.Get(context.TODO(), types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, foundDeployment) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating deployment", "name", deployment.Name) - return r.createResource(owner, deployment, reqLogger) + return r.Create(owner, deployment, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if deployment exists") return err @@ -116,7 +132,7 @@ func (r *ReconcileClusterInstallation) createSecretIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, foundSecret) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating secret", "name", secret.Name) - return r.createResource(owner, secret, reqLogger) + return r.Create(owner, secret, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if secret exists") return err diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index 167c05198..d552f956b 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -6,13 +6,12 @@ import ( "reflect" "time" - objectMatcher "github.com/banzaicloud/k8s-objectmatcher/patch" "github.com/go-logr/logr" "github.com/pkg/errors" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" - v1beta1 "k8s.io/api/extensions/v1beta1" + "k8s.io/api/extensions/v1beta1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -188,7 +187,7 @@ func (r *ReconcileClusterInstallation) checkMattermostDeployment(mattermost *mat return err } - err = r.updateMattermostDeployment(mattermost, deployment, foundDeployment, imageName, reqLogger) + err = r.updateMattermostDeployment(mattermost, foundDeployment, deployment, imageName, reqLogger) if err != nil { reqLogger.Error(err, "Failed to update mattermost deployment") return err @@ -201,13 +200,13 @@ func (r *ReconcileClusterInstallation) checkMattermostDeployment(mattermost *mat // If an update is required then the deployment spec is set to: // - roll forward version // - keep active MattermostInstallation available by setting maxUnavailable=N-1 -func (r *ReconcileClusterInstallation) updateMattermostDeployment(mi *mattermostv1alpha1.ClusterInstallation, new, original *appsv1.Deployment, imageName string, reqLogger logr.Logger) error { +func (r *ReconcileClusterInstallation) updateMattermostDeployment(mattermost *mattermostv1alpha1.ClusterInstallation, current, desired *appsv1.Deployment, imageName string, reqLogger logr.Logger) error { var update bool // Look for mattermost container in pod spec and determine if the image // needs to be updated. - for _, container := range original.Spec.Template.Spec.Containers { - if container.Name == new.Spec.Template.Spec.Containers[0].Name { + for _, container := range current.Spec.Template.Spec.Containers { + if container.Name == desired.Spec.Template.Spec.Containers[0].Name { if container.Image != imageName { reqLogger.Info("Current image is not the same as the requested, will upgrade the Mattermost installation") update = true @@ -228,14 +227,14 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mi *mattermost job := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: updateName, - Namespace: mi.GetNamespace(), + Namespace: mattermost.GetNamespace(), }, Spec: batchv1.JobSpec{ Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{"app": updateName}, }, - Spec: *new.Spec.Template.Spec.DeepCopy(), + Spec: *desired.Spec.Template.Spec.DeepCopy(), }, }, } @@ -272,7 +271,7 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mi *mattermost context.TODO(), types.NamespacedName{ Name: updateName, - Namespace: mi.GetNamespace(), + Namespace: mattermost.GetNamespace(), }, foundJob, ) @@ -290,22 +289,5 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mi *mattermost } } } - - reqLogger.Info("Upgrade image job ran successfully") - - patchResult, err := objectMatcher.DefaultPatchMaker.Calculate(original, new) - if err != nil { - return errors.Wrap(err, "error checking the difference in the deployment") - } - - if !patchResult.IsEmpty() { - err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(new) - if err != nil { - return errors.Wrap(err, "error applying the annotation in the deployment") - } - - return r.client.Update(context.TODO(), new) - } - - return nil + return r.Update(current, desired, reqLogger) } diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index 741b8e9f5..edc9af20e 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -28,50 +28,54 @@ func (r *ReconcileClusterInstallation) checkMinio(mattermost *mattermostv1alpha1 return r.checkMinioInstance(mattermost, reqLogger) } -func (r *ReconcileClusterInstallation) checkMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { - // Check if custom secret was specified - if mattermost.Spec.Minio.Secret != "" { - // Check if the Secret exists - var secret *corev1.Secret - err := r.client.Get(context.TODO(), types.NamespacedName{Name: mattermost.Spec.Minio.Secret, Namespace: mattermost.Namespace}, secret) - if err != nil { - // Secret does not exist - return errors.Wrap(err, "unable to locate custom minio secret") - } - - // Check if the Secret has required fields - if _, ok := secret.Data["accesskey"]; !ok { - return fmt.Errorf("custom Minio Secret %s does not have an 'accesskey' value", mattermost.Spec.Minio.Secret) - } - if _, ok := secret.Data["secretkey"]; !ok { - return fmt.Errorf("custom Minio Secret %s does not have an 'secretkey' value", mattermost.Spec.Minio.Secret) - } - - reqLogger.Info("Skipping minio secret creation, using custom secret") - return nil - } - - secret := mattermostMinio.Secret(mattermost) - - err := r.createSecretIfNotExists(mattermost, secret, reqLogger) +func (r *ReconcileClusterInstallation) checkCustomMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { + var secret *corev1.Secret + err := r.client.Get(context.TODO(), types.NamespacedName{Name: mattermost.Spec.Minio.Secret, Namespace: mattermost.Namespace}, secret) if err != nil { - return err + return errors.Wrap(err, "unable to locate custom minio secret") + } + // Validate custom secret required fields + if _, ok := secret.Data["accesskey"]; !ok { + return fmt.Errorf("custom Minio Secret %s does not have an 'accesskey' value", mattermost.Spec.Minio.Secret) } + if _, ok := secret.Data["secretkey"]; !ok { + return fmt.Errorf("custom Minio Secret %s does not have an 'secretkey' value", mattermost.Spec.Minio.Secret) + } + reqLogger.Info("skipping minio secret creation, using custom secret") + return nil +} - foundSecret := &corev1.Secret{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, foundSecret) - if err != nil { +func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { + var current *corev1.Secret + desired := mattermostMinio.Secret(mattermost) + err := r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) + switch { + case err != nil && kerrors.IsNotFound(err): + // Create new secret + reqLogger.Info("creating secret", "name", current.Name) + return r.Create(mattermost, current, reqLogger) + case err != nil: + // Something go wrong badly + reqLogger.Error(err, "failed to check if secret exists") return err } - - updatedLabels := ensureLabels(secret.Labels, foundSecret.Labels) - if !reflect.DeepEqual(updatedLabels, foundSecret.Labels) { - reqLogger.Info("Updating minio secret labels") - foundSecret.Labels = updatedLabels - return r.client.Update(context.TODO(), foundSecret) + // Validate secret required fields + if _, ok := current.Data["accesskey"]; !ok { + return fmt.Errorf("minio secret %s does not have an 'accesskey' value", desired.Name) } + if _, ok := current.Data["secretkey"]; !ok { + return fmt.Errorf("minio secret %s does not have an 'secretkey' value", desired.Name) + } + // Preserve data fields + desired.Data = current.Data + return r.Update(current, desired, reqLogger) +} - return nil +func (r *ReconcileClusterInstallation) checkMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { + if mattermost.Spec.Minio.Secret != "" { + return r.checkCustomMinioSecret(mattermost, reqLogger) + } + return r.checkMattermostMinioSecret(mattermost, reqLogger) } func (r *ReconcileClusterInstallation) checkMinioInstance(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { @@ -119,7 +123,7 @@ func (r *ReconcileClusterInstallation) createMinioInstanceIfNotExists(mattermost err := r.client.Get(context.TODO(), types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, foundInstance) if err != nil && kerrors.IsNotFound(err) { reqLogger.Info("Creating minio instance") - return r.createResource(mattermost, instance, reqLogger) + return r.Create(mattermost, instance, reqLogger) } else if err != nil { reqLogger.Error(err, "Unable to get minio instance") return err diff --git a/pkg/controller/clusterinstallation/mysql.go b/pkg/controller/clusterinstallation/mysql.go index 4aff2423c..83cf793d5 100644 --- a/pkg/controller/clusterinstallation/mysql.go +++ b/pkg/controller/clusterinstallation/mysql.go @@ -58,7 +58,7 @@ func (r *ReconcileClusterInstallation) createMySQLClusterIfNotExists(mattermost err := r.client.Get(context.TODO(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, foundCluster) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating mysql cluster") - return r.createResource(mattermost, cluster, reqLogger) + return r.Create(mattermost, cluster, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if mysql cluster exists") return err @@ -86,7 +86,7 @@ func (r *ReconcileClusterInstallation) getOrCreateMySQLSecrets(mattermost *matte "DATABASE": []byte("mattermost"), } - return userPassword, r.createResource(mattermost, dbSecret, reqLogger) + return userPassword, r.Create(mattermost, dbSecret, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if mysql secret exists") return "", err From 63c6a6e61e5a9691026eb1220ff6b0ac04345900 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Wed, 28 Aug 2019 00:50:26 +0200 Subject: [PATCH 02/18] revert log removal + typo fix --- pkg/controller/clusterinstallation/create_resources.go | 4 ++-- pkg/controller/clusterinstallation/mattermost.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 493b8d492..8eb751125 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -7,7 +7,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - v1beta1 "k8s.io/api/extensions/v1beta1" + "k8s.io/api/extensions/v1beta1" rbacv1beta1 "k8s.io/api/rbac/v1beta1" "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -60,7 +60,7 @@ func (r *ReconcileClusterInstallation) createServiceAccountIfNotExists(owner v1. foundServiceAccount := &corev1.ServiceAccount{} err := r.client.Get(context.TODO(), types.NamespacedName{Name: serviceAccount.Name, Namespace: serviceAccount.Namespace}, foundServiceAccount) - if err != nil && kerrors.IsNotFound(err) { + if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating service account", "name", serviceAccount.Name) return r.Create(owner, serviceAccount, reqLogger) } else if err != nil { diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index d552f956b..145e02081 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -289,5 +289,6 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mattermost *ma } } } + reqLogger.Info("Upgrade image job ran successfully") return r.Update(current, desired, reqLogger) } From 5e8cafcefa51f96972d8cd05794e6d809ab77e09 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Wed, 4 Sep 2019 22:14:18 +0200 Subject: [PATCH 03/18] unexport create --- .../clusterinstallation/create_resources.go | 16 ++++++++-------- pkg/controller/clusterinstallation/minio.go | 4 ++-- pkg/controller/clusterinstallation/mysql.go | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 8eb751125..8fb8c5557 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -22,8 +22,8 @@ type Object interface { v1.Object } -// Create creates the provided resource and sets the owner -func (r *ReconcileClusterInstallation) Create(owner v1.Object, desired Object, reqLogger logr.Logger) error { +// create creates the provided resource and sets the owner +func (r *ReconcileClusterInstallation) create(owner v1.Object, desired Object, reqLogger logr.Logger) error { // adding the last applied annotation to use the object matcher later // see: https://github.com/banzaicloud/k8s-objectmatcher err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) @@ -62,7 +62,7 @@ func (r *ReconcileClusterInstallation) createServiceAccountIfNotExists(owner v1. err := r.client.Get(context.TODO(), types.NamespacedName{Name: serviceAccount.Name, Namespace: serviceAccount.Namespace}, foundServiceAccount) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating service account", "name", serviceAccount.Name) - return r.Create(owner, serviceAccount, reqLogger) + return r.create(owner, serviceAccount, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if service account exists") return err @@ -76,7 +76,7 @@ func (r *ReconcileClusterInstallation) createRoleBindingIfNotExists(owner v1.Obj err := r.client.Get(context.TODO(), types.NamespacedName{Name: roleBinding.Name, Namespace: roleBinding.Namespace}, foundRoleBinding) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating role binding", "name", roleBinding.Name) - return r.Create(owner, roleBinding, reqLogger) + return r.create(owner, roleBinding, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if role binding exists") return err @@ -90,7 +90,7 @@ func (r *ReconcileClusterInstallation) createServiceIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: service.Name, Namespace: service.Namespace}, foundService) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating service", "name", service.Name) - return r.Create(owner, service, reqLogger) + return r.create(owner, service, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if service exists") return err @@ -104,7 +104,7 @@ func (r *ReconcileClusterInstallation) createIngressIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: ingress.Name, Namespace: ingress.Namespace}, foundIngress) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating ingress", "name", ingress.Name) - return r.Create(owner, ingress, reqLogger) + return r.create(owner, ingress, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if ingress exists") return err @@ -118,7 +118,7 @@ func (r *ReconcileClusterInstallation) createDeploymentIfNotExists(owner v1.Obje err := r.client.Get(context.TODO(), types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, foundDeployment) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating deployment", "name", deployment.Name) - return r.Create(owner, deployment, reqLogger) + return r.create(owner, deployment, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if deployment exists") return err @@ -132,7 +132,7 @@ func (r *ReconcileClusterInstallation) createSecretIfNotExists(owner v1.Object, err := r.client.Get(context.TODO(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, foundSecret) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating secret", "name", secret.Name) - return r.Create(owner, secret, reqLogger) + return r.create(owner, secret, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if secret exists") return err diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index edc9af20e..0c561d8f9 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -53,7 +53,7 @@ func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *ma case err != nil && kerrors.IsNotFound(err): // Create new secret reqLogger.Info("creating secret", "name", current.Name) - return r.Create(mattermost, current, reqLogger) + return r.create(mattermost, current, reqLogger) case err != nil: // Something go wrong badly reqLogger.Error(err, "failed to check if secret exists") @@ -123,7 +123,7 @@ func (r *ReconcileClusterInstallation) createMinioInstanceIfNotExists(mattermost err := r.client.Get(context.TODO(), types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, foundInstance) if err != nil && kerrors.IsNotFound(err) { reqLogger.Info("Creating minio instance") - return r.Create(mattermost, instance, reqLogger) + return r.create(mattermost, instance, reqLogger) } else if err != nil { reqLogger.Error(err, "Unable to get minio instance") return err diff --git a/pkg/controller/clusterinstallation/mysql.go b/pkg/controller/clusterinstallation/mysql.go index 83cf793d5..107f90da1 100644 --- a/pkg/controller/clusterinstallation/mysql.go +++ b/pkg/controller/clusterinstallation/mysql.go @@ -58,7 +58,7 @@ func (r *ReconcileClusterInstallation) createMySQLClusterIfNotExists(mattermost err := r.client.Get(context.TODO(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, foundCluster) if err != nil && errors.IsNotFound(err) { reqLogger.Info("Creating mysql cluster") - return r.Create(mattermost, cluster, reqLogger) + return r.create(mattermost, cluster, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if mysql cluster exists") return err @@ -86,7 +86,7 @@ func (r *ReconcileClusterInstallation) getOrCreateMySQLSecrets(mattermost *matte "DATABASE": []byte("mattermost"), } - return userPassword, r.Create(mattermost, dbSecret, reqLogger) + return userPassword, r.create(mattermost, dbSecret, reqLogger) } else if err != nil { reqLogger.Error(err, "Failed to check if mysql secret exists") return "", err From d9d2c8ef4e48dd666f6077c40978d5722162cf1a Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Thu, 5 Sep 2019 00:10:43 +0200 Subject: [PATCH 04/18] unexport crate/update + fixing input for Get + override secret if have invalid data --- .../clusterinstallation/create_resources.go | 2 +- pkg/controller/clusterinstallation/mattermost.go | 2 +- pkg/controller/clusterinstallation/minio.go | 14 ++++++++------ 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 8fb8c5557..101a69690 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -39,7 +39,7 @@ func (r *ReconcileClusterInstallation) create(owner v1.Object, desired Object, r return controllerutil.SetControllerReference(owner, desired, r.scheme) } -func (r *ReconcileClusterInstallation) Update(current, desired Object, reqLogger logr.Logger) error { +func (r *ReconcileClusterInstallation) update(current, desired Object, reqLogger logr.Logger) error { patchResult, err := objectMatcher.DefaultPatchMaker.Calculate(current, desired) if err != nil { reqLogger.Error(err, "error checking the difference in the deployment") diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index 145e02081..e98b9d7ef 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -290,5 +290,5 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mattermost *ma } } reqLogger.Info("Upgrade image job ran successfully") - return r.Update(current, desired, reqLogger) + return r.update(current, desired, reqLogger) } diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index 0c561d8f9..cca953dd3 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -29,7 +29,7 @@ func (r *ReconcileClusterInstallation) checkMinio(mattermost *mattermostv1alpha1 } func (r *ReconcileClusterInstallation) checkCustomMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { - var secret *corev1.Secret + secret := &corev1.Secret{} err := r.client.Get(context.TODO(), types.NamespacedName{Name: mattermost.Spec.Minio.Secret, Namespace: mattermost.Namespace}, secret) if err != nil { return errors.Wrap(err, "unable to locate custom minio secret") @@ -46,7 +46,7 @@ func (r *ReconcileClusterInstallation) checkCustomMinioSecret(mattermost *matter } func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { - var current *corev1.Secret + current := &corev1.Secret{} desired := mattermostMinio.Secret(mattermost) err := r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) switch { @@ -59,16 +59,18 @@ func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *ma reqLogger.Error(err, "failed to check if secret exists") return err } - // Validate secret required fields + // Validate secret required fields, if not exist recreate. if _, ok := current.Data["accesskey"]; !ok { - return fmt.Errorf("minio secret %s does not have an 'accesskey' value", desired.Name) + reqLogger.Info("minio secret does not have an 'accesskey' value, overriding", "secret", desired.Name) + return r.update(current, desired, reqLogger) } if _, ok := current.Data["secretkey"]; !ok { - return fmt.Errorf("minio secret %s does not have an 'secretkey' value", desired.Name) + reqLogger.Info("minio secret does not have an 'secretkey' value, overriding", "secret", desired.Name) + return r.update(current, desired, reqLogger) } // Preserve data fields desired.Data = current.Data - return r.Update(current, desired, reqLogger) + return r.update(current, desired, reqLogger) } func (r *ReconcileClusterInstallation) checkMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { From e567d26c50b4dd1d1c07f366b668c3b68c8e6ab7 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 10:21:05 +0200 Subject: [PATCH 05/18] minio secret fix create --- pkg/controller/clusterinstallation/minio.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index cca953dd3..7507022e4 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -52,8 +52,8 @@ func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *ma switch { case err != nil && kerrors.IsNotFound(err): // Create new secret - reqLogger.Info("creating secret", "name", current.Name) - return r.create(mattermost, current, reqLogger) + reqLogger.Info("creating minio secret", "name", desired.Name, "namespace", desired.Namespace) + return r.create(mattermost, desired, reqLogger) case err != nil: // Something go wrong badly reqLogger.Error(err, "failed to check if secret exists") @@ -61,11 +61,11 @@ func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *ma } // Validate secret required fields, if not exist recreate. if _, ok := current.Data["accesskey"]; !ok { - reqLogger.Info("minio secret does not have an 'accesskey' value, overriding", "secret", desired.Name) + reqLogger.Info("minio secret does not have an 'accesskey' value, overriding", "name", desired.Name) return r.update(current, desired, reqLogger) } if _, ok := current.Data["secretkey"]; !ok { - reqLogger.Info("minio secret does not have an 'secretkey' value, overriding", "secret", desired.Name) + reqLogger.Info("minio secret does not have an 'secretkey' value, overriding", "name", desired.Name) return r.update(current, desired, reqLogger) } // Preserve data fields From 2c3790a29c83fbdfae9395db14676ad10cf5bdb6 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 10:23:26 +0200 Subject: [PATCH 06/18] update mysql cluster with objectmatcher --- pkg/controller/clusterinstallation/mysql.go | 30 ++++----------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/pkg/controller/clusterinstallation/mysql.go b/pkg/controller/clusterinstallation/mysql.go index 107f90da1..c3deefec9 100644 --- a/pkg/controller/clusterinstallation/mysql.go +++ b/pkg/controller/clusterinstallation/mysql.go @@ -3,7 +3,6 @@ package clusterinstallation import ( "context" "fmt" - "reflect" "github.com/go-logr/logr" mysqlOperator "github.com/presslabs/mysql-operator/pkg/apis/mysql/v1alpha1" @@ -18,39 +17,20 @@ import ( func (r *ReconcileClusterInstallation) checkMySQLCluster(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { reqLogger = reqLogger.WithValues("Reconcile", "mysql") - cluster := mattermostmysql.Cluster(mattermost) + desired := mattermostmysql.Cluster(mattermost) - err := r.createMySQLClusterIfNotExists(mattermost, cluster, reqLogger) + err := r.createMySQLClusterIfNotExists(mattermost, desired, reqLogger) if err != nil { return err } - foundCluster := &mysqlOperator.MysqlCluster{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, foundCluster) + current := &mysqlOperator.MysqlCluster{} + err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) if err != nil { return err } - var update bool - - updatedLabels := ensureLabels(cluster.Labels, foundCluster.Labels) - if !reflect.DeepEqual(updatedLabels, foundCluster.Labels) { - reqLogger.Info("Updating mysql cluster labels") - foundCluster.Labels = updatedLabels - update = true - } - - if !reflect.DeepEqual(cluster.Spec, foundCluster.Spec) { - reqLogger.Info("Updating mysql cluster spec") - foundCluster.Spec = cluster.Spec - update = true - } - - if update { - return r.client.Update(context.TODO(), foundCluster) - } - - return nil + return r.update(current, desired, reqLogger) } func (r *ReconcileClusterInstallation) createMySQLClusterIfNotExists(mattermost *mattermostv1alpha1.ClusterInstallation, cluster *mysqlOperator.MysqlCluster, reqLogger logr.Logger) error { From ac77c2d4b443f5e23571249f691b7cc9e0cc7d35 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 11:03:41 +0200 Subject: [PATCH 07/18] log generic update message + clean unused --- .../clusterinstallation/create_resources.go | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 101a69690..08ea5b4ba 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -42,15 +42,16 @@ func (r *ReconcileClusterInstallation) create(owner v1.Object, desired Object, r func (r *ReconcileClusterInstallation) update(current, desired Object, reqLogger logr.Logger) error { patchResult, err := objectMatcher.DefaultPatchMaker.Calculate(current, desired) if err != nil { - reqLogger.Error(err, "error checking the difference in the deployment") + reqLogger.Error(err, "error checking the difference in the resource") return err } if !patchResult.IsEmpty() { err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) if err != nil { - reqLogger.Error(err, "error applying the annotation in the deployment") + reqLogger.Error(err, "error applying the annotation in the resource") return err } + reqLogger.Info("updating resource", "name", desired.GetName(), "namespace", desired.GetNamespace()) return r.client.Update(context.TODO(), desired) } return nil @@ -126,17 +127,3 @@ func (r *ReconcileClusterInstallation) createDeploymentIfNotExists(owner v1.Obje return nil } - -func (r *ReconcileClusterInstallation) createSecretIfNotExists(owner v1.Object, secret *corev1.Secret, reqLogger logr.Logger) error { - foundSecret := &corev1.Secret{} - err := r.client.Get(context.TODO(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, foundSecret) - if err != nil && errors.IsNotFound(err) { - reqLogger.Info("Creating secret", "name", secret.Name) - return r.create(owner, secret, reqLogger) - } else if err != nil { - reqLogger.Error(err, "Failed to check if secret exists") - return err - } - - return nil -} From ffbdcca065a8328cea6cbf96eaaa78fd22e196a3 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 11:07:25 +0200 Subject: [PATCH 08/18] mattermost: update resources with objectmatcher --- .../clusterinstallation/mattermost.go | 86 ++++--------------- 1 file changed, 18 insertions(+), 68 deletions(-) diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index e98b9d7ef..9aaf1063d 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -3,7 +3,6 @@ package clusterinstallation import ( "context" "fmt" - "reflect" "time" "github.com/go-logr/logr" @@ -45,102 +44,53 @@ func (r *ReconcileClusterInstallation) checkMattermost(mattermost *mattermostv1a } func (r *ReconcileClusterInstallation) checkMattermostService(mattermost *mattermostv1alpha1.ClusterInstallation, resourceName, selectorName string, reqLogger logr.Logger) error { - service := mattermost.GenerateService(resourceName, selectorName) + desired := mattermost.GenerateService(resourceName, selectorName) - err := r.createServiceIfNotExists(mattermost, service, reqLogger) + err := r.createServiceIfNotExists(mattermost, desired, reqLogger) if err != nil { return err } - foundService := &corev1.Service{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: service.Name, Namespace: service.Namespace}, foundService) + current := &corev1.Service{} + err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) if err != nil { return err } - var update bool - - updatedLabels := ensureLabels(service.Labels, foundService.Labels) - if !reflect.DeepEqual(updatedLabels, foundService.Labels) { - reqLogger.Info("Updating mattermost service labels") - foundService.Labels = updatedLabels - update = true - } - - if !reflect.DeepEqual(service.Annotations, foundService.Annotations) { - reqLogger.Info("Updating mattermost service annotations") - foundService.Annotations = service.Annotations - update = true - } - // If we are using the loadBalancer the ClusterIp is immutable // and other fields are created in the first time if mattermost.Spec.UseServiceLoadBalancer { - service.Spec.ClusterIP = foundService.Spec.ClusterIP - service.Spec.ExternalTrafficPolicy = foundService.Spec.ExternalTrafficPolicy - service.Spec.SessionAffinity = foundService.Spec.SessionAffinity - for _, foundPort := range foundService.Spec.Ports { - for i, servicePort := range service.Spec.Ports { - if foundPort.Name == servicePort.Name { - service.Spec.Ports[i].NodePort = foundPort.NodePort - service.Spec.Ports[i].Protocol = foundPort.Protocol + desired.Spec.ClusterIP = current.Spec.ClusterIP + desired.Spec.ExternalTrafficPolicy = current.Spec.ExternalTrafficPolicy + desired.Spec.SessionAffinity = current.Spec.SessionAffinity + for _, currentPort := range current.Spec.Ports { + for i, servicePort := range desired.Spec.Ports { + if currentPort.Name == servicePort.Name { + desired.Spec.Ports[i].NodePort = currentPort.NodePort + desired.Spec.Ports[i].Protocol = currentPort.Protocol } } } } - if !reflect.DeepEqual(service.Spec, foundService.Spec) { - reqLogger.Info("Updating mattermost service spec") - foundService.Spec = service.Spec - update = true - } - if update { - return r.client.Update(context.TODO(), foundService) - } - - return nil + return r.update(current, desired, reqLogger) } func (r *ReconcileClusterInstallation) checkMattermostIngress(mattermost *mattermostv1alpha1.ClusterInstallation, resourceName, ingressName string, reqLogger logr.Logger) error { - ingress := mattermost.GenerateIngress(resourceName, ingressName) + desired := mattermost.GenerateIngress(resourceName, ingressName) - err := r.createIngressIfNotExists(mattermost, ingress, reqLogger) + err := r.createIngressIfNotExists(mattermost, desired, reqLogger) if err != nil { return err } - foundIngress := &v1beta1.Ingress{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: ingress.Name, Namespace: ingress.Namespace}, foundIngress) + current := &v1beta1.Ingress{} + err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) if err != nil { return err } - var update bool - - updatedLabels := ensureLabels(ingress.Labels, foundIngress.Labels) - if !reflect.DeepEqual(updatedLabels, foundIngress.Labels) { - reqLogger.Info("Updating mattermost ingress labels") - foundIngress.Labels = updatedLabels - update = true - } - - if !reflect.DeepEqual(ingress.Annotations, foundIngress.Annotations) { - reqLogger.Info("Updating mattermost ingress annotations") - foundIngress.Annotations = ingress.Annotations - update = true - } - - if !reflect.DeepEqual(ingress.Spec, foundIngress.Spec) { - reqLogger.Info("Updating mattermost ingress spec") - foundIngress.Spec = ingress.Spec - update = true - } - - if update { - return r.client.Update(context.TODO(), foundIngress) - } - - return nil + return r.update(current, desired, reqLogger) } func (r *ReconcileClusterInstallation) checkMattermostDeployment(mattermost *mattermostv1alpha1.ClusterInstallation, resourceName, ingressName, imageName string, reqLogger logr.Logger) error { From 59ac13203983e67af3d890655212f80d8476fdcf Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 11:08:12 +0200 Subject: [PATCH 09/18] minio/instance: update with objectmatcher --- pkg/controller/clusterinstallation/minio.go | 30 ++++----------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index 7507022e4..89c1874c2 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -3,7 +3,6 @@ package clusterinstallation import ( "context" "fmt" - "reflect" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -81,43 +80,24 @@ func (r *ReconcileClusterInstallation) checkMinioSecret(mattermost *mattermostv1 } func (r *ReconcileClusterInstallation) checkMinioInstance(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { - instance := mattermostMinio.Instance(mattermost) + desired := mattermostMinio.Instance(mattermost) - err := r.createMinioInstanceIfNotExists(mattermost, instance, reqLogger) + err := r.createMinioInstanceIfNotExists(mattermost, desired, reqLogger) if err != nil { return err } - foundInstance := &minioOperator.MinIOInstance{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, foundInstance) + current := &minioOperator.MinIOInstance{} + err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) if err != nil { return err } - var update bool - // Note: // For some reason, our current minio operator seems to remove labels on // the instance resource when we add them. For that reason, trying to // ensure the labels are correct doesn't work. - updatedLabels := ensureLabels(instance.Labels, foundInstance.Labels) - if !reflect.DeepEqual(updatedLabels, foundInstance.Labels) { - reqLogger.Info("Updating minio instance labels") - foundInstance.Labels = updatedLabels - update = true - } - - if !reflect.DeepEqual(instance.Spec, foundInstance.Spec) { - reqLogger.Info("Updating minio instance spec") - foundInstance.Spec = instance.Spec - update = true - } - - if update { - return r.client.Update(context.TODO(), foundInstance) - } - - return nil + return r.update(current, desired, reqLogger) } func (r *ReconcileClusterInstallation) createMinioInstanceIfNotExists(mattermost *mattermostv1alpha1.ClusterInstallation, instance *minioOperator.MinIOInstance, reqLogger logr.Logger) error { From 783ff835513028213140dd9f8c5b03e7143c99af Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 11:57:16 +0200 Subject: [PATCH 10/18] add resource version update --- pkg/controller/clusterinstallation/create_resources.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 08ea5b4ba..1c1f65c17 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -52,6 +52,9 @@ func (r *ReconcileClusterInstallation) update(current, desired Object, reqLogger return err } reqLogger.Info("updating resource", "name", desired.GetName(), "namespace", desired.GetNamespace()) + // Resource version is required for the update, but need to be set after + // the last applied annotation to avoid unnecessary diffs + desired.SetResourceVersion(current.GetResourceVersion()) return r.client.Update(context.TODO(), desired) } return nil From 988f3dbb12aa592e9e5d0b07159d4b729cef2cfd Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 12:04:25 +0200 Subject: [PATCH 11/18] naming unification --- pkg/controller/clusterinstallation/mattermost.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index 9aaf1063d..98c3800b6 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -124,20 +124,20 @@ func (r *ReconcileClusterInstallation) checkMattermostDeployment(mattermost *mat isLicensed = true } - deployment := mattermost.GenerateDeployment(resourceName, ingressName, imageName, dbUser, dbPassword, externalDB, isLicensed, minioService) - err = r.createDeploymentIfNotExists(mattermost, deployment, reqLogger) + desired := mattermost.GenerateDeployment(resourceName, ingressName, imageName, dbUser, dbPassword, externalDB, isLicensed, minioService) + err = r.createDeploymentIfNotExists(mattermost, desired, reqLogger) if err != nil { return err } - foundDeployment := &appsv1.Deployment{} - err = r.client.Get(context.TODO(), types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, foundDeployment) + current := &appsv1.Deployment{} + err = r.client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current) if err != nil { reqLogger.Error(err, "Failed to get mattermost deployment") return err } - err = r.updateMattermostDeployment(mattermost, foundDeployment, deployment, imageName, reqLogger) + err = r.updateMattermostDeployment(mattermost, current, desired, imageName, reqLogger) if err != nil { reqLogger.Error(err, "Failed to update mattermost deployment") return err From 878712e406a3ec8d25dec9943cf022445ba3b9f6 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 12:19:42 +0200 Subject: [PATCH 12/18] minio: move logging --- pkg/controller/clusterinstallation/minio.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/clusterinstallation/minio.go b/pkg/controller/clusterinstallation/minio.go index 89c1874c2..65d2796c2 100644 --- a/pkg/controller/clusterinstallation/minio.go +++ b/pkg/controller/clusterinstallation/minio.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/go-logr/logr" - "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -31,7 +30,8 @@ func (r *ReconcileClusterInstallation) checkCustomMinioSecret(mattermost *matter secret := &corev1.Secret{} err := r.client.Get(context.TODO(), types.NamespacedName{Name: mattermost.Spec.Minio.Secret, Namespace: mattermost.Namespace}, secret) if err != nil { - return errors.Wrap(err, "unable to locate custom minio secret") + reqLogger.Error(err, "failed to check if custom minio secret exists") + return err } // Validate custom secret required fields if _, ok := secret.Data["accesskey"]; !ok { @@ -40,7 +40,6 @@ func (r *ReconcileClusterInstallation) checkCustomMinioSecret(mattermost *matter if _, ok := secret.Data["secretkey"]; !ok { return fmt.Errorf("custom Minio Secret %s does not have an 'secretkey' value", mattermost.Spec.Minio.Secret) } - reqLogger.Info("skipping minio secret creation, using custom secret") return nil } @@ -74,6 +73,7 @@ func (r *ReconcileClusterInstallation) checkMattermostMinioSecret(mattermost *ma func (r *ReconcileClusterInstallation) checkMinioSecret(mattermost *mattermostv1alpha1.ClusterInstallation, reqLogger logr.Logger) error { if mattermost.Spec.Minio.Secret != "" { + reqLogger.Info("skipping minio secret creation, using custom secret") return r.checkCustomMinioSecret(mattermost, reqLogger) } return r.checkMattermostMinioSecret(mattermost, reqLogger) From caa17af7fcfce56a360d709d427bc30b9c9ecba5 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sat, 7 Sep 2019 13:36:45 +0200 Subject: [PATCH 13/18] move logging into proper scope for upgrade job --- pkg/controller/clusterinstallation/mattermost.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index 98c3800b6..3d171a6c2 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -238,7 +238,7 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mattermost *ma time.Sleep(1 * time.Second) } } + reqLogger.Info("Upgrade image job ran successfully") } - reqLogger.Info("Upgrade image job ran successfully") return r.update(current, desired, reqLogger) } From 91cb5ed9fb636c848b1ea8129ddf8dcf038e5a32 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sun, 8 Sep 2019 12:53:40 +0200 Subject: [PATCH 14/18] custom objectMatcher annotation --- .../clusterinstallation/create_resources.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/controller/clusterinstallation/create_resources.go b/pkg/controller/clusterinstallation/create_resources.go index 1c1f65c17..ead1f3c37 100644 --- a/pkg/controller/clusterinstallation/create_resources.go +++ b/pkg/controller/clusterinstallation/create_resources.go @@ -16,6 +16,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) +const LastAppliedConfig = "mattermost.com/last-applied" + +var DefaultAnnotator = objectMatcher.NewAnnotator(LastAppliedConfig) + // Object combines the interfaces that all Kubernetes objects must implement. type Object interface { runtime.Object @@ -26,7 +30,7 @@ type Object interface { func (r *ReconcileClusterInstallation) create(owner v1.Object, desired Object, reqLogger logr.Logger) error { // adding the last applied annotation to use the object matcher later // see: https://github.com/banzaicloud/k8s-objectmatcher - err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) + err := DefaultAnnotator.SetLastAppliedAnnotation(desired) if err != nil { reqLogger.Error(err, "Error applying the annotation in the resource") return err @@ -40,14 +44,13 @@ func (r *ReconcileClusterInstallation) create(owner v1.Object, desired Object, r } func (r *ReconcileClusterInstallation) update(current, desired Object, reqLogger logr.Logger) error { - patchResult, err := objectMatcher.DefaultPatchMaker.Calculate(current, desired) + patchResult, err := objectMatcher.NewPatchMaker(DefaultAnnotator).Calculate(current, desired) if err != nil { reqLogger.Error(err, "error checking the difference in the resource") return err } if !patchResult.IsEmpty() { - err := objectMatcher.DefaultAnnotator.SetLastAppliedAnnotation(desired) - if err != nil { + if err := DefaultAnnotator.SetLastAppliedAnnotation(desired); err != nil { reqLogger.Error(err, "error applying the annotation in the resource") return err } From d1f0809714ae9ebfc4879d44782be292358f7014 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sun, 8 Sep 2019 12:53:50 +0200 Subject: [PATCH 15/18] Reverting to simple spec and labels update for mysql operator. There is race interaction between mysql operator and mattermost operator in case of applying patch to whole resource. We should only update here what is expected to change otherwise it wrecks mysql operator. Observed behaviour was for example: - mysql operator stops responding to any changes in resource, - database remains in read only mode. --- pkg/controller/clusterinstallation/mysql.go | 22 ++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/controller/clusterinstallation/mysql.go b/pkg/controller/clusterinstallation/mysql.go index c3deefec9..970f5e052 100644 --- a/pkg/controller/clusterinstallation/mysql.go +++ b/pkg/controller/clusterinstallation/mysql.go @@ -3,6 +3,7 @@ package clusterinstallation import ( "context" "fmt" + "reflect" "github.com/go-logr/logr" mysqlOperator "github.com/presslabs/mysql-operator/pkg/apis/mysql/v1alpha1" @@ -30,7 +31,26 @@ func (r *ReconcileClusterInstallation) checkMySQLCluster(mattermost *mattermostv return err } - return r.update(current, desired, reqLogger) + var update bool + + updatedLabels := ensureLabels(desired.Labels, current.Labels) + if !reflect.DeepEqual(updatedLabels, current.Labels) { + reqLogger.Info("Updating mysql cluster labels") + current.Labels = updatedLabels + update = true + } + + if !reflect.DeepEqual(desired.Spec, current.Spec) { + reqLogger.Info("Updating mysql cluster spec") + current.Spec = desired.Spec + update = true + } + + if update { + return r.client.Update(context.TODO(), current) + } + + return nil } func (r *ReconcileClusterInstallation) createMySQLClusterIfNotExists(mattermost *mattermostv1alpha1.ClusterInstallation, cluster *mysqlOperator.MysqlCluster, reqLogger logr.Logger) error { From cab74e9fef3941319beda907c275332bf00d39ee Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sun, 8 Sep 2019 13:01:44 +0200 Subject: [PATCH 16/18] add comment on mysql operator --- pkg/controller/clusterinstallation/mysql.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller/clusterinstallation/mysql.go b/pkg/controller/clusterinstallation/mysql.go index 970f5e052..fcd580501 100644 --- a/pkg/controller/clusterinstallation/mysql.go +++ b/pkg/controller/clusterinstallation/mysql.go @@ -31,6 +31,8 @@ func (r *ReconcileClusterInstallation) checkMySQLCluster(mattermost *mattermostv return err } + // Updating mysql.cluster with objectMatcher breaks mysql operator. + // Only fields that are expected to be changed by mattermost-operator should be included here. var update bool updatedLabels := ensureLabels(desired.Labels, current.Labels) From c174ab4bb64efde961d65260bb72b70f35f499f4 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Sun, 8 Sep 2019 13:06:40 +0200 Subject: [PATCH 17/18] generated fields should be preserved --- pkg/controller/clusterinstallation/mattermost.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index 3d171a6c2..0cf548c28 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -57,22 +57,6 @@ func (r *ReconcileClusterInstallation) checkMattermostService(mattermost *matter return err } - // If we are using the loadBalancer the ClusterIp is immutable - // and other fields are created in the first time - if mattermost.Spec.UseServiceLoadBalancer { - desired.Spec.ClusterIP = current.Spec.ClusterIP - desired.Spec.ExternalTrafficPolicy = current.Spec.ExternalTrafficPolicy - desired.Spec.SessionAffinity = current.Spec.SessionAffinity - for _, currentPort := range current.Spec.Ports { - for i, servicePort := range desired.Spec.Ports { - if currentPort.Name == servicePort.Name { - desired.Spec.Ports[i].NodePort = currentPort.NodePort - desired.Spec.Ports[i].Protocol = currentPort.Protocol - } - } - } - } - return r.update(current, desired, reqLogger) } From bcb4c908182751c4fd99c1bb3c10b38dff1bdb90 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Thu, 19 Sep 2019 23:04:07 +0200 Subject: [PATCH 18/18] remove empty line - merge artifact --- pkg/controller/clusterinstallation/mattermost.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller/clusterinstallation/mattermost.go b/pkg/controller/clusterinstallation/mattermost.go index b7876d556..da0469011 100644 --- a/pkg/controller/clusterinstallation/mattermost.go +++ b/pkg/controller/clusterinstallation/mattermost.go @@ -189,7 +189,6 @@ func (r *ReconcileClusterInstallation) updateMattermostDeployment(mattermost *ma // we will return and not upgrade the deployment. if update { reqLogger.Info(fmt.Sprintf("Running Mattermost image %s upgrade job check", imageName)) - alreadyRunning, err := r.fetchRunningUpdateJob(mattermost, reqLogger) if err != nil && k8sErrors.IsNotFound(err) { reqLogger.Info("Launching update job")