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

Prebuild Events #5116

Merged
merged 3 commits into from
Sep 1, 2021
Merged

Prebuild Events #5116

merged 3 commits into from
Sep 1, 2021

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Aug 9, 2021

This PR includes

  • events on new prebuilds and updates of the Prebuilds page
  • storing of prebuild infos on prebuild creation instead of computing over and over again
  • /werft with-clean-slate-deployment

@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 13, 2021

Removing the automatic review request because this is still Draft. 😊

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Aug 19, 2021

/werft run

👍 started the job as gitpod-build-at-prebuild-events.23

@AlexTugarev AlexTugarev force-pushed the at/prebuild-events branch 2 times, most recently from f90bba1 to 8c84609 Compare August 20, 2021 08:33
@AlexTugarev AlexTugarev changed the title WIP Prebuild Events Prebuild Events Aug 20, 2021
@roboquat roboquat added size/XL and removed size/XXL labels Aug 24, 2021
@roboquat roboquat removed the size/XL label Aug 24, 2021
@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 25, 2021

/werft run

👍 started the job as gitpod-build-at-prebuild-events.36

@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 25, 2021

Error: create: failed to create: secrets "sh.helm.release.v1.gitpod.v1" is forbidden: unable to create new content in namespace staging-at-prebuild-events because it is being terminated

I guess the old namespace needs to be cleaned up first.

EDIT: This prebuild was stuck in Terminating for 9 hours:

$ kubectl get pods
NAME                                            READY   STATUS        RESTARTS   AGE
prebuild-aceac695-3de4-4e55-8bde-aab5e202f1b0   0/1     Terminating   0          9h

Fixed like so:

$ kubectl delete pod prebuild-aceac695-3de4-4e55-8bde-aab5e202f1b0 --force --grace-period 0
warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
pod "prebuild-aceac695-3de4-4e55-8bde-aab5e202f1b0" force deleted

@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 25, 2021

/werft run

👍 started the job as gitpod-build-at-prebuild-events.37

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Seeing prebuilds pop-up on the Prebuilds list feels great! UX looks good! 🌟 🔝

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 03a58e29a0ba7afe522c8bfc7fff93755a2a1fe1

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Many thanks @AlexTugarev! 👍

The UX also looks good to me (the Prebuilds page is so fast now! ⚡⚡ thank you so much for that)

I've left a few questions & suggestions in-line. Most notably, I think the events on "start prebuild" and on "start workspace"(!) should be triggered asynchronously, so as to:

  • Not make these important API calls slower with extra awaits
  • Not make these important API calls fail if triggering an event throws

/hold

@@ -52,7 +52,7 @@ export default function () {
if (!prebuild) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I did get to see a Prebuild with working logs, but for another Prebuild (for https://gitlab.com/gitpod-io/gitlab/-/merge_requests/1) I got this slightly worrying error:

Uncaught (in promise) Error: Request findPrebuilds failed with message: ER_PARSE_ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1

Screenshot 2021-08-25 at 10 57 11

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jankeromnes, I've add a check to prevent it from hitting the db layer: https://github.com/gitpod-io/gitpod/pull/5116/files#diff-9c81815b0337b4e00a2ab1615cab6010a2497412f99dfc63c73a17f8bf28a90cR877

Can you invite me to that team please? I'd like to reproduce/verify the fix

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on my user 😬 but please feel free to move it to a team by hacking the DB

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexTugarev I tried reproducing this bug, but I couldn't. Maybe that's a somewhat good sign? Although quite inconclusive, due to the problems on dev-staging / the partial deployment. 😅

  • I tried triggering new prebuilds, but they get lost in the "randomly-sorted 30-limited Prebuilds list" (the current deployment probably doesn't have the sort-order or creationTime fixes)

Screenshot 2021-08-26 at 11 45 23

Screenshot 2021-08-26 at 11 59 49

components/server/ee/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved
components/server/ee/src/workspace/workspace-factory.ts Outdated Show resolved Hide resolved
components/server/ee/src/workspace/workspace-factory.ts Outdated Show resolved Hide resolved
components/server/src/projects/projects-service.ts Outdated Show resolved Hide resolved
Comment on lines +145 to +146
const pbws = await this.workspaceDb.trace({}).findPrebuiltWorkspaceById(prebuildId);
const info = (await this.workspaceDb.trace({}).findPrebuildInfos([prebuildId]))[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be cool to send an actual trace context here? (E.g. a few recently added methods in ProjectsService now take a TraceContext -- maybe findPrebuilds could too?)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻 for tracing. I

components/server/src/workspace/workspace-starter.ts Outdated Show resolved Hide resolved
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Aug 27, 2021

/werft run

👍 started the job as gitpod-build-at-prebuild-events.40

@AlexTugarev
Copy link
Member Author

/approve no-issue

@AlexTugarev
Copy link
Member Author

/hold cancel

@svenefftinge
Copy link
Member

/lgtm

@roboquat
Copy link
Contributor

roboquat commented Sep 1, 2021

LGTM label has been added.

Git tree hash: 1b9a810a8e32404584b4553fa3b110b91735739d

@roboquat
Copy link
Contributor

roboquat commented Sep 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexTugarev, gtsiolis, jankeromnes, svenefftinge

Associated issue requirement bypassed by: AlexTugarev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 6bc80eb into main Sep 1, 2021
@roboquat roboquat deleted the at/prebuild-events branch September 1, 2021 07:27
@AlexTugarev
Copy link
Member Author

Thanks @svenefftinge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants