-
Notifications
You must be signed in to change notification settings - Fork 42
Add new options for migration throttling #897
Add new options for migration throttling #897
Conversation
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
ec17331
to
4d1fcb5
Compare
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Show resolved
Hide resolved
4d1fcb5
to
c6092b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to look at this closer when I'm done with my other PR, but so far it looks great!
name="max_concurrent_tasks_per_provider" | ||
component={NumberInput} | ||
normalize={NumberInput.normalizeStringToInt} | ||
min={0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this minimum needs to be 1
instead of 0
.
min | ||
min, | ||
max, | ||
postfix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I didn't know TouchSpin
supported postfix
.
@vconzola I wanted to run Milan's screenshot above by you. He made use of the touchspin plugin ( |
We could change the color of the unit suffixes ( |
e9abc2c
to
5abecc8
Compare
I checked and Milan's implementation matches the Bootstrap touchspin control which is what the PF3 references. i don't think it looks very good though. I'm asking for guidance on the PF slack. |
@mzazrivec here's Vince's latest mockup that shows the new checkboxes for enabling/disabling each of the latter two fields: https://docs.google.com/presentation/d/15r00yDsnGo0K0owHxUb842AA-PnCfO6u5bc4NZNCgeU/edit?usp=sharing |
5abecc8
to
643ae56
Compare
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
be5297f
to
8ab3b88
Compare
bc150e0
to
a207c9d
Compare
@vconzola do you know which BZ we need to add here for the throttling changes? |
Fix tests we missed in PR #897
I edited your PR description here to add a recording of the onChange constraints being enforced, for reference for Avital. |
Comment about the tooltip for max # per conversion host. I think "limited to 20" for VDDK sounds like a UI constraint. Since the field goes up to 100, I suggest you change "limited to 20" to something like "should not exceed 20", so that it's clear that this is a recommendation, not a constraint. |
@mzazrivec Backport conflicts probably due to #851 and others not in cc @mturley @fdupont-redhat |
@mzazrivec I discussed with @simaishi and we are planning to backport #851 before this one to prevent these conflicts, since #851 has no user-facing effect while the Note that after we backport #851, we'll want to make sure the changes in #892 (already backported) are still applied (renaming the tab to "Migration Throttling" that is introduced in #851 as "Concurrent Migrations") |
…throttling Add new options for migration throttling (cherry picked from commit 5cee58d) https://bugzilla.redhat.com/show_bug.cgi?id=1693746
Fix tests we missed in PR #897 (cherry picked from commit d462568) https://bugzilla.redhat.com/show_bug.cgi?id=1693746
Hammer backport details:
|
Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1688951
This PR adds new option for maximum concurrent migrations per provider.
New option:
With tooltip active:
Note that these two fields are constrained so that the max concurrent migrations per provider will always be greater than or equal to the max concurrent migrations per conversion host. If one field is incremented or decremented outside these constraints, the other field will be adjusted to match the new value:
Fixes: #867