DFC uninstall flow #55

ezio-auditore · 2023-05-04T03:53:14Z

checks for PVs using ocs storage classes and restricts delete if found
delete default storageclass claims
delete storageclient

controllers/datafoundationclient.go

bindrad

@ezio-auditore I don't see a function that will indicate whether the offering is safe to be removed or not. We should have dfcIsReadyToBeRemoved function which will be called in pluginIsReadyToBeRemoved function.

leelavg · 2023-05-09T02:22:00Z

I don't see a function that will indicate

good call, seems I need to look at agent code (standalone) before reviewing next PRs 😅

controllers/datafoundationclient.go

nb-ohad · 2023-05-16T10:17:56Z

controllers/datafoundationclient.go

+		}
+	}
+	pvList := &corev1.PersistentVolumeList{}
+	if err := r.list(pvList); err != nil {


I believe that this will only list PVs from the set of namespaces the operator group allows for it.
We need to list all PVs! for that I believe you can use the Unrestricted Client

Updated to fetch using UnrestrictedClient

nb-ohad · 2023-05-16T10:21:44Z

controllers/datafoundationclient.go

+	ocsStorageClass := map[string]bool{}
+	for i := range storageClassList.Items {
+		storageClass := storageClassList.Items[i]
+		if storageClass.Provisioner == r.offering.Namespace+ocsCephFsProvisionerSuffix || storageClass.Provisioner == r.offering.Namespace+ocsRBDProvisionerSuffix {
+			ocsStorageClass[storageClass.Name] = true
+		}
+	}


Why do we need to calculate the provisioners name on each loop iteration?

Suggested change

ocsStorageClass := map[string]bool{}

for i := range storageClassList.Items {

storageClass := storageClassList.Items[i]

if storageClass.Provisioner == r.offering.Namespace+ocsCephFsProvisionerSuffix || storageClass.Provisioner == r.offering.Namespace+ocsRBDProvisionerSuffix {

ocsStorageClass[storageClass.Name] = true

}

}

ocsStorageClass := map[string]bool{}

rbdProvisionerName = r.offering.Namespace + ocsRBDProvisionerSuffix

fsProvisionerName := r.offering.Namespace + ocsCephFsProvisionerSuffix

for i := range storageClassList.Items {

storageClass := storageClassList.Items[i]

if storageClass.Provisioner == rbdProvisionerName || storageClass.Provisioner == fsProvisionerName {

ocsStorageClass[storageClass.Name] = true

}

}

bindrad · 2023-05-18T05:40:25Z

controllers/datafoundationclient.go

+		r.Log.Info("issuing a delete for Default StorageClassClaims")
+		if err := r.delete(&r.defaultBlockStorageClassClaim); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to issue a delete for defaultBlockStorageClassClaim: %v", err)
+		}
+		if err := r.delete(&r.defaultFSStorageClassClaim); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to issue a delete for defaultFSStorageClassClaim: %v", err)
+		}


Shouldn't we delete non-default storageClassClaims as well that are provisioned by cephfs.csi.ceph.com or rbd.csi.ceph.com?

That is a very good question! @ezio-auditore what do you think?

What I think
If the user creates his custom storageClassClaim using the the default storageclient spec only then we need to delete those storageClassClaims,else we need to ignore it.

@nb-ohad @bindrad If this sounds ok I will implement the same here.

default storageclient spec

Are we even allowing customer to create non-default storageClient? Or do we have plans to allow customer to create non-default storageClients?

If the user creates his custom storageClassClaim using the the default storageclient spec

@ezio-auditore that does not make any sense. The reason someone will create another claim is if they want some aspects of the storageclass to be different

@nb-ohad, the ocs-client-operator is already checking for any PV before deleting the SCC. Why do we need to recheck the PV here again?

The question here was about SCC, I think we should just delete the lot of them

You mean list all the SCC, delete them, and wait for them to be deleted before deleting the storage client right?

Do we support Users creating custom storageclass and static provisioning without SCC in managed service?

Do we support Users creating custom storageclass and static provisioning without SCC in managed service?

On consumer clusters, customer can do anything they want to.

You mean list all the SCC, delete them, and wait for them to be deleted before deleting the storage client right?

I think we can issue a delete for all SCCs and then issue a delete for storageClient. No need to wait for the removal of all SCCs before deleting storageClient because client-operator won't delete storageClient unless all SCCs are removed.

@Madhu-1 I am aware the ocs-client-operator is already checking for that.
But a k8s delete operation is unrevertable so we check for potential lock conditions, from the operator that owns the lifecycle of the offering (in this case the client operator) before issuing the delete request.

controllers/datafoundationclient.go

Signed-off-by: Kaustav Majumder <[email protected]>

nb-ohad · 2023-05-22T07:28:04Z

controllers/datafoundationclient.go

 )

 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclients,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclassclaims,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=operators.coreos.com,resources=clusterserviceversions,verbs=get;list;watch;delete;update;patch
+//+kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch


Would this give us enough privileges to read and delete over all namespaces?

Yes, it gives us enough privileges

nb-ohad · 2023-05-22T07:29:23Z

controllers/datafoundationclient.go

 )

 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclients,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclassclaims,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=operators.coreos.com,resources=clusterserviceversions,verbs=get;list;watch;delete;update;patch
+//+kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch
+//+kubebuilder:rbac:groups="storage.k8s.io",resources=storageclass,verbs=get;list;watch


Why do we need access to storageclass? Doesn't uninstallation of ocs-client-operator already take care of deletion of storageclassclaims and as a result the attached storageclasses?

We need access to get, list and watch storageClass to fetch the PVs that are using ocs storageclasses

nb-ohad · 2023-05-22T07:30:18Z

controllers/datafoundationclient.go

+	r.defaultBlockStorageClassClaim.Name = defaultBlockStorageClassClaimName

-	r.defaultFSStorageClassClaim.Name = "ocs-storagecluster-cephfs"
+	r.defaultFSStorageClassClaim.Name = defaultFSStorageClassClaimName


Is there a reason why we are treating these storageclassclaims differently then ant other storagelcassclaim?

These are the default storageClassClaims that the agent is creating.

nb-ohad · 2023-05-23T12:04:45Z

controllers/datafoundationclient.go

+		r.Log.Info("issuing a delete for Default StorageClassClaims")
+		if err := r.delete(&r.defaultBlockStorageClassClaim); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to issue a delete for defaultBlockStorageClassClaim: %v", err)
+		}
+		if err := r.delete(&r.defaultFSStorageClassClaim); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to issue a delete for defaultFSStorageClassClaim: %v", err)
+		}


The existence of storageclassclaims or even storageclasses does not infer usage of storage. I am against blocking uninstallation based on that

nb-ohad · 2023-05-23T12:06:08Z

controllers/datafoundationclient.go

+	rbdProvisionerName := r.offering.Namespace + ocsRBDProvisionerSuffix
+	fsProvisionerName := r.offering.Namespace + ocsCephFsProvisionerSuffix
+	for i := range storageClassList.Items {
+		storageClass := storageClassList.Items[i]


To prevent a copy operation, please take a reference

Suggested change

storageClass := storageClassList.Items[i]

storageClass := &storageClassList.Items[i]

nb-ohad · 2023-05-23T12:06:56Z

controllers/datafoundationclient.go

+	rbdProvisionerName := r.offering.Namespace + ocsRBDProvisionerSuffix
+	fsProvisionerName := r.offering.Namespace + ocsCephFsProvisionerSuffix


The ocsRBDProvisionerSuffix and ocsCephFsProvisionerSuffix are only used once here. Can we just embed the values?

nb-ohad · 2023-05-23T12:07:35Z

controllers/datafoundationclient.go

+	}
+	pvList := &corev1.PersistentVolumeList{}
+	pvCount := 0
+	if err := r.UnrestrictedClient.List(r.ctx, pvList); err != nil {


I believe we had an r.UnrestrictedList(pvList) operation.

nb-ohad · 2023-05-23T12:07:56Z

controllers/datafoundationclient.go

+	if err := r.list(storageClassList); err != nil {
+		return false, fmt.Errorf("failed to list Storage Classes: %v", err)
+	}
+	ocsStorageClass := map[string]bool{}


Suggested change

ocsStorageClass := map[string]bool{}

ocsStorageClasses := map[string]bool{}

nb-ohad · 2023-05-23T12:10:50Z

controllers/datafoundationclient.go

+	if pvCount > 0 {
+		r.Log.Info(fmt.Sprintf("Current count of PVs blocking uninstallation: %v", pvCount))
+		return true, nil
+	}
+
+	return false, nil


It does not make sense to have logic/logging of messages base on the result of this func inside the func itself.
Let's have this function be countOCSVolumes which returns a number and handles the decision-making process outside.

nb-ohad · 2023-05-23T12:12:04Z

main.go

-		Log:    ctrl.Log.WithName("controllers").WithName("ManagedFusionOffering"),
-		Scheme: mgr.GetScheme(),
+		Client:             mgr.GetClient(),
+		UnrestrictedClient: getUnrestrictedClient(),


Can we just create the unrestricted client once and rescue it in all controllers?

ezio-auditore · 2023-07-24T04:44:12Z

closing since another PR #63 already addressing this

ezio-auditore force-pushed the uninstall_dfc branch from 29b0c2a to 0c5935f Compare May 4, 2023 03:54

rewantsoni reviewed May 4, 2023

View reviewed changes

controllers/datafoundationclient.go Show resolved Hide resolved

ezio-auditore force-pushed the uninstall_dfc branch from 0c5935f to af45798 Compare May 5, 2023 06:07

leelavg reviewed May 8, 2023

View reviewed changes

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

ezio-auditore force-pushed the uninstall_dfc branch from af45798 to 31b048f Compare May 8, 2023 08:36

ezio-auditore requested a review from leelavg May 8, 2023 10:22

leelavg previously approved these changes May 8, 2023

View reviewed changes

ezio-auditore requested a review from bindrad May 8, 2023 11:12

bindrad requested changes May 8, 2023

View reviewed changes

ezio-auditore dismissed leelavg’s stale review via 09c5614 May 8, 2023 12:24

ezio-auditore force-pushed the uninstall_dfc branch 2 times, most recently from 09c5614 to fea9ac2 Compare May 8, 2023 12:27

ezio-auditore requested a review from bindrad May 8, 2023 12:29

bindrad requested changes May 9, 2023

View reviewed changes

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

ezio-auditore force-pushed the uninstall_dfc branch from fea9ac2 to 6470baf Compare May 9, 2023 09:31

ezio-auditore requested a review from bindrad May 9, 2023 10:34

bindrad previously approved these changes May 9, 2023

View reviewed changes

ezio-auditore requested a review from nb-ohad May 9, 2023 11:45

ezio-auditore force-pushed the uninstall_dfc branch from 6470baf to 387e4d1 Compare May 10, 2023 08:50

nb-ohad requested changes May 11, 2023

View reviewed changes

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

controllers/datafoundationclient.go Outdated Show resolved Hide resolved

ezio-auditore dismissed bindrad’s stale review via 9e61b17 May 16, 2023 08:53

ezio-auditore force-pushed the uninstall_dfc branch from 387e4d1 to 9e61b17 Compare May 16, 2023 08:53

ezio-auditore requested a review from nb-ohad May 16, 2023 08:54

nb-ohad requested changes May 16, 2023

View reviewed changes

ezio-auditore force-pushed the uninstall_dfc branch 2 times, most recently from 9b9151c to e00697f Compare May 18, 2023 05:08

bindrad reviewed May 18, 2023

View reviewed changes

ezio-auditore force-pushed the uninstall_dfc branch from e00697f to 1649216 Compare May 18, 2023 11:12

DFC uninstall flow

5a30712

Signed-off-by: Kaustav Majumder <[email protected]>

ezio-auditore force-pushed the uninstall_dfc branch from 1649216 to 5a30712 Compare May 19, 2023 04:09

bindrad mentioned this pull request May 23, 2023

Uninstallation flow for DFC offering #63

Open

nb-ohad requested changes May 23, 2023

View reviewed changes

ezio-auditore closed this Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DFC uninstall flow #55

DFC uninstall flow #55

ezio-auditore commented May 4, 2023 •

edited

Loading

bindrad left a comment •

edited

Loading

leelavg commented May 9, 2023 •

edited

Loading

nb-ohad May 16, 2023

ezio-auditore May 18, 2023

nb-ohad May 16, 2023

ezio-auditore May 18, 2023

bindrad May 18, 2023

nb-ohad May 18, 2023

ezio-auditore May 18, 2023

bindrad May 18, 2023

nb-ohad May 18, 2023 •

edited

Loading

Madhu-1 May 25, 2023

Madhu-1 May 25, 2023

bindrad May 26, 2023

bindrad May 26, 2023

nb-ohad May 28, 2023

nb-ohad May 22, 2023

bindrad May 24, 2023

nb-ohad May 22, 2023

bindrad May 24, 2023

nb-ohad May 22, 2023

bindrad May 24, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

nb-ohad May 23, 2023

ezio-auditore commented Jul 24, 2023

	storageClass := storageClassList.Items[i]
	storageClass := &storageClassList.Items[i]

		rbdProvisionerName := r.offering.Namespace + ocsRBDProvisionerSuffix
		fsProvisionerName := r.offering.Namespace + ocsCephFsProvisionerSuffix

	ocsStorageClass := map[string]bool{}
	ocsStorageClasses := map[string]bool{}

DFC uninstall flow #55

DFC uninstall flow #55

Conversation

ezio-auditore commented May 4, 2023 • edited Loading

bindrad left a comment • edited Loading

Choose a reason for hiding this comment

leelavg commented May 9, 2023 • edited Loading

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

nb-ohad May 18, 2023 • edited Loading

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

ezio-auditore commented Jul 24, 2023

ezio-auditore commented May 4, 2023 •

edited

Loading

bindrad left a comment •

edited

Loading

leelavg commented May 9, 2023 •

edited

Loading

nb-ohad May 18, 2023 •

edited

Loading