storageClusterPeer: add logic for storageclusterpeer controller #2678

rewantsoni · 2024-07-01T05:39:46Z

add logic for storage cluster peer controller

Depends on:

Part of RHSTOR-4886

(review only the last two commits)

rewantsoni · 2024-07-01T09:37:30Z

/retest-required

config/rbac/role.yaml

controllers/util/k8sutil.go

api/v1/storageclusterpeer_types.go

controllers/storageclusterpeer/storageclusterpeer_controller.go

controllers/storagecluster/reconcile.go

controllers/storageclusterpeer/storageclusterpeer_controller.go

rewantsoni · 2024-11-06T14:28:32Z

/retest-required

controllers/storagecluster/reconcile.go

controllers/storageclusterpeer/storageclusterpeer_controller.go

openshift-ci · 2024-11-07T06:46:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nb-ohad · 2024-11-07T15:43:22Z

controllers/storagecluster/reconcile.go

+	storageClusterPeerList := ocsv1.StorageClusterPeerList{}
+	if err := r.List(ctx, &storageClusterPeerList, client.InNamespace(sc.Namespace)); err != nil {
+		return reconcile.Result{}, err
+	}
+	for i := range storageClusterPeerList.Items {
+		storageClusterPeer := &storageClusterPeerList.Items[i]
+		if err = controllerutil.SetOwnerReference(sc, storageClusterPeer, r.Client.Scheme()); err != nil {
+			return reconcile.Result{}, fmt.Errorf("failed to set owner reference on storageClusterPeer %s: %w", storageClusterPeer.Name, err)
+		}
+	}


This should not be located here. This should be part of a reconcile function that runs as part of the storage cluster main reconcile

nb-ohad · 2024-11-07T15:48:47Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+		r.log.Error(err, "failed to fetch StorageCluster for StorageClusterPeer found in the same namespace.")
+		if k8serrors.IsNotFound(err) {
+			storageClusterPeer.Status.State = ocsv1.ReconcileFailed
+			_ = r.updateStatus(storageClusterPeer)


Don't update the status inside the reconciliation, have a central point outside of the reconciliation that ensures that no matter how we exist the reconcile a status update will happen. Ee take a similar approach in all of our controllers across the majority of our projects.

If you don't do that then all contributors need to always make sure they are sending an update status comment on every return. This is erroneous.

nb-ohad · 2024-11-07T15:49:54Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+		storageClusterPeer.Status.State = ocsv1.ReconcileFailed
+		return ctrl.Result{}, err


As a continuation of the my last comment, here is an example of a missed update that will not get persistent

nb-ohad · 2024-11-07T15:51:31Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	}
+	storageClusterPeer.Status.State = ocsv1.StorageClusterPeerStateInitializing
+
+	peerStorageClusterUID, err := readStorageClusterUIDFromTicket(storageClusterPeer.Spec.OnboardingToken)


we use token not ticket in code

This is only needed once. Why is there a need for another function to be invoked?

Suggested change

peerStorageClusterUID, err := readStorageClusterUIDFromTicket(storageClusterPeer.Spec.OnboardingToken)

peerStorageClusterUID, err := readStorageClusterUIDFromTicket(storageClusterPeer.Spec.OnboardingToken)

nb-ohad · 2024-11-07T15:53:06Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	if storageClusterPeer.Status.PeerInfo == nil {
+		storageClusterPeer.Status.PeerInfo = &ocsv1.PeerInfo{}
+	}
+	storageClusterPeer.Status.PeerInfo.StorageClusterUid = string(peerStorageClusterUID)


If you are updating the storage cluster uid in the peer info then why do you assume all other information should not needs to be wiped out? If the existing storage cluster uid is different then the old info is meaningless and even misleading

nb-ohad · 2024-11-07T15:53:52Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+	_, err = ocsClient.PeerStorageCluster(r.ctx, storageClusterPeer.Spec.OnboardingToken, string(storageCluster.UID))
+	if err != nil {
+		r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, code: %v.", err))


Suggested change

r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, code: %v.", err))

r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, reason: %v.", err))

nb-ohad · 2024-11-07T15:54:51Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	_, err = ocsClient.PeerStorageCluster(r.ctx, storageClusterPeer.Spec.OnboardingToken, string(storageCluster.UID))
+	if err != nil {
+		r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, code: %v.", err))
+		st, _ := status.FromError(err)


You should not assume zero errors.

nb-ohad · 2024-11-07T15:55:38Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+func (r *StorageClusterPeerReconciler) updateStatus(obj client.Object) error {
+	return r.Client.Status().Update(r.ctx, obj)
+}


Do we really need this? Status update should be located at a single place in the code when you exist the reconcile logic and should only run once for each reconcile.

nb-ohad · 2024-11-07T15:56:29Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	var ticketData services.OnboardingTicket
+	err = json.Unmarshal(message, &ticketData)
+	if err != nil {
+		return "", fmt.Errorf("failed to unmarshal onboarding ticket message. %v", err)


Suggested change

return "", fmt.Errorf("failed to unmarshal onboarding ticket message. %v", err)

return "", fmt.Errorf("Onboarding ticket message is not a valid JSON. %v", err)

Signed-off-by: Rewant Soni <[email protected]>

rchikatw reviewed Jul 8, 2024

View reviewed changes

config/rbac/role.yaml Outdated Show resolved Hide resolved

controllers/util/k8sutil.go Outdated Show resolved Hide resolved

leelavg reviewed Jul 16, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2024

rewantsoni force-pushed the scp-controller branch from 1ea99b9 to 274a469 Compare July 29, 2024 06:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2024

rewantsoni force-pushed the scp-controller branch 6 times, most recently from c31ebbe to cd4371e Compare July 31, 2024 10:56

leelavg reviewed Aug 9, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2024

rewantsoni force-pushed the scp-controller branch from cd4371e to 47ad908 Compare October 10, 2024 09:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2024

rewantsoni force-pushed the scp-controller branch from 47ad908 to 4b3c038 Compare October 10, 2024 09:39

leelavg reviewed Oct 24, 2024

View reviewed changes

rewantsoni force-pushed the scp-controller branch 4 times, most recently from 0fe696a to cfbc4bf Compare November 5, 2024 15:03

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2024

rewantsoni force-pushed the scp-controller branch from cfbc4bf to 34be7a1 Compare November 5, 2024 15:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2024

rewantsoni force-pushed the scp-controller branch 2 times, most recently from 44b0276 to dc8a088 Compare November 6, 2024 13:51

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024

rewantsoni force-pushed the scp-controller branch from dc8a088 to 4f94c9e Compare November 6, 2024 13:56

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024

nb-ohad requested changes Nov 6, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Nov 6, 2024

rewantsoni force-pushed the scp-controller branch from 4f94c9e to d69f6e7 Compare November 7, 2024 06:46

rewantsoni force-pushed the scp-controller branch 8 times, most recently from 8d456a4 to 99e35e9 Compare November 7, 2024 15:09

nb-ohad requested changes Nov 7, 2024

View reviewed changes

rewantsoni force-pushed the scp-controller branch 4 times, most recently from c37c951 to 44f8993 Compare November 7, 2024 17:06

storageclusterpeer: implement logic for storageclusterpeer

ae29829

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the scp-controller branch from 44f8993 to ae29829 Compare November 8, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storageClusterPeer: add logic for storageclusterpeer controller #2678

storageClusterPeer: add logic for storageclusterpeer controller #2678

rewantsoni commented Jul 1, 2024 •

edited

Loading

rewantsoni commented Jul 1, 2024

rewantsoni commented Nov 6, 2024

openshift-ci bot commented Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

nb-ohad Nov 7, 2024

		storageClusterPeer.Status.State = ocsv1.ReconcileFailed
		return ctrl.Result{}, err

	peerStorageClusterUID, err := readStorageClusterUIDFromTicket(storageClusterPeer.Spec.OnboardingToken)
	peerStorageClusterUID, err := readStorageClusterUIDFromTicket(storageClusterPeer.Spec.OnboardingToken)

	r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, code: %v.", err))
	r.log.Error(err, fmt.Sprintf("failed to Peer Storage Cluster, reason: %v.", err))

	return "", fmt.Errorf("failed to unmarshal onboarding ticket message. %v", err)
	return "", fmt.Errorf("Onboarding ticket message is not a valid JSON. %v", err)

storageClusterPeer: add logic for storageclusterpeer controller #2678

Are you sure you want to change the base?

storageClusterPeer: add logic for storageclusterpeer controller #2678

Conversation

rewantsoni commented Jul 1, 2024 • edited Loading

rewantsoni commented Jul 1, 2024

rewantsoni commented Nov 6, 2024

openshift-ci bot commented Nov 7, 2024

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

rewantsoni commented Jul 1, 2024 •

edited

Loading