make purge-osd interactive with preflight checks and step-by-step prompt by taraasrita10 · Pull Request #455 · red-hat-storage/odf-cli

taraasrita10 · 2026-11-05T15:06:45Z

This change replaces the existing purge-osd command with a fully interactive process that walks the user through each step of OSD removal with pre-flight checks and step-by-step prompts.

The following steps take place as a part of the interactive purge flow -

OSD selection (selectUnhealthyOSD) - Scans the cluster for down OSDs and presents a list to the user, who picks the OSD to be purged interactively instead of passing it as an argument.
LSO preflight check (checkLSOScaledDown) - Verifies the LSO operator is scaled down before proceeding.
Cluster health check (clusterHealthCheck) - Checks cluster health before purge. If the cluster is not healthy, the output is shown and user is prompted to confirm before continuing.
OSD detail gathering (gatherOSDDetails) - Queries the OSD deployment, PVC, and PV from k8s to collect the host name, PVC/PV names, device path and encryption state before deletion begins and to show the user a summary of resources being removed.
OSD process stop (stopOSDIfRunning) - Scales the OSD deployment to 0. Skips if already at 0.
Mark OSD out (markOSDOut) - Marks the OSD out and triggers data migration. Skips if already marked out from a previous run.
Data migration wait (dataMigrationPrompt) - Prompts to wait for data migration. If yes, polls safeToDestroy every 15 seconds till it returns true (incase of an exit, prints a resume message as the previous steps are idempotent). If no, sets force=true so Rook skips the safeToDestroy check internally.
Dry run summary (printDryRun): Lists all k8s and Ceph resources that will be deleted (deployment, PVC, PV, DM crypt mapping, device) before the final confirmation prompt.
Final confirm and next steps: Asks for confirmation before calling rook.PurgeOsds. On success, prints the PVC pending status and instructions for scaling LSO back up after disk replacement.

subhamkrai

Can you also, share your testing result or SS of output?

subhamkrai · 2026-11-06T08:07:58Z

-	_, err := strconv.Atoi(osdID)
+// selectUnhealthyOSD queries ceph for all down OSDs and prompts the user to pick one.
+// Exits if the cluster cannot be reached or no unhealthy OSDs are found.
+func selectUnhealthyOSD(ctx context.Context, clientsets *k8sutil.Clientsets, operatorNS, storageNS string) int {


Let's move all the implementation details under pkg/, it could be new under ``pkg/ceph/osd_purge.go

subhamkrai · 2026-11-06T08:09:19Z

+		clientsets := root.ClientSets
+		operatorNS := root.OperatorNamespace
+		storageNS := root.StorageClusterNamespace


Suggested change

clientsets := root.ClientSets

operatorNS := root.OperatorNamespace

storageNS := root.StorageClusterNamespace

we can directly use root.* any reason for above change?

subhamkrai · 2026-11-06T08:13:44Z

+		if details.pvcName != "" {
+			fmt.Printf("PVC %q is now Pending (awaiting disk).\n", details.pvcName)
+		}
+		fmt.Println("\nNext steps:")


Suggested change

fmt.Println("\nNext steps:")

fmt.Println("Next steps:")

instead of \n can we use Println so that we can avoid \n

subhamkrai · 2026-11-06T08:13:52Z

+			fmt.Printf("PVC %q is now Pending (awaiting disk).\n", details.pvcName)
+		}
+		fmt.Println("\nNext steps:")
+		fmt.Println("  Replace the physical disk and scale the LSO operator back up.")


Suggested change

fmt.Println(" Replace the physical disk and scale the LSO operator back up.")

fmt.Println("Replace the physical disk and scale the LSO operator back up.")

subhamkrai · 2026-11-06T08:14:32Z

