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

UI: Task Group Scaling Controls #8207

Merged
merged 28 commits into from
Jun 19, 2020
Merged

Conversation

DingoEatingFuzz
Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz commented Jun 19, 2020

A fan favorite feature from Hashi UI is finally landing in the Nomad UI 🎉

We have always been cautious about allowing casual changes to job definitions outside of the source control loop given our core principle of infrastructure as code. However! The new scaling APIs means we can do the best of both worlds.

With this feature, in its initial form, if you have a task group that specifies a scaling block, the UI will let you scale a task group up and down within the min/max range declared in the job.

This can be done one of two ways:

  1. Quick action: From the job detail page, task group rows now have +/- buttons when appropriate. Clicking these buttons will hit the scale endpoint and change the underlying count for the task group going through the expected deployment process. These buttons are debounced, so if you quickly click the + button five times, only one deployment with count+5 will be created.
  2. Precise action: From the task group detail page, a task group can be scaled using +/- buttons just like in the table row, but it can also be scaled using manual text entry. Debouncing is used here as well.

Kapture 2020-06-18 at 23 42 21

image

This prevents the svg from being a target in click events.
This was a disturbing discovery. Requests in watch loops would recycle
AbortControllers meaning once any request was aborted, all requests
forever after were skipped. I noticed it with deployments and job
summary on the job detail page.

I suspect this regression occurred when jQuery was removed. This needs
test coverage still to make sure it doesn't happen again.
@DingoEatingFuzz DingoEatingFuzz requested a review from a team June 19, 2020 06:50
@github-actions
Copy link

github-actions bot commented Jun 19, 2020

Ember Asset Size action

As of c70ea97

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +14 kB +1.89 kB
nomad-ui.css +4.77 kB +706 B

Files that stayed the same size 🤷‍:

File raw gzip
auto-import-fastboot.js 0 B 0 B
vendor.js 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jun 19, 2020

Ember Test Audit comparison

master c70ea97 change
passes 1392 1415 +23
failures 0 0 0
flaky 0 0 0
duration 5m 11s 322ms 5m 21s 826ms +10s 504ms

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

I found this fun to toy around with, which to me is one aspect of a good design. I have some mega-detail UX suggestions but nothing blocking this from being merged. Success! 🥳


@computed('rulesForActiveNamespace.@each.capabilities')
get policiesSupportScaling() {
return this.activeNamespaceIncludesCapability('scale-job');
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we are slowly building up useful abstractions here as appropriate 🤩

export default scope => ({
scope,

isPresent: isPresent(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Really quite minor (ignorable even??) but since this is a page object “component”, isPresent comes for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤯 Today I learned!

);
assert.equal(scaleRequests.length, 1);
assert.equal(JSON.parse(scaleRequests[0].requestBody).Count, 4);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the granularity here 😀

await StepperInput.input.esc();
assert.equal(StepperInput.input.value, this.value);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If I enter a decimal value in the field, it results in a failed POST …/scale event with a string response (!):

json: cannot unmarshal number 3.5 into Go struct field ScalingRequest.Count of type int64

Some kind of error-handling would be nice to have, or maybe a few kinds. I’m not sure what the best approach to this specific case would be, maybe reverting to the last number that worked? 🤔 That could be confusing though if you enter a valid number and it fails for unrelated reasons, there’s not really anywhere obvious to indicate what happened.

UX suggestions:
• when I tried entering a non-number (“x”) or deleting the contents of the field, it reset to the minimum count, what if it reverted to the last number instead? Or maybe the current behaviour is fine but should have attendant edge case assertions?
• could clicking the pseudo-label of “count” cause the number to become highlighted for editing?
• and maybe even clicking the field itself could highlight it instead of showing the caret? But maybe that’s too much of a departure from standard input field UX

That’s a lot of tiny details though, more questions than things I would insist on 😀 AND I can tell there’s a lot of UX attention already, like handling the Escape key for instance, I love 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.

I really appreciate you hammering on this component! I of course want the UX of all the components to be as good as possible.

I tried to keep as much of the default number input experience as possible, but I agree with your critique. I'll add the following to our known issues list:

  1. invalid input should revert the value, not set to min (number input default)
  2. clicking the label of the stepper input should focus the input field
  3. when the input field is focused, the value should be highlighted so you don't have to deleting the existing value before providing a new one

avg_conn: {
Source: 'prometheus',
Query:
'scalar(avg((haproxy_server_current_sessions{backend="http_back"}) and (haproxy_server_up{backend="http_back"} == 1)))',
Copy link
Contributor

Choose a reason for hiding this comment

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

wildness 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right? I copied it straight from the autoscaler docs.

@max={{model.scaling.max}}
@value={{model.count}}
@class="is-primary is-small"
@disabled={{or model.job.runningDeployment (cannot "scale job")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The task group row tells you you lack permissions when it’s turned off but this one is just turned off with no indication of why (example on Netlify), maybe the same tooltip could show? I hope it’s not Hard 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeeeaaah. The reason (if you're curious) is there's a running deployment.

This definitely needs a tooltip, but it wasn't easy to shim it in with a good enough message, so I cut that corner to make the beta. I'll log it as a known issue though.

@@ -68,4 +68,20 @@ export default class JobAdapter extends WatchableNamespaceIDs {
},
});
}

scale(job, group, count, reason) {
const url = addToPath(this.urlForFindRecord(job.get('id'), 'job'), '/scale');
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn’t known about addToPath before, I feel like I could have used it at some point, I’ll try to remember it exists 😯

@DingoEatingFuzz DingoEatingFuzz merged commit fcf08db into master Jun 19, 2020
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui/manual-scaling-controls branch June 19, 2020 17:29
@jippi
Copy link
Contributor

jippi commented Jun 20, 2020

Nice, excited to see hashi-ui becoming less and less relevant!

@github-actions
Copy link

github-actions bot commented Jan 2, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2023
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.

None yet

3 participants