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.

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] Jobs list should handle 502s and 504s gracefully #23427

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .changelog/23427.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: fix an issue where gateway timeouts would cause the jobs list to revert to null, gives users a Pause Fetch option
```
77 changes: 77 additions & 0 deletions ui/app/controllers/jobs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export default class JobsIndexController extends Controller {
@service store;
@service userSettings;
@service watchList;
@service notifications;

@tracked pageSize;

Expand Down Expand Up @@ -156,12 +157,70 @@ export default class JobsIndexController extends Controller {
);
}

/**
* In case the user wants to specifically stop polling for new jobs
*/
@action pauseJobFetching() {
let notification = this.notifications.queue.find(
(n) => n.title === 'Error fetching jobs'
);
if (notification) {
notification.destroyMessage();
}
this.watchList.jobsIndexIDsController.abort();
this.watchList.jobsIndexDetailsController.abort();
this.watchJobIDs.cancelAll();
this.watchJobs.cancelAll();
}

@action restartJobList() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I let the user click "Try fetching again" before they've paused here even though the current retry on failure is 5sec with an eye to the future: we might extend that timeout beyond 5sec, or more likely add something like exponential backoff.

For now, though, it's an "I want to see the network request fire immediately to see if my new proxy config settings work" button.

this.showingCachedJobs = false;
let notification = this.notifications.queue.find(
(n) => n.title === 'Error fetching jobs'
);
if (notification) {
notification.destroyMessage();
}
this.watchList.jobsIndexIDsController.abort();
this.watchList.jobsIndexDetailsController.abort();
this.watchJobIDs.cancelAll();
this.watchJobs.cancelAll();
this.watchJobIDs.perform({}, JOB_LIST_THROTTLE);
this.watchJobs.perform(this.jobIDs, JOB_DETAILS_THROTTLE);
}

@localStorageProperty('nomadLiveUpdateJobsIndex', true) liveUpdatesEnabled;

// #endregion pagination

//#region querying

/**
*
* Let the user know that there was difficulty fetching jobs, but don't overload their screen with notifications.
* Set showingCachedJobs to tell the template to prompt them to extend timeouts
* @param {Error} e
*/
notifyFetchError(e) {
const firstError = e.errors[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could .any() this, but ¯_(ツ)_/¯

this.notifications.add({
title: 'Error fetching jobs',
message: `The backend returned an error with status ${firstError.status} while fetching jobs`,
color: 'critical',
sticky: true,
preventDuplicates: true,
});
// Specific check for a proxy timeout error
if (
!this.showingCachedJobs &&
(firstError.status === '502' || firstError.status === '504')
) {
this.showingCachedJobs = true;
}
}

@tracked showingCachedJobs = false;

jobQuery(params) {
this.watchList.jobsIndexIDsController.abort();
this.watchList.jobsIndexIDsController = new AbortController();
Expand All @@ -172,9 +231,17 @@ export default class JobsIndexController extends Controller {
abortController: this.watchList.jobsIndexIDsController,
},
})
.then((jobs) => {
this.showingCachedJobs = false;
return jobs;
})
.catch((e) => {
if (e.name !== 'AbortError') {
console.log('error fetching job ids', e);
this.notifyFetchError(e);
}
if (this.jobs.length) {
return this.jobs;
}
return;
});
Expand All @@ -194,6 +261,10 @@ export default class JobsIndexController extends Controller {
.catch((e) => {
if (e.name !== 'AbortError') {
console.log('error fetching job allocs', e);
this.notifyFetchError(e);
}
if (this.jobs.length) {
return this.jobs;
}
return;
});
Expand Down Expand Up @@ -257,8 +328,14 @@ export default class JobsIndexController extends Controller {
this.pendingJobIDs = jobIDs;
this.pendingJobs = newJobs;
}
if (Ember.testing) {
break;
}
yield timeout(throttle);
} else {
if (Ember.testing) {
break;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A small change to my behaviour for testing purposes: I want to break out of a test on error now, too, which hadn't come up before. Without these lines, my test will be caught in a terrible loop.

// This returns undefined on page change / cursorAt change, resulting from the aborting of the old query.
yield timeout(throttle);
this.watchJobs.perform(this.jobIDs, throttle);
Expand Down
8 changes: 7 additions & 1 deletion ui/app/serializers/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,13 @@ export default class JobSerializer extends ApplicationSerializer {
return super.normalize(typeHash, hash);
}

normalizeQueryResponse(store, primaryModelClass, payload, id, requestType) {
normalizeQueryResponse(
store,
primaryModelClass,
payload = [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only notable change here is a default of payload to [] so I don't have to do a bunch of conditional guards with elvis operators later

id,
requestType
) {
// What jobs did we ask for?
if (payload._requestBody?.jobs) {
let requestedJobIDs = payload._requestBody.jobs;
Expand Down
4 changes: 4 additions & 0 deletions ui/app/styles/components/jobs-list.scss
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@
}
}

#jobs-list-cache-warning {
margin-bottom: 1rem;
}

.status-cell {
display: flex;
gap: 0.5rem;
Expand Down
Loading
Loading