+	}
+
+	if len(unhealthy) == 0 {
+		logging.Fatal(fmt.Errorf("no unhealthy OSDs found"))


Suggested change

logging.Fatal(fmt.Errorf("no unhealthy OSDs found"))

logging.Fatal(fmt.Errorf("no unhealthy OSDs found to be purged"))

subhamkrai · 2026-11-06T08:15:15Z

+		logging.Fatal(fmt.Errorf("invalid selection"))
+	}
+
+	return unhealthy[choice-1].OSD


Suggested change

return unhealthy[choice-1].OSD

return unhealthy[choice1].OSD

Is '- 1 ' before the OSD ID?

The index into the unhealthy slice is not related to the OSD ID. Don't you just want to return the OSD ID? Also, you need to confirm that the ID is in the unhealthy slice.

Suggested change

return unhealthy[choice-1].OSD

return choice

choice is the index, not the ID. Therefore return unhealthy[choice-1].OSD would return the OSD ID. The user is prompted to enter the index according to the listed unhealthy OSDs, not the OSD ID. But I think it would be better if they were prompted to enter the ID. Will make that change.

subhamkrai · 2026-11-06T08:15:39Z

+// checkLSOScaledDown verifies that the LSO operator is scaled down before purging.
+// If LSO is running, it could reprovision the disk we are about to wipe.
+// Returns false if LSO is running or its status cannot be verified.
+func checkLSOScaledDown(ctx context.Context, clientsets *k8sutil.Clientsets) bool {


Users need to scale down the LSO, right? We are not scalling it down ourself.

Yes, the users need to scale it down. We would just be verifying if it has been scaled down. If not, we'd stop the purge and ask the user to scale it down, and exit.

subhamkrai · 2026-11-06T08:16:32Z

+
+// clusterHealthCheck runs `ceph health detail` and prompts the user if the cluster
+// is not healthy. Returns false if the user chooses to cancel.
+func clusterHealthCheck(ctx context.Context, clientsets *k8sutil.Clientsets, operatorNS, storageNS string) bool {


We should having something in rook to ceph health see if we can use existing method if not we can have the current change.

Found IsCephHealthy in Rook, but it returns a boolean. We need the health err/warning to display it to the user so that they can decide whether to proceed with the purge.

subhamkrai · 2026-11-06T08:18:23Z

+	if _, err := clientsets.Kube.AppsV1().Deployments(storageNS).Update(ctx, deployment, metav1.UpdateOptions{}); err != nil {
+		logging.Fatal(fmt.Errorf("failed to stop OSD deployment %q: %v", details.deploymentName, err))
+	}
+	fmt.Printf("Stopped osd.%d process.\n", details.id)


Suggested change

fmt.Printf("Stopped osd.%d process.\n", details.id)

Logger.Info("Stopped osd.%d process.\n", details.id)

travisn · 2026-11-06T18:03:29Z

+	}
+
+	if len(unhealthy) == 0 {
+		logging.Fatal(fmt.Errorf("no unhealthy OSDs found"))


We need to be clear in the error message how they can force an OSD to be unhealthy so they can proceed to remove it if desired.

Suggested change

logging.Fatal(fmt.Errorf("no unhealthy OSDs found"))

logging.Fatal(fmt.Errorf("no unhealthy OSDs found to be purgred, to force remove an OSD that is running, first scale down the OSD deployment. "))

travisn · 2026-11-06T18:07:30Z

+		logging.Fatal(fmt.Errorf("invalid selection"))
+	}
+
+	return unhealthy[choice-1].OSD


The index into the unhealthy slice is not related to the OSD ID. Don't you just want to return the OSD ID? Also, you need to confirm that the ID is in the unhealthy slice.

Suggested change

return unhealthy[choice-1].OSD

return choice

travisn · 2026-11-06T18:11:05Z

+	lsoNS := "openshift-local-storage"
+	deployment, err := clientsets.Kube.AppsV1().Deployments(lsoNS).Get(ctx, "local-storage-operator", metav1.GetOptions{})
+	if err != nil {
+		logging.Warning("could not verify LSO operator status: %v. Scale it down before running this command.", err)


Why exactly does the LSO operator need to be scaled down? Does it only need to be scaled down while we are running this cleanup operation? If so, perhaps we offer to scale it down before we start, and scale it up when we are done. But if there is some other side effect like re-provisioning the same volume after it is clean, the user would need clear instructions to decide when to scale it back up.

We are scaling the LSO operator down only during the cleanup operation because if not scaled down, the disk being wiped would get re-provisioned again.
We're not offering to scale it down because -

The operator can be scaled back up only after the disk has been replaced. The time taken for that is not definite.

The user would be instructed to scale the operator down, then run the command. Upon success, the command exits with instructions about when and how to scale the LSO operator back up. Incase the user would like to replace the disk, they can scale it back up after replacing it. Incase they would like to redeploy the same disk, then they can scale it back up as soon as the purge is complete and the command exits.

travisn · 2026-11-06T18:13:06Z

+		logging.Fatal(fmt.Errorf("could not determine node for osd.%d: PV %q has no node affinity", osdID, details.pvName))
+	}
+
+	fmt.Printf("\nOSD Details:\n")


Is this duplicate for what is printed by printDryRun()?

Yes, I have removed it now.

travisn · 2026-11-06T18:14:14Z

+	if _, err := clientsets.Kube.AppsV1().Deployments(storageNS).Update(ctx, deployment, metav1.UpdateOptions{}); err != nil {
+		logging.Fatal(fmt.Errorf("failed to stop OSD deployment %q: %v", details.deploymentName, err))
+	}
+	fmt.Printf("Stopped osd.%d process.\n", details.id)


The OSD pod might not actually be stopped at this point. The pod could take a few seconds to scale down, so we should wait for it.

Making the changes

travisn · 2026-11-06T18:15:40Z

+// If yes: polls safeToDestroy every 15 seconds with Ctrl+C support for graceful exit.
+// If no: returns true so the caller passes force=true to the purge.
+func dataMigrationPrompt(ctx context.Context, clientsets *k8sutil.Clientsets, operatorNS, storageNS, osdIDStr string, osdID int) bool {
+	if !promptYN("Wait for data migration to complete?") {


We need more details in the question. What data migration is in progress? Do we know data migration is in progress? If PGs are already active+clean, then we don't need this prompt either.

I've added more details to the question. The data migration in question is the one that happens when an OSD is marked out, and data is migrated from the OSD to other nodes. I've also added a ceph status check, which sets force=true when PGs are in an active+clean state (and the safeToDestroy check is failing). In case they are not in active+clean, then the user is prompted to approve skipping data migration.

openshift-ci · 2026-11-06T18:15:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

…mpts This change replaces the existing purge-osd command with a fully interactive process that walks the user through each step of OSD removal. The user is prompted to choose the OSD to purge, check current cluster health, collect the details of resources to be deleted, stop the osd, mark it out, wait for data migration, check if the osd is safe to destroy and affirm the purge. After the purge takes place, the user is guided to scale the LSO operator back up after disk replacement. Signed-off-by: Prabhala Tara Aasrita <taraprabhala@ibm.com>

taraasrita10 · 2026-11-11T20:05:24Z

@subhamkrai @travisn I've made the suggested changes, PTAL. Thanks!

travisn

Can you also, share your testing result or SS of output?

As Subham requested, please share the output of a test run, to see what the flow looks like from the user's perspective. This can help see what to refine for user clarity.

travisn · 2026-11-13T18:53:43Z

-	PreRunE: validateOsdID,
+	Use:   "purge-osd",
+	Short: "Permanently remove an OSD from the cluster.",
+	Long: `Safely remove an OSD from the cluster with interactive prompts.


@subhamkrai Such a detailed long description is not expected, right?

travisn · 2026-11-13T18:55:39Z

+		}
+	}
+	logging.Fatal(fmt.Errorf("OSD %d is not unhealthy. Only down OSDs can be purged.", osdID))
+	return -1


linter feedback ^

travisn · 2026-11-13T18:56:48Z

+// Returns false if LSO is running or its status cannot be verified.
+func CheckLSOScaledDown(ctx context.Context, clientsets *k8sutil.Clientsets) bool {
+	lsoNS := "openshift-local-storage"
+	deployment, err := clientsets.Kube.AppsV1().Deployments(lsoNS).Get(ctx, "local-storage-operator", metav1.GetOptions{})


Is there a way to override this requirement? It doesn't seem strictly necessary to stop the LSO operator before continuing to purge the OSD.

travisn · 2026-11-13T18:57:45Z

+		return true
+	}
+
+	logging.Warning("%s", out)


What warning is this going to print? We may need to give more context that helps them decide if they want to continue.

travisn · 2026-11-13T18:59:37Z

+		fmt.Println()
+		fmt.Println("Next steps:")
+		fmt.Println("Replace the physical disk and scale the LSO operator back up.")
+		fmt.Println("If reusing the same disk: scale the LSO operator back up.")


If they want to re-use the same disk, seems like we should give them the option in the first place to not stop the LSO operator.

travisn · 2026-11-13T19:01:33Z

+		return true
+	}
+
+	if !PromptYN("The OSD process is currently running. Stop it before proceeding?") {


Suggested change

if !PromptYN("The OSD process is currently running. Stop it before proceeding?") {

if !PromptYN("The OSD pod is currently running. Stop it before proceeding?") {

travisn · 2026-11-13T19:05:57Z

+
+	// Wait for pod to actually terminate
+	logging.Info("Waiting for OSD pod to terminate...")
+	deadline := time.Now().Add(1 * time.Minute)


Always use UTC time in code to avoid any issues with time zones, daylight savings, etc.

Suggested change

deadline := time.Now().Add(1 * time.Minute)

deadline := time.Now().UTC().Add(1 * time.Minute)

travisn · 2026-11-13T19:09:15Z

+	for {
+		select {
+		case <-sigCh:
+			fmt.Printf("\nosd.%d is marked out. Re-run this command to resume the purge.\n", osdID)


This signal indicates that the CLI tool was canceled, not that the OSD was marked out, right? This message seems wrong.

taraasrita10 changed the title ~~make purge-osd interactive with preflight checks and step-by-step pro…~~ make purge-osd interactive with preflight checks and step-by-step prompt Nov 6, 2026

subhamkrai requested changes Nov 6, 2026

View reviewed changes

openshift-ci Bot assigned subhamkrai Nov 6, 2026

travisn requested changes Nov 6, 2026

View reviewed changes

openshift-ci Bot assigned travisn Nov 6, 2026

taraasrita10 force-pushed the purge-osd-9490 branch from 3f05535 to f8242de Compare November 11, 2026 20:04

travisn requested changes Nov 13, 2026

View reviewed changes

	clientsets := root.ClientSets
	operatorNS := root.OperatorNamespace
	storageNS := root.StorageClusterNamespace

	fmt.Println(" Replace the physical disk and scale the LSO operator back up.")
	fmt.Println("Replace the physical disk and scale the LSO operator back up.")

	logging.Fatal(fmt.Errorf("no unhealthy OSDs found"))
	logging.Fatal(fmt.Errorf("no unhealthy OSDs found to be purged"))

	fmt.Printf("Stopped osd.%d process.\n", details.id)
	Logger.Info("Stopped osd.%d process.\n", details.id)

	if !PromptYN("The OSD process is currently running. Stop it before proceeding?") {
	if !PromptYN("The OSD pod is currently running. Stop it before proceeding?") {

	deadline := time.Now().Add(1 * time.Minute)
	deadline := time.Now().UTC().Add(1 * time.Minute)

Conversation

taraasrita10 commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subhamkrai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Nov 6, 2026

Uh oh!

taraasrita10 commented Nov 11, 2026

Uh oh!

travisn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

taraasrita10 commented Nov 5, 2026 •

edited

Loading