honour PR comments

Author: shivaprasadmb

api/v1alpha1/ha_validators.go

@@ -49,11 +49,13 @@ func getHAValidator(dbType string) (HAParamsValidator, bool) {
 type MysqlHAParamsValidator struct{}
 
 // Validate checks MySQL-specific HA constraints. Fields validated:
-//   - haConfig.mysql           — must be present (required)
-//   - haConfig.mysql.innoDBClusterName — must be non-empty
-//   - haConfig.nodes[*].nodeType — must be "database" or "mysqlrouter"
-//   - haConfig.nodes           — exactly one database node must have role "Master"
-//   - haConfig.nodes           — mysqlrouter nodes must not have a role set
+//   - haConfig.mysql                    — must be present (required)
+//   - haConfig.mysql.innoDBClusterName  — must be non-empty
+//   - haConfig.nodes[*].nodeType        — must be "database" or "mysqlrouter"
+//   - haConfig.nodes                    — exactly one database node must have role "Master"
+//   - haConfig.nodes                    — mysqlrouter nodes must not have a role set
+//   - haConfig.mysql.deployMySQLRouter  — if true, at least one mysqlrouter node must be present;
+//     if false, no mysqlrouter nodes must be present
 func (v *MysqlHAParamsValidator) Validate(haConfig *InstanceHAConfig, haPath *field.Path, errors *field.ErrorList) {
 	myPath := haPath.Child("mysql")
 
@@ -69,8 +71,10 @@ func (v *MysqlHAParamsValidator) Validate(haConfig *InstanceHAConfig, haPath *fi
 			my.InnoDBClusterName, "innoDBClusterName must be specified"))
 	}
 
-	// Validate node types and enforce exactly one Master database node.
+	// Validate node types, enforce exactly one Master database node, and
+	// count mysqlrouter nodes for the deployMySQLRouter consistency check below.
 	masterCount := 0
+	routerCount := 0
 	for i, node := range haConfig.Nodes {
 		nodePath := haPath.Child("nodes").Index(i)
 		if node.NodeType != common.HA_NODE_TYPE_DATABASE && node.NodeType != common.HA_NODE_TYPE_MYSQLROUTER {
@@ -84,12 +88,25 @@ func (v *MysqlHAParamsValidator) Validate(haConfig *InstanceHAConfig, haPath *fi
 		if node.NodeType == common.HA_NODE_TYPE_DATABASE && node.Role == common.HA_NODE_ROLE_MASTER {
 			masterCount++
 		}
+		if node.NodeType == common.HA_NODE_TYPE_MYSQLROUTER {
+			routerCount++
+		}
 	}
 
 	if len(haConfig.Nodes) > 0 && masterCount != 1 {
 		*errors = append(*errors, field.Invalid(haPath.Child("nodes"), haConfig.Nodes,
 			"exactly one database node must have role 'Master'"))
 	}
+
+	// Enforce consistency between deployMySQLRouter and the nodes list.
+	if my.DeployMySQLRouter && routerCount == 0 {
+		*errors = append(*errors, field.Invalid(myPath.Child("deployMySQLRouter"), my.DeployMySQLRouter,
+			"deployMySQLRouter is true but no mysqlrouter nodes are present in haConfig.nodes"))
+	}
+	if !my.DeployMySQLRouter && routerCount > 0 {
+		*errors = append(*errors, field.Invalid(haPath.Child("nodes"), haConfig.Nodes,
+			"mysqlrouter nodes are present but deployMySQLRouter is false"))
+	}
 }
 
 // PostgresHAParamsValidator validates Patroni and HAProxy specific fields for Postgres HA.

api/v1alpha1/ha_validators_test.go

@@ -94,7 +94,7 @@ func TestMysqlHAParamsValidator_Validate(t *testing.T) {
 
 	t.Run("valid config with router enabled produces no errors", func(t *testing.T) {
 		haConfig := &InstanceHAConfig{
-			MySQL: validMySQLConfig(),
+			MySQL: &MySQLHAConfig{InnoDBClusterName: "innodb-cluster", DeployMySQLRouter: true},
 			Nodes: []InstanceHANode{mysqlRouterNode("router1"), mysqlRouterNode("router2"), masterNode("db1"), replicaNode("db2"), replicaNode("db3")},
 		}
 		errors := &field.ErrorList{}
@@ -112,6 +112,32 @@ func TestMysqlHAParamsValidator_Validate(t *testing.T) {
 		assert.Empty(t, *errors)
 	})
 
+	t.Run("deployMySQLRouter true but no router nodes returns invalid error", func(t *testing.T) {
+		haConfig := &InstanceHAConfig{
+			MySQL: &MySQLHAConfig{InnoDBClusterName: "innodb-cluster", DeployMySQLRouter: true},
+			Nodes: []InstanceHANode{masterNode("db1"), replicaNode("db2"), replicaNode("db3")},
+		}
+		errors := &field.ErrorList{}
+		validator.Validate(haConfig, haPath, errors)
+		assert.Len(t, *errors, 1)
+		assert.Equal(t, field.ErrorTypeInvalid, (*errors)[0].Type)
+		assert.Equal(t, "haConfig.mysql.deployMySQLRouter", (*errors)[0].Field)
+		assert.Contains(t, (*errors)[0].Detail, "no mysqlrouter nodes")
+	})
+
+	t.Run("router nodes present but deployMySQLRouter false returns invalid error", func(t *testing.T) {
+		haConfig := &InstanceHAConfig{
+			MySQL: validMySQLConfig(), // DeployMySQLRouter defaults to false
+			Nodes: []InstanceHANode{mysqlRouterNode("router1"), masterNode("db1"), replicaNode("db2")},
+		}
+		errors := &field.ErrorList{}
+		validator.Validate(haConfig, haPath, errors)
+		assert.Len(t, *errors, 1)
+		assert.Equal(t, field.ErrorTypeInvalid, (*errors)[0].Type)
+		assert.Equal(t, "haConfig.nodes", (*errors)[0].Field)
+		assert.Contains(t, (*errors)[0].Detail, "deployMySQLRouter is false")
+	})
+
 	t.Run("nil mysql returns required error and stops further node validation", func(t *testing.T) {
 		haConfig := &InstanceHAConfig{
 			MySQL: nil,
@@ -153,7 +179,7 @@ func TestMysqlHAParamsValidator_Validate(t *testing.T) {
 
 	t.Run("mysqlrouter node with role set returns error", func(t *testing.T) {
 		haConfig := &InstanceHAConfig{
-			MySQL: validMySQLConfig(),
+			MySQL: &MySQLHAConfig{InnoDBClusterName: "innodb-cluster", DeployMySQLRouter: true},
 			Nodes: []InstanceHANode{
 				{VmName: "router1", NodeType: common.HA_NODE_TYPE_MYSQLROUTER, Role: "Master", ClusterName: "cluster-a"},
 				masterNode("db1"),

api/v1alpha1/webhook_helpers.go

@@ -322,18 +322,18 @@ func initializeObjects(spec *DatabaseSpec) {
 			}
 		}
 		if pg := spec.Instance.HAConfig.Postgres; pg != nil {
-			if pg.WritePort == 0 {
+			if pg.WritePort <= 0 {
 				pg.WritePort = common.HA_PROXY_DEFAULT_WRITE_PORT
 			}
-			if pg.ReadPort == 0 {
+			if pg.ReadPort <= 0 {
 				pg.ReadPort = common.HA_PROXY_DEFAULT_READ_PORT
 			}
 		}
 		if my := spec.Instance.HAConfig.MySQL; my != nil {
-			if my.RouterRWPort == 0 {
+			if my.RouterRWPort <= 0 {
 				my.RouterRWPort = common.HA_MYSQL_DEFAULT_RW_PORT
 			}
-			if my.RouterROPort == 0 {
+			if my.RouterROPort <= 0 {
 				my.RouterROPort = common.HA_MYSQL_DEFAULT_RO_PORT
 			}
 			if my.MySQLClusterUsername == "" {

controllers/database_reconciler_helpers.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 
 	ndbv1alpha1 "github.com/nutanix-cloud-native/ndb-operator/api/v1alpha1"
@@ -122,13 +123,24 @@ func (r *DatabaseReconciler) handleDelete(ctx context.Context, req ctrl.Request,
 	log.Info("Database CR is being deleted")
 	instanceManager := getInstanceManager(*database)
 	if controllerutil.ContainsFinalizer(database, common.FINALIZER_INSTANCE) {
-		// Check if the deregistration operation id (database.Status.DeregistrationOperationId) is empty
-		// If so, then make a deprovisionDatabase API call to NDB
-		// else proceed check for the operation completion before removing finalizer.
+		// Check if the deregistration operation id (database.Status.DeregistrationOperationId) is empty.
+		// If so, make a deprovisionDatabase API call to NDB,
+		// else poll for operation completion before removing the finalizer.
 		deregistrationOperationId := database.Status.DeregistrationOperationId
 		if deregistrationOperationId == "" {
 			deregistrationOp, err := instanceManager.deregister(ctx, r, ndbClient, database)
 			if err != nil {
+				// If NDB reports the database is already gone (e.g. manually deleted via NDB UI),
+				// skip deregistration and proceed to remove the finalizer.
+				if strings.Contains(err.Error(), "ERA-ENT-0000001") {
+					log.Info("Database already removed from NDB, skipping deregistration and removing finalizer " + common.FINALIZER_INSTANCE)
+					r.recorder.Event(database, "Normal", EVENT_DEREGISTRATION_COMPLETED, "Database was already removed from NDB; skipping deregistration.")
+					controllerutil.RemoveFinalizer(database, common.FINALIZER_INSTANCE)
+					if err := r.Update(ctx, database); err != nil {
+						return requeueOnErr(err)
+					}
+					return requeue()
+				}
 				// Not logging here, already done in the deregister function
 				return requeueOnErr(err)
 			}
@@ -215,6 +227,21 @@ func (r *DatabaseReconciler) handleSync(ctx context.Context, database *ndbv1alph
 		databaseStatus.Id = taskResponse.EntityId
 		databaseStatus.CreationOperationId = taskResponse.OperationId
 		r.recorder.Event(database, "Normal", EVENT_CREATION_STARTED, "Database creation initiated on NDB")
+
+		// Persist CREATING status immediately and return — do not fall through to the
+		// External Sync block below. The NDBServer CR won't have this DB yet (NDB just
+		// started the operation), so the sync block would overwrite the status to
+		// NOT_FOUND before it is ever written to the CR.
+		database.Status = *databaseStatus
+		base := database.DeepCopy()
+		base.Status = ndbv1alpha1.DatabaseStatus{}
+		if err := r.Status().Patch(ctx, database, client.MergeFrom(base)); err != nil {
+			errStatement := "Failed to update status of database custom resource after provisioning"
+			log.Error(err, errStatement)
+			r.recorder.Eventf(database, "Warning", EVENT_CR_STATUS_UPDATE_FAILED, "Error: %s. %s.", err.Error())
+			return requeueOnErr(err)
+		}
+		return requeueWithTimeout(common.DATABASE_RECONCILE_INTERVAL_SECONDS)
 	}
 
 	// Handle External Sync
@@ -253,9 +280,9 @@ func (r *DatabaseReconciler) handleSync(ctx context.Context, database *ndbv1alph
 	}
 
 	if !reflect.DeepEqual(database.Status, *databaseStatus) {
+		base := database.DeepCopy()
 		database.Status = *databaseStatus
-		err := r.Status().Update(ctx, database)
-		if err != nil {
+		if err := r.Status().Patch(ctx, database, client.MergeFrom(base)); err != nil {
 			errStatement := "Failed to update status of database custom resource"
 			log.Error(err, errStatement)
 			r.recorder.Eventf(database, "Warning", EVENT_CR_STATUS_UPDATE_FAILED, "Error: %s. %s.", err.Error())
@@ -314,14 +341,12 @@ func (r *DatabaseReconciler) handleSync(ctx context.Context, database *ndbv1alph
 // extra services (e.g. -ro-svc for Postgres/MySQL) via the HAConnectivityManager registry.
 //
 // ndbClient is used only when the read-only endpoint needs different IPs than the primary
-// (currently: MySQL HA router-disabled, where the -ro-svc targets Replica VMs instead of the Master).
 func (r *DatabaseReconciler) setupConnectivity(ctx context.Context, database *ndbv1alpha1.Database, ndbClient ndb_client.NDBClientHTTPInterface) (err error) {
 	log := ctrllog.FromContext(ctx)
 	log.Info("Entered database_reconciler_helpers.setupConnectivity")
 
 	ns := database.Namespace
 
-	// Primary IPs come from Status.IPAddress (set by the HAIPResolver during NDBServer sync).
 	primaryIPs := strings.Split(database.Status.IPAddress, ",")
 
 	// Determine the port for the primary -svc and whether an HA manager is needed.
@@ -333,6 +358,20 @@ func (r *DatabaseReconciler) setupConnectivity(ctx context.Context, database *nd
 		primaryPort = haManager.PrimaryPort(database.Spec.Instance.HAConfig)
 	}
 
+	// For MySQL HA router-disabled, all nodes (Master and Replicas) listen on the same port.
+	// If the user has overridden listener_port in additionalArguments, honour it.
+	mysqlRouterDisabled := database.Spec.Instance != nil &&
+		database.Spec.Instance.HAConfig != nil &&
+		database.Spec.Instance.HAConfig.MySQL != nil &&
+		!database.Spec.Instance.HAConfig.MySQL.DeployMySQLRouter
+	if mysqlRouterDisabled {
+		if portStr, ok := database.Spec.Instance.AdditionalArguments["listener_port"]; ok {
+			if port, parseErr := strconv.ParseInt(portStr, 10, 32); parseErr == nil && port > 0 {
+				primaryPort = int32(port)
+			}
+		}
+	}
+
 	// All databases get a primary -svc pointing to the primary (RW) IPs.
 	if err = r.setupServiceAndEndpoints(ctx, database, database.Name+"-svc", ns, primaryPort, primaryIPs); err != nil {
 		return
@@ -342,23 +381,24 @@ func (r *DatabaseReconciler) setupConnectivity(ctx context.Context, database *nd
 	if haManager != nil {
 		for _, svcSpec := range haManager.AdditionalServices(database.Spec.Instance.HAConfig) {
 			roIPs := primaryIPs
-
-			// MySQL HA router-disabled: the -ro-svc must target Replica VMs, not the Master.
-
-			if database.Spec.Instance.Type == common.DATABASE_TYPE_MYSQL &&
-				database.Spec.Instance.HAConfig.MySQL != nil &&
-				!database.Spec.Instance.HAConfig.MySQL.DeployMySQLRouter &&
-				database.Status.DatabaseServerId != "" {
-				replicaIPs, resolveErr := ndb_api.GetMySQLReplicaIPsFromDPC(ctx, ndbClient, database.Status.DatabaseServerId)
-				if resolveErr != nil {
-					log.Error(resolveErr, "Failed to resolve MySQL Replica IPs for -ro-svc; falling back to primary IPs")
-				} else if len(replicaIPs) > 0 {
-					roIPs = replicaIPs
-					log.Info("MySQL HA router-disabled: using Replica IPs for -ro-svc", "ips", roIPs)
+			roPort := svcSpec.Port
+
+			// MySQL HA router-disabled: the -ro-svc must target Replica VMs, not the Master,
+			// and must use the same effective listener port as the primary -svc.
+			if mysqlRouterDisabled {
+				roPort = primaryPort
+				if database.Status.DatabaseServerId != "" {
+					replicaIPs, resolveErr := ndb_api.GetMySQLReplicaIPsFromDPC(ctx, ndbClient, database.Status.DatabaseServerId)
+					if resolveErr != nil {
+						log.Error(resolveErr, "Failed to resolve MySQL Replica IPs for -ro-svc; falling back to primary IPs")
+					} else if len(replicaIPs) > 0 {
+						roIPs = replicaIPs
+						log.Info("MySQL HA router-disabled: using Replica IPs for -ro-svc", "ips", roIPs)
+					}
 				}
 			}
 
-			if err = r.setupServiceAndEndpoints(ctx, database, database.Name+svcSpec.NameSuffix, ns, svcSpec.Port, roIPs); err != nil {
+			if err = r.setupServiceAndEndpoints(ctx, database, database.Name+svcSpec.NameSuffix, ns, roPort, roIPs); err != nil {
 				return
 			}
 		}
@@ -432,7 +472,6 @@ func (r *DatabaseReconciler) setupService(ctx context.Context, database *ndbv1al
 
 // buildEndpointAddresses converts a slice of IP strings into Kubernetes EndpointAddresses.
 // Empty and whitespace-only entries are skipped. Each unique IP becomes a separate
-// EndpointAddress so kube-proxy load-balances across all of them.
 func buildEndpointAddresses(ips []string) []corev1.EndpointAddress {
 	addrs := make([]corev1.EndpointAddress, 0, len(ips))
 	for _, ip := range ips {

controllers/instance_manager.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	ndbv1alpha1 "github.com/nutanix-cloud-native/ndb-operator/api/v1alpha1"
 	"github.com/nutanix-cloud-native/ndb-operator/common"
@@ -122,6 +123,13 @@ func (dm *DatabaseManager) deleteDatabaseServer(ctx context.Context, r *Database
 			// before giving up, making it robust against the common 409-Conflict case.
 			opIds, apiErr := ndb_api.DeprovisionHADatabaseServers(ctx, ndbClient, database.Status.DatabaseServerId)
 			if apiErr != nil {
+				// If NDB reports the DB server is already gone (e.g. cluster manually deleted
+				// via NDB UI), skip deprovision and signal done so the finalizer is removed.
+				if strings.Contains(apiErr.Error(), "ERA-ENT-0000001") {
+					log.Info("HA DB server already removed from NDB, skipping deprovision and removing finalizer " + common.FINALIZER_DATABASE_SERVER)
+					r.recorder.Eventf(database, "Normal", EVENT_DEREGISTRATION_COMPLETED, "HA DB server was already removed from NDB; skipping deprovision.")
+					return true, nil
+				}
 				log.Error(apiErr, "Failed to deprovision one or more HA database servers")
 				r.recorder.Eventf(database, "Warning", EVENT_DEREGISTRATION_FAILED, "Error: %s", apiErr.Error())
 				return false, apiErr

ndb_api/db_helpers.go

@@ -408,12 +408,9 @@ func (a *MySqlRequestAppender) appendProvisioningRequest(req *DatabaseProvisionR
 		"auto_tune_staging_drive": "true",
 	}
 
-	// Appending/overwriting database actionArguments to actionArguments
-	if err := setConfiguredActionArguments(database, actionArguments); err != nil {
-		return nil, err
-	}
-
-	// HA-specific request overrides (override req fields and merge HA action arguments)
+	// HA-specific request overrides (override req fields and merge HA action arguments).
+	// This runs before setConfiguredActionArguments so that user-provided additionalArguments
+	// take final precedence over the HA defaults below.
 	if database.IsMysqlHA() {
 		haConfig := database.GetInstanceHAConfig()
 
@@ -468,6 +465,12 @@ func (a *MySqlRequestAppender) appendProvisioningRequest(req *DatabaseProvisionR
 		}
 	}
 
+	// Apply user-provided additionalArguments last so they override both the base
+	// defaults and the HA defaults merged above.
+	if err := setConfiguredActionArguments(database, actionArguments); err != nil {
+		return nil, err
+	}
+
 	// Converting action arguments map to list and appending to req.ActionArguments
 	req.ActionArguments = append(req.ActionArguments, convertMapToActionArguments(actionArguments)...)