-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[server] Use image-builder from workspace cluster #9337
Conversation
a2daa49
to
20caf33
Compare
4f5fb05
to
b6906d7
Compare
20caf33
to
4d34259
Compare
/werft run with-clean-slate 👍 started the job as gitpod-build-cw-imgbldr-mv-ii.3 |
Hi 👋 , @csweichel #9335 is deployed. 👍 🙌 @geropl , do we have a governed workspace cluster in What scenarios do we have to test?
@aledbf is there anything we need to prepare for, that you can think of, that we should add here? |
We don't have separate workspace clusters, for have EU/US as "full" installations, with the ws-manager being dynamically registered. So we should be able to test just like in prod. 👍 Will review here today. |
4d34259
to
b4175c6
Compare
b4175c6
to
e681a76
Compare
ctx.span?.setTag("workspace.imageBuild.logs.fallback", true); | ||
await this.deprecatedDoWatchWorkspaceImageBuildLogs(ctx, logCtx, workspace); | ||
return; | ||
log.error(logCtx, "cannot watch imagebuild logs for workspaceId: no image build info available"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
BTW: reviewed yesterday, and code looks good! 🙏 Now waiting for a preview to test and play around with. |
/werft run 👍 started the job as gitpod-build-cw-imgbldr-mv-ii.6 |
@csweichel Can we move forward with this, or is it blocked somehow? 🤔 |
e681a76
to
6e37dca
Compare
Taking this back to "ready for review" because the PR is indeed ready for review. /hold |
/werft run with-clean-slate-deployment=true 👍 started the job as gitpod-build-cw-imgbldr-mv-ii.17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, except for one minor typo, but approving with hold to address that fix.
/hold
Tested in preview env using @jankeromnes two test repos and everything worked as expected.
@@ -92,14 +92,14 @@ export class WorkspaceManagerClientProvider implements Disposable { | |||
let client = this.connectionCache.get(name); | |||
if (!client) { | |||
const info = await getConnectionInfo(); | |||
client = this.createClient(info, grpcOptions); | |||
client = client = this.createConnection(WorkspaceManagerClient, info, grpcOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csweichel this is still not addressed.
964929b
to
c1508ba
Compare
/unhold |
/hold |
We have that now in gen46. What's missing? |
We need to investigate if the new image builder log path is being used. I.e. the path where image builder reports the log location and server consumes it the same way we do for prebuilds. |
What's the state of this issue? We did various improvements and bug fixes around the "Prebuild in Progress" view in #10361, but without a refresh on image build logs mentioned previously, it's overshadowed by the lack of logs. From UX perspective the view remains broken, as no one will know how long to wait for the results if the logs aren't served. |
I'm moving this into draft as currently it is unclear what remains to do on this PR. Feel free to bring back into review if this is in a reviewable (and mergeable) state. |
@easyCZ 👍 for moving into draft. FYI, @kylos101 and I agreed that the next step is two things:
ℹ️ The logs-PR contains some of the changes from this branch, so I will rebase it once those one hit |
To move forward I split up both aspects of this PR and put them behind feature flags. The 2nd PR is here: #11103 . |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@csweichel @kylos101 what would you like to do with this PR? |
Closing. Done. |
Description
This PR makes server use the image builder from workspace cluster.
**Note: ** merge and deploy this only once #9335 is merged, deployed and running.
Related Issue(s)
Fixes #9248
How to test
Start a workspace which requires an image-build
Release Notes