Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Add new options for migration throttling #897

Merged

Conversation

mzazrivec
Copy link
Contributor

@mzazrivec mzazrivec commented Mar 6, 2019

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1688951

This PR adds new option for maximum concurrent migrations per provider.

New option:
throttling04

With tooltip active:
throttling05

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:

xoQxhjxxwk

Fixes: #867

@mzazrivec mzazrivec changed the title Add new options for migration throttling [WIP] Add new options for migration throttling Mar 6, 2019
@mzazrivec mzazrivec added the wip label Mar 6, 2019
@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch from ec17331 to 4d1fcb5 Compare March 7, 2019 15:54
@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch from 4d1fcb5 to c6092b9 Compare March 7, 2019 16:06
Copy link
Contributor

@mturley mturley left a 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}
Copy link
Contributor

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
Copy link
Contributor

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.

@mzazrivec
Copy link
Contributor Author

mzazrivec commented Mar 7, 2019

Current work in progress visuals:

throttling01

An alternative, with modified background-color of the unit suffixes:
throttling02

Another one with checkboxes:
throttling03

@mturley
Copy link
Contributor

mturley commented Mar 7, 2019

@vconzola I wanted to run Milan's screenshot above by you. He made use of the touchspin plugin (- and + buttons) for the CPU % field, I think it's quite nice but it doesn't match the mockup.

@mturley
Copy link
Contributor

mturley commented Mar 7, 2019

We could change the color of the unit suffixes (% and MB/s) so they don't look so similar to the buttons.

@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch 2 times, most recently from e9abc2c to 5abecc8 Compare March 7, 2019 17:18
@vconzola
Copy link

vconzola commented Mar 7, 2019

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.

@mturley
Copy link
Contributor

mturley commented Mar 7, 2019

@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

@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch from 5abecc8 to 643ae56 Compare March 8, 2019 13:50
@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch 3 times, most recently from be5297f to 8ab3b88 Compare March 8, 2019 20:59
@mzazrivec mzazrivec force-pushed the add_new_options_for_migration_throttling branch 4 times, most recently from bc150e0 to a207c9d Compare March 11, 2019 17:54
@mzazrivec mzazrivec marked this pull request as ready for review March 11, 2019 17:54
@mzazrivec mzazrivec changed the title [WIP] Add new options for migration throttling Add new options for migration throttling Mar 11, 2019
@mturley mturley merged commit 5cee58d into ManageIQ:master Mar 12, 2019
@mturley
Copy link
Contributor

mturley commented Mar 12, 2019

@vconzola do you know which BZ we need to add here for the throttling changes?

mturley added a commit to mturley/manageiq-v2v that referenced this pull request Mar 12, 2019
@mturley
Copy link
Contributor

mturley commented Mar 12, 2019

Crap! Merging this broke the build? Where was Travis CI?? We'll have to look into why there was no build error blocking this merge... I updated the snapshots in #905 in the meantime.

Note: #905 will need to be backported after this PR to prevent build errors in hammer!

mturley added a commit that referenced this pull request Mar 12, 2019
@mturley
Copy link
Contributor

mturley commented Mar 13, 2019

I edited your PR description here to add a recording of the onChange constraints being enforced, for reference for Avital.

@mzazrivec mzazrivec deleted the add_new_options_for_migration_throttling branch March 14, 2019 06:32
@mturley
Copy link
Contributor

mturley commented Mar 14, 2019

@apinnick
Copy link

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.

@mturley mturley added v1.2 and removed v1.1 labels Mar 25, 2019
@mturley mturley added bz Issues filed by QE or having a BZ and removed bugzilla needed labels Mar 27, 2019
@simaishi
Copy link
Contributor

simaishi commented Apr 1, 2019

@mzazrivec Backport conflicts probably due to #851 and others not in hammer branch. Since PR851 won't be backported to hammer yet, can you create a separate PR for hammer?

cc @mturley @fdupont-redhat

@mturley
Copy link
Contributor

mturley commented Apr 1, 2019

@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 hideConversionHostSettings flag is in place.

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")

simaishi pushed a commit that referenced this pull request Apr 5, 2019
…throttling

Add new options for migration throttling

(cherry picked from commit 5cee58d)

https://bugzilla.redhat.com/show_bug.cgi?id=1693746
simaishi pushed a commit that referenced this pull request Apr 5, 2019
@simaishi
Copy link
Contributor

simaishi commented Apr 5, 2019

Hammer backport details:

$ git log -1
commit 9b2a950b3a2dbdb7a7bc44a5a2c7e38dbd004d43
Author: Mike Turley <mike.turley@alum.cs.umass.edu>
Date:   Tue Mar 12 17:18:12 2019 -0400

    Merge pull request #897 from mzazrivec/add_new_options_for_migration_throttling
    
    Add new options for migration throttling
    
    (cherry picked from commit 5cee58ddbbbbe7e322363309a576d5729d58964b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1693746

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants