-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Preemptions #5594
UI: Preemptions #5594
Conversation
Show the count in the allocations table next to the existing total alloc count badge. Clicking either will filter by all or by preemptions.
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.
Code looks good! Left a couple of questions and a thing about the documentation just incase.
@@ -45,6 +45,10 @@ export default ApplicationSerializer.extend({ | |||
hash.NextAllocationID = hash.NextAllocation ? hash.NextAllocation : null; | |||
hash.FollowUpEvaluationID = hash.FollowupEvalID ? hash.FollowupEvalID : null; | |||
|
|||
hash.PreemptedAllocationIDs = hash.PreemptedAllocations || []; | |||
hash.PreemptedByAllocationID = hash.PreemptedByAllocation || null; | |||
hash.WasPreempted = !!hash.PreemptedByAllocationID; |
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.
Wondering if this boolean casting would be done by the attr('boolean')
in the model?
Also I had a quick scan over the docs here:
https://www.nomadproject.io/docs/internals/scheduling/preemption.html#preemptedbyallocid
Does hash.PreemptedByAllocationID
== PreemptedByAllocID
? (Allocation vs Alloc). Thinking it could be something wrong in the docs, thought I'd mention 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.
The boolean casting is probably taken care of by the boolean
transformer, yes, but I'm not sure I want to rely on a transformer for it.
Yes, the docs are wrong in this case. Here's the go struct to confirm: https://github.com/hashicorp/nomad/blob/master/nomad/structs/structs.go#L7261-L7267
@@ -36,7 +36,6 @@ | |||
|
|||
&.is-right { | |||
margin-left: $gutter-width; | |||
overflow-x: auto; |
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.
Did you mean to include this here? Would this mean you can't pan on mobile anymore?
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 did mean to include this. It was causing the icon tooltip to be clipped on the left side where it goes out of the bounds of the page-column.
As for panning on mobile, good question. I just checked and it appears to still work.
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. |
Changes to the allocation tables, the allocation detail page, the client detail page, and the job run plan to add visibility to preemptions.
Required preemptions, as seen from the job plan output
Preemption indicator on any allocation row
Preempted By details on the allocation detail for allocations that have been preempted
Preempted allocations table on the allocation detail for allocations that have preempted other allocations
Clickable preemptions badge on the allocations table on the client detail page
Allocations table on the client detail page when filtered to only show preemptions