-
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] Disconnected Clients: "Unknown" allocations in the UI #12544
[ui] Disconnected Clients: "Unknown" allocations in the UI #12544
Conversation
// from being visually centered | ||
&:nth-child(2n + 1):last-child { | ||
margin-right: calc(35% + 0.75em); | ||
} |
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.
@@ -211,6 +211,7 @@ export default class ClientController extends Controller.extend( | |||
{ key: 'complete', label: 'Complete' }, | |||
{ key: 'failed', label: 'Failed' }, | |||
{ key: 'lost', label: 'Lost' }, | |||
{ key: 'unknown', label: 'Unknown' }, |
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.
Ember Asset Size actionAs of 9f8ae31 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
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.
One minor suggestion to the help text but nothing blocking. Nice job!
Co-authored-by: Derek Strickland <1111455+DerekStrickland@users.noreply.github.com>
d2187e4
to
25c57c6
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.
There's 11 failing tests which definitely makes me think we should poke around a little bit more and we might need to update our utility for computing derived state for job-client-status
.
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.
Excellent job! I think there's a few things we should discuss. I like to use a bottoms-up approach starting with views (chatting about components if necessary), then moving up routes/controllers, and then models + services and then tie it all back with tests.
The following views are impacting by the surface area of this change, which sends back a new status on the Allocation
and Node
models:
- ui/clients
- /ui/clients/:id
- /ui/allocations/:id
- /ui/jobs/:id
- /ui/jobs/:id/allocations
- /ui/jobs/:id/deployments
- /ui/topology
I'm unsure if we're addressing the changes in /clients
or allocations
.
What should we do about the Topology Visualization?
@DerekStrickland did a great job spotting this out in his RFC. And I think, during the design process we missed talking about impact to the Topology visualization which visualizes Allocations
and decorates them based on their status (starting
, running
, or failed
).
We should turn back to @mikenomitch and @juliezzhou regarding this.
How should we handle the new Allocation clientStatus
of unknown
and the Node
being in a state of disconnected
instead of down?
This issue is more than a color-swatch
but about getting an inventory of derived state computations that will be used throughout the application via related Ember Data models and services (our Stats Tracker Registry). I don't think there will be too much to look into re: Stats Tracker Registry at a cursory glance, but just wanted to highlight all of the possible places we should be looking.
How can I set this environment up myself?
I think we'll want to include some documentation about how to reproduce this without Mirage. @DerekStrickland is there any documentation about setting this environment up?
There are certain actions the user can take against nodes
and allocations
such as setting node eligibility and draining the node in the UI, and we'll need intense testing around these possible behaviors.
@@ -74,6 +74,7 @@ module('Unit | Util | JobClientStatus', function () { | |||
notScheduled: [], | |||
queued: [], | |||
starting: [], | |||
unknown: [], |
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.
thought: should we write a unit test for unknown
statuses and how it can impact our computation?
@@ -18,6 +18,7 @@ export default Factory.extend({ | |||
Running: faker.random.number(10), | |||
Starting: faker.random.number(10), | |||
Lost: faker.random.number(10), | |||
Unknown: faker.random.number(10), |
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.
question: Should this impact all jobs?
In the edge case, we're developing this feature to enable those users to maintain their up-time requirements. That makes me feel like this situation is only regarding service
jobs which are long-standing jobs compared to system
and batch
jobs.
cc: @DerekStrickland @mikenomitch do I have this to be, correct?
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.
System, sysbatch, service and batch jobs can all transition to unknown.
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 all of them can be unknown.
Not important for the code, but actually in some ways batch is more vital than service. Since batch jobs are "stateful" more often that service, the cost of restarting them can often be higher. So losing connection and coming back gracefully is more important.
There is! You can follow the steps outlined here for setting up a Vagrant environment. I can also help support you as you work through that process if it would help. Another alternative is to set up an E2E environment using our E2E test terraform. The README is pretty awesome. If you need/want to do it together just let me know. Once you have the environment up and running, you can find some sample jobs and basic instructions in this repo. I sent you an invite. The instructions in the root readme should be G2G for the basic use case. They are geared towards the Vagrant environment, so if you go with E2E you'll have to make some adjustments. Just let me know if you need help. |
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
Ember Test Audit detected these flaky tests on 9f8ae31:
|
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. |
Resolves #12485
Resolves #12486
Adds "Unknown" as a valid status in allocation tables, to the allocation status bar, and the allocation legend.
![image](https://user-images.githubusercontent.com/713991/162802116-635c6f7f-2e94-4f57-a95b-1d3c6cd31986.png)