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.

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

Allow passing explicit PrimaryAffinity #1178

Merged

Conversation

synarete
Copy link
Contributor

@synarete synarete commented May 6, 2021

Allow passing explicit primary-affinity upon OSD's deployment using
annotations mechanism (same as DeviceClass/InitialWeight). This may be
useful for cases where user wants to have more fine-grained control on
the overall (read) load of specific devices. Valid values are within
range [0, 1) (where 1 is the default).

As an example, consider the (real-life) case of a PV which resides on a
partition alongside OS partition. In such case, user can reduce I/O load
of this specific OSD by choosing explicit primary-affinity value.

Issue: https://issues.redhat.com/browse/KNIP-1616
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1924946

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

Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

Overall this seems to make sense. Two requests:

  • Please provide at least a short description of what "primary affinity" means in the comments in storagecluster_types.go
  • Split the generated CSV changes into their own commit

@synarete
Copy link
Contributor Author

synarete commented May 6, 2021

Overall this seems to make sense. Two requests:

  • Please provide at least a short description of what "primary affinity" means in the comments in storagecluster_types.go
  • Split the generated CSV changes into their own commit

@jarrpa Will fix soon and re-send the PR

Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

Good split. Suggesting some edits to the additional comment. Your comment gave me a better understanding on what this does, but the language was a little unclear. A direct link to the documentation will probably be useful here, since anyone modifying this value is already in a place where they should know more about this.

Comment on lines 166 to 172
// PrimaryAffinity is an optional OSD primary-affinity value within the
// range [0,1). This value influence the way Ceph's CRUSH selection of
// primary OSDs. Lower value reduce performance bottlenecks (especially
// on read operations). If not set, default value is 1.
// +kubebuilder:validation:Pattern=`^0.[0-9]+$`
// +optional
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// PrimaryAffinity is an optional OSD primary-affinity value within the
// range [0,1). This value influence the way Ceph's CRUSH selection of
// primary OSDs. Lower value reduce performance bottlenecks (especially
// on read operations). If not set, default value is 1.
// +kubebuilder:validation:Pattern=`^0.[0-9]+$`
// +optional
// PrimaryAffinity is an optional OSD primary-affinity value within the
// range [0,1). This value influences Ceph's CRUSH selection of
// primary OSDs. A lower value reduces the probability that the OSD will
// be selected. If not set, default value is 1.
// https://docs.ceph.com/en/latest/rados/operations/crush-map/#primary-affinity
// +kubebuilder:validation:Pattern=`^0.[0-9]+$`
// +optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jarrpa Should I re-submit my PR with those changes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Allow passing explicit primary-affinity upon OSD's deployment using
annotations mechanism (same as DeviceClass/InitialWeight). This may be
useful for cases where user wants to have more fine-grained control on
the overall (read) load of specific devices. Valid values are within
range [0, 1) (where 1 is the default).

As an example, consider the (real-life) case of a PV which resides on a
partition alongside OS partition. In such case, user can reduce I/O load
of this specific OSD by choosing explicit primary-affinity value.

Issue: https://issues.redhat.com/browse/KNIP-1616
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1924946

ROOK issue: rook/rook#7773

Signed-off-by: Shachar Sharon <ssharon@redhat.com>
Update crds after adding 'primaryAffinity' option.

Signed-off-by: Shachar Sharon <ssharon@redhat.com>
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 6, 2021
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 1c0df4c into red-hat-storage:master May 6, 2021
@obnoxxx
Copy link
Contributor

obnoxxx commented May 11, 2021

@synarete , @jarrpa - I think we need a backport of this to release-4.8.

@synarete
Copy link
Contributor Author

@synarete , @jarrpa - I think we need a backport of this to release-4.8.

@jarrpa I issued a PR (#1179)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants