Skip to content

[prebuilds] Introduce 'failed' state for prebuilds #8601

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

Closed
wants to merge 1 commit into from
Closed

[prebuilds] Introduce 'failed' state for prebuilds #8601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions components/dashboard/src/projects/Prebuilds.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ export function prebuildStatusLabel(prebuild?: PrebuildWithStatus) {
return (<span className="font-medium text-blue-500 uppercase">running</span>);
case "aborted":
return (<span className="font-medium text-gray-500 uppercase">canceled</span>);
case "failed":
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment: You probably want to handle "failed" in prebuildStatusIcon below as well (there is already a StatusFailed icon you can re-use).

Copy link
Member

@easyCZ easyCZ Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As the number of error cases grows, it becomes harder to enumerate them. One possible way to simplify the API UX is to have a single error state and extend it with a cause which is set to a computer readable reason for the failure - TimedOut, RateLimited, FailedToStart, etc. The error message then becomes a human readable string to show in the UI etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @easyCZ. Alternatively, I think we could also special-case the few non-error cases, then consider "all the rest" as errors?

Copy link
Member

Choose a reason for hiding this comment

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

Depends how far we want to go. Some failures are due to user issues, some can be due to system issues. I think the it mostly depends if we control all the clients which will be interpreting the status (Dashboard, etc) or if some of the clients are/will be out of our control (implementing against an API). In the former case, it doesn't matter much as we can largely reach a shared understanding of what is an error and what isn't. But if an external frontend/server needs to switch on these, it may be less obivous to them with a large number of cases.

I think this is probs not something we need/should decide now and I don't think it blocks the change.

return (<span className="font-medium text-red-500 uppercase">failed</span>);
case "timeout":
return (<span className="font-medium text-red-500 uppercase">failed</span>);
case "available":
Expand Down
2 changes: 1 addition & 1 deletion components/dashboard/src/projects/Project.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ export default function () {
<a href={gitpodHostUrl.withContext(`${branch.url}`).toString()}>
<button className={`primary mr-2 py-2 opacity-0 group-hover:opacity-100`}>New Workspace</button>
</a>
<ItemFieldContextMenu className="py-0.5" menuEntries={(!prebuild || prebuild.status === 'aborted' || prebuild.status === 'timeout' || !!prebuild.error)
<ItemFieldContextMenu className="py-0.5" menuEntries={(!prebuild || prebuild.status === 'aborted' || prebuild.status === 'failed' || prebuild.status === 'timeout' || !!prebuild.error)
? [{
title: `${prebuild ? 'Rerun' : 'Run'} Prebuild (${branch.name})`,
onClick: () => triggerPrebuild(branch),
Expand Down
1 change: 1 addition & 0 deletions components/gitpod-protocol/src/headless-workspace-log.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export enum HeadlessWorkspaceEventType {
FinishedButFailed = "finish-fail",
AbortedTimedOut = "aborted-timeout",
Aborted = "aborted",
Failed = "failed",
Started = "started"
}
export namespace HeadlessWorkspaceEventType {
Expand Down
4 changes: 3 additions & 1 deletion components/gitpod-protocol/src/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,9 @@ export type PrebuiltWorkspaceState
// the prebuild timed out
| "timeout"
// the prebuild has finished and a snapshot is available
| "available";
| "available"
//
| "failed";

export interface PrebuiltWorkspace {
id: string;
Expand Down
2 changes: 1 addition & 1 deletion components/server/ee/src/prebuilds/prebuild-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export class PrebuildManager {
}
const existingPB = await this.workspaceDB.trace({ span }).findPrebuiltWorkspaceByCommit(cloneURL, commit);
// If the existing prebuild is failed, we want to retrigger it.
if (!!existingPB && existingPB.state !== 'aborted' && existingPB.state !== 'timeout' && !existingPB.error) {
if (!!existingPB && existingPB.state !== 'aborted' && existingPB.state !== 'failed' && existingPB.state !== 'timeout') {
// If the existing prebuild is based on an outdated project config, we also want to retrigger it.
const existingPBWS = await this.workspaceDB.trace({ span }).findById(existingPB.buildWorkspaceId);
const existingConfig = existingPBWS?.config;
Expand Down
10 changes: 6 additions & 4 deletions components/server/ee/src/prebuilds/prebuilt-status-maintainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,16 +102,18 @@ export class PrebuildStatusMaintainer implements Disposable {
protected getConclusionFromPrebuildState(pws: PrebuiltWorkspace): "error" | "failure" | "pending" | "success" {
if (pws.state === "aborted") {
return "error";
} else if (pws.state === "queued") {
return "pending";
} else if (pws.state === "failed") {
return "error";
} else if (pws.state === "timeout") {
return "error";
} else if (pws.state === "queued") {
return "pending";
} else if (pws.state === "building") {
return "pending";
} else if (pws.state === "available" && !pws.error) {
return "success";
} else if (pws.state === "available" && !!pws.error) {
return "failure";
} else if (pws.state === "building") {
return "pending";
} else {
log.warn("Should have updated prebuilt workspace updatable, but don't know how. Resorting to error conclusion.", { pws });
return "error";
Expand Down
7 changes: 5 additions & 2 deletions components/ws-manager-bridge/ee/src/bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ export class WorkspaceManagerBridgeEE extends WorkspaceManagerBridge {
prebuild.error = status.conditions!.timeout;
headlessUpdateType = HeadlessWorkspaceEventType.AbortedTimedOut;
} else if (!!status.conditions!.failed) {
prebuild.state = "aborted";
prebuild.state = "failed";
prebuild.error = status.conditions!.failed;
headlessUpdateType = HeadlessWorkspaceEventType.Aborted;
headlessUpdateType = HeadlessWorkspaceEventType.Failed;
} else if (!!status.conditions!.stoppedByRequest) {
prebuild.state = "aborted";
prebuild.error = "Cancelled";
Expand All @@ -89,6 +89,9 @@ export class WorkspaceManagerBridgeEE extends WorkspaceManagerBridge {
prebuild.error = status.conditions!.headlessTaskFailed;
prebuild.snapshot = status.conditions!.snapshot;
headlessUpdateType = HeadlessWorkspaceEventType.FinishedButFailed;
} else if (!status.conditions!.snapshot) {
prebuild.state = "failed";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Here you could also set prebuild.error to something like "Prebuild does not have a snapshot".

Copy link
Contributor Author

@svenefftinge svenefftinge Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That is a symptom not the actual error. I've discussed with Gero that we are also going to not overwrite actual errors if they exist (in case of multiple stop messages).

headlessUpdateType = HeadlessWorkspaceEventType.Failed;
} else {
prebuild.state = "available";
prebuild.snapshot = status.conditions!.snapshot;
Expand Down