From bc62d7a4e959e4ce52acee186fade2edb93d5625 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Wed, 23 Oct 2024 09:28:53 +0000 Subject: [PATCH 01/18] issue-747: add ValidatingAdmissionPolicy --- .../moco/templates/generated/generated.yaml | 44 +++++++++++++++++++ config/webhook/kustomization.yaml | 1 + config/webhook/validate_preventdelete.yaml | 27 ++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 config/webhook/validate_preventdelete.yaml diff --git a/charts/moco/templates/generated/generated.yaml b/charts/moco/templates/generated/generated.yaml index 8bd11a5fc..7050d09f0 100644 --- a/charts/moco/templates/generated/generated.yaml +++ b/charts/moco/templates/generated/generated.yaml @@ -373,6 +373,50 @@ spec: app.kubernetes.io/name: '{{ include "moco.name" . }}' --- apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + labels: + app.kubernetes.io/managed-by: '{{ .Release.Service }}' + app.kubernetes.io/name: '{{ include "moco.name" . }}' + app.kubernetes.io/version: '{{ .Chart.AppVersion }}' + helm.sh/chart: '{{ include "moco.chart" . }}' + name: moco-delete-validator + namespace: '{{ .Release.Namespace }}' +spec: + failurePolicy: Fail + matchConstraints: + resourceRules: + - apiGroups: + - "" + apiVersions: + - '*' + operations: + - DELETE + resources: + - pods + validations: + - expression: | + !has(oldObject.metadata.annotations) || + !("moco.cybozu.com/prevent" in oldObject.metadata.annotations) || + !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") + messageExpression: oldObject.metadata.name + ' is protected from deletion' +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + labels: + app.kubernetes.io/managed-by: '{{ .Release.Service }}' + app.kubernetes.io/name: '{{ include "moco.name" . }}' + app.kubernetes.io/version: '{{ .Chart.AppVersion }}' + helm.sh/chart: '{{ include "moco.chart" . }}' + name: moco-delete-validator + namespace: '{{ .Release.Namespace }}' +spec: + policyName: moco-delete-validator + validationActions: + - Deny +--- +apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: annotations: diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml index 9cf26134e..81caa76c3 100644 --- a/config/webhook/kustomization.yaml +++ b/config/webhook/kustomization.yaml @@ -1,6 +1,7 @@ resources: - manifests.yaml - service.yaml +- validate_preventdelete.yaml configurations: - kustomizeconfig.yaml diff --git a/config/webhook/validate_preventdelete.yaml b/config/webhook/validate_preventdelete.yaml new file mode 100644 index 000000000..660831c56 --- /dev/null +++ b/config/webhook/validate_preventdelete.yaml @@ -0,0 +1,27 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: delete-validator +spec: + failurePolicy: Fail + matchConstraints: + resourceRules: + - apiGroups: [""] + apiVersions: ["*"] + operations: ["DELETE"] + resources: ["pods"] + validations: + - expression: | + !has(oldObject.metadata.annotations) || + !("moco.cybozu.com/prevent" in oldObject.metadata.annotations) || + !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") + messageExpression: oldObject.metadata.name + ' is protected from deletion' +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: delete-validator +spec: + policyName: moco-delete-validator + validationActions: + - Deny From d769d8e6b6d9eb6249ccae39a4653e62c64a424b Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 29 Oct 2024 10:08:45 +0000 Subject: [PATCH 02/18] issue-747: add featureGates --- e2e/kind-config.yaml | 2 ++ e2e/kind-config_actions.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/e2e/kind-config.yaml b/e2e/kind-config.yaml index c61aead9f..371ae22af 100644 --- a/e2e/kind-config.yaml +++ b/e2e/kind-config.yaml @@ -1,5 +1,7 @@ apiVersion: kind.x-k8s.io/v1alpha4 kind: Cluster +featureGates: + ValidatingAdmissionPolicy: true nodes: - role: control-plane - role: worker diff --git a/e2e/kind-config_actions.yaml b/e2e/kind-config_actions.yaml index 1c7ee2e24..2b624de79 100644 --- a/e2e/kind-config_actions.yaml +++ b/e2e/kind-config_actions.yaml @@ -1,5 +1,7 @@ apiVersion: kind.x-k8s.io/v1alpha4 kind: Cluster +featureGates: + ValidatingAdmissionPolicy: true nodes: - role: control-plane - role: worker From e0a072ae20909480c020e60f040f0ed4e9b78069 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 5 Nov 2024 09:15:31 +0000 Subject: [PATCH 03/18] issue-747: remove namespace --- Makefile | 2 +- charts/moco/templates/generated/generated.yaml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 80175d763..751df0db8 100644 --- a/Makefile +++ b/Makefile @@ -63,7 +63,7 @@ manifests: controller-gen kustomize yq ## Generate WebhookConfiguration, Cluster mkdir -p charts/moco/templates/generated/crds/ $(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases $(KUSTOMIZE) build config/crd -o config/crd/tests # Outputs static CRDs for use with Envtest. - $(KUSTOMIZE) build config/kustomize-to-helm/overlays/templates | $(YQ) e "." - > charts/moco/templates/generated/generated.yaml + $(KUSTOMIZE) build config/kustomize-to-helm/overlays/templates | $(YQ) e ". | del(select(.kind==\"ValidatingAdmissionPolicy\" or .kind==\"ValidatingAdmissionPolicyBinding\").metadata.namespace)" - > charts/moco/templates/generated/generated.yaml # Manually remove namespaces because the API version supported by kustomize is out of date. echo '{{- if .Values.crds.enabled }}' > charts/moco/templates/generated/crds/moco_crds.yaml $(KUSTOMIZE) build config/kustomize-to-helm/overlays/crds | $(YQ) e "." - >> charts/moco/templates/generated/crds/moco_crds.yaml echo '{{- end }}' >> charts/moco/templates/generated/crds/moco_crds.yaml diff --git a/charts/moco/templates/generated/generated.yaml b/charts/moco/templates/generated/generated.yaml index 7050d09f0..6ae03f4de 100644 --- a/charts/moco/templates/generated/generated.yaml +++ b/charts/moco/templates/generated/generated.yaml @@ -381,7 +381,6 @@ metadata: app.kubernetes.io/version: '{{ .Chart.AppVersion }}' helm.sh/chart: '{{ include "moco.chart" . }}' name: moco-delete-validator - namespace: '{{ .Release.Namespace }}' spec: failurePolicy: Fail matchConstraints: @@ -410,7 +409,6 @@ metadata: app.kubernetes.io/version: '{{ .Chart.AppVersion }}' helm.sh/chart: '{{ include "moco.chart" . }}' name: moco-delete-validator - namespace: '{{ .Release.Namespace }}' spec: policyName: moco-delete-validator validationActions: From 1d80b63a18f4ae95f7576e3005eace029fcc895a Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Wed, 6 Nov 2024 08:02:47 +0000 Subject: [PATCH 04/18] issue-747: change api version --- charts/moco/templates/generated/generated.yaml | 4 ++-- config/webhook/validate_preventdelete.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/charts/moco/templates/generated/generated.yaml b/charts/moco/templates/generated/generated.yaml index 6ae03f4de..bc36177ea 100644 --- a/charts/moco/templates/generated/generated.yaml +++ b/charts/moco/templates/generated/generated.yaml @@ -372,7 +372,7 @@ spec: app.kubernetes.io/component: moco-controller app.kubernetes.io/name: '{{ include "moco.name" . }}' --- -apiVersion: admissionregistration.k8s.io/v1 +apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingAdmissionPolicy metadata: labels: @@ -400,7 +400,7 @@ spec: !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") messageExpression: oldObject.metadata.name + ' is protected from deletion' --- -apiVersion: admissionregistration.k8s.io/v1 +apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingAdmissionPolicyBinding metadata: labels: diff --git a/config/webhook/validate_preventdelete.yaml b/config/webhook/validate_preventdelete.yaml index 660831c56..96fd342c0 100644 --- a/config/webhook/validate_preventdelete.yaml +++ b/config/webhook/validate_preventdelete.yaml @@ -1,4 +1,4 @@ -apiVersion: admissionregistration.k8s.io/v1 +apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingAdmissionPolicy metadata: name: delete-validator @@ -17,7 +17,7 @@ spec: !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") messageExpression: oldObject.metadata.name + ' is protected from deletion' --- -apiVersion: admissionregistration.k8s.io/v1 +apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingAdmissionPolicyBinding metadata: name: delete-validator From 4b10475ce8147c0ccdde7523affb1d6e789f59ed Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Wed, 6 Nov 2024 09:10:41 +0000 Subject: [PATCH 05/18] issue-747: add runtimeConfig --- .github/workflows/helm.yaml | 1 + e2e/kind-config.yaml | 2 ++ e2e/kind-config_actions.yaml | 2 ++ 3 files changed, 5 insertions(+) diff --git a/.github/workflows/helm.yaml b/.github/workflows/helm.yaml index 07b6b5f3b..b094b804e 100644 --- a/.github/workflows/helm.yaml +++ b/.github/workflows/helm.yaml @@ -40,6 +40,7 @@ jobs: version: v0.23.0 node_image: kindest/node:v1.31.0 kubectl_version: v1.31.0 + config: e2e/kind-config.yaml - name: Apply cert-manager run: | diff --git a/e2e/kind-config.yaml b/e2e/kind-config.yaml index 371ae22af..b6dc40763 100644 --- a/e2e/kind-config.yaml +++ b/e2e/kind-config.yaml @@ -2,6 +2,8 @@ apiVersion: kind.x-k8s.io/v1alpha4 kind: Cluster featureGates: ValidatingAdmissionPolicy: true +runtimeConfig: + admissionregistration.k8s.io/v1beta1: true nodes: - role: control-plane - role: worker diff --git a/e2e/kind-config_actions.yaml b/e2e/kind-config_actions.yaml index 2b624de79..7cde60a85 100644 --- a/e2e/kind-config_actions.yaml +++ b/e2e/kind-config_actions.yaml @@ -2,6 +2,8 @@ apiVersion: kind.x-k8s.io/v1alpha4 kind: Cluster featureGates: ValidatingAdmissionPolicy: true +runtimeConfig: + admissionregistration.k8s.io/v1beta1: true nodes: - role: control-plane - role: worker From 2d8b5d9a6766f337d8e156fc0a6d5248f568e9de Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Wed, 6 Nov 2024 12:39:59 +0000 Subject: [PATCH 06/18] issue-747: fixed bugs in test --- controllers/mysqlcluster_controller.go | 10 ++++++---- controllers/mysqlcluster_controller_test.go | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/controllers/mysqlcluster_controller.go b/controllers/mysqlcluster_controller.go index 526b5ecd8..f68e0afd5 100644 --- a/controllers/mysqlcluster_controller.go +++ b/controllers/mysqlcluster_controller.go @@ -514,12 +514,14 @@ func (r *MySQLClusterReconciler) reconcileV1MyCnf(ctx context.Context, req ctrl. return cms.Items[i].CreationTimestamp.Time.After(cms.Items[j].CreationTimestamp.Time) }) - for i, old := range cms.Items { - if i < r.MySQLConfigMapHistoryLimit { + oldMyCnfCount := 0 + for _, old := range cms.Items { + if !strings.HasPrefix(old.Name, prefix) || old.Name == cmName { continue } - - if strings.HasPrefix(old.Name, prefix) && old.Name != cmName { + oldMyCnfCount++ + log.Info("found my.cnf configmap", "configMapName", old.Name, "count", oldMyCnfCount, "created", old.CreationTimestamp.Time) + if oldMyCnfCount > r.MySQLConfigMapHistoryLimit-1 { if err := r.Delete(ctx, &old); err != nil { return nil, fmt.Errorf("failed to delete old my.cnf configmap %s/%s: %w", old.Namespace, old.Name, err) } diff --git a/controllers/mysqlcluster_controller_test.go b/controllers/mysqlcluster_controller_test.go index 70218d6be..ab2eb087e 100644 --- a/controllers/mysqlcluster_controller_test.go +++ b/controllers/mysqlcluster_controller_test.go @@ -493,7 +493,7 @@ var _ = Describe("MySQLCluster reconciler", func() { } if mycnfCount != 2 { - return fmt.Errorf("the number of config maps is not history limits: %d", len(cms.Items)) + return fmt.Errorf("the number of config maps is not history limits: %d", mycnfCount) } var mycnfCMs []*corev1.ConfigMap @@ -538,7 +538,7 @@ var _ = Describe("MySQLCluster reconciler", func() { } if mycnfCount != 2 { - return fmt.Errorf("the number of config maps is not history limits: %d", len(cms.Items)) + return fmt.Errorf("the number of config maps is not history limits: %d", mycnfCount) } var mycnfCMs []*corev1.ConfigMap From 323ff55a8dcc6c0139730b1ae9a77b92f1f4602c Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Thu, 7 Nov 2024 07:27:15 +0000 Subject: [PATCH 07/18] issue-747: add maxDelaySecondsForPodDeletion param --- api/v1beta2/mysqlcluster_types.go | 9 +++++++++ charts/moco/templates/generated/crds/moco_crds.yaml | 6 ++++++ config/crd/bases/moco.cybozu.com_mysqlclusters.yaml | 6 ++++++ ...resourcedefinition_mysqlclusters.moco.cybozu.com.yaml | 6 ++++++ docs/crd_mysqlcluster_v1beta2.md | 1 + 5 files changed, 28 insertions(+) diff --git a/api/v1beta2/mysqlcluster_types.go b/api/v1beta2/mysqlcluster_types.go index 3d3de8a88..04ca78f0e 100644 --- a/api/v1beta2/mysqlcluster_types.go +++ b/api/v1beta2/mysqlcluster_types.go @@ -87,6 +87,15 @@ type MySQLClusterSpec struct { // +optional MaxDelaySeconds *int `json:"maxDelaySeconds,omitempty"` + // MaxDelaySecondsForPodDeletion configures the delay threshold to delete a pod. + // If replication delay of the mysqld instance is over this threshold, the pod will not be deleted. + // The default is 0 seconds. + // Setting this field to 0 disables the delay check for pod deletion. + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:default=0 + // +optional + MaxDelaySecondsForPodDeletion int64 `json:"maxDelaySecondsForPodDeletion,omitempty"` + // StartupWaitSeconds is the maximum duration to wait for `mysqld` container to start working. // The default is 3600 seconds. // +kubebuilder:validation:Minimum=0 diff --git a/charts/moco/templates/generated/crds/moco_crds.yaml b/charts/moco/templates/generated/crds/moco_crds.yaml index 1d4f4b4ff..d237a1c83 100644 --- a/charts/moco/templates/generated/crds/moco_crds.yaml +++ b/charts/moco/templates/generated/crds/moco_crds.yaml @@ -2261,6 +2261,12 @@ spec: description: 'MaxDelaySeconds configures the readiness probe of ' minimum: 0 type: integer + maxDelaySecondsForPodDeletion: + default: 0 + description: MaxDelaySecondsForPodDeletion configures the delay + format: int64 + minimum: 0 + type: integer mysqlConfigMapName: description: 'MySQLConfigMapName is a `ConfigMap` name of MySQL ' nullable: true diff --git a/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml b/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml index f40ee3a27..abf5ecaf6 100644 --- a/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml +++ b/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml @@ -78,6 +78,12 @@ spec: description: 'MaxDelaySeconds configures the readiness probe of ' minimum: 0 type: integer + maxDelaySecondsForPodDeletion: + default: 0 + description: MaxDelaySecondsForPodDeletion configures the delay + format: int64 + minimum: 0 + type: integer mysqlConfigMapName: description: 'MySQLConfigMapName is a `ConfigMap` name of MySQL ' nullable: true diff --git a/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml b/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml index d99dfc77d..10c2d393c 100644 --- a/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml +++ b/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml @@ -78,6 +78,12 @@ spec: description: 'MaxDelaySeconds configures the readiness probe of ' minimum: 0 type: integer + maxDelaySecondsForPodDeletion: + default: 0 + description: MaxDelaySecondsForPodDeletion configures the delay + format: int64 + minimum: 0 + type: integer mysqlConfigMapName: description: 'MySQLConfigMapName is a `ConfigMap` name of MySQL ' nullable: true diff --git a/docs/crd_mysqlcluster_v1beta2.md b/docs/crd_mysqlcluster_v1beta2.md index a42743e30..3c5a0f6a3 100644 --- a/docs/crd_mysqlcluster_v1beta2.md +++ b/docs/crd_mysqlcluster_v1beta2.md @@ -78,6 +78,7 @@ MySQLClusterSpec defines the desired state of MySQLCluster | collectors | Collectors is the list of collector flag names of mysqld_exporter. If this field is not empty, MOCO adds mysqld_exporter as a sidecar to collect and export mysqld metrics in Prometheus format.\n\nSee https://github.com/prometheus/mysqld_exporter/blob/master/README.md#collector-flags for flag names.\n\nExample: [\"engine_innodb_status\", \"info_schema.innodb_metrics\"] | []string | false | | serverIDBase | ServerIDBase, if set, will become the base number of server-id of each MySQL instance of this cluster. For example, if this is 100, the server-ids will be 100, 101, 102, and so on. If the field is not given or zero, MOCO automatically sets a random positive integer. | int32 | false | | maxDelaySeconds | MaxDelaySeconds configures the readiness probe of mysqld container. For a replica mysqld instance, if it is delayed to apply transactions over this threshold, the mysqld instance will be marked as non-ready. The default is 60 seconds. Setting this field to 0 disables the delay check in the probe. | *int | false | +| maxDelaySecondsForPodDeletion | MaxDelaySecondsForPodDeletion configures the delay threshold to delete a pod. If replication delay of the mysqld instance is over this threshold, the pod will not be deleted. The default is 0 seconds. Setting this field to 0 disables the delay check for pod deletion. | int64 | false | | startupWaitSeconds | StartupWaitSeconds is the maximum duration to wait for `mysqld` container to start working. The default is 3600 seconds. | int32 | false | | logRotationSchedule | LogRotationSchedule specifies the schedule to rotate MySQL logs. If not set, the default is to rotate logs every 5 minutes. See https://pkg.go.dev/github.com/robfig/cron/v3#hdr-CRON_Expression_Format for the field format. | string | false | | backupPolicyName | The name of BackupPolicy custom resource in the same namespace. If this is set, MOCO creates a CronJob to take backup of this MySQL cluster periodically. | *string | false | From f7268724f35aec81be9cd831a5b879d30e1049ac Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Fri, 8 Nov 2024 04:54:36 +0000 Subject: [PATCH 08/18] issue-747: add AnnPreventDelete --- pkg/constants/meta.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/constants/meta.go b/pkg/constants/meta.go index f7cabf8e6..60eca8c3d 100644 --- a/pkg/constants/meta.go +++ b/pkg/constants/meta.go @@ -22,6 +22,7 @@ const ( AnnClusteringStopped = "moco.cybozu.com/clustering-stopped" AnnReconciliationStopped = "moco.cybozu.com/reconciliation-stopped" AnnForceRollingUpdate = "moco.cybozu.com/force-rolling-update" + AnnPrevent = "moco.cybozu.com/prevent" ) // MySQLClusterFinalizer is the finalizer specifier for MySQLCluster. From c3c6f62b1855c2a3db7c7fd51a0278140361109d Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Fri, 8 Nov 2024 05:23:35 +0000 Subject: [PATCH 09/18] issue-747: Ensure pods are not deleted when replication is delayed --- clustering/status.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/clustering/status.go b/clustering/status.go index 037606241..49adeef0c 100644 --- a/clustering/status.go +++ b/clustering/status.go @@ -235,6 +235,44 @@ func (p *managerProcess) GatherStatus(ctx context.Context) (*StatusSet, error) { ss.ExecutedGTID = pst.GlobalVariables.ExecutedGTID } + // detect replication delay + if cluster.Spec.MaxDelaySecondsForPodDeletion > 0 { + preventDelete := false + for i, ist := range ss.MySQLStatus { + if i == ss.Primary { + continue + } + if ist == nil { + continue + } + if ist.ReplicaStatus == nil { + continue + } + if ist.ReplicaStatus.SecondsBehindSource.Valid && ist.ReplicaStatus.SecondsBehindSource.Int64 > cluster.Spec.MaxDelaySecondsForPodDeletion { + preventDelete = true + } + } + if preventDelete { + primary := ss.Pods[ss.Primary] + if primary.Annotations == nil { + primary.Annotations = make(map[string]string) + } + if _, exists := primary.Annotations[constants.AnnPrevent]; !exists { + logFromContext(ctx).Info("replication delay detected, prevent pod deletion", "instance", ss.Primary) + primary.Annotations[constants.AnnPrevent] = "delete" + p.client.Update(ctx, primary) + } + } else { + for i, pod := range ss.Pods { + if pod.Annotations != nil { + logFromContext(ctx).Info("replication delay resolved, allow pod deletion", "instance", i) + delete(pod.Annotations, constants.AnnPrevent) + p.client.Update(ctx, pod) + } + } + } + } + // detect errant replicas if ss.ExecutedGTID != "" { pst := ss.MySQLStatus[ss.Primary] From 9aa05c23989e57ddfc03cee6d0fa112fa7a2c16d Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Mon, 11 Nov 2024 05:21:02 +0000 Subject: [PATCH 10/18] issue-747: rename annotaion --- charts/moco/templates/generated/generated.yaml | 4 ++-- clustering/status.go | 12 +++++++----- config/webhook/validate_preventdelete.yaml | 4 ++-- pkg/constants/meta.go | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/charts/moco/templates/generated/generated.yaml b/charts/moco/templates/generated/generated.yaml index bc36177ea..ed07e253f 100644 --- a/charts/moco/templates/generated/generated.yaml +++ b/charts/moco/templates/generated/generated.yaml @@ -396,8 +396,8 @@ spec: validations: - expression: | !has(oldObject.metadata.annotations) || - !("moco.cybozu.com/prevent" in oldObject.metadata.annotations) || - !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") + !("moco.cybozu.com/prevent-delete" in oldObject.metadata.annotations) || + !(oldObject.metadata.annotations["moco.cybozu.com/prevent-delete"] == "true") messageExpression: oldObject.metadata.name + ' is protected from deletion' --- apiVersion: admissionregistration.k8s.io/v1beta1 diff --git a/clustering/status.go b/clustering/status.go index 49adeef0c..dd7e5a628 100644 --- a/clustering/status.go +++ b/clustering/status.go @@ -257,17 +257,19 @@ func (p *managerProcess) GatherStatus(ctx context.Context) (*StatusSet, error) { if primary.Annotations == nil { primary.Annotations = make(map[string]string) } - if _, exists := primary.Annotations[constants.AnnPrevent]; !exists { + if _, exists := primary.Annotations[constants.AnnPreventDelete]; !exists { logFromContext(ctx).Info("replication delay detected, prevent pod deletion", "instance", ss.Primary) - primary.Annotations[constants.AnnPrevent] = "delete" + primary.Annotations[constants.AnnPreventDelete] = "true" p.client.Update(ctx, primary) } } else { for i, pod := range ss.Pods { if pod.Annotations != nil { - logFromContext(ctx).Info("replication delay resolved, allow pod deletion", "instance", i) - delete(pod.Annotations, constants.AnnPrevent) - p.client.Update(ctx, pod) + if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { + logFromContext(ctx).Info("replication delay resolved, allow pod deletion", "instance", i) + delete(pod.Annotations, constants.AnnPreventDelete) + p.client.Update(ctx, pod) + } } } } diff --git a/config/webhook/validate_preventdelete.yaml b/config/webhook/validate_preventdelete.yaml index 96fd342c0..a5ab99aa4 100644 --- a/config/webhook/validate_preventdelete.yaml +++ b/config/webhook/validate_preventdelete.yaml @@ -13,8 +13,8 @@ spec: validations: - expression: | !has(oldObject.metadata.annotations) || - !("moco.cybozu.com/prevent" in oldObject.metadata.annotations) || - !(oldObject.metadata.annotations["moco.cybozu.com/prevent"] == "delete") + !("moco.cybozu.com/prevent-delete" in oldObject.metadata.annotations) || + !(oldObject.metadata.annotations["moco.cybozu.com/prevent-delete"] == "true") messageExpression: oldObject.metadata.name + ' is protected from deletion' --- apiVersion: admissionregistration.k8s.io/v1beta1 diff --git a/pkg/constants/meta.go b/pkg/constants/meta.go index 60eca8c3d..970f28b29 100644 --- a/pkg/constants/meta.go +++ b/pkg/constants/meta.go @@ -22,7 +22,7 @@ const ( AnnClusteringStopped = "moco.cybozu.com/clustering-stopped" AnnReconciliationStopped = "moco.cybozu.com/reconciliation-stopped" AnnForceRollingUpdate = "moco.cybozu.com/force-rolling-update" - AnnPrevent = "moco.cybozu.com/prevent" + AnnPreventDelete = "moco.cybozu.com/prevent-delete" ) // MySQLClusterFinalizer is the finalizer specifier for MySQLCluster. From 117076766a544f6f43c61e839b9e8fcf4970c39b Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Mon, 11 Nov 2024 05:48:24 +0000 Subject: [PATCH 11/18] issue-747: modify description --- api/v1beta2/mysqlcluster_types.go | 4 ++-- charts/moco/templates/generated/crds/moco_crds.yaml | 2 +- config/crd/bases/moco.cybozu.com_mysqlclusters.yaml | 2 +- ...ustomresourcedefinition_mysqlclusters.moco.cybozu.com.yaml | 2 +- docs/crd_mysqlcluster_v1beta2.md | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/v1beta2/mysqlcluster_types.go b/api/v1beta2/mysqlcluster_types.go index 04ca78f0e..a40923309 100644 --- a/api/v1beta2/mysqlcluster_types.go +++ b/api/v1beta2/mysqlcluster_types.go @@ -87,8 +87,8 @@ type MySQLClusterSpec struct { // +optional MaxDelaySeconds *int `json:"maxDelaySeconds,omitempty"` - // MaxDelaySecondsForPodDeletion configures the delay threshold to delete a pod. - // If replication delay of the mysqld instance is over this threshold, the pod will not be deleted. + // MaxDelaySecondsForPodDeletion configures the maximum allowed replication delay before a Pod deletion is blocked. + // If the replication delay exceeds this threshold, deletion of the primary pod will be prevented. // The default is 0 seconds. // Setting this field to 0 disables the delay check for pod deletion. // +kubebuilder:validation:Minimum=0 diff --git a/charts/moco/templates/generated/crds/moco_crds.yaml b/charts/moco/templates/generated/crds/moco_crds.yaml index d237a1c83..ee735f164 100644 --- a/charts/moco/templates/generated/crds/moco_crds.yaml +++ b/charts/moco/templates/generated/crds/moco_crds.yaml @@ -2263,7 +2263,7 @@ spec: type: integer maxDelaySecondsForPodDeletion: default: 0 - description: MaxDelaySecondsForPodDeletion configures the delay + description: MaxDelaySecondsForPodDeletion configures the maxim format: int64 minimum: 0 type: integer diff --git a/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml b/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml index abf5ecaf6..b7c407545 100644 --- a/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml +++ b/config/crd/bases/moco.cybozu.com_mysqlclusters.yaml @@ -80,7 +80,7 @@ spec: type: integer maxDelaySecondsForPodDeletion: default: 0 - description: MaxDelaySecondsForPodDeletion configures the delay + description: MaxDelaySecondsForPodDeletion configures the maxim format: int64 minimum: 0 type: integer diff --git a/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml b/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml index 10c2d393c..b9be3f332 100644 --- a/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml +++ b/config/crd/tests/apiextensions.k8s.io_v1_customresourcedefinition_mysqlclusters.moco.cybozu.com.yaml @@ -80,7 +80,7 @@ spec: type: integer maxDelaySecondsForPodDeletion: default: 0 - description: MaxDelaySecondsForPodDeletion configures the delay + description: MaxDelaySecondsForPodDeletion configures the maxim format: int64 minimum: 0 type: integer diff --git a/docs/crd_mysqlcluster_v1beta2.md b/docs/crd_mysqlcluster_v1beta2.md index 3c5a0f6a3..e89a8ebe7 100644 --- a/docs/crd_mysqlcluster_v1beta2.md +++ b/docs/crd_mysqlcluster_v1beta2.md @@ -78,7 +78,7 @@ MySQLClusterSpec defines the desired state of MySQLCluster | collectors | Collectors is the list of collector flag names of mysqld_exporter. If this field is not empty, MOCO adds mysqld_exporter as a sidecar to collect and export mysqld metrics in Prometheus format.\n\nSee https://github.com/prometheus/mysqld_exporter/blob/master/README.md#collector-flags for flag names.\n\nExample: [\"engine_innodb_status\", \"info_schema.innodb_metrics\"] | []string | false | | serverIDBase | ServerIDBase, if set, will become the base number of server-id of each MySQL instance of this cluster. For example, if this is 100, the server-ids will be 100, 101, 102, and so on. If the field is not given or zero, MOCO automatically sets a random positive integer. | int32 | false | | maxDelaySeconds | MaxDelaySeconds configures the readiness probe of mysqld container. For a replica mysqld instance, if it is delayed to apply transactions over this threshold, the mysqld instance will be marked as non-ready. The default is 60 seconds. Setting this field to 0 disables the delay check in the probe. | *int | false | -| maxDelaySecondsForPodDeletion | MaxDelaySecondsForPodDeletion configures the delay threshold to delete a pod. If replication delay of the mysqld instance is over this threshold, the pod will not be deleted. The default is 0 seconds. Setting this field to 0 disables the delay check for pod deletion. | int64 | false | +| maxDelaySecondsForPodDeletion | MaxDelaySecondsForPodDeletion configures the maximum allowed replication delay before a Pod deletion is blocked. If the replication delay exceeds this threshold, deletion of the primary pod will be prevented. The default is 0 seconds. Setting this field to 0 disables the delay check for pod deletion. | int64 | false | | startupWaitSeconds | StartupWaitSeconds is the maximum duration to wait for `mysqld` container to start working. The default is 3600 seconds. | int32 | false | | logRotationSchedule | LogRotationSchedule specifies the schedule to rotate MySQL logs. If not set, the default is to rotate logs every 5 minutes. See https://pkg.go.dev/github.com/robfig/cron/v3#hdr-CRON_Expression_Format for the field format. | string | false | | backupPolicyName | The name of BackupPolicy custom resource in the same namespace. If this is set, MOCO creates a CronJob to take backup of this MySQL cluster periodically. | *string | false | From aa234b307f4d4910d0e042aa78a66ddf8852b841 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 12 Nov 2024 07:32:44 +0000 Subject: [PATCH 12/18] issue-747: add testcase --- e2e/prevent_delete_test.go | 226 +++++++++++++++++++++++++++++++ e2e/testdata/prevent_delete.yaml | 36 +++++ 2 files changed, 262 insertions(+) create mode 100644 e2e/prevent_delete_test.go create mode 100644 e2e/testdata/prevent_delete.yaml diff --git a/e2e/prevent_delete_test.go b/e2e/prevent_delete_test.go new file mode 100644 index 000000000..cb211e330 --- /dev/null +++ b/e2e/prevent_delete_test.go @@ -0,0 +1,226 @@ +package e2e + +import ( + _ "embed" + "encoding/json" + "errors" + "fmt" + "strconv" + "time" + + mocov1beta2 "github.com/cybozu-go/moco/api/v1beta2" + "github.com/cybozu-go/moco/pkg/constants" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +//go:embed testdata/prevent_delete.yaml +var preventDeleteYAML string + +func initializeMySQL() { + kubectlSafe(fillTemplate(preventDeleteYAML), "apply", "-f", "-") + Eventually(func() error { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + for _, cond := range cluster.Status.Conditions { + if cond.Type != mocov1beta2.ConditionHealthy { + continue + } + if cond.Status == metav1.ConditionTrue { + return nil + } + return fmt.Errorf("cluster is not healthy: %s", cond.Status) + } + return errors.New("no health condition") + }).Should(Succeed()) + time.Sleep(30 * time.Second) + + // wait for primary to be 1 + Eventually(func() int { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + if cluster.Status.CurrentPrimaryIndex != 1 { + kubectlSafe(nil, "moco", "-n", "prevent-delete", "switchover", "test") + time.Sleep(10 * time.Second) + } + return cluster.Status.CurrentPrimaryIndex + }).Should(Equal(1)) + + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-writable", "test", "--", + "-e", "CREATE DATABASE test") + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-writable", "test", "--", + "-e", "CREATE TABLE test.t (i INT)") +} + +func cleanupMySQL() { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + primary := cluster.Status.CurrentPrimaryIndex + for i := 0; i < 3; i++ { + if i == primary { + continue + } + setSourceDelay(i, 0) + } + time.Sleep(10 * time.Second) + + kubectlSafe(nil, "delete", "mysqlclusters", "-n", "prevent-delete", "--all") + verifyAllPodsDeleted("prevent-delete") +} + +func setSourceDelay(index, delay int) { + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-admin", "--index", strconv.Itoa(index), "test", "--", "-e", "STOP REPLICA SQL_THREAD") + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-admin", "--index", strconv.Itoa(index), "test", "--", "-e", fmt.Sprintf("CHANGE REPLICATION SOURCE TO SOURCE_DELAY=%d", delay)) + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-admin", "--index", strconv.Itoa(index), "test", "--", "-e", "START REPLICA") + if delay != 0 { + kubectlSafe(nil, "moco", "mysql", "-n", "prevent-delete", "-u", "moco-writable", "test", "--", "-e", "INSERT INTO test.t VALUES (1); COMMIT;") + } +} + +var _ = Context("PreventDelete", func() { + if doUpgrade { + return + } + + BeforeEach(func() { + initializeMySQL() + }) + + AfterEach(func() { + cleanupMySQL() + }) + + It("should add or remove prevent-delete annotation by replication delay", func() { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + primary := cluster.Status.CurrentPrimaryIndex + + // add prevent-delete annotation and wait for it to be removed + for i := 0; i < 3; i++ { + kubectlSafe(nil, "annotate", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", i), "moco.cybozu.com/prevent-delete=true") + Eventually(func() error { + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", i), "-o", "json") + Expect(err).NotTo(HaveOccurred()) + pod := &corev1.Pod{} + err = json.Unmarshal(out, pod) + Expect(err).NotTo(HaveOccurred()) + if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { + return errors.New("annotation is not removed") + } + return nil + }).Should(Succeed()) + } + + // set huge replication delay + setSourceDelay(0, 10000) + + // wait for prevent-delete annotation to be added + Eventually(func() error { + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + Expect(err).NotTo(HaveOccurred()) + pod := &corev1.Pod{} + err = json.Unmarshal(out, pod) + Expect(err).NotTo(HaveOccurred()) + if val, exists := pod.Annotations[constants.AnnPreventDelete]; !exists { + return errors.New("annotation is not added") + } else if val != "true" { + return fmt.Errorf("annotation value is not true: %s", val) + } + return nil + }).Should(Succeed()) + + // fail to delete pod with prevent-delete annotation + _, err = kubectl(nil, "delete", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary)) + Expect(err.Error()).To(ContainSubstring("moco-test-%d is protected from deletion", primary)) + + // resolve replication delay + setSourceDelay(0, 0) + + // wait for prevent-delete annotation to be removed + Eventually(func() error { + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + Expect(err).NotTo(HaveOccurred()) + pod := &corev1.Pod{} + err = json.Unmarshal(out, pod) + Expect(err).NotTo(HaveOccurred()) + if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { + return errors.New("annotation is not removed") + } + return nil + }).Should(Succeed()) + }) + + It("should not finish rollout restart if replication delay occurs", func() { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + primary := cluster.Status.CurrentPrimaryIndex + + // set huge replication delay + setSourceDelay(0, 10000) + + // wait for prevent-delete annotation to be added + Eventually(func() error { + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + Expect(err).NotTo(HaveOccurred()) + pod := &corev1.Pod{} + err = json.Unmarshal(out, pod) + Expect(err).NotTo(HaveOccurred()) + if val, exists := pod.Annotations[constants.AnnPreventDelete]; !exists { + return errors.New("annotation is not added") + } else if val != "true" { + return fmt.Errorf("annotation value is not true: %s", val) + } + return nil + }).Should(Succeed()) + + // never finish rollout restart + kubectlSafe(nil, "rollout", "restart", "sts", "-n", "prevent-delete", "moco-test") + Consistently(func() error { + out, err := kubectl(nil, "get", "sts", "-n", "prevent-delete", "moco-test", "-o", "json") + Expect(err).NotTo(HaveOccurred()) + sts := &appsv1.StatefulSet{} + err = json.Unmarshal(out, sts) + Expect(err).NotTo(HaveOccurred()) + if sts.Status.UpdatedReplicas != sts.Status.Replicas { + return errors.New("rollout restart is not finished") + } + return nil + }, 3*time.Minute).ShouldNot(Succeed()) + + // resolve replication delay + setSourceDelay(0, 0) + + // wait for rollout restart to be finished + Eventually(func() error { + out, err := kubectl(nil, "get", "sts", "-n", "prevent-delete", "moco-test", "-o", "json") + Expect(err).NotTo(HaveOccurred()) + sts := &appsv1.StatefulSet{} + err = json.Unmarshal(out, sts) + Expect(err).NotTo(HaveOccurred()) + if sts.Status.UpdatedReplicas != sts.Status.Replicas { + return errors.New("rollout restart is not finished") + } + return nil + }).Should(Succeed()) + + // wait for cluster to be healthy + Eventually(func() error { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + for _, cond := range cluster.Status.Conditions { + if cond.Type != mocov1beta2.ConditionHealthy { + continue + } + if cond.Status == metav1.ConditionTrue { + return nil + } + return fmt.Errorf("cluster is not healthy: %s", cond.Status) + } + return errors.New("no health condition") + }).Should(Succeed()) + time.Sleep(30 * time.Second) + }) +}) diff --git a/e2e/testdata/prevent_delete.yaml b/e2e/testdata/prevent_delete.yaml new file mode 100644 index 000000000..cc3048030 --- /dev/null +++ b/e2e/testdata/prevent_delete.yaml @@ -0,0 +1,36 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: prevent-delete +--- +apiVersion: v1 +kind: ConfigMap +metadata: + namespace: prevent-delete + name: mycnf +data: + innodb_log_file_size: "10M" +--- +apiVersion: moco.cybozu.com/v1beta2 +kind: MySQLCluster +metadata: + namespace: prevent-delete + name: test +spec: + mysqlConfigMapName: mycnf + replicas: 3 + maxDelaySeconds: 0 + maxDelaySecondsForPodDeletion: 10 + podTemplate: + spec: + containers: + - name: mysqld + image: ghcr.io/cybozu-go/moco/mysql:{{ . }} + volumeClaimTemplates: + - metadata: + name: mysql-data + spec: + accessModes: [ "ReadWriteOnce" ] + resources: + requests: + storage: 1Gi From 22631c307d34655944a1a610b072dcf8e3cdea26 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Thu, 14 Nov 2024 05:30:57 +0000 Subject: [PATCH 13/18] issue-747: use Patch method --- clustering/status.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/clustering/status.go b/clustering/status.go index dd7e5a628..fbd531c96 100644 --- a/clustering/status.go +++ b/clustering/status.go @@ -253,22 +253,28 @@ func (p *managerProcess) GatherStatus(ctx context.Context) (*StatusSet, error) { } } if preventDelete { - primary := ss.Pods[ss.Primary] - if primary.Annotations == nil { - primary.Annotations = make(map[string]string) + ppod := ss.Pods[ss.Primary] + newPod := ppod.DeepCopy() + if newPod.Annotations == nil { + newPod.Annotations = make(map[string]string) } - if _, exists := primary.Annotations[constants.AnnPreventDelete]; !exists { + if _, exists := newPod.Annotations[constants.AnnPreventDelete]; !exists { logFromContext(ctx).Info("replication delay detected, prevent pod deletion", "instance", ss.Primary) - primary.Annotations[constants.AnnPreventDelete] = "true" - p.client.Update(ctx, primary) + newPod.Annotations[constants.AnnPreventDelete] = "true" + if err := p.client.Patch(ctx, newPod, client.MergeFrom(ppod)); err != nil { + return nil, fmt.Errorf("failed to add moco.cybozu.com/prevent-delete annotation: %w", err) + } } } else { for i, pod := range ss.Pods { if pod.Annotations != nil { if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { + newPod := pod.DeepCopy() logFromContext(ctx).Info("replication delay resolved, allow pod deletion", "instance", i) - delete(pod.Annotations, constants.AnnPreventDelete) - p.client.Update(ctx, pod) + delete(newPod.Annotations, constants.AnnPreventDelete) + if err := p.client.Patch(ctx, newPod, client.MergeFrom(pod)); err != nil { + return nil, fmt.Errorf("failed to remove moco.cybozu.com/prevent-delete annotation: %w", err) + } } } } From d03e320473d3f62052d1f2c208c8ea9ad6b0be19 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Fri, 15 Nov 2024 06:39:39 +0000 Subject: [PATCH 14/18] issue-747: Move the process to the appropriate function --- clustering/operations.go | 32 ++++++++++++++++++++++++++++++++ clustering/process.go | 12 ++++++++++++ clustering/status.go | 40 ++++++++-------------------------------- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/clustering/operations.go b/clustering/operations.go index 6a4887ec2..f8edb83f6 100644 --- a/clustering/operations.go +++ b/clustering/operations.go @@ -315,6 +315,38 @@ func (p *managerProcess) addRoleLabel(ctx context.Context, ss *StatusSet, noRole return nil } +func (p *managerProcess) removeAnnPreventDelete(ctx context.Context, ss *StatusSet) error { + log := logFromContext(ctx) + for _, pod := range ss.Pods { + if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { + newPod := pod.DeepCopy() + delete(newPod.Annotations, constants.AnnPreventDelete) + log.Info("replication delay resolved, allow pod deletion", "pod", pod.Name) + if err := p.client.Patch(ctx, newPod, client.MergeFrom(pod)); err != nil { + return fmt.Errorf("failed to remove moco.cybozu.com/prevent-delete annotation: %w", err) + } + } + } + return nil +} + +func (p *managerProcess) addAnnPreventDelete(ctx context.Context, ss *StatusSet) error { + log := logFromContext(ctx) + ppod := ss.Pods[ss.Primary] + newPod := ppod.DeepCopy() + if newPod.Annotations == nil { + newPod.Annotations = make(map[string]string) + } + if _, exists := newPod.Annotations[constants.AnnPreventDelete]; !exists { + newPod.Annotations[constants.AnnPreventDelete] = "true" + log.Info("replication delay detected, prevent pod deletion", "pod", ppod.Name) + if err := p.client.Patch(ctx, newPod, client.MergeFrom(ppod)); err != nil { + return fmt.Errorf("failed to add moco.cybozu.com/prevent-delete annotation: %w", err) + } + } + return nil +} + func (p *managerProcess) configure(ctx context.Context, ss *StatusSet) (bool, error) { redo := false diff --git a/clustering/process.go b/clustering/process.go index 054713f81..d0b1eeebd 100644 --- a/clustering/process.go +++ b/clustering/process.go @@ -185,6 +185,18 @@ func (p *managerProcess) do(ctx context.Context) (bool, error) { return false, fmt.Errorf("failed to update status fields in MySQLCluster: %w", err) } + if ss.PreventPodDeletion { + err := p.addAnnPreventDelete(ctx, ss) + if err != nil { + return false, fmt.Errorf("failed to add annotation to prevent pod deletion: %w", err) + } + } else { + err := p.removeAnnPreventDelete(ctx, ss) + if err != nil { + return false, fmt.Errorf("failed to remove annotation to prevent pod deletion: %w", err) + } + } + logFromContext(ctx).Info("cluster state is " + ss.State.String()) switch ss.State { case StateOffline: diff --git a/clustering/status.go b/clustering/status.go index fbd531c96..b55b10d4e 100644 --- a/clustering/status.go +++ b/clustering/status.go @@ -92,9 +92,10 @@ type StatusSet struct { Errants []int Candidates []int - NeedSwitch bool - Candidate int - State ClusterState + NeedSwitch bool + PreventPodDeletion bool + Candidate int + State ClusterState } // Close closes `ss.DBOps`. @@ -237,7 +238,7 @@ func (p *managerProcess) GatherStatus(ctx context.Context) (*StatusSet, error) { // detect replication delay if cluster.Spec.MaxDelaySecondsForPodDeletion > 0 { - preventDelete := false + preventPodDeletion := false for i, ist := range ss.MySQLStatus { if i == ss.Primary { continue @@ -249,36 +250,11 @@ func (p *managerProcess) GatherStatus(ctx context.Context) (*StatusSet, error) { continue } if ist.ReplicaStatus.SecondsBehindSource.Valid && ist.ReplicaStatus.SecondsBehindSource.Int64 > cluster.Spec.MaxDelaySecondsForPodDeletion { - preventDelete = true - } - } - if preventDelete { - ppod := ss.Pods[ss.Primary] - newPod := ppod.DeepCopy() - if newPod.Annotations == nil { - newPod.Annotations = make(map[string]string) - } - if _, exists := newPod.Annotations[constants.AnnPreventDelete]; !exists { - logFromContext(ctx).Info("replication delay detected, prevent pod deletion", "instance", ss.Primary) - newPod.Annotations[constants.AnnPreventDelete] = "true" - if err := p.client.Patch(ctx, newPod, client.MergeFrom(ppod)); err != nil { - return nil, fmt.Errorf("failed to add moco.cybozu.com/prevent-delete annotation: %w", err) - } - } - } else { - for i, pod := range ss.Pods { - if pod.Annotations != nil { - if _, exists := pod.Annotations[constants.AnnPreventDelete]; exists { - newPod := pod.DeepCopy() - logFromContext(ctx).Info("replication delay resolved, allow pod deletion", "instance", i) - delete(newPod.Annotations, constants.AnnPreventDelete) - if err := p.client.Patch(ctx, newPod, client.MergeFrom(pod)); err != nil { - return nil, fmt.Errorf("failed to remove moco.cybozu.com/prevent-delete annotation: %w", err) - } - } - } + preventPodDeletion = true + break } } + ss.PreventPodDeletion = preventPodDeletion } // detect errant replicas From 4ee87e569d5ca7a3c3196fe585abe9e7620e1684 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Fri, 15 Nov 2024 07:31:30 +0000 Subject: [PATCH 15/18] issue-747: add manager test --- clustering/manager_test.go | 67 ++++++++++++++++++++++++++++++++++++++ clustering/mock_test.go | 12 +++++++ 2 files changed, 79 insertions(+) diff --git a/clustering/manager_test.go b/clustering/manager_test.go index 2e363a10b..56eb86764 100644 --- a/clustering/manager_test.go +++ b/clustering/manager_test.go @@ -1016,4 +1016,71 @@ var _ = Describe("manager", func() { Expect(ms.backupWorkDirUsage).To(MetricsIs("==", 30)) Expect(ms.backupWarnings).To(MetricsIs("==", 2)) }) + It("shoud delect replication delay and prevent deletion of primary", func() { + testSetupResources(ctx, 3, "") + + cm := NewClusterManager(1*time.Second, mgr, of, af, stdr.New(nil)) + defer cm.StopAll() + + cluster, err := testGetCluster(ctx) + Expect(err).NotTo(HaveOccurred()) + cm.Update(client.ObjectKeyFromObject(cluster), "test") + defer func() { + cm.Stop(client.ObjectKeyFromObject(cluster)) + time.Sleep(400 * time.Millisecond) + Eventually(func(g Gomega) { + ch := make(chan prometheus.Metric, 2) + metrics.ErrantReplicasVec.Collect(ch) + g.Expect(ch).NotTo(Receive()) + }).Should(Succeed()) + }() + + // set MaxDelaySecondsForPodDeletion to 10sec + cluster.Spec.MaxDelaySecondsForPodDeletion = 10 + err = k8sClient.Update(ctx, cluster) + Expect(err).NotTo(HaveOccurred()) + + // wait for cluster's condition changes + Eventually(func(g Gomega) { + cluster, err = testGetCluster(ctx) + g.Expect(err).NotTo(HaveOccurred()) + + condHealthy, err := testGetCondition(cluster, mocov1beta2.ConditionHealthy) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(condHealthy.Status).To(Equal(metav1.ConditionTrue)) + }).Should(Succeed()) + + // set replication delay to 100sec + primary := cluster.Status.CurrentPrimaryIndex + for i := 0; i < 3; i++ { + if i == primary { + continue + } + of.setSecondsBehindSource(cluster.PodHostname(i), 100) + } + + // wait for the pods' annotations are updated + Eventually(func(g Gomega) { + pod := &corev1.Pod{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "test", Name: cluster.PodName(primary)}, pod) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(pod.Annotations).To(HaveKeyWithValue(constants.AnnPreventDelete, "true")) + }).Should(Succeed()) + + // set replication delay to 0sec + for i := 0; i < 3; i++ { + if i == primary { + continue + } + of.setSecondsBehindSource(cluster.PodHostname(i), 0) + } + + // wait for the pods' annotations are updated + Eventually(func(g Gomega) { + pod := &corev1.Pod{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "test", Name: cluster.PodName(primary)}, pod) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(pod.Annotations).NotTo(HaveKey(constants.AnnPreventDelete)) + }).Should(Succeed()) + }) }) diff --git a/clustering/mock_test.go b/clustering/mock_test.go index 604443d4c..a8efa085b 100644 --- a/clustering/mock_test.go +++ b/clustering/mock_test.go @@ -2,6 +2,7 @@ package clustering import ( "context" + "database/sql" "errors" "fmt" "sort" @@ -447,6 +448,12 @@ func (m *mockMySQL) setRetrievedGTIDSet(gtid string) { m.status.ReplicaStatus.RetrievedGtidSet = gtid } +func (m *mockMySQL) setSecondsBehindSource(seconds int64) { + m.mu.Lock() + defer m.mu.Unlock() + m.status.ReplicaStatus.SecondsBehindSource = sql.NullInt64{Int64: seconds, Valid: true} +} + type mockOpFactory struct { orphaned int64 @@ -548,3 +555,8 @@ func (f *mockOpFactory) getKillConnectionsCount(name string) int { defer f.mu.Unlock() return f.countKillConnections[name] } + +func (f *mockOpFactory) setSecondsBehindSource(name string, seconds int64) { + m := f.getInstance(name) + m.setSecondsBehindSource(seconds) +} From 2e16074c851ac4c2bcb8ad6d78ca97cd9281c87b Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 19 Nov 2024 04:51:46 +0000 Subject: [PATCH 16/18] issue-747: Prevent switchover during replication delays --- clustering/process.go | 2 +- e2e/failover_test.go | 2 +- e2e/prevent_delete_test.go | 65 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) diff --git a/clustering/process.go b/clustering/process.go index d0b1eeebd..7d9be5ee5 100644 --- a/clustering/process.go +++ b/clustering/process.go @@ -218,7 +218,7 @@ func (p *managerProcess) do(ctx context.Context) (bool, error) { return false, nil case StateHealthy, StateDegraded: - if ss.NeedSwitch { + if ss.NeedSwitch && !ss.PreventPodDeletion { if err := p.switchover(ctx, ss); err != nil { event.SwitchOverFailed.Emit(ss.Cluster, p.recorder, err) return false, fmt.Errorf("failed to switchover: %w", err) diff --git a/e2e/failover_test.go b/e2e/failover_test.go index cadd97f30..f3caee390 100644 --- a/e2e/failover_test.go +++ b/e2e/failover_test.go @@ -14,7 +14,7 @@ import ( //go:embed testdata/failover.yaml var failoverYAML string -var _ = Context("failure", Ordered, func() { +var _ = Context("failover", Ordered, func() { if doUpgrade { return } diff --git a/e2e/prevent_delete_test.go b/e2e/prevent_delete_test.go index cb211e330..630b1a6ec 100644 --- a/e2e/prevent_delete_test.go +++ b/e2e/prevent_delete_test.go @@ -223,4 +223,69 @@ var _ = Context("PreventDelete", func() { }).Should(Succeed()) time.Sleep(30 * time.Second) }) + + It("should not finish switchover if replication delay occurs", func() { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + primary := cluster.Status.CurrentPrimaryIndex + + // set huge replication delay + setSourceDelay(0, 10000) + + // wait for prevent-delete annotation to be added + Eventually(func() error { + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + Expect(err).NotTo(HaveOccurred()) + pod := &corev1.Pod{} + err = json.Unmarshal(out, pod) + Expect(err).NotTo(HaveOccurred()) + if val, exists := pod.Annotations[constants.AnnPreventDelete]; !exists { + return errors.New("annotation is not added") + } else if val != "true" { + return fmt.Errorf("annotation value is not true: %s", val) + } + return nil + }).Should(Succeed()) + + // never finish switchover + kubectlSafe(nil, "moco", "switchover", "-n", "prevent-delete", "test") + Consistently(func() error { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + if cluster.Status.CurrentPrimaryIndex == primary { + return errors.New("switchover is not finished") + } + return nil + }, 1*time.Minute).ShouldNot(Succeed()) + + // resolve replication delay + setSourceDelay(0, 0) + + // wait for switchover to be finished + Eventually(func() error { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + if cluster.Status.CurrentPrimaryIndex == primary { + return errors.New("switchover is not finished") + } + return nil + }).Should(Succeed()) + + // wait for cluster to be healthy + Eventually(func() error { + cluster, err := getCluster("prevent-delete", "test") + Expect(err).NotTo(HaveOccurred()) + for _, cond := range cluster.Status.Conditions { + if cond.Type != mocov1beta2.ConditionHealthy { + continue + } + if cond.Status == metav1.ConditionTrue { + return nil + } + return fmt.Errorf("cluster is not healthy: %s", cond.Status) + } + return errors.New("no health condition") + }).Should(Succeed()) + time.Sleep(30 * time.Second) + }) }) From 4daa5e060488dbca8bf60fd169d5ae16e5aa9fed Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 19 Nov 2024 05:45:10 +0000 Subject: [PATCH 17/18] issue-747: fix typo --- clustering/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clustering/manager_test.go b/clustering/manager_test.go index 56eb86764..209a340c0 100644 --- a/clustering/manager_test.go +++ b/clustering/manager_test.go @@ -1016,7 +1016,7 @@ var _ = Describe("manager", func() { Expect(ms.backupWorkDirUsage).To(MetricsIs("==", 30)) Expect(ms.backupWarnings).To(MetricsIs("==", 2)) }) - It("shoud delect replication delay and prevent deletion of primary", func() { + It("should detect replication delay and prevent deletion of primary", func() { testSetupResources(ctx, 3, "") cm := NewClusterManager(1*time.Second, mgr, of, af, stdr.New(nil)) From f53773917da5eb57905cb204175b325dbe1ee8a0 Mon Sep 17 00:00:00 2001 From: shunki-fujita Date: Tue, 19 Nov 2024 09:13:21 +0000 Subject: [PATCH 18/18] issue-747: fix podname --- clustering/manager_test.go | 1 + e2e/prevent_delete_test.go | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/clustering/manager_test.go b/clustering/manager_test.go index 209a340c0..d619c934e 100644 --- a/clustering/manager_test.go +++ b/clustering/manager_test.go @@ -1016,6 +1016,7 @@ var _ = Describe("manager", func() { Expect(ms.backupWorkDirUsage).To(MetricsIs("==", 30)) Expect(ms.backupWarnings).To(MetricsIs("==", 2)) }) + It("should detect replication delay and prevent deletion of primary", func() { testSetupResources(ctx, 3, "") diff --git a/e2e/prevent_delete_test.go b/e2e/prevent_delete_test.go index 630b1a6ec..5f126336f 100644 --- a/e2e/prevent_delete_test.go +++ b/e2e/prevent_delete_test.go @@ -100,9 +100,9 @@ var _ = Context("PreventDelete", func() { // add prevent-delete annotation and wait for it to be removed for i := 0; i < 3; i++ { - kubectlSafe(nil, "annotate", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", i), "moco.cybozu.com/prevent-delete=true") + kubectlSafe(nil, "annotate", "pod", "-n", "prevent-delete", cluster.PodName(i), "moco.cybozu.com/prevent-delete=true") Eventually(func() error { - out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", i), "-o", "json") + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", cluster.PodName(i), "-o", "json") Expect(err).NotTo(HaveOccurred()) pod := &corev1.Pod{} err = json.Unmarshal(out, pod) @@ -119,7 +119,7 @@ var _ = Context("PreventDelete", func() { // wait for prevent-delete annotation to be added Eventually(func() error { - out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", cluster.PodName(primary), "-o", "json") Expect(err).NotTo(HaveOccurred()) pod := &corev1.Pod{} err = json.Unmarshal(out, pod) @@ -133,15 +133,15 @@ var _ = Context("PreventDelete", func() { }).Should(Succeed()) // fail to delete pod with prevent-delete annotation - _, err = kubectl(nil, "delete", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary)) - Expect(err.Error()).To(ContainSubstring("moco-test-%d is protected from deletion", primary)) + _, err = kubectl(nil, "delete", "pod", "-n", "prevent-delete", cluster.PodName(primary)) + Expect(err.Error()).To(ContainSubstring("%s is protected from deletion", cluster.PodName(primary))) // resolve replication delay setSourceDelay(0, 0) // wait for prevent-delete annotation to be removed Eventually(func() error { - out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", cluster.PodName(primary), "-o", "json") Expect(err).NotTo(HaveOccurred()) pod := &corev1.Pod{} err = json.Unmarshal(out, pod) @@ -163,7 +163,7 @@ var _ = Context("PreventDelete", func() { // wait for prevent-delete annotation to be added Eventually(func() error { - out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", cluster.PodName(primary), "-o", "json") Expect(err).NotTo(HaveOccurred()) pod := &corev1.Pod{} err = json.Unmarshal(out, pod) @@ -234,7 +234,7 @@ var _ = Context("PreventDelete", func() { // wait for prevent-delete annotation to be added Eventually(func() error { - out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", fmt.Sprintf("moco-test-%d", primary), "-o", "json") + out, err := kubectl(nil, "get", "pod", "-n", "prevent-delete", cluster.PodName(primary), "-o", "json") Expect(err).NotTo(HaveOccurred()) pod := &corev1.Pod{} err = json.Unmarshal(out, pod)