Introduce maintenance mode #482

chanchiwai-ray · 2024-12-19T08:20:44Z

Description

MicroCeph currently supports scaling up and down, but not maintenance. In the actual operation, the operators may also need to perform regular maintenance to the storage nodes, and MicroCeph does not offer a simple solution to this problem. We would like to introduce sub-commands (e.g. microceph cluster maintenance enter and microceph cluster maintenance exit) to MicroCeph to enable the feature.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
CleanCode (Code refactor, test updates, does not introduce functional changes)
Documentation update (Doc only change)

How Has This Been Tested?

NOTE: All functional changes should accompany corresponding tests (unit tests, functional tests etc).

Please describe the addition/modification of tests done to verify this change. Please also list any relevant details for your test configuration.

Contributor's Checklist

Please check that you have:

self-reviewed the code in this PR.
added code comments, particularly in hard-to-understand areas.
updated the user documentation with corresponding changes.
added tests to verify effectiveness of this change.

Add one endpoint `/osds` to enable stopping and starting snap service for cluster members. The endpoint only accept PUT method. Signed-off-by: Chi Wai Chan <[email protected]>

Introduce two subcommands: `microceph cluster maintenance enter` and `microceph cluster maintenance exit` allow cloud operators to do maintenance operations on the node without affecting the ceph cluster. Signed-off-by: Chi Wai Chan <[email protected]>

jneo8

Overall no big issue. Only some suggestions.

microceph/ceph/maintenance.go

microceph/ceph/osd.go

microceph/ceph/maintenance.go

Signed-off-by: Chi Wai Chan <[email protected]>

jneo8

Thanks, LGTM.

jneo8 · 2024-12-20T08:06:53Z

microceph/api/osds.go

@@ -0,0 +1,51 @@
+package api


suggestion (non-blocking):

It will be nice if you can separate the API part to another PR, which may be more easy for reviewer.

sabaini

Hey @chanchiwai-ray thanks for this, think this is a sound approach! I've did a first pass on review and left some suggestions / nits.

Also it would be good to have functional testing on this too.

microceph/api/osds.go

microceph/api/types/osds.go

microceph/ceph/operations.go

sabaini · 2024-12-20T11:29:14Z

microceph/ceph/operations.go

+
+// DryRun prints out the action plan.
+func (o *CheckOsdOkToStopOps) DryRun(name string) string {
+	return fmt.Sprintf("Check if osds in node '%s' are ok-to-stop.", name)


Suggest to list which OSDs exactly are going to be stopped

I believe this is addressed

Signed-off-by: Chi Wai Chan <[email protected]>

sabaini

Hey @jneo8 thanks for these!
Some suggestions inline.

I'd also like to suggest running an API test against the newly created /osds endpoint. See the hurl() function and tests/hurl dir. I think a simple dry-run test would be sufficient.

.github/workflows/tests.yml

microceph/ceph/operations.go

sabaini · 2025-01-09T17:12:33Z

microceph/ceph/operations.go

+}
+
+// Run checks if a node is in the microceph cluster.
+func (o *CheckNodeInClusterOps) Run(name string) error {


I wonder if its necessary to go via the API client here. At this point we're in server code and could call the resp. handler directly I think.
Similarly in the below client calls

Sorry, I am not sure what do you mean here, can you elaborate more?

At this point we're in server code

The operations are not run in server side (i.e. microcephd)

Oh apologies, missed that the operations are run client-side.

As a rule we try to make as much logic as possible available server-side. Many clients such as charm-microceph (and therefore sunbeam) control microceph via the API so for the sake of those clients we probably would need to have an API for this.

Should be addressed in the recent commits

microceph/cmd/microceph/cluster_maintenance_exit.go

tests/scripts/actionutils.sh

Signed-off-by: Chi Wai Chan <[email protected]>

This test only check if it will handle PUT requests expectedly. Signed-off-by: Chi Wai Chan <[email protected]>

samuelallan72 · 2025-01-14T01:35:38Z

microceph/cmd/microceph/cluster_maintenance_enter.go

+	if c.flagSetNoout {
+		operations = append(operations, []ceph.Operation{
+			&ceph.SetNooutOps{ClusterOps: clusterOps},
+			&ceph.AssertNooutFlagSetOps{ClusterOps: clusterOps},
+		}...)
+	}
+
+	// optionally stop osd service
+	if c.flagStopOsds {
+		operations = append(operations, []ceph.Operation{
+			&ceph.StopOsdOps{ClusterOps: clusterOps},
+		}...)
+	}


I'm curious, if these are optional, what actually happens if the user runs microceph maintenance enter --set-noout=false --stop-osds=false? What does maintenance mode mean without those?

it will then be only checks

samuelallan72 · 2025-01-14T01:39:00Z

microceph/ceph/osd.go

@@ -954,27 +954,55 @@ func outDownOSD(osd int64) error {
 	return nil
 }

-func safetyCheckStop(osd int64) error {
+func setOsdNooutFlag(set bool) error {


This might be clearer as two functions:

func setOsdNooutFlag() error func UnsetOsdNooutFlag() error

Then we don't need to deal with a boolean argument.

That's my initial implementation, but it turns out it's just simple to add a boolean argument to remove the all-most-identical code. Moreover, I think the function name is a bit clear now: "set the osd noout flag" true means set the flag, false means don't set the flag.

samuelallan72 · 2025-01-14T01:42:44Z

microceph/ceph/operations.go

+func (o *UnsetNooutOps) Run(name string) error {
+	err := setOsdNooutFlag(false)
+	if err != nil {
+		return err
+	}
+	logger.Info("unset osd noout.")
+	return nil
+}
+
+// DryRun prints out the action plan.
+func (o *UnsetNooutOps) DryRun(name string) string {
+	return "Run `ceph osd unset noout`."
+}


There's a little bit of abstraction leaking between Run and DryRun here: Run uses the setOsdNooutFlag() function, but DryRun prints the lower level ceph command used. Just an observation. Not sure how nitpicky this is, or the best way to go - perhaps inlining the setOsdNooutFlag function to remove the extra layer of indirection?

The DryRun function should give you the actual action performed, not the string representation of the function

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray added 2 commits December 19, 2024 12:33

Add an api endpoint for microcephd

90e55c2

Add one endpoint `/osds` to enable stopping and starting snap service for cluster members. The endpoint only accept PUT method. Signed-off-by: Chi Wai Chan <[email protected]>

Add maintenance mode to microceph

e45b338

Introduce two subcommands: `microceph cluster maintenance enter` and `microceph cluster maintenance exit` allow cloud operators to do maintenance operations on the node without affecting the ceph cluster. Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray force-pushed the maintenance-mode branch from f342430 to e45b338 Compare December 19, 2024 08:29

jneo8 reviewed Dec 19, 2024

View reviewed changes

Apply suggestion.

0d05a7f

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray requested a review from jneo8 December 20, 2024 07:23

jneo8 approved these changes Dec 20, 2024

View reviewed changes

sabaini requested changes Dec 20, 2024

View reviewed changes

chanchiwai-ray added 5 commits January 2, 2025 11:03

Drop "Location" in in OsdPut struct

4b9b476

Signed-off-by: Chi Wai Chan <[email protected]>

Fix lint issues and apply error message convention

759e960

Signed-off-by: Chi Wai Chan <[email protected]>

Small refactoring: using embedding

23953b5

Signed-off-by: Chi Wai Chan <[email protected]>

Show which osds are going to be stoped.

5d850e1

Signed-off-by: Chi Wai Chan <[email protected]>

Skip the error if forced to enter maintenance mode

81663ad

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray force-pushed the maintenance-mode branch 2 times, most recently from 88b1d19 to 6d1aa77 Compare January 3, 2025 13:06

Add function tests for maintenance mode

c184eea

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray force-pushed the maintenance-mode branch from 6d1aa77 to c184eea Compare January 6, 2025 04:14

chanchiwai-ray added 5 commits January 6, 2025 15:30

Add more test cases

be111c3

Signed-off-by: Chi Wai Chan <[email protected]>

Add some buffer for the cluster to settle

dadbbf9

Signed-off-by: Chi Wai Chan <[email protected]>

Add some buffer between each maintenance mode ops.

b60de4e

Signed-off-by: Chi Wai Chan <[email protected]>

Increase buffer and add more debug info.

01d8064

Signed-off-by: Chi Wai Chan <[email protected]>

Fix thinko in --force logic

7902e4c

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray marked this pull request as ready for review January 7, 2025 01:23

chanchiwai-ray requested a review from sabaini January 7, 2025 01:23

chanchiwai-ray mentioned this pull request Jan 8, 2025

Add maintenance mode actions canonical/charm-microceph#135

Draft

9 tasks

sabaini requested changes Jan 9, 2025

View reviewed changes

chanchiwai-ray added 2 commits January 10, 2025 12:20

Address comments for non-functional change.

53e85e1

Signed-off-by: Chi Wai Chan <[email protected]>

More explict about the name.

829d5c2

Signed-off-by: Chi Wai Chan <[email protected]>

chanchiwai-ray requested a review from sabaini January 10, 2025 06:04

Add test for /osds endpoint.

6172c4c

This test only check if it will handle PUT requests expectedly. Signed-off-by: Chi Wai Chan <[email protected]>

samuelallan72 reviewed Jan 14, 2025

View reviewed changes

chanchiwai-ray added 3 commits January 15, 2025 21:43

Introduce ops/maintenance endpoint.

02b4268

Signed-off-by: Chi Wai Chan <[email protected]>

Use ProxyTarget=true for the endpoint.

4985f28

Signed-off-by: Chi Wai Chan <[email protected]>

Remove unuse tests.

20cec7b

Signed-off-by: Chi Wai Chan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce maintenance mode #482

Introduce maintenance mode #482

chanchiwai-ray commented Dec 19, 2024 •

edited

Loading

jneo8 left a comment

jneo8 left a comment

jneo8 Dec 20, 2024

sabaini left a comment

sabaini Dec 20, 2024

chanchiwai-ray Jan 16, 2025

sabaini left a comment

sabaini Jan 9, 2025

chanchiwai-ray Jan 10, 2025

sabaini Jan 10, 2025

chanchiwai-ray Jan 16, 2025

samuelallan72 Jan 14, 2025

chanchiwai-ray Jan 15, 2025

samuelallan72 Jan 14, 2025

chanchiwai-ray Jan 16, 2025

samuelallan72 Jan 14, 2025

chanchiwai-ray Jan 16, 2025

Introduce maintenance mode #482

Are you sure you want to change the base?

Introduce maintenance mode #482

Conversation

chanchiwai-ray commented Dec 19, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Contributor's Checklist

jneo8 left a comment

Choose a reason for hiding this comment

jneo8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabaini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabaini left a 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

chanchiwai-ray commented Dec 19, 2024 •

edited

Loading