Refactor device tuning code #946
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
throttleDiskTypes --> knownSlowDiskTypes throttleFastDiskTypes --> knownFastDiskTypes This is just what it is about. Signed-off-by: Michael Adam <obnox@redhat.com>
obnoxxx
requested review from
jarrpa
and removed request for
davidvossel and
agarwal-mudit
openshift-ci-robot
added
lgtm
Instead of having a list of known fast devices, and a list of known slow devices, and hard-coding in the throttleStorageDevices function for which provisioner each list is applicable, create known disk type that encapsulates the provisioner, storage class name and whether this is known to be fast or slow. The function then just becomes a simple loop. IMHO, this makes the code much more obvious and easier to extend in the future. Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
This function is not throttling anything. It is checking whether corresponding devices should be marked for explicit tuning for rook. Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
Signed-off-by: Michael Adam <obnox@redhat.com>
…f list of bools Returning two booleans is not a robust API. This changes checkTuneStorageDevices to return the speed instead. Thus, the caller can take action based on the returned device. Signed-off-by: Michael Adam <obnox@redhat.com>
obnoxxx
force-pushed
the
refactor-device-tuning
branch
from
a21ce3c
to
b1e0233
Compare
|
/lgtm |
|
[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 |
|
@obnoxxx: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewing PR #864 made me consider the naming of functions and variables and the structure of functions. I found it confusing that it talked about throttling internally and converted this to tuning toggles for rook, while ocs-operator is not doing any throttling. Furthermore, having two lists of fast and slow device storage classes, but having the the detection function hardcode which provisioner to apply each list against did not seem very intuitive and robust to me.
This PR refactors the code to be (IMHO) more obvious and more prone to future expansions of known devices.