-
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: Topology Visualization #9077
Conversation
Ember Asset Size actionAs of 8f94a98 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
346cbd4
to
28a7cc2
Compare
- Plot all datacenters - For each datacenter, plot all nodes - For each node, plot all allocations by memory and cpu - For empty nodes, highlight the emptiness - When hovering over allocations, give them visual focus
TBH, it's buggy and I don't like it.
… structures Now all data loading happens in the TopoViz component as well as computation of resource proportions. Allocation selection state is also managed centrally uses a dedicated structure indexed by group key (job id and task group name). This way allocations don't need to be scanned at the node level, which is O(n) at the best (assuming no ember overhead on recomputes).
447fdcc
to
73dbb3e
Compare
73dbb3e
to
b5809d4
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.
I documented some browser-specific problems but those can be addressed in followups, maybe the only thing I’d consider blocking is the changes to the environment file/default scenario, unless that was deliberate. I tried this out locally (only with Mirage) but it’s fun to toy with and seems like it’ll be useful!
|
||
test('formats x > 1024 * 1024 * 1024 * 1024 as GiB, since it is the highest allowed unit', function(assert) { | ||
assert.equal(formatBytes([1024 * 1024 * 1024 * 1024]), '1024 GiB'); | ||
assert.equal(formatBytes([1024 * 1024 * 1024 * 1024 * 4]), '4096 GiB'); |
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.
nice, maybe I can use this elsewhere 😀
|
||
allocationAssociationsArePresent: isPresent('[data-test-allocation-associations]'), | ||
allocationAssociations: collection('[data-test-allocation-association]'), | ||
}); |
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 is surely not something you have any influence on but starting with this file there’s no more syntax highlighting, strangely!
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 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 find this happens pretty regularly with mega PRs.
margin-left: 1.5em; | ||
} | ||
} | ||
} |
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 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.
Good call.
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.
looks good 🎉
transform: translate(50%, 50%); | ||
text-anchor: middle; | ||
alignment-baseline: central; | ||
} |
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 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.
😱 Browsers, please.
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.
whew, sad times but nice fix
{{/each}} | ||
</g> | ||
</svg> | ||
{{/if}} |
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 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 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 resizing might be a bug, but the lines pointing to weird places is a quirk of the fixtures.
I didn't bother making sure allocations weren't "placed" on clients that didn't have room for them. The result is allocations taking up more than 100% of the width. So that line going to the empty client is actually going to the memory rect for the client in the left column, except it's beyond the width of the client.
It would be nice to make this better at some point, but I'm not sure how important it is since it's just for our local dev experience.
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.
ah understood, doesn’t seem worth fixing to me then, thanks
ui/app/controllers/topology.js
Outdated
.reduce((sum, memory) => sum + (memory || 0), 0); | ||
const totalReservedCPU = node.allocations | ||
.mapBy('cpu') | ||
.reduce((sum, cpu) => sum + (cpu || 0), 0); |
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 looks like these reducer functions could be shared?
// the yOffset to match heights with unstroked shapes. | ||
get selectedYOffset() { | ||
return this.height + 2.5; | ||
} |
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 appreciate these explanations, sad that it has to be this way 😐
// Steps are used to restrict the range of curves. The closer control points are placed, the less | ||
// curvature the curve generator will generate. | ||
const stepsMain = [0, 0.8, 1.0]; | ||
// The second prong the fork does not need to retrace the entire path from the activePoint |
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 appreciate these comments, would be nice if it were easy to have a reference diagram, but such is not the way hehe
ui/app/components/flex-masonry.js
Outdated
@action | ||
reflow() { | ||
run.next(() => { | ||
// There's nothing to do if this is single column layout |
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!
|
||
// Set the max height of the container to the height of the tallest column | ||
this.element.style.maxHeight = max(columns.mapBy('height')) + 1 + 'px'; | ||
}); |
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 appreciate the explanations, I hope this ends up being useful somewhere else too!
I forgot to mention this failing test, not sure if it’s a random flakiness problem that’s unrelated to this PR though as I don’t see any obvious change here that would relate 🤔 |
Yeah, this is the first time I've seen that test file. I cleaned up everything other than the SVG bugs, so I'll push that now to see what Circle says this time. |
Okay @backspace, ready for re-review. A few things I won't address right now (but feel free to disagree with 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.
ya I’m definitely fine with you ignoring those things! I had a question about a remaining change to the environment file but it’s looking good 😀
transform: translate(50%, 50%); | ||
text-anchor: middle; | ||
alignment-baseline: central; | ||
} |
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.
whew, sad times but nice fix
margin-left: 1.5em; | ||
} | ||
} | ||
} |
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.
looks good 🎉
ui/config/environment.js
Outdated
@@ -25,10 +25,10 @@ module.exports = function(environment) { | |||
|
|||
APP: { | |||
blockingQueries: false, | |||
mirageScenario: 'topoSmall', | |||
mirageScenario: 'smallCluster', |
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 still has blockingQueries
changed to false
by default, just making sure that’s by choice 🤓
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 wasn't...but I also like it? Ha. Blocking queries + Mirage is kinda janky anyway.
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 shoot, nvm, I'll flip it back. I forget that this impacts non-mirage too.
ui/mirage/scenarios/topo.js
Outdated
|
||
export function topoMediumBatch(server) {} | ||
|
||
export function topoMediumVariadic(server) {} |
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 another empty topoSmallProblems
higher up… not a big deal but might be worth removing. I’m viewing “changes since” or whatever hence me not commenting on that line 🙃
// Used in glimmer component unit tests. Glimmer components should typically | ||
// be tested with integration tests, but occasionally individual methods or | ||
// properties have logic that isn't coupled to rendering or the DOM and can | ||
// be better tested in a unit fashion. |
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.
thanks, now I understand why I might use this 😀
f48bd75
to
8f94a98
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. |
This adds a topology visualization to the UI. It can be used to the see the state of every client and allocation at a glance. You can read more about it on Discuss where prototypes were posted.
TODO
TopoViz
TopoViz::Datacenter
/topology