Skip to content
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.

Sign up for GitHub

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, bugfix] Link fix for volumes where per_alloc=true #12713

Merged
merged 8 commits into from
Apr 21, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3926584
Allocation page linkfix
philrenaud Apr 20, 2022
f16fdc5
fix added to task page and computed prop moved to allocation model
philrenaud Apr 20, 2022
ffc2d4c
Fallback query added to task group when specific volume isnt knowable
philrenaud Apr 20, 2022
190370d
Delog
philrenaud Apr 20, 2022
633b877
link text reflects alloc suffix
philrenaud Apr 20, 2022
0ebdeb6
Helper instead of in-template conditionals
philrenaud Apr 21, 2022
f1ee4bd
formatVolumeName unit test
philrenaud Apr 21, 2022
78c7282
Removing unused helper import
philrenaud Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ui/app/models/allocation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ export default class Allocation extends Model {
return [];
}

// When per_alloc is set to true on a volume, the volumes are duplicated between active allocations.
// 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('['));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@fragmentArray('task-state') states;
@fragmentArray('reschedule-event') rescheduleEvents;

Expand Down
1 change: 1 addition & 0 deletions ui/app/models/volume-definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default class VolumeDefinition extends Fragment {
@attr('string') source;
@attr('string') type;
@attr('boolean') readOnly;
@attr('boolean') perAlloc;

@equal('type', 'csi') isCSI;
@alias('taskGroup.job.namespace') namespace;
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/allocations/allocation/task/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,10 @@
{{#if row.model.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.volume "@" row.model.namespace.id
@model={{concat row.model.source (if row.model.volumeDeclaration.perAlloc this.model.allocation.volumeExtension) "@" row.model.namespace.id
}}
>
{{row.model.volume}}
{{row.model.source}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Screen Shot 2022-04-20 at 9 37 04 AM

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 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screen Shot 2022-04-20 at 9 43 20 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</LinkTo>
{{else}}
{{row.model.source}}
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/task-row.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
{{#if volume.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat volume.source "@" volume.namespace.id}}
@model={{concat volume.source (if volume.volumeDeclaration.perAlloc this.task.allocation.volumeExtension) "@" volume.namespace.id}}
>
{{volume.source}}
</LinkTo>
Expand Down
17 changes: 11 additions & 6 deletions ui/app/templates/jobs/job/task-group.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,17 @@
<tr data-test-volume>
<td data-test-volume-name>
{{#if row.model.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.source "@" row.model.namespace.id}}
>
{{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 --}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{#if row.model.perAlloc}}
<LinkTo @route="csi.volumes.index" @query={{hash search=row.model.source}}>{{row.model.name}}</LinkTo>
{{else}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.source "@" row.model.namespace.id}}
>
{{row.model.name}}
</LinkTo>
{{/if}}
{{else}}
{{row.model.name}}
{{/if}}
Expand Down