Implemented a node replace flow #658

RostislavPorohnya · 2023-12-22T15:04:11Z

No description provided.

DoodgeMatvey · 2023-12-22T15:05:25Z

config/samples/onpremises/clusters_v1beta1_cassandra.yaml

 spec:
-  name: "danylo-on-prem-cassandra"
+  name: "rostyslp-on-prem-cassandra"


Lets remove names from samples

DoodgeMatvey · 2023-12-22T15:14:05Z

controllers/clusters/on_premises.go

+	}
+
+	dvList := &cdiv1beta1.DataVolumeList{}
+	err = k8sClient.List(context.TODO(), dvList, &client.ListOptions{


Please change context.TODO() to ctx from functions arguments and change for all such cases

DoodgeMatvey · 2023-12-22T15:19:02Z

controllers/clusters/cassandra_controller.go

+					deleteUsed, err := deleteReplacedNodeResources(context.TODO(), r.Client, oldNode.ID)
+					if err != nil {
+						l.Error(err, "Cannot delete replaced node resources",
+							"old node id", oldNode.ID)
+
+						return err
+					}


Do you really need this deleteUsed? err == nill Isn't that an option for you?

Tried to remove deleteUsed var. What do you think about current approach?

DoodgeMatvey · 2023-12-22T15:20:04Z

controllers/clusters/on_premises.go

+	objectName := strings.ToLower(b.K8sObject.GetName())
+	vm := &virtcorev1.VirtualMachine{}
+
+	for i := 0; i < 20; i++ {


Lets put this 20 to the consts?

DoodgeMatvey · 2023-12-22T15:29:20Z

controllers/clusters/on_premises.go

+
+	var svcName string
+	if role == models.HeadlessServiceRoleLabel {
+		svcName = fmt.Sprintf("%s-%s", role, strings.ToLower(b.K8sObject.GetName()))


Why do you need this? strings.ToLower(b.K8sObject.GetName())) it should also work with identical name

You are right, I can just use GetName()

DoodgeMatvey · 2023-12-22T15:37:28Z

controllers/clusters/on_premises.go

+func getReplacedNodes(oldNodesDC, newNodesDC *v1beta1.DataCentreStatus) (newNode, oldNode *v1beta1.Node) {
+	for _, aNode := range oldNodesDC.Nodes {
+		for _, bNode := range newNodesDC.Nodes {
+			if aNode.ID == bNode.ID {
+				oldNode = nil
+				newNode = nil
+				break
+			}
+			oldNode = aNode
+			newNode = bNode
+		}
+
+		if oldNode != nil {
+			return
+		}
+	}
+
+	return nil, nil
+}


Can you simplify this func?

ribaraka · 2024-01-02T15:27:31Z

pkg/models/errors.go

@@ -70,4 +70,5 @@ var (
 	ErrDebeziumImmutable                          = errors.New("debezium array is immutable")
 	ErrCreateClusterWithMultiDC                   = errors.New("Multiple data center is still not supported. Please create a cluster with one data centre and add a second one when the cluster is in the running state")
 	ErrOnPremicesWithMultiDC                      = errors.New("on-premises cluster can be provisioned with only one data centre")
+	ErrGenerateAvailableVMNameFailed              = errors.New("failed to generate available VM name")


please change to ErrGenerateVMName

ribaraka · 2024-01-02T16:18:26Z

controllers/clusters/cassandra_controller.go

@@ -800,7 +800,67 @@ func (r *CassandraReconciler) newWatchStatusJob(c *v1beta1.Cassandra) scheduler.
 			return err
 		}

+		// TODO move everything not related to the watch status job to other places


It's better to create an issue for this and write a detailed description. For example what exactly is not related to the status job? What other places of unrelated things should be moved?

Created issue. #661

ribaraka · 2024-01-02T16:22:26Z

controllers/clusters/cassandra_controller.go

 		if !areStatusesEqual(&iCassandra.Status.ClusterStatus, &c.Status.ClusterStatus) {
+			if len(c.Status.DataCentres) != 0 &&
+				len(iCassandra.Status.DataCentres) != 0 {
+				bootstrap := newOnPremisesBootstrap(


This statement seems to be missing one more condition for on-premises check

Thanks. Added condition to check if cluster running on-premises

ribaraka · 2024-01-03T09:31:50Z

controllers/clusters/cassandra_controller.go

+					err = deleteReplacedNodeResources(ctx, r.Client, oldNode.ID)
+					if err != nil {
+						if !k8serrors.IsNotFound(err) {
+							l.Info("Node replace is in progress. Deleting old vm resources",
+								"cluster name", c.Name,
+								"old id", oldNode.ID)
+
+							return nil
+						}
+
+						l.Error(err, "Cannot delete replaced node resources",
+							"old node id", oldNode.ID)
+
+						return err
+					}
+
+					l.Info("Node replace is in progress. Creating new resources",


error handling seems off, please refactor

controllers/clusters/cassandra_controller.go

ribaraka · 2024-01-03T13:35:11Z

controllers/clusters/on_premises.go

+
+		return nil
+	}
+
+	return models.ErrGenerateVMName
+}


If there is no node to replace, we return an error - I think this is a no good way to end the func? At least return an error like "No node to replace".

Changed code a bit, now should look better

ribaraka · 2024-01-03T14:25:52Z

controllers/clusters/on_premises.go

+// Tries to delete old node resources and returns NotFound error if all resources already deleted
+func deleteReplacedNodeResources(ctx context.Context, k8sClient client.Client, oldID string) (deleteDone bool, err error) {


please, refactor the doc for the func.

testisnullus · 2024-01-05T12:08:48Z

controllers/clusters/on_premises.go

+		}
+	}
+
+	return false, nil


Should we return true, nil here?

testisnullus · 2024-01-05T12:13:57Z

controllers/clusters/on_premises.go

+		svc := &k8scorev1.Service{}
+		svcName := fmt.Sprintf("%s-%s", models.ClusterIPServiceRoleLabel, nodeName)


Why do we set a new label for a ClusterIP service only? Should we set updated labels for the LB service if the cluster is a public one?

testisnullus · 2024-01-05T12:20:00Z

controllers/clusters/on_premises.go

+		return vm, nil
+	}
+
+	return &vmList.Items[0], nil


Let's also check that len(vmList.Items) is equal to 1 maybe?

testisnullus · 2024-01-05T12:20:44Z

controllers/clusters/on_premises.go

-		}
-		if k8serrors.IsNotFound(err) {
-			headlessSVC = newHeadlessService(
+	return &dvList.Items[0], nil


The same check here

RostislavPorohnya added the enhancement New feature or request label Dec 22, 2023

RostislavPorohnya requested review from ribaraka and testisnullus December 22, 2023 15:04

RostislavPorohnya self-assigned this Dec 22, 2023

DoodgeMatvey requested changes Dec 22, 2023

View reviewed changes

RostislavPorohnya force-pushed the onpremises-provisioning-nodereplace branch from a225611 to 55b4430 Compare January 2, 2024 13:57

RostislavPorohnya requested a review from DoodgeMatvey January 2, 2024 13:57

DoodgeMatvey approved these changes Jan 2, 2024

View reviewed changes

RostislavPorohnya force-pushed the onpremises-provisioning-nodereplace branch from 55b4430 to b967cfb Compare January 2, 2024 14:49

ribaraka requested changes Jan 2, 2024

View reviewed changes

ribaraka reviewed Jan 3, 2024

View reviewed changes

RostislavPorohnya force-pushed the onpremises-provisioning-nodereplace branch from b967cfb to 48325d5 Compare January 3, 2024 12:04

RostislavPorohnya requested a review from ribaraka January 3, 2024 12:05

ribaraka requested changes Jan 3, 2024

View reviewed changes

Implemented a node replace flow

24db632

RostislavPorohnya force-pushed the onpremises-provisioning-nodereplace branch from 48325d5 to 24db632 Compare January 3, 2024 13:50

RostislavPorohnya requested a review from ribaraka January 3, 2024 13:52

ribaraka reviewed Jan 3, 2024

View reviewed changes

testisnullus requested changes Jan 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented a node replace flow #658

Implemented a node replace flow #658

RostislavPorohnya commented Dec 22, 2023

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Dec 22, 2023

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Dec 22, 2023

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Jan 2, 2024

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Jan 2, 2024

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Jan 2, 2024

DoodgeMatvey Dec 22, 2023

RostislavPorohnya Jan 2, 2024

RostislavPorohnya Jan 2, 2024

ribaraka Jan 2, 2024

RostislavPorohnya Jan 3, 2024

ribaraka Jan 2, 2024

RostislavPorohnya Jan 3, 2024

ribaraka Jan 2, 2024

RostislavPorohnya Jan 3, 2024

ribaraka Jan 3, 2024

RostislavPorohnya Jan 3, 2024

ribaraka Jan 3, 2024

RostislavPorohnya Jan 3, 2024

ribaraka Jan 3, 2024

testisnullus Jan 5, 2024

testisnullus Jan 5, 2024

testisnullus Jan 5, 2024

testisnullus Jan 5, 2024

		// Tries to delete old node resources and returns NotFound error if all resources already deleted
		func deleteReplacedNodeResources(ctx context.Context, k8sClient client.Client, oldID string) (deleteDone bool, err error) {

		svc := &k8scorev1.Service{}
		svcName := fmt.Sprintf("%s-%s", models.ClusterIPServiceRoleLabel, nodeName)

Implemented a node replace flow #658

Are you sure you want to change the base?

Implemented a node replace flow #658

Conversation

RostislavPorohnya commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment