[ui, bugfix] Link fix for volumes where per_alloc=true #12713

philrenaud · 2022-04-20T04:29:23Z

In situations where per_alloc of a volume stanza is set to true, we currently throw a broken link: we try to find a base volume in a situation where it is split into multiples.

With this change, we disambiguate the per_alloc'd volume where possible.

This happens in 3 places:

on the allocation route, in the Tasks table
on the task route, in the Volumes table
on the task group route, in the Volume Requirements table.

Importantly, this third situation does not have the ability to disambiguate: because a task group does not know which allocation it refers to, I've opted to provide the user with a link to queried volumes route instead.

Assumptions

Allocations spread across multiple clients always receive a [#] suffix in their name property, and this is always the last bracketed text.

Side-effects

The Volumes table on the Task route was formerly using volumeMount.volume in its Client Source column. I've changed this to volumeMount.source, which I think is the intended string.

Resolves #11965

github-actions · 2022-04-20T04:35:45Z

Ember Asset Size action

As of 78c7282

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+1.61 kB	+349 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.js	0 B	0 B
nomad-ui.css	0 B	0 B
vendor.css	0 B	0 B

github-actions · 2022-04-20T05:09:49Z

Ember Test Audit comparison

	main	`78c7282`	change
passes	1273	1276	+3
failures	0	0	0
flaky	0	0	0
duration	11m 10s 144ms	10m 25s 969ms	-44s 175ms

github-actions · 2022-04-20T05:15:15Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on ffc2d4c:

Acceptance | task group detail: facet Status | the Status facet filters the allocations list by Status

tgross

Hi @philrenaud this mostly looks perfect. I've left a note about the the link text for the per-alloc volumes.

(Also, it'd be nice if at some point we could get CSI examples in the Vercel preview... it'd cut down on having to stand up a whole cluster with plugins and volumes and consuming tasks to review the results of PRs like this. But obviously that's out of scope for this PR! If there's anything I can do to help out with that let me know, but how that all works is a little out of my wheelhouse)

tgross · 2022-04-20T13:04:58Z

ui/app/templates/jobs/job/task-group.hbs

-                  >
-                    {{row.model.name}}
-                  </LinkTo>
+                  {{!-- if volume is per_alloc=true, there's no one specific volume. So, link to the volumes index with an active query --}}


Good catch. This seems like the best we can do here.

Probably unrelated to this PR, but on one attempt to click this link the back button broke and I couldn't get back to this page. Unfortunately it only happened once and I can't reproduce it anymore.

tgross · 2022-04-20T13:40:37Z

ui/app/templates/allocations/allocation/task/index.hbs

                    }}
                  >
-                    {{row.model.volume}}
+                    {{row.model.source}}


The link text won't match the ID in the case of per-alloc volumes. For example, the second link here for the per-alloc volume is for /ui/csi/volumes/csi-volume-nfs%5B0%5D@prod so I'd expect the link text to be csi-volume-nfs[0]

I see we also have this for ui/app/templates/components/task-row.hbs below, but I also can't figure out which of these two divs I'm seeing when I look at an allocation detail page like /ui/allocations/039a9058-6e9c-a793-f589-27552d2e56c2 😊

Probably unrelated to this PR but while trying to get to this page from the Job page I'm sometimes getting an error with the following message:

Error: Assertion Failed: You made a 'findRecord' request for a 'job' with id '["httpd","prod"]', but the adapter's response did not have any data

Probably unrelated to this PR but while trying to get to this page from the Job page I'm sometimes getting an error with the following message:

I've been noticing this quite a bit, too. I don't think this is newly introduced, but I'm keeping an eye on it.

Good catch on the link text; updated to reflect the suffix.

I noticed these intermittent issues as well. I think it may be related to #12082 but I need to investigate deeper.

The findRecord not-founds are being tracked here: #12724

github-actions · 2022-04-20T19:16:29Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on 633b877:

Acceptance | optimize search and facets: the Datacenter facet has the correct options

tgross

LGTM 👍

lgfa29

LGTM, left a small nit-pick comment. Another thing I would add, if possible, would be a mirage option that has perAlloc, somewhere around here?
https://github.com/hashicorp/nomad/blob/main/ui/mirage/scenarios/default.js#L53-L59

ui/app/templates/allocations/allocation/task/index.hbs

lgfa29 · 2022-04-20T20:06:05Z

Also missing a CHANGELOG entry 🙂
https://github.com/hashicorp/nomad/blob/main/contributing/CHANGELOG.md

ChaiWithJai · 2022-04-20T20:52:17Z

ui/app/models/allocation.js

+  // We differentiate them with a [#] suffix, inferred from a volume's allocation's name property.
+  @computed('name')
+  get volumeExtension() {
+    return this.name.substring(this.name.lastIndexOf('['));


praise: great job! This is such a simple, elegant solution.

ChaiWithJai

LGTM. I think template readability could be a little different, but its not a blocker.

github-actions · 2022-10-15T02:46:23Z

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.

philrenaud added 3 commits April 19, 2022 23:40

Allocation page linkfix

3926584

fix added to task page and computed prop moved to allocation model

f16fdc5

Fallback query added to task group when specific volume isnt knowable

ffc2d4c

philrenaud requested review from tgross and ChaiWithJai April 20, 2022 04:29

philrenaud self-assigned this Apr 20, 2022

Delog

190370d

vercel bot deployed to Preview – nomad-storybook-and-ui April 20, 2022 04:33 View deployment

tgross reviewed Apr 20, 2022

View reviewed changes

lgfa29 added this to the 1.3.0 milestone Apr 20, 2022

link text reflects alloc suffix

633b877

vercel bot deployed to Preview – nomad-storybook-and-ui April 20, 2022 18:43 View deployment

tgross approved these changes Apr 20, 2022

View reviewed changes

lgfa29 approved these changes Apr 20, 2022

View reviewed changes

ui/app/templates/allocations/allocation/task/index.hbs Outdated Show resolved Hide resolved

ChaiWithJai reviewed Apr 20, 2022

View reviewed changes

ChaiWithJai approved these changes Apr 20, 2022

View reviewed changes

Helper instead of in-template conditionals

0ebdeb6

vercel bot deployed to Preview – nomad-storybook-and-ui April 21, 2022 15:31 View deployment

formatVolumeName unit test

f1ee4bd

vercel bot deployed to Preview – nomad-storybook-and-ui April 21, 2022 16:20 View deployment

Removing unused helper import

78c7282

vercel bot deployed to Preview – nomad-storybook-and-ui April 21, 2022 16:36 View deployment

philrenaud merged commit a977577 into main Apr 21, 2022

philrenaud deleted the 11965-per_alloc-volume-ui-fix branch April 21, 2022 17:57

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ui, bugfix] Link fix for volumes where per_alloc=true #12713

[ui, bugfix] Link fix for volumes where per_alloc=true #12713

philrenaud commented Apr 20, 2022

github-actions bot commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022

tgross left a comment

tgross Apr 20, 2022

tgross Apr 20, 2022

tgross Apr 20, 2022

tgross Apr 20, 2022

philrenaud Apr 20, 2022

philrenaud Apr 20, 2022

lgfa29 Apr 20, 2022

philrenaud Apr 21, 2022

github-actions bot commented Apr 20, 2022

tgross left a comment

lgfa29 left a comment

lgfa29 commented Apr 20, 2022

ChaiWithJai Apr 20, 2022

ChaiWithJai left a comment

github-actions bot commented Oct 15, 2022

[ui, bugfix] Link fix for volumes where per_alloc=true #12713

[ui, bugfix] Link fix for volumes where per_alloc=true #12713

Conversation

philrenaud commented Apr 20, 2022

Assumptions

Side-effects

github-actions bot commented Apr 20, 2022 • edited Loading

Ember Asset Size action

github-actions bot commented Apr 20, 2022 • edited Loading

Ember Test Audit comparison

github-actions bot commented Apr 20, 2022

Ember Test Audit flaky tests

tgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 20, 2022

Ember Test Audit flaky tests

tgross left a comment

Choose a reason for hiding this comment

lgfa29 left a comment

Choose a reason for hiding this comment

lgfa29 commented Apr 20, 2022

Choose a reason for hiding this comment

ChaiWithJai left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2022

github-actions bot commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading