Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ceph: allow passing 'osd-crush-initial-weight' #7472

Merged
merged 1 commit into from Apr 20, 2021
Merged

ceph: allow passing 'osd-crush-initial-weight' #7472

merged 1 commit into from Apr 20, 2021

Conversation

synarete
Copy link
Contributor

Ceph support the option '--osd-crush-initial-weight' upon OSD start,
which sets an explicit weight (in TiB units) to specific OSD. Allow
passing this option all the way from the user (similar to
'DeviceClass'), for the special case where end users wants it cluster
to have non-even balance over specific OSDs (e.g., one of the OSDs is
placed over a partition alongside OS-partition).

ROOK issue: #7448

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

Description of your changes:
Allow set OSD initial-weight, similar to device-class.

Which issue is resolved by this Pull Request:
Resolves #7448

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

@mergify mergify bot added the ceph main ceph tag label Mar 24, 2021
@synarete synarete force-pushed the crush-initial-weight branch 2 times, most recently from f976ef7 to 6e9734a Compare March 25, 2021 10:52
leseb
leseb previously requested changes Mar 26, 2021
View reviewed changes
pkg/apis/rook.io/v1/types.go
@@ -217,6 +217,9 @@ type VolumeSource struct {
// CrushDeviceClass represents the crush device class for an OSD
// +optional
CrushDeviceClass string `json:"crushDeviceClass,omitempty"`
// CrushInitialWeight represents initial OSD weight (float, TiB units)
// +optional
CrushInitialWeight string `json:"crushInitialWeight,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's optional please make it a pointer instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please investigate resource.Quantity and see if it makes sense here, it could allow use to enter more arbitrary
value that can be easily converted internally. If not, please add an pattern validation like:

// +kubebuilder:validation:Pattern=`^[0-9]*([T]i[B])$`

If the expectation is TiB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With strings I would hope we could treat the empty string as not set instead of using a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, using empty string as "not-set" feels more intuitive than using a pointer, and as far as I can tell, it is also a common pattern in other places in the code. Using resource.Quantity could have been better, but unfortunately, it has rather limited meaning withing kubernetes terminology (cpu/memory; extending it to capacity is not a trivial effort).

travisn
travisn reviewed Mar 29, 2021
View reviewed changes
pkg/apis/rook.io/v1/types.go
@@ -217,6 +217,9 @@ type VolumeSource struct {
// CrushDeviceClass represents the crush device class for an OSD
// +optional
CrushDeviceClass string `json:"crushDeviceClass,omitempty"`
// CrushInitialWeight represents initial OSD weight (float, TiB units)
// +optional
CrushInitialWeight string `json:"crushInitialWeight,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With strings I would hope we could treat the empty string as not set instead of using a pointer.

pkg/daemon/ceph/osd/volume.go Outdated
// assign the initial weight specific to the device
initialWeight := device.Config.InitialWeight
if initialWeight != "" {
immediateExecuteArgs = append(immediateExecuteArgs, []string{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the initial weight needed both during the ceph-volume provisioining of the OSD and the OSD daemon? I would have thought it would only be needed in one place or the other, but not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct -- I blindly followed the 'DeviceClass'. Will fix.

@mergify
Copy link

mergify bot commented Mar 29, 2021

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

@synarete synarete force-pushed the crush-initial-weight branch 3 times, most recently from 5f5d0a6 to 14591f0 Compare April 7, 2021 13:41
@mergify
Copy link

mergify bot commented Apr 15, 2021

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

@mergify
Copy link

mergify bot commented Apr 19, 2021

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

@synarete synarete force-pushed the crush-initial-weight branch 3 times, most recently from 214564b to c9395ff Compare April 20, 2021 16:27
travisn
travisn requested changes Apr 20, 2021
View reviewed changes
cluster/charts/rook-ceph/templates/resources.yaml Outdated
@@ -3160,6 +3160,10 @@ spec:
crushDeviceClass:
description: CrushDeviceClass represents the crush device class for an OSD
type: string
crushInitialWeight:
description: CrushInitialWeight represents initial OSD weight in TiB units
pattern: ^([0-9]*[.])?[0-9]+(Ti[B])$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about not including the TiB suffix? Then no need to strip it later. If they could specify different units it could make sense to have the suffix, but if TiB is the only suffix allowed, seems simpler to just document the expected units, and just have the number value.

Suggested change
pattern: ^([0-9]*[.])?[0-9]+(Ti[B])$
pattern: ^([0-9]*[.])?[0-9]+$

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ceph expects weight as TiB units. We use those kind of suffix all around, so I think it make sense here as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it is expected to be in TiB units, but it looks like Ceph doesn't require or allow the units in the value? For example, in the Ceph doc, I see an example weight of 2.0, rather than 2.0TiB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. And for that reason, I strip the suffix (if exists) before passing it on to Ceph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm proposing to match the Ceph doc, which documents the units, then only the number is specified in the value. I don't see why the TiB suffix needs to be allowed, the code is simpler without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will fix

@synarete
ceph: allow passing 'osd-crush-initial-weight'
9766e9b 
Ceph support the option '--osd-crush-initial-weight' upon OSD start,
which sets an explicit weight (in TiB units) to specific OSD. Allow
passing this option all the way from the user (similar to
'DeviceClass'), for the special case where end users wants it cluster
to have non-even balance over specific OSDs (e.g., one of the OSDs is
placed over a partition alongside OS-partition).

ROOK issue: rook#7448

Signed-off-by: Shachar Sharon <ssharon@redhat.com>
@travisn travisn dismissed leseb’s stale review April 20, 2021 18:06

Feedback addressed, discussed to leave as a string

@travisn travisn merged commit 6fc3570 into rook:master Apr 20, 2021
@mergify mergify bot mentioned this pull request Apr 20, 2021
Merged
mergify bot added a commit that referenced this pull request Apr 20, 2021
@mergify
Merge pull request #7702 from rook/mergify/bp/release-1.6/pr-7472
e0e6be8 
ceph: allow passing 'osd-crush-initial-weight' (backport #7472)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisn travisn travisn approved these changes

@leseb leseb leseb left review comments

Assignees
No one assigned
Labels
ceph main ceph tag
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ceph: pass explicit value for OSD's initial weight
3 participants
@synarete @leseb @travisn