diff --git a/api/v1/coherenceresourcespec_types.go b/api/v1/coherenceresourcespec_types.go index ae38d014..2686dacc 100644 --- a/api/v1/coherenceresourcespec_types.go +++ b/api/v1/coherenceresourcespec_types.go @@ -823,7 +823,7 @@ func (in *CoherenceResourceSpec) GetImagePullSecrets() []corev1.LocalObjectRefer // GetServiceAccountName returns the service account name for the cluster. func (in *CoherenceResourceSpec) GetServiceAccountName() string { - if in != nil && in.ServiceAccountName != DefaultServiceAccount { + if in != nil { return in.ServiceAccountName } return "" diff --git a/api/v1/create_job_test.go b/api/v1/create_job_test.go index 40ff19ed..4fbd332d 100644 --- a/api/v1/create_job_test.go +++ b/api/v1/create_job_test.go @@ -129,12 +129,12 @@ func TestCreateJobWithEnvVarsFrom(t *testing.T) { // Create the test deployment deployment := createTestCoherenceJobDeployment(spec) - // Create expected StatefulSet + // Create expected Job expected := createMinimalExpectedJob(deployment) addEnvVarsFromToJob(expected, coh.ContainerNameCoherence, from...) - // assert that the StatefulSet is as expected + // assert that the Job is as expected assertJobCreation(t, deployment, expected) } @@ -162,10 +162,53 @@ func TestAddLifecycleToJobCoherenceContainer(t *testing.T) { // Create the test deployment deployment := createTestCoherenceJobDeployment(spec) - // Create expected StatefulSet + // Create expected Job expected := createMinimalExpectedJob(deployment) expected.Spec.Template.Spec.Containers[0].Lifecycle = lc - // assert that the StatefulSet is as expected + // assert that the Job is as expected assertJobCreation(t, deployment, expected) } + +func TestCreateJobWithServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{ + ServiceAccountName: "Foo", + } + + // Create the test deployment + deployment := createTestCoherenceJob(spec) + // Create expected Job + jobExpected := createMinimalExpectedJob(deployment) + jobExpected.Spec.Template.Spec.ServiceAccountName = "Foo" + + // assert that the Job is as expected + assertJobCreation(t, deployment, jobExpected) +} + +func TestCreateJobWithDefaultServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{ + ServiceAccountName: "default", + } + + // Create the test deployment + deployment := createTestCoherenceJob(spec) + // Create expected Job + jobExpected := createMinimalExpectedJob(deployment) + jobExpected.Spec.Template.Spec.ServiceAccountName = "default" + + // assert that the Job is as expected + assertJobCreation(t, deployment, jobExpected) +} + +func TestCreateJobWithoutServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{} + + // Create the test deployment + deployment := createTestCoherenceJob(spec) + // Create expected Job + jobExpected := createMinimalExpectedJob(deployment) + jobExpected.Spec.Template.Spec.ServiceAccountName = "" + + // assert that the Job is as expected + assertJobCreation(t, deployment, jobExpected) +} diff --git a/api/v1/create_statefulset_test.go b/api/v1/create_statefulset_test.go index d9bc53ac..579d26ca 100644 --- a/api/v1/create_statefulset_test.go +++ b/api/v1/create_statefulset_test.go @@ -827,3 +827,46 @@ func TestCreateStatefulSetWithMinReadySecondsSetToZero(t *testing.T) { // assert that the StatefulSet is as expected assertStatefulSetCreation(t, deployment, stsExpected) } + +func TestCreateStatefulSetWithServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{ + ServiceAccountName: "Foo", + } + + // Create the test deployment + deployment := createTestDeployment(spec) + // Create expected StatefulSet + stsExpected := createMinimalExpectedStatefulSet(deployment) + stsExpected.Spec.Template.Spec.ServiceAccountName = "Foo" + + // assert that the StatefulSet is as expected + assertStatefulSetCreation(t, deployment, stsExpected) +} + +func TestCreateStatefulSetWithDefaultServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{ + ServiceAccountName: "default", + } + + // Create the test deployment + deployment := createTestDeployment(spec) + // Create expected StatefulSet + stsExpected := createMinimalExpectedStatefulSet(deployment) + stsExpected.Spec.Template.Spec.ServiceAccountName = "default" + + // assert that the StatefulSet is as expected + assertStatefulSetCreation(t, deployment, stsExpected) +} + +func TestCreateStatefulSetWithoutServiceAccount(t *testing.T) { + spec := coh.CoherenceResourceSpec{} + + // Create the test deployment + deployment := createTestDeployment(spec) + // Create expected StatefulSet + stsExpected := createMinimalExpectedStatefulSet(deployment) + stsExpected.Spec.Template.Spec.ServiceAccountName = "" + + // assert that the StatefulSet is as expected + assertStatefulSetCreation(t, deployment, stsExpected) +} diff --git a/controllers/statefulset/statefulset_controller.go b/controllers/statefulset/statefulset_controller.go index 8094bdf7..82641098 100644 --- a/controllers/statefulset/statefulset_controller.go +++ b/controllers/statefulset/statefulset_controller.go @@ -44,6 +44,8 @@ const ( EventReasonScale string = "Scaling" statusHaRetryEnv = "STATUS_HA_RETRY" + + lastAppliedConfigAnnotation string = "kubectl.kubernetes.io/last-applied-configuration" ) // blank assignment to verify that ReconcileStatefulSet implements reconcile.Reconciler. @@ -478,6 +480,11 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo current.Spec.Template.Spec.Containers[0].Args = []string{} original.Spec.Template.Spec.Containers[0].Args = []string{} + // do not patch the annotation "kubectl.kubernetes.io/last-applied-configuration" + delete(desired.Annotations, lastAppliedConfigAnnotation) + delete(original.Annotations, lastAppliedConfigAnnotation) + delete(current.Annotations, lastAppliedConfigAnnotation) + desiredPodSpec := desired.Spec.Template currentPodSpec := current.Spec.Template originalPodSpec := original.Spec.Template @@ -514,6 +521,12 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo // fix the CreationTimestamp so that it is not in the patch desired.SetCreationTimestamp(current.GetCreationTimestamp()) + + sa1 := current.Spec.Template.Spec.ServiceAccountName + sa2 := original.Spec.Template.Spec.ServiceAccountName + sa3 := desired.Spec.Template.Spec.ServiceAccountName + logger.Info("**** About to create patch for StatefulSet", "CurrentSA", sa1, "OriginalSA", sa2, "DesiredSA", sa3) + // create the patch to see whether there is anything to update patch, data, err := in.CreateThreeWayPatch(current.GetName(), original, desired, current, patching.PatchIgnore) if err != nil { @@ -529,7 +542,7 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo // Check we have the expected number of ready replicas if readyReplicas != currentReplicas { logger.Info("Re-queuing update request. StatefulSet Status not all replicas are ready", "Ready", readyReplicas, "CurrentReplicas", currentReplicas) - return reconcile.Result{Requeue: true, RequeueAfter: time.Minute}, nil + return reconcile.Result{RequeueAfter: time.Minute}, nil } // perform the StatusHA check... @@ -537,7 +550,7 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo ha := checker.IsStatusHA(ctx, deployment, current) if !ha { logger.Info("Coherence cluster is not StatusHA - re-queuing update request.") - return reconcile.Result{Requeue: true, RequeueAfter: time.Minute}, nil + return reconcile.Result{RequeueAfter: time.Minute}, nil } } else { // the user specifically set a forced update! @@ -549,7 +562,7 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo suspended := in.suspendServices(ctx, deployment, current) switch suspended { case probe.ServiceSuspendFailed: - return reconcile.Result{Requeue: true, RequeueAfter: time.Minute}, fmt.Errorf("failed to suspend services prior to updating single member deployment") + return reconcile.Result{RequeueAfter: time.Minute}, fmt.Errorf("failed to suspend services prior to updating single member deployment") case probe.ServiceSuspendSkipped: logger.Info("Skipping suspension of Coherence services in single member deployment " + deployment.GetName() + " prior to update StatefulSet") @@ -566,7 +579,7 @@ func (in *ReconcileStatefulSet) maybePatchStatefulSet(ctx context.Context, deplo logger.Info("Error patching StatefulSet " + err.Error()) return in.HandleErrAndRequeue(ctx, err, deployment, fmt.Sprintf(FailedToPatchMessage, deployment.GetName(), err.Error()), logger) case !patched: - return reconcile.Result{Requeue: true}, nil + return reconcile.Result{RequeueAfter: time.Minute}, nil } return reconcile.Result{}, nil diff --git a/controllers/status/status_manager.go b/controllers/status/status_manager.go index 8d832805..a48eb882 100644 --- a/controllers/status/status_manager.go +++ b/controllers/status/status_manager.go @@ -32,10 +32,11 @@ func (sm *StatusManager) UpdateCoherenceStatusPhase(ctx context.Context, namespa } // Update the status phase - deployment.Status.Phase = phase + updated := deployment.DeepCopy() + updated.Status.Phase = phase // Update the resource - err = sm.Client.Status().Update(ctx, deployment) + err = sm.Client.Status().Patch(ctx, deployment, client.MergeFrom(updated)) if err != nil { return errors.Wrapf(err, "updating status phase for Coherence resource %s/%s", namespacedName.Namespace, namespacedName.Name) } @@ -53,11 +54,12 @@ func (sm *StatusManager) UpdateDeploymentStatusHash(ctx context.Context, namespa } // Update the status hash - deployment.Status.Hash = hash - deployment.Status.SetVersion(operator.GetVersion()) + updated := deployment.DeepCopy() + updated.Status.Hash = hash + updated.Status.SetVersion(operator.GetVersion()) // Update the resource - err = sm.Client.Status().Update(ctx, deployment) + err = sm.Client.Status().Patch(ctx, deployment, client.MergeFrom(updated)) if err != nil { return errors.Wrapf(err, "updating status hash for Coherence resource %s/%s", namespacedName.Namespace, namespacedName.Name) } diff --git a/pkg/patching/patcher.go b/pkg/patching/patcher.go index b824f151..796bc5c0 100644 --- a/pkg/patching/patcher.go +++ b/pkg/patching/patcher.go @@ -176,7 +176,8 @@ func (in *patcher) ApplyThreeWayPatchWithCallback(ctx context.Context, name stri in.logger.WithValues().Info(fmt.Sprintf("Patching %s/%s", kind, name), "Patch", string(data)) err := in.mgr.GetClient().Patch(ctx, current, patch) if err != nil { - return false, errors.Wrapf(err, "failed to patching %s/%s with %s", kind, name, string(data)) + in.logger.WithValues().Info(fmt.Sprintf("Failed to patch %s/%s", kind, name), "Patch", string(data), "Error", err.Error()) + return false, errors.Wrapf(err, "failed to patch %s/%s with %s", kind, name, string(data)) } return true, nil @@ -200,7 +201,7 @@ func (in *patcher) CreateThreeWayPatch(name string, original, desired, current r // log the patching kind := current.GetObjectKind().GroupVersionKind().Kind - in.logger.Info(fmt.Sprintf("Created patching for %s/%s", kind, name), "Patch", string(data)) + in.logger.Info(fmt.Sprintf("Created patch for %s/%s", kind, name), "Patch", string(data)) return client.RawPatch(in.patchType, data), data, nil }