Repository: mariadb-operator/mariadb-operator

Issue:
[Bug] After switchover in semi-sync cluster, the new master still reports slave metrics 
**Documentation**
- [x] I acknowledge that I have read the relevant [documentation](https://github.com/mariadb-operator/mariadb-operator/tree/main/docs).

**Describe the bug**
I've noticed that after I trigger a switchover in a semi-sync cluster by updating the primary pod index, the new master still has `mariadb-operator` slave connection. It is not shown when you run `SHOW SLAVE STATUS` on the new master. But it is shown when you run `SHOW ALL SLAVES STATUS`. 

This is unfortunate because `mysqld_exporter` runs exactly [that](https://github.com/prometheus/mysqld_exporter/blob/main/collector/slave_status.go), so it starts reporting slave metrics on our new master, which will trigger fake prometheus alerts that use `mysql_slave_status_slave_sql_running == 0` rule or something similar.

Does it make sense to actually run [here](https://github.com/mariadb-operator/mariadb-operator/blob/09a7aa7910258b243fd0ee0d0c6a808381514917/pkg/sql/sql.go#L472) `RESET SLAVE 'mariadb-operator' ALL;`  like suggested [here](https://mariadb.com/kb/en/multi-source-replication/)? Manually running this command on the new master did solve the issue for me.

**Expected behaviour**
After performing a manual switchover, `SHOW ALL SLAVES STATUS` must show nothing on the new master to avoid creating fake metrics for `mysqld_exporter` that confues prometheus.

**Environment details**:
Kubernetes version: [1.26]
Kubernetes distribution: [Rancher]
mariadb-operator version: [0.36.0]
Install method: [helm]
Install flavor: [recommended]

Hints from issue comments:
@hedgieinsocks: Hm, after giving it another thought, `ResetAllSlaves()` is triggered within `ConfigurePrimary()` so it will break the initial configuration (and other scenarios) when there is no `mariadb-operator` connection yet, and thus raise error. But now I wonder whether `ResetAllSlaves()` currently does anything at all, or it completely misses the mark because `default_master_connection` is empty...@github-actions: This issue is stale because it has been open 60 days with no activity.@hedgieinsocks: Today Rancher buggy update resulted in restart of nodes on a dozen of our k8s clusters, which caused automatic failovers, and we had to manually reset named slave on 48 mariadb new masters to resolve all the alerts and find really broken clusters among them. @mmontes11 do you have any thoughts how this could be tackled?@mmontes11: Hey @hedgieinsocks ! Great to hear back from you.

> broken clusters among them.

Would love to see the symptoms, to see if we can extrapolate them to the most common replication issues reported so far:
- Unable to rebuild a replica because binary logs have been purged, or any other error replaying them  after executing `CHANGE MASTER`. To minimize this, we need to support automated replica rebuilds on the operator based on physical backups. This way, a replica will be started from the same physical state as the primary, and hopefully most of the binary log operations will be skipped.
- Switchover logic needs to be revisited. My plan is to monitor the lag and allow the user to define a threshold in which a replica can be considered up to date, and therefore allowing the operator could promote it as primary. There might be scenarios where it is not safe to proceed with the switchover i.e. all replicas stale, in such case the operator will explicitly mark the MariaDB status reflecting this.

The plan is to start supporting physical backups in next release in a new `PhysicalBackup` CR, which will be used internally for the automated replica rebuild.  As I said, it would be great to confirm if your issues fall inside the groups above, to better orient the new replication implementation.@hedgieinsocks: Thank you for your reply @mmontes11 ! I'm glad to hear this is being worked on! But my latest reply was about the fact that now any failover (automated or manually initiated) results in the master having the remnants of the slave connection hanging around, which confuses the alert system that relies on mysqld_exporter. And it's very annoying to manually run `RESET SLAVE 'mariadb-operator' ALL;` on the new master after each failover. You can imagine how even a clean failover results in prometheus firing alert and waking up people in the middle of the night to run this command manually. And if that happens for many clusters like in my example, you end up running around not knowing which failover was clean or which was broken until you go one after another cluster by cluster.@mmontes11: I've heard you, thanks again for your report, and sorry if this woke you up in the middle of the night.

We do reset the slaves when configuring one instance as primary:
https://github.com/mariadb-operator/mariadb-operator/blob/296047537da99e3a40291a5edc68d33294317392/pkg/controller/replication/config.go#L52C19-L52C33

> But now I wonder whether ResetAllSlaves() currently does anything at all, or it completely misses the mark because default_master_connection is empty...

Maybe there is something missing on the `ConfigurePrimary` func? I don't recall having recently changed it. What I do know is that we don't rely on the default connection at all, we use the custom `mariadb-operator` connection. This is something we could revisit in the next implementation, I doubt there is any benefit in using a non default connection. There is more, I think the MaxScale switchover may benefit from using the default one as well. As a quick test, you can tweak the `CHANGE MASTER` query to use the default connection and see what happens: 
https://github.com/mariadb-operator/mariadb-operator/blob/296047537da99e3a40291a5edc68d33294317392/pkg/sql/sql.go#L742

The way to appropriate tackling this issue is checking the metrics related to the `PrometheusRule` that is triggering the alert, and inspect the query that the mysqld_exporter is executing to export the metric used in the rule.

One more thing. I've seen a lot of progress on the MariaDB server side regarding replication. Which MariaDB server version are you using? Have you recently updated? @mmontes11: > Does it make sense to actually run [here](https://github.com/mariadb-operator/mariadb-operator/blob/09a7aa7910258b243fd0ee0d0c6a808381514917/pkg/sql/sql.go#L472) RESET SLAVE 'mariadb-operator' ALL; like suggested [here](https://mariadb.com/kb/en/multi-source-replication/)? Manually running this command on the new master did solve the issue for me.

Sorry, it seems like you are one step ahead of me debugging this. I also got the feeling that our custom connection is causing more problems than actually any benefit, I would try locally removing the custom connection name.@mmontes11: All the custom connection names should be in the SQL package:
https://github.com/mariadb-operator/mariadb-operator/tree/main/pkg/sql@hedgieinsocks: Gotcha, now when you confirmed that named connection is not something crucial (famous last words), I'll try not named connection. But I wonder whether that would be a breaking change for those who have already deployed clusters@mmontes11: > But I wonder whether that would be a breaking change for those who have already deployed clusters

The new implentation will have significant architectural changes, such as dropping the `StatefulSet` and use N independent `Deployments` with [strategy recreate ](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#recreate-deployment). The same way [rook-ceph](https://github.com/rook/rook) does for storage clusters.

We need to work on a migration guide anyway, this could be one more point to be migrated.

We could allow the user to provide the connection name, being nil by default, and allowing to keep using the named if needed. Maybe this is useful for migration.@hedgieinsocks: Wow, it sounded to me like next release or something. But it's only plans for now right? I mean I believe you suggested that I test no named connection flow and PR if it works? Or was your suggestion just to perform the test and report back to obtain knowledge for the future operator v.2@mmontes11: > Wow, it sounded to me like next release or something. But it's only plans for now right?

Only plans, but we will start working on `PhysicalBackups` shortly. 

>  I mean I believe you suggested that I test no named connection flow and PR if it works? 

Yes, that would be appreciated. If we conclude that the issues are related to the named connection, we could come up with a migration guide just for that. I'm happy to introduce this change prior the architectural change if it really solves the problem.

The migration steps may involve introducing a new field to customize the connection name.

Solution patch:
diff --git a/api/v1alpha1/base_types.go b/api/v1alpha1/base_types.go
index aa19a461..c8b47877 100644
--- a/api/v1alpha1/base_types.go
+++ b/api/v1alpha1/base_types.go
@@ -795,6 +795,10 @@ type Exporter struct {
 	// +optional
 	// +operator-sdk:csv:customresourcedefinitions:type=spec
 	ImagePullSecrets []LocalObjectReference `json:"imagePullSecrets,omitempty" webhook:"inmutable"`
+	// Args to be used in the Container.
+	// +optional
+	// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:advanced"}
+	Args []string `json:"args,omitempty"`
 	// Port where the exporter will be listening for connections.
 	// +optional
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:number"}
diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
index c84b9818..cbf2585d 100644
--- a/api/v1alpha1/zz_generated.deepcopy.go
+++ b/api/v1alpha1/zz_generated.deepcopy.go
@@ -945,6 +945,11 @@ func (in *Exporter) DeepCopyInto(out *Exporter) {
 		*out = make([]LocalObjectReference, len(*in))
 		copy(*out, *in)
 	}
+	if in.Args != nil {
+		in, out := &in.Args, &out.Args
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	if in.Resources != nil {
 		in, out := &in.Resources, &out.Resources
 		*out = new(ResourceRequirements)
diff --git a/config/crd/bases/k8s.mariadb.com_mariadbs.yaml b/config/crd/bases/k8s.mariadb.com_mariadbs.yaml
index 08c05cec..7360ebc9 100644
--- a/config/crd/bases/k8s.mariadb.com_mariadbs.yaml
+++ b/config/crd/bases/k8s.mariadb.com_mariadbs.yaml
@@ -3204,6 +3204,11 @@ spec:
                                     x-kubernetes-list-type: atomic
                                 type: object
                             type: object
+                          args:
+                            description: Args to be used in the Container.
+                            items:
+                              type: string
+                            type: array
                           image:
                             description: |-
                               Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
@@ -4030,6 +4035,11 @@ spec:
                                 x-kubernetes-list-type: atomic
                             type: object
                         type: object
+                      args:
+                        description: Args to be used in the Container.
+                        items:
+                          type: string
+                        type: array
                       image:
                         description: |-
                           Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
diff --git a/config/crd/bases/k8s.mariadb.com_maxscales.yaml b/config/crd/bases/k8s.mariadb.com_maxscales.yaml
index 7be2662a..c975fee5 100644
--- a/config/crd/bases/k8s.mariadb.com_maxscales.yaml
+++ b/config/crd/bases/k8s.mariadb.com_maxscales.yaml
@@ -1223,6 +1223,11 @@ spec:
                                 x-kubernetes-list-type: atomic
                             type: object
                         type: object
+                      args:
+                        description: Args to be used in the Container.
+                        items:
+                          type: string
+                        type: array
                       image:
                         description: |-
                           Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
diff --git a/deploy/charts/mariadb-operator-crds/templates/crds.yaml b/deploy/charts/mariadb-operator-crds/templates/crds.yaml
index 5aa25fe1..342628cf 100644
--- a/deploy/charts/mariadb-operator-crds/templates/crds.yaml
+++ b/deploy/charts/mariadb-operator-crds/templates/crds.yaml
@@ -4932,6 +4932,11 @@ spec:
                                     x-kubernetes-list-type: atomic
                                 type: object
                             type: object
+                          args:
+                            description: Args to be used in the Container.
+                            items:
+                              type: string
+                            type: array
                           image:
                             description: |-
                               Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
@@ -5758,6 +5763,11 @@ spec:
                                 x-kubernetes-list-type: atomic
                             type: object
                         type: object
+                      args:
+                        description: Args to be used in the Container.
+                        items:
+                          type: string
+                        type: array
                       image:
                         description: |-
                           Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
@@ -9004,6 +9014,11 @@ spec:
                                 x-kubernetes-list-type: atomic
                             type: object
                         type: object
+                      args:
+                        description: Args to be used in the Container.
+                        items:
+                          type: string
+                        type: array
                       image:
                         description: |-
                           Image name to be used as metrics exporter. The supported format is `<image>:<tag>`.
diff --git a/pkg/builder/deployment_builder.go b/pkg/builder/deployment_builder.go
index 7130ca39..03888222 100644
--- a/pkg/builder/deployment_builder.go
+++ b/pkg/builder/deployment_builder.go
@@ -259,6 +259,10 @@ func (b *Builder) exporterContainer(exporter *mariadbv1alpha1.Exporter, args []s
 		resources = exporter.Resources.ToKubernetesType()
 	}
 
+	if exporter.Args != nil {
+		args = append(args, exporter.Args...)
+	}
+
 	probe := &corev1.Probe{
 		ProbeHandler: corev1.ProbeHandler{
 			HTTPGet: &corev1.HTTPGetAction{
diff --git a/pkg/controller/replication/config.go b/pkg/controller/replication/config.go
index 0e0b9825..3f0bd891 100644
--- a/pkg/controller/replication/config.go
+++ b/pkg/controller/replication/config.go
@@ -20,9 +20,8 @@ import (
 )
 
 var (
-	replUser       = "repl"
-	replUserHost   = "%"
-	connectionName = "mariadb-operator"
+	replUser     = "repl"
+	replUserHost = "%"
 )
 
 type ReplicationConfig struct {
@@ -89,7 +88,7 @@ func (r *ReplicationConfig) ConfigureReplica(ctx context.Context, mariadb *maria
 	if err := r.changeMaster(ctx, mariadb, client, primaryPodIndex); err != nil {
 		return fmt.Errorf("error changing master: %v", err)
 	}
-	if err := client.StartSlave(ctx, connectionName); err != nil {
+	if err := client.StartSlave(ctx); err != nil {
 		return fmt.Errorf("error starting slave: %v", err)
 	}
 	return nil
@@ -158,7 +157,6 @@ func (r *ReplicationConfig) changeMaster(ctx context.Context, mariadb *mariadbv1
 
 	changeMasterOpts := []sql.ChangeMasterOpt{
 		changeMasterHostOpt,
-		sql.WithChangeMasterConnection(connectionName),
 		sql.WithChangeMasterPort(mariadb.Spec.Port),
 		sql.WithChangeMasterCredentials(replUser, password),
 		sql.WithChangeMasterGtid(gtidString),
diff --git a/pkg/controller/replication/switchover.go b/pkg/controller/replication/switchover.go
index bd9e3319..2a74a5ec 100644
--- a/pkg/controller/replication/switchover.go
+++ b/pkg/controller/replication/switchover.go
@@ -360,7 +360,7 @@ func (r *ReplicationReconciler) resetSlave(ctx context.Context, client *sqlClien
 	if err := client.ResetSlavePos(ctx); err != nil {
 		return fmt.Errorf("error resetting slave position: %v", err)
 	}
-	return client.StartSlave(ctx, connectionName)
+	return client.StartSlave(ctx)
 }
 
 func (r *ReplicationReconciler) currentPrimaryReady(ctx context.Context, mariadb *mariadbv1alpha1.MariaDB) (bool, error) {
diff --git a/pkg/sql/sql.go b/pkg/sql/sql.go
index 1f4527ec..3e73cb1f 100644
--- a/pkg/sql/sql.go
+++ b/pkg/sql/sql.go
@@ -634,9 +634,8 @@ func (c *Client) ResetMaster(ctx context.Context) error {
 	return c.Exec(ctx, "RESET MASTER;")
 }
 
-func (c *Client) StartSlave(ctx context.Context, connName string) error {
-	sql := fmt.Sprintf("START SLAVE '%s';", connName)
-	return c.Exec(ctx, sql)
+func (c *Client) StartSlave(ctx context.Context) error {
+	return c.Exec(ctx, "START SLAVE;")
 }
 
 func (c *Client) StopAllSlaves(ctx context.Context) error {
@@ -667,13 +666,12 @@ func (c *Client) WaitForReplicaGtid(ctx context.Context, gtid string, timeout ti
 }
 
 type ChangeMasterOpts struct {
-	Connection string
-	Host       string
-	Port       int32
-	User       string
-	Password   string
-	Gtid       string
-	Retries    int
+	Host     string
+	Port     int32
+	User     string
+	Password string
+	Gtid     string
+	Retries  int
 
 	SSLEnabled  bool
 	SSLCertPath string
@@ -683,12 +681,6 @@ type ChangeMasterOpts struct {
 
 type ChangeMasterOpt func(*ChangeMasterOpts)
 
-func WithChangeMasterConnection(connection string) ChangeMasterOpt {
-	return func(cmo *ChangeMasterOpts) {
-		cmo.Connection = connection
-	}
-}
-
 func WithChangeMasterHost(host string) ChangeMasterOpt {
 	return func(cmo *ChangeMasterOpts) {
 		cmo.Host = host
@@ -739,10 +731,9 @@ func (c *Client) ChangeMaster(ctx context.Context, changeMasterOpts ...ChangeMas
 
 func buildChangeMasterQuery(changeMasterOpts ...ChangeMasterOpt) (string, error) {
 	opts := ChangeMasterOpts{
-		Connection: "mariadb-operator",
-		Port:       3306,
-		Gtid:       "CurrentPos",
-		Retries:    10,
+		Port:    3306,
+		Gtid:    "CurrentPos",
+		Retries: 10,
 	}
 	for _, setOpt := range changeMasterOpts {
 		setOpt(&opts)
@@ -757,7 +748,7 @@ func buildChangeMasterQuery(changeMasterOpts ...ChangeMasterOpt) (string, error)
 		return "", errors.New("all SSL paths must be provided when SSL is enabled")
 	}
 
-	tpl := createTpl("change-master.sql", `CHANGE MASTER '{{ .Connection }}' TO
+	tpl := createTpl("change-master.sql", `CHANGE MASTER TO
 MASTER_HOST='{{ .Host }}',
 MASTER_PORT={{ .Port }},
 MASTER_USER='{{ .User }}',



Test patch:
diff --git a/pkg/builder/deployment_builder_test.go b/pkg/builder/deployment_builder_test.go
index 461971e6..7a47dfec 100644
--- a/pkg/builder/deployment_builder_test.go
+++ b/pkg/builder/deployment_builder_test.go
@@ -858,3 +858,68 @@ func TestExporterVolumes(t *testing.T) {
 		})
 	}
 }
+
+func TestExporterArgs(t *testing.T) {
+	builder := newDefaultTestBuilder(t)
+	objMeta := metav1.ObjectMeta{
+		Name:      "mariadb-metrics-resources",
+		Namespace: "test",
+	}
+
+	tests := []struct {
+		name     string
+		mariadb  *mariadbv1alpha1.MariaDB
+		wantArgs []string
+	}{
+		{
+			name: "Without args",
+			mariadb: &mariadbv1alpha1.MariaDB{
+				ObjectMeta: objMeta,
+				Spec: mariadbv1alpha1.MariaDBSpec{
+					Metrics: &mariadbv1alpha1.MariadbMetrics{
+						Enabled: true,
+					},
+				},
+			},
+			wantArgs: []string{
+				"--config.my-cnf=/etc/config/exporter.cnf",
+			},
+		},
+		{
+			name: "With args",
+			mariadb: &mariadbv1alpha1.MariaDB{
+				ObjectMeta: objMeta,
+				Spec: mariadbv1alpha1.MariaDBSpec{
+					Metrics: &mariadbv1alpha1.MariadbMetrics{
+						Enabled: true,
+						Exporter: mariadbv1alpha1.Exporter{
+							Args: []string{
+								"--no-collect.auto_increment.columns",
+								"--collect.sys.user_summary",
+							},
+						},
+					},
+				},
+			},
+			wantArgs: []string{
+				"--config.my-cnf=/etc/config/exporter.cnf",
+				"--no-collect.auto_increment.columns",
+				"--collect.sys.user_summary",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			deploy, err := builder.BuildExporterDeployment(tt.mariadb, nil)
+			if err != nil {
+				t.Fatalf("unexpected error building Deployment: %v", err)
+			}
+
+			args := deploy.Spec.Template.Spec.Containers[0].Args
+			if !reflect.DeepEqual(tt.wantArgs, args) {
+				t.Errorf("unexpected Args, want: %v got: %v", tt.wantArgs, args)
+			}
+		})
+	}
+}
diff --git a/pkg/sql/sql_test.go b/pkg/sql/sql_test.go
index 5354731f..c7fe85b0 100644
--- a/pkg/sql/sql_test.go
+++ b/pkg/sql/sql_test.go
@@ -41,7 +41,7 @@ func TestBuildChangeMasterQuery(t *testing.T) {
 				WithChangeMasterCredentials("repl", "password"),
 				WithChangeMasterGtid("CurrentPos"),
 			},
-			wantQuery: `CHANGE MASTER 'mariadb-operator' TO
+			wantQuery: `CHANGE MASTER TO
 MASTER_HOST='127.0.0.1',
 MASTER_PORT=3306,
 MASTER_USER='repl',
@@ -71,7 +71,7 @@ MASTER_CONNECT_RETRY=10;
 				WithChangeMasterGtid("CurrentPos"),
 				WithChangeMasterSSL("/etc/pki/client.crt", "/etc/pki/client.key", "/etc/pki/ca.crt"),
 			},
-			wantQuery: `CHANGE MASTER 'mariadb-operator' TO
+			wantQuery: `CHANGE MASTER TO
 MASTER_HOST='127.0.0.1',
 MASTER_PORT=3306,
 MASTER_USER='repl',