[ws-manager-mk2] Rely on controller concurrency mechanism for content init and backup #16823
Conversation
Furisto
added
team: workspace
|
started the job as gitpod-build-fo-mk2-daemon-restart.2 because the annotations in the pull request description changed |
Furisto
force-pushed
the
fo/mk2-daemon-restart
branch
from
c595e4d to
6815f8c
Compare
Furisto
force-pushed
the
fo/mk2-daemon-restart
branch
from
1854ad3 to
10bea8a
Compare
Furisto
changed the title
roboquat
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
| if !wsc.latestWorkspace(ctx, &workspace) { | ||
| return ctrl.Result{Requeue: true}, nil | ||
| } |
There was a problem hiding this comment.
we should only do this at the point we're trying to start a content init or backup, otherwise we'll be sending an update on every reconciliation (which then again requeues a reconciliation, doesn't this result in an endless loop of reconciliations?)
There was a problem hiding this comment.
Haven't seen it 🤔 but makes a ton of sense.
| @@ -140,6 +144,13 @@ func (wsc *WorkspaceController) Reconcile(ctx context.Context, req ctrl.Request) | |||
| return ctrl.Result{}, nil | |||
| } | |||
|
|
|||
| func (wsc *WorkspaceController) latestWorkspace(ctx context.Context, ws *workspacev1.Workspace) bool { | |||
There was a problem hiding this comment.
Could you add some comments on why we need this? For the future reader 😄
| @@ -152,7 +163,7 @@ func (wsc *WorkspaceController) handleWorkspaceInit(ctx context.Context, ws *wor | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
i.e. move the latestWorkspace check to here, just before we're attempting to start the content init
|
@WVerlaek PTAL |
| ws.Status.SetCondition(workspacev1.NewWorkspaceConditionRefresh()) | ||
|
|
||
| err := wsc.Client.Status().Update(ctx, ws) | ||
| return !errors.IsConflict(err) |
There was a problem hiding this comment.
if there's a different error (e.g. api server temporarily unavailable), we'd want to also skip init/dispose I think, as we weren't able to confirm if the resource is the latest version. Maybe return a (bool, error) here, where err is non-nil on any other error than a conflict error?
Wdyt? Other than that lgtm
There was a problem hiding this comment.
Wouldn't that just mean that we would want to skip on every error? So we could just return an error and skip if it is non nil.
Furisto
force-pushed
the
fo/mk2-daemon-restart
branch
from
e83d563 to
ea5adbf
Compare
roboquat
added
deployed: workspace
Description
Replaces our own in-memory concurrency mechanism with the concurrency mechanism of the controller runtime. This is the first step towards making ws-daemon restartable but we are not there yet with this PR. Further testing needs to be done to ensure that this works in all cases.
Related Issue(s)
n.a.
How to test
Release Notes
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh