ceph: allow setting primary-affinity to osd by synarete · Pull Request #7807 · rook/rook

synarete · 2021-05-02T15:17:05Z

A user with unbalanced system (e.g., some OSDs use a partition which
resides on a device along-side OS partition) may want to have a
fine-grained control on the overall load of specific OSD. While osd
weight affects write-load, primary-affinity affects read-load.

This patch allow user to define osd's primary-affinity (similar to
'DeviceClass' and 'InitialWeight'), which is sent to the osd (via mon)
as part of deployment sequence, using 'ceph osd primary-affinity' command.

Valid values are within the range [0.0, 1.0). If not set, default value
is 1.

ROOK issue: #7773

Signed-off-by: Shachar Sharon ssharon@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

pkg/daemon/ceph/client/osd.go

leseb · 2021-05-03T07:50:40Z

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


+	err = prepareForDeployment(c, osdProps, osd, osdLongName)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to apply deployment fixup for %s", osdLongName)


fixup? Also please use %q instead of %s

@leseb used "%s" to follow same pattern as rest of this error-formatting in this function. Note that "%q" is already used to format 'osdLongName'

leseb · 2021-05-03T07:51:05Z

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


+	err = prepareForDeployment(c, osdProps, osd, osdLongName)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to prepare for deployment %s", osdLongName)


Suggested change

return nil, errors.Wrapf(err, "failed to prepare for deployment %s", osdLongName)

return nil, errors.Wrapf(err, "failed to prepare for deployment %q", osdLongName)

leseb · 2021-05-03T07:53:15Z

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

 	return d, nil
 }

+func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo, osdLongName string) error {


It feels unnecessary to pass osdLongNamesince you have osd OSDInfo, you can build it directly within prepareForDeployment()

leseb · 2021-05-03T07:53:50Z

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


+func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo, osdLongName string) error {
+	if osdProps.storeConfig.PrimaryAffinity != "" {
+		logger.Infof("Set primary-affinity %s for %s", osdProps.storeConfig.PrimaryAffinity, osdLongName)


Move this logger inside cephclient.OsdPrimaryAffinity.

sp98

(asking to better understand how primary-affinity works) I've seen some docs where its mentioned that we need to set mon osd allow primary affinity = true on ceph.conf to enable this functionality. Do we no longer need to do that ?

Some nits

pkg/daemon/ceph/client/osd.go

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

travisn · 2021-05-03T16:45:38Z

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

 	// +optional
 	CrushInitialWeight string `json:"crushInitialWeight,omitempty"`
+	// CrushPrimaryAffinity represents OSD primary-affinity within range [0,1)
+	// +kubebuilder:validation:Pattern=`^0.[0-9]+$`


Could you use a float type with a min and max value, instead of the string?

We actually don't use the schema for VolumeSource so you don't need to worry about this schema. I'm opening a separate PR to clean that up...

It appears that floats are highly discouraged in k8s API (kubernetes-sigs/controller-tools#245)

However, as there is no schema for VolumeSource, I will change it to float (optional, pointer).

We already have the flag to force the generation if the API has floats, I'm not saying we should continue though. Also, I think the validation only works on strings IIRC.

IMHO, if float is acceptable I think it would be better (and I will also make a fix for InitialWeight).

IMHO, if float is acceptable I think it would be better (and I will also make a fix for InitialWeight).

Can we still use the validation pattern?

@leseb No we can't. +kubebuilder:validation:Minimum/Maximum apply only for integers, not floats.
However, with @travisn #7813 there is no more validation pattern to any field in VolumeSource

travisn · 2021-05-03T16:47:56Z

pkg/daemon/ceph/client/osd.go

 }
+
+// OsdPrimaryAffinity assigns primary-affinity (within range [0.0, 1.0]) to a specific OSD.
+func OsdPrimaryAffinity(context *clusterd.Context, clusterInfo *ClusterInfo, osdID int, affinity string) error {


If the method doesn't indicate taking action, it's typically a "get" operation. How about this?

Suggested change

func OsdPrimaryAffinity(context *clusterd.Context, clusterInfo *ClusterInfo, osdID int, affinity string) error {

func SetPrimaryAffinity(context *clusterd.Context, clusterInfo *ClusterInfo, osdID int, affinity string) error {

pkg/daemon/ceph/client/osd.go

travisn · 2021-05-03T17:38:25Z

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


+func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo) error {
+	if osdProps.storeConfig.PrimaryAffinity != "" {
+		return cephclient.OsdPrimaryAffinity(c.context, c.clusterInfo, osd.ID, osdProps.storeConfig.PrimaryAffinity)


I'm not excited about setting this property with every reconcile, so would prefer if we can set it with a command line param.

travisn · 2021-05-03T17:38:51Z

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

 	return d, nil
 }

+func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo) error {


Suggested change

func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo) error {

func setOSDProperties(c *Cluster, osdProps osdProperties, osd OSDInfo) error {

mergify · 2021-05-04T13:49:53Z

This pull request has merge conflicts that must be resolved before it can be merged. @synarete please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork

travisn

The code changes look good. Please add this new setting to the docs in ceph-cluster-crds.md next to the deviceClass setting.

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

sp98

just one small nit

sp98 · 2021-05-05T16:17:03Z

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


+	err = setOSDProperties(c, osdProps, osd)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to prepare for deployment %s", osdLongName)


Suggested change

return nil, errors.Wrapf(err, "failed to prepare for deployment %s", osdLongName)

return nil, errors.Wrapf(err, "failed to prepare deployment for %s", osdLongName)

and there is already a comment about using %q

@sp98 You are correct!

A user with unbalanced system (e.g., some OSDs use a partition which resides on a device along-side OS partition) may want to have a fine-grained control on the overall load of specific OSD. While osd weight affects write-load, primary-affinity affects read-load. This patch allow user to define osd's primary-affinity (similar to 'DeviceClass' and 'InitialWeight'), which is sent to the osd (via mon) as part of deployment sequence, using 'ceph osd primary-affinity' command. Valid values are within the range [0.0, 1.0). If not set, default value is 1. ROOK issue: rook#7773 Signed-off-by: Shachar Sharon <ssharon@redhat.com>

ceph: allow setting primary-affinity to osd (backport #7807)

mergify bot added the ceph main ceph tag label May 2, 2021

leseb requested changes May 3, 2021

View reviewed changes

sp98 requested changes May 3, 2021

View reviewed changes

pkg/daemon/ceph/client/osd.go Outdated Show resolved Hide resolved

pkg/apis/rook.io/v1/types.go Outdated Show resolved Hide resolved

synarete force-pushed the osd-primary-affinity branch from 6cd6785 to 786cdc7 Compare May 3, 2021 16:35

travisn requested changes May 3, 2021

View reviewed changes

synarete force-pushed the osd-primary-affinity branch from 786cdc7 to a22b0fe Compare May 5, 2021 09:11

travisn requested changes May 5, 2021

View reviewed changes

pkg/operator/ceph/cluster/osd/osd.go Show resolved Hide resolved

synarete force-pushed the osd-primary-affinity branch from a22b0fe to 2016e35 Compare May 5, 2021 14:17

leseb approved these changes May 5, 2021

View reviewed changes

sp98 requested changes May 5, 2021

View reviewed changes

synarete force-pushed the osd-primary-affinity branch 2 times, most recently from fe07667 to f252aaf Compare May 6, 2021 13:25

travisn approved these changes May 6, 2021

View reviewed changes

synarete force-pushed the osd-primary-affinity branch from f252aaf to 2a3cfc6 Compare May 6, 2021 14:52

sp98 approved these changes May 6, 2021

View reviewed changes

travisn merged commit f6ae92f into rook:master May 6, 2021

travisn added the backport-release-1.6 label May 6, 2021

mergify bot mentioned this pull request May 6, 2021

ceph: allow setting primary-affinity to osd (backport #7807) #7846

Merged

travisn added a commit that referenced this pull request May 6, 2021

Merge pull request #7846 from rook/mergify/bp/release-1.6/pr-7807

c0b1ea1

ceph: allow setting primary-affinity to osd (backport #7807)

	return nil, errors.Wrapf(err, "failed to prepare for deployment %s", osdLongName)
	return nil, errors.Wrapf(err, "failed to prepare for deployment %q", osdLongName)

	func OsdPrimaryAffinity(context clusterd.Context, clusterInfo ClusterInfo, osdID int, affinity string) error {
	func SetPrimaryAffinity(context clusterd.Context, clusterInfo ClusterInfo, osdID int, affinity string) error {

	func prepareForDeployment(c *Cluster, osdProps osdProperties, osd OSDInfo) error {
	func setOSDProperties(c *Cluster, osdProps osdProperties, osd OSDInfo) error {

Conversation

synarete commented May 2, 2021

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!

sp98 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 4, 2021

Uh oh!

travisn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sp98 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants