[wip] osd: reweight osd while resizing #14435

parth-gr · 2024-07-08T15:14:54Z

osd got resized by cryptsetup bluestore cmd
but it should also be reweight to balance the pgs properly

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn · 2024-07-08T23:24:31Z

I'm thinking that instead of modifying the init container, we should check this during the osd reconcile. For example:

The OSD reconcile starts
Query for the crush weights of all OSDs
As each OSD is reconciled, if the weight does not equal the size of the PVC (in TiB), then execute the command:

ceph osd crush set osd.<id> <expected_weight> <crush_location_properties>

This way, we just check it during reconcile, and no need to modify the init containers to check it every time OSDs start.

This will follow a similar pattern to #14056, which should also check the OSD device class during reconcile. The pattern for updating these OSD properties will be very similar.

parth-gr · 2024-07-10T08:55:05Z

@travisn would this be the right place

rook/pkg/operator/ceph/cluster/osd/osd.go

Line 369 in 8241a22

err = setOSDProperties(c, osdProps, osd)

, for checking the ceph osd df and osd PVC size

parth-gr · 2024-07-10T09:34:31Z

But the actuall update happen here

rook/pkg/operator/ceph/cluster/osd/update.go

Line 208 in 8241a22

    
           failures := updateMultipleDeploymentsAndWaitFunc(c.cluster.clusterInfo.Context, c.cluster.context.Clientset, updatedDeployments, listFunc)

So we should do after this?

parth-gr · 2024-07-10T15:12:09Z

Or it should be here https://github.com/rook/rook/blob/master/pkg/operator/ceph/cluster/osd/osd.go#L274

BlaineEXE · 2024-07-10T15:24:41Z

pkg/operator/ceph/cluster/osd/spec.go

+	osdResize := `
+	DM_name=%s
+	cryptsetup --verbose status "$DM_name"
+	ceph osd crush reweight osd.0 1


Is this supposed to only be osd.0?

This was prototyping, we are going a different direction now.

travisn

Or it should be here https://github.com/rook/rook/blob/master/pkg/operator/ceph/cluster/osd/osd.go#L274

As discussed, let's do it here since the osd reconcile is all done at this point and we can check all the OSDs for the needed weights and device classes.

travisn · 2024-07-10T18:53:02Z

pkg/operator/ceph/cluster/osd/spec.go

+	osdResize := `
+	DM_name=%s
+	cryptsetup --verbose status "$DM_name"
+	ceph osd crush reweight osd.0 1


This was prototyping, we are going a different direction now.

osd got resized by cryptsetup bluestore cmd but it should also be reweight to balance the pgs properly closes: rook#14430 Signed-off-by: parth-gr <partharora1010@gmail.com>

travisn · 2024-08-01T16:04:43Z

pkg/apis/ceph.rook.io/v1/types.go

@@ -2833,6 +2833,9 @@ type StorageScopeSpec struct {
 	// +nullable
 	// +optional
 	StorageClassDeviceSets []StorageClassDeviceSet `json:"storageClassDeviceSets,omitempty"`
+	// resize the osd crush weight once size of the osd pvc is increased
+	// +optional
+	ResizeOsdCrushWeight bool `json:"resizeOsdCrushWeight,omitempty"`


How about moving this down next to the AllowDeviceClassUpdate setting? Also, suggest this name for consistency.

Suggested change

ResizeOsdCrushWeight bool `json:"resizeOsdCrushWeight,omitempty"`

AllowOsdWeightUpdate bool `json:"resizeOsdCrushWeight,omitempty"`

How about AllowOsdCrushWeightUpdate?

travisn · 2024-08-01T16:05:47Z

pkg/operator/ceph/cluster/osd/osd.go

-			}
+		desiredOSD, ok := desiredOSDs[actualOSD.ID]
+		if !ok {
+			return nil


we need to continue the loop

Suggested change

return nil

continue

travisn · 2024-08-01T16:06:50Z

pkg/operator/ceph/cluster/osd/osd.go

+		return nil
+	}
+	if actualOSD.KB != actualOSD.CrushWeight {
+		args := []string{"osd", "crush", "reweight", fmt.Sprintf("osd.%d", actualOSD.ID), fmt.Sprintf("%q", actualOSD.KB.String())}


as discussed, let's check on the units

BlaineEXE · 2024-08-01T16:02:49Z

pkg/apis/ceph.rook.io/v1/types.go

@@ -2833,6 +2833,9 @@ type StorageScopeSpec struct {
 	// +nullable
 	// +optional
 	StorageClassDeviceSets []StorageClassDeviceSet `json:"storageClassDeviceSets,omitempty"`
+	// resize the osd crush weight once size of the osd pvc is increased


Doc comments get rendered to rook.io now, so let's try to keep things capitalized and following good doc comment hygiene.

And since this setting affects cluster rebalancing/stability, let's make sure we note that for users. I think this change might be helpful and still concise enough for users.

Suggested change

// resize the osd crush weight once size of the osd pvc is increased

// ResizeOsdCrushWeight allows Rook to resize the OSD CRUSH weight when the OSD PVC is increased. This allows cluster data to be rebalanced to make most effective use of new OSD space; however, data rebalancing can cause temporary cluster slowdown.

Thanks this make more sense

BlaineEXE · 2024-08-01T16:08:05Z

pkg/operator/ceph/cluster/osd/osd.go

+		args := []string{"osd", "crush", "reweight", fmt.Sprintf("osd.%d", actualOSD.ID), fmt.Sprintf("%q", actualOSD.KB.String())}
+		buf, err := cephclient.NewCephCommand(c.context, c.clusterInfo, args).Run()
+		if err != nil {
+			return errors.Wrap(err, "failed to reweight. "+string(buf))


errors.Wrap uses a colon to separate error messages. Let's also capture the OSD ID in the error message to help debug.

Suggested change

return errors.Wrap(err, "failed to reweight. "+string(buf))

return errors.Wrapf(err, "failed to reweight osd %d: %s", actualOSD.ID, string(buf))

BlaineEXE · 2024-08-01T16:10:46Z

pkg/operator/ceph/cluster/osd/osd.go

+			logger.Error(err)
+		}
+		if err := c.updateDeviceClassIfChanged(actualOSD.ID, desiredOSD.DeviceClass, actualOSD.DeviceClass); err != nil {
+			// Log the error and allow other updates to continue
+			logger.Error(err)


Because these errors are only logged, let's also be sure to include the namespace of the Ceph cluster that corresponds to the error by appending that. Otherwise, we might wind up with scenarios where we can't tell which cluster is giving the error.

logger.Error(fmt.Sprintf("failed to XYZ on cluster in namespace %q: %s", namespace, err))

BlaineEXE · 2024-08-01T16:12:44Z

pkg/operator/ceph/cluster/osd/osd.go

+	if !c.spec.Storage.ResizeOsdCrushWeight {
+		return nil
+	}
+	if actualOSD.KB != actualOSD.CrushWeight {


Shouldn't actualOSD.KB be translated into a TiB value? Or is it TB? We should make sure we know which should be used.

For that matter, we should check whether the KB value is truly KB or whether it's KiB.

parth-gr changed the title ~~osd: reweight osd while resizing~~ [wip] osd: reweight osd while resizing Jul 8, 2024

BlaineEXE reviewed Jul 10, 2024

View reviewed changes

BlaineEXE marked this pull request as draft July 10, 2024 15:25

travisn requested changes Jul 10, 2024

View reviewed changes

parth-gr force-pushed the osd-resize branch from 47644d5 to 819f210 Compare August 1, 2024 15:11

parth-gr marked this pull request as ready for review August 1, 2024 15:11

parth-gr requested review from travisn and BlaineEXE August 1, 2024 15:11

osd: reweight osd while resizing

7fa1f74

osd got resized by cryptsetup bluestore cmd but it should also be reweight to balance the pgs properly closes: rook#14430 Signed-off-by: parth-gr <partharora1010@gmail.com>

parth-gr force-pushed the osd-resize branch from 819f210 to 7fa1f74 Compare August 1, 2024 15:17

travisn requested changes Aug 1, 2024

View reviewed changes

BlaineEXE requested changes Aug 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] osd: reweight osd while resizing #14435

[wip] osd: reweight osd while resizing #14435

parth-gr commented Jul 8, 2024

travisn commented Jul 8, 2024

parth-gr commented Jul 10, 2024

parth-gr commented Jul 10, 2024

parth-gr commented Jul 10, 2024

BlaineEXE Jul 10, 2024

travisn Jul 10, 2024

travisn left a comment

travisn Jul 10, 2024

travisn Aug 1, 2024

parth-gr Aug 5, 2024 •

edited

Loading

travisn Aug 1, 2024

travisn Aug 1, 2024

BlaineEXE Aug 1, 2024

parth-gr Aug 5, 2024

BlaineEXE Aug 1, 2024

BlaineEXE Aug 1, 2024

BlaineEXE Aug 1, 2024 •

edited

Loading

	ResizeOsdCrushWeight bool `json:"resizeOsdCrushWeight,omitempty"`
	AllowOsdWeightUpdate bool `json:"resizeOsdCrushWeight,omitempty"`

	// resize the osd crush weight once size of the osd pvc is increased
	// ResizeOsdCrushWeight allows Rook to resize the OSD CRUSH weight when the OSD PVC is increased. This allows cluster data to be rebalanced to make most effective use of new OSD space; however, data rebalancing can cause temporary cluster slowdown.

	return errors.Wrap(err, "failed to reweight. "+string(buf))
	return errors.Wrapf(err, "failed to reweight osd %d: %s", actualOSD.ID, string(buf))

[wip] osd: reweight osd while resizing #14435

Are you sure you want to change the base?

[wip] osd: reweight osd while resizing #14435

Conversation

parth-gr commented Jul 8, 2024

travisn commented Jul 8, 2024

parth-gr commented Jul 10, 2024

parth-gr commented Jul 10, 2024

parth-gr commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Aug 5, 2024 • 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

BlaineEXE Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

parth-gr Aug 5, 2024 •

edited

Loading

BlaineEXE Aug 1, 2024 •

edited

Loading