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

Jump to bottom

[ui] Disconnected Clients: "Unknown" allocations in the UI #12544

Merged
merged 13 commits into from
Apr 22, 2022
Merged

[ui] Disconnected Clients: "Unknown" allocations in the UI #12544

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3fcff4c
Unknown status for allocations accounted for
philrenaud Apr 11, 2022
24ce5f8
Canary string removed
philrenaud Apr 11, 2022
0fd2b7e
Test cleanup
philrenaud Apr 11, 2022
8101e06
Generate unknown in mirage
philrenaud Apr 11, 2022
dc6a463
aacidentally oovervoowled
philrenaud Apr 12, 2022
25c57c6
Update ui/app/components/allocation-status-bar.js
philrenaud Apr 12, 2022
8db1b19
Disconnected state on job status in client
philrenaud Apr 13, 2022
4aadf31
Renaming Disconnected to Unknown in the job-status-in-client
philrenaud Apr 13, 2022
6377f93
Unknown accounted for on job rows filtering and testsfix
philrenaud Apr 13, 2022
7f40945
Adding lostAllocs as a computed dependency
philrenaud Apr 13, 2022
44d0fd5
Unknown client status within acceptance test
philrenaud Apr 13, 2022
2daa06c
Swatches updated and PR comments addressed
philrenaud Apr 21, 2022
9f8ae31
Unknown and disconnected added to test fixtures
philrenaud Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions ui/app/components/allocation-status-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export default class AllocationStatusBar extends DistributionBar {
}

@computed(
'allocationContainer.{queuedAllocs,completeAllocs,failedAllocs,runningAllocs,startingAllocs}',
'allocationContainer.{queuedAllocs,completeAllocs,failedAllocs,runningAllocs,startingAllocs,lostAllocs,unknownAllocs}',
'job.namespace'
)
get data() {
Expand All @@ -39,7 +39,8 @@ export default class AllocationStatusBar extends DistributionBar {
'failedAllocs',
'runningAllocs',
'startingAllocs',
'lostAllocs'
'lostAllocs',
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
'unknownAllocs'
);
return [
{
Expand Down Expand Up @@ -67,6 +68,13 @@ export default class AllocationStatusBar extends DistributionBar {
className: 'complete',
legendLink: this.generateLegendLink(this.job, 'complete'),
},
{
label: 'Unknown',
value: allocs.unknownAllocs,
className: 'unknown',
legendLink: this.generateLegendLink(this.job, 'unknown'),
help: 'Allocation is unknown since its node is disconnected.',
},
{
label: 'Failed',
value: allocs.failedAllocs,
Expand Down
13 changes: 13 additions & 0 deletions ui/app/components/job-client-status-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export default class JobClientStatusBar extends DistributionBar {
failed,
lost,
notScheduled,
unknown,
} = this.jobClientStatus.byStatus;

return [
Expand Down Expand Up @@ -71,6 +72,18 @@ export default class JobClientStatusBar extends DistributionBar {
},
},
},
{
label: 'Unknown',
value: unknown.length,
className: 'unknown',
legendLink: {
queryParams: {
status: JSON.stringify(['unknown']),
namespace: this.job.namespace.get('id'),
},
},
help: 'Some allocations for this job were degraded or lost connectivity.',
},
{
label: 'Degraded',
value: degraded.length,
Expand Down
4 changes: 4 additions & 0 deletions ui/app/components/job-client-status-row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default class ClientRow extends Component {
runningAllocs: 0,
startingAllocs: 0,
lostAllocs: 0,
unknownAllocs: 0,
};

switch (this.args.row.model.jobStatus) {
Expand Down Expand Up @@ -77,6 +78,9 @@ export default class ClientRow extends Component {
case 'starting':
statusSummary.startingAllocs++;
break;
case 'unknown':
statusSummary.unknownAllocs++;
break;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions ui/app/controllers/clients/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change (and similar changes to jobs/job/allocations controller, etc.) all correspond to these filters:

image

];
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/controllers/jobs/job/allocations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export default class AllocationsController extends Controller.extend(
{ key: 'complete', label: 'Complete' },
{ key: 'failed', label: 'Failed' },
{ key: 'lost', label: 'Lost' },
{ key: 'unknown', label: 'Unknown' },
];
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/controllers/jobs/job/clients.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export default class ClientsController extends Controller.extend(
{ key: 'degraded', label: 'Degraded' },
{ key: 'failed', label: 'Failed' },
{ key: 'lost', label: 'Lost' },
{ key: 'unknown', label: 'Unknown' },
];
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/controllers/jobs/job/task-group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export default class TaskGroupController extends Controller.extend(
{ key: 'complete', label: 'Complete' },
{ key: 'failed', label: 'Failed' },
{ key: 'lost', label: 'Lost' },
{ key: 'unknown', label: 'Unknown' },
];
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/models/job-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default class JobSummary extends Model {
@sumAggregation('taskGroupSummaries', 'runningAllocs') runningAllocs;
@sumAggregation('taskGroupSummaries', 'completeAllocs') completeAllocs;
@sumAggregation('taskGroupSummaries', 'failedAllocs') failedAllocs;
@sumAggregation('taskGroupSummaries', 'unknownAllocs') unknownAllocs;
@sumAggregation('taskGroupSummaries', 'lostAllocs') lostAllocs;

@collect(
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
1 change: 1 addition & 0 deletions ui/app/models/job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ export default class Job extends Model {
@alias('summary.completeAllocs') completeAllocs;
@alias('summary.failedAllocs') failedAllocs;
@alias('summary.lostAllocs') lostAllocs;
@alias('summary.unknownAllocs') unknownAllocs;
@alias('summary.totalAllocs') totalAllocs;
@alias('summary.pendingChildren') pendingChildren;
@alias('summary.runningChildren') runningChildren;
Expand Down
4 changes: 3 additions & 1 deletion ui/app/models/task-group-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ export default class TaskGroupSummary extends Fragment {
@attr('number') completeAllocs;
@attr('number') failedAllocs;
@attr('number') lostAllocs;
@attr('number') unknownAllocs;

@collect(
'queuedAllocs',
'startingAllocs',
'runningAllocs',
'completeAllocs',
'failedAllocs',
'lostAllocs'
'lostAllocs',
'unknownAllocs'
)
allocsList;

Expand Down
8 changes: 8 additions & 0 deletions ui/app/styles/charts/colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ $canceled: $dark;
.degraded {
fill: $degraded;
}

.unknown {
fill: $unknown;
}
}

.color-swatch {
Expand Down Expand Up @@ -114,6 +118,10 @@ $canceled: $dark;
background: $lost;
}

&.unknown {
background: $unknown;
}

philrenaud marked this conversation as resolved.
Show resolved Hide resolved
&.not-scheduled {
background: $not-scheduled;
}
Expand Down
9 changes: 8 additions & 1 deletion ui/app/styles/charts/distribution-bar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
opacity: 0;
}

$color-sequence: $orange, $yellow, $green, $turquoise, $blue, $purple, $red;
$color-sequence: $orange, $yellow, $green, $turquoise, $blue, $purple,
$red;

@for $i from 1 through length($color-sequence) {
.slice-#{$i - 1} {
Expand Down Expand Up @@ -113,6 +114,12 @@
color: darken($grey-blue, 20%);
}
}

// Prevent Orphaned last-elements in the list
// from being visually centered
&:nth-child(2n + 1):last-child {
margin-right: calc(35% + 0.75em);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this rule:
image

With this rule:
image

}
}
}
Expand Down
6 changes: 4 additions & 2 deletions ui/app/styles/core/variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ $purple: $terraform-purple;
$red: #c84034;
$grey-blue: #bbc4d1;
$blue-light: #c0d5ff;
$yellow: #fac402;

$primary: $nomad-green;
$warning: $orange;
$warning-invert: $white;
$danger: $red;
$info: $blue;
$unknown: $yellow;
$dark: #234;
$dark-2: darken($dark, 5%);
$dark-3: darken($dark, 10%);
Expand All @@ -25,8 +27,8 @@ $size-7: 0.85rem;

$title-weight: $weight-semibold;

$family-sans-serif: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen-Sans, Ubuntu,
Cantarell, 'Helvetica Neue', sans-serif;
$family-sans-serif: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto,
Oxygen-Sans, Ubuntu, Cantarell, 'Helvetica Neue', sans-serif;
ChaiWithJai marked this conversation as resolved.
Show resolved Hide resolved

$text: $black;

Expand Down
5 changes: 5 additions & 0 deletions ui/app/utils/properties/job-client-status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const STATUS = [
'degraded',
'failed',
'lost',
'unknown',
];

// An Ember.Computed property that computes the aggregated status of a job in a
Expand Down Expand Up @@ -137,5 +138,9 @@ function jobStatus(allocs, expected) {
return 'running';
}

if (summary['unknown'] > 0) {
return 'unknown';
}

return 'starting';
}
3 changes: 2 additions & 1 deletion ui/mirage/factories/job-summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export default Factory.extend({
namespace: null,

withSummary: trait({
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
Summary: function() {
Summary: function () {
return this.groupNames.reduce((summary, group) => {
summary[group] = {
Queued: faker.random.number(10),
Expand All @@ -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),
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

};
return summary;
}, {});
Expand Down
17 changes: 13 additions & 4 deletions ui/tests/acceptance/client-detail-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,12 +1140,21 @@ module('Acceptance | client detail', function (hooks) {
testFacet('Status', {
facet: ClientDetail.facets.status,
paramName: 'status',
expectedOptions: ['Pending', 'Running', 'Complete', 'Failed', 'Lost'],
expectedOptions: [
'Pending',
'Running',
'Complete',
'Failed',
'Lost',
'Unknown',
],
async beforeEach() {
server.createList('job', 5, { createAllocations: false });
['pending', 'running', 'complete', 'failed', 'lost'].forEach((s) => {
server.createList('allocation', 5, { clientStatus: s });
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
});
['pending', 'running', 'complete', 'failed', 'lost', 'unknown'].forEach(
(s) => {
server.createList('allocation', 5, { clientStatus: s });
}
);

await ClientDetail.visit({ id: node.id });
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
},
Expand Down
17 changes: 13 additions & 4 deletions ui/tests/acceptance/job-allocations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,20 @@ module('Acceptance | job allocations', function (hooks) {
testFacet('Status', {
facet: Allocations.facets.status,
paramName: 'status',
expectedOptions: ['Pending', 'Running', 'Complete', 'Failed', 'Lost'],
expectedOptions: [
'Pending',
'Running',
'Complete',
'Failed',
'Lost',
'Unknown',
],
async beforeEach() {
['pending', 'running', 'complete', 'failed', 'lost'].forEach((s) => {
server.createList('allocation', 5, { clientStatus: s });
});
['pending', 'running', 'complete', 'failed', 'lost', 'unknown'].forEach(
(s) => {
server.createList('allocation', 5, { clientStatus: s });
}
);
await Allocations.visit({ id: job.id });
},
filter: (alloc, selection) =>
Expand Down
1 change: 1 addition & 0 deletions ui/tests/acceptance/job-clients-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ module('Acceptance | job clients', function (hooks) {
'Degraded',
'Failed',
'Lost',
'Unknown',
],
async beforeEach() {
await Clients.visit({ id: job.id });
Expand Down
17 changes: 13 additions & 4 deletions ui/tests/acceptance/task-group-detail-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,11 +669,20 @@ module('Acceptance | task group detail', function (hooks) {
testFacet('Status', {
facet: TaskGroup.facets.status,
paramName: 'status',
expectedOptions: ['Pending', 'Running', 'Complete', 'Failed', 'Lost'],
expectedOptions: [
'Pending',
'Running',
'Complete',
'Failed',
'Lost',
'Unknown',
],
async beforeEach() {
['pending', 'running', 'complete', 'failed', 'lost'].forEach((s) => {
server.createList('allocation', 5, { clientStatus: s });
});
['pending', 'running', 'complete', 'failed', 'lost', 'unknown'].forEach(
(s) => {
server.createList('allocation', 5, { clientStatus: s });
}
);
await TaskGroup.visit({ id: job.id, name: taskGroup.name });
},
filter: (alloc, selection) =>
Expand Down
1 change: 1 addition & 0 deletions ui/tests/integration/components/job-client-status-bar-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ module('Integration | Component | job-client-status-bar', function (hooks) {
failed: [],
lost: [],
notScheduled: [],
unknown: [],
},
},
isNarrow: true,
Expand Down