-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: Invoke Node Drains #6819
UI: Invoke Node Drains #6819
Conversation
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.
Overall this seems great to me, I have a few suggestions and questions, but 🥳
ui/app/components/drain-popover.js
Outdated
try { | ||
deadline = parseDuration(this.deadline); | ||
} catch (err) { | ||
this.set('parseError', 'Failed to parse duration'); |
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 tried this out locally and wondered why the parseDuration
utility generated specific errors but those get made generic here, so the UI only shows this one message:
It seems like it would be more helpful to print the parse errors specifically, maybe with a fallback if the error is somehow unknown?
Also/if not, maybe there could be a link to documentation that spells out what the expectations are, apart from the placeholder example?
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.
The reason is a pretty shallow one: I didn't want line wrapping. I agree this could use more attention though.
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.
Drain System Jobs | ||
{{/toggle}} | ||
<span class="tooltip multiline" aria-label="Stop allocations for system jobs."> | ||
{{x-icon "info-circle-outline" class="is-faded"}} |
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.
This isn’t quite as necessary as it seemed in this PR (see GIF) but I wonder if something like the is-right-aligned
thing would be useful here to address the slight truncation I see:
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.
Yeah, I think that makes sense here.
|
||
await Toggle.toggle(); | ||
assert.equal(props.onToggle.callCount, 1); | ||
}); |
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.
In the vein of the above comment on keyboard access, I tried it out and it all worked as expected (apart from a moment of 🤔 where triggering “Drain” with the keyboard happened too quickly for the progress indicator to even show that I’d actually done it successfully, but I think that’s something that won’t happen in a real scenario vs local testing), but what do you think of having an assertion for that at this level too? I find it somewhat unfortunate to having a multiplication of tests for different triggers but I also like to have some assertions that catch possible regressions… so maybe this could be Toggle.click
and then Toggle.spaceBar
in another test, or the like?
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 had that at one point, but then I couldn't get it to work. Turns out the spacebar action on inputs actually invoke the click
handler.
So I agree that it would be good to capture, since it's not hard to imagine the checkbox being replaced with a span
or something at some point and we accidentally lose the keyboard support, but I'm not sure how to mock it since it isn't actually a different event.
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’ve tested keyboard things with page objects before; I haven’t tried it in this situation but maybe this would work:
spaceBar: triggerable('keypress', '[data-test-input]', { eventProperties: { keyCode: 32 } })
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.
The issue isn't emulating the key event. It's that emulating the key event doesn't toggle the checkbox.
I don't know what the browser is doing, but when you press space when the checkbox is active, it sets checked
and fires a click
event.
When you emulate the space press, nothing happens. It's as if key controls for form inputs is a browser detail that gets translated into different DOM events before making it into javascript land.
I pushed up a commit to show you what I tried: 050f60a
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.
oh wow, well that’s disappointing, sorry for misunderstanding 😞
this.setProperties(props); | ||
await this.render(commonTemplate); | ||
|
||
assert.ok(Array.from(find('[data-test-popover-trigger]').classList).includes('is-special')); |
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.
Since qunit-dom
is present, what do you think about something like this slightly-more-direct assertion?
assert.dom('[data-test-popover-trigger]').hasClass('is-special');
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 brush up on qunit-dom
instead of continuing to reach for my primitive tools, ha.
|
||
await PopoverMenu.focusNext(); | ||
|
||
assert.equal(document.activeElement, find('#mock-input-for-test')); |
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.
Similarly this could be isFocused
😀
await waitUntil(() => server.pretender.handledRequests.findBy('method', 'POST')); | ||
|
||
assert.ok(ClientDetail.eligibilityToggle.isDisabled); | ||
server.pretender.resolve(server.pretender.requestReferences[0].request); |
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.
ooh nice, I haven’t seen this before, useful!
assert.notOk(ClientDetail.eligibilityError.isPresent); | ||
}); | ||
|
||
test('when navigating away from a client that as an error message to another client, the error is not shown', async function(assert) { |
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.
Tiny typo here, as
➡️ has
?
|
||
await ClientDetail.visit({ id: node2.id }); | ||
|
||
assert.notOk(ClientDetail.eligibilityError.isPresent); |
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 love the thoroughness of these tests 😍
<p data-test-message>Allocations have been drained and the node has been set to ineligible.</p> | ||
</div> | ||
<div class="column is-centered is-minimum"> | ||
<button data-test-dimiss class="button is-info" onclick={{action (mut showDrainNotification) false}}>Okay</button> |
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 see that there’s a clickable
dismiss
in the page object but I didn’t see any use of it in any tests, is it an oversight? I did try it out with this particular notification and it could be overkill to have assertions that dismissal works for every variation, but maybe the major one could have one? Or maybe that clickable
can just be removed.
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.
It's used a few times in the acceptance tests.
nomad/ui/tests/acceptance/client-detail-test.js
Lines 811 to 812 in 73c9ce4
await ClientDetail.stopDrainError.dismiss(); | |
assert.notOk(ClientDetail.stopDrainError.isPresent); |
Github may have auto-hid the diff and tricked you if you were trying to cmd+f for it.
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.
oops, that’s entirely true, thanks 😯
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.
Looking good! I had a couple of questions and requests, but it is looking great!
ui/app/styles/components/toggle.scss
Outdated
@@ -0,0 +1,75 @@ | |||
$size: 10px; |
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.
This should be 12px so that there is a 2px padding on the top, bottom, and outer edge of the 8px knob.
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.
@@ -1,4 +1,4 @@ | |||
$size: 0.75em; | |||
$size: 1.3rem; |
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.
Can we add the icons to the status light as well? And possibly update the striped gray fill for initializing? https://share.goabstract.com/54443b43-f285-4873-8dd6-711acc63ad8c?sha=74bc51e24f125d7b56cab17f39ca674bc6c3b793
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.
Yep, I plan on doing this in a follow up PR (but still before we release this without it) if that's alright.
onDrain=(action "drainNotify") | ||
onError=(action "drainError")}} | ||
</div> | ||
</div> | ||
|
||
<div class="boxed-section is-small"> | ||
<div class="boxed-section-body inline-definitions"> |
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.
Can we remove the Draining and Eligibility status from here now that the UI accounts for it via the eligibility toggle and the drain strategy component? Not sure this is the right spot to comment but wanted to make sure you saw this.
ui/app/styles/core/title.scss
Outdated
@@ -8,4 +8,8 @@ | |||
&.with-headroom { | |||
margin-top: 1rem; | |||
} | |||
|
|||
&.with-subheading { | |||
margin-bottom: 0.5rem; |
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.
Can we bump this to .85rem? The 16px I had before was probably a little too generous, but .85 gets us to 12px, which is still a standard spacing class for Structure.
ui/app/utils/parse-duration.js
Outdated
duration *= 1000000; | ||
|
||
return duration; | ||
}; |
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 was wondering if using something like https://github.com/jkroso/parse-duration might be a replacement for this? That's the first one I saw when I looked, but it seems well tested, zero dependency, small and well used. Might not be ideal, or you might have to use Nomad specific things that I'm not aware of, but thought I'd ask just incase.
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.
Yeah, I will probably end up switching this out for Duration.js since Vault is already using that one.
I was determined to write this implementation without regex, but I'll swallow my pride and use a lib 😄
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.
Sounds good to me!
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.
Officially swapped.
9f0e16d
to
1eb9e51
Compare
Thanks for the followup changes, seems ready to merge to me 🥳 |
+1 to that ^ 🥳 |
1eb9e51
to
4793dc9
Compare
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. |
Closes #6352
This brings the ability to submit, update, stop, and force node drains from the UI. It also brings along the related task of toggling node eligibility.
The updated client detail page, including a drain control, an eligibility toggle, and rearranged title bar elements.
The drain options dropdown/popover, including deadline, force, and system job (equivalent to cli options).
The deadline option includes some convenient presets.
The custom deadline allows for text input of a well formatted duration.
When a drain is running, the number of running/migrating/completed allocations are shown and updated in realtime.
The stop and force actions use the familiar two-step pattern to prevents accidents.
If you are on the page when a drain completes, you will receive a completion notification.