From 82901b4938222f2717cab10521b78e6de774c1fe Mon Sep 17 00:00:00 2001 From: Pascal Breuninger Date: Thu, 16 Jan 2025 09:51:59 +0100 Subject: [PATCH] fix(pro): always check for available template updates and wait for changed parameters to be applied Changes the template behaviour for pro instances like so: 1. If template is not versioned _always_ keep the workspace in sync with template, even if that means rescheduling due to workspace changes 2. If the template is versioned and the workspace uses an explicit version we keep the workspace up to date with the template unless a new version is available. We expect versioned templates to introduce changes through a new version, not by updating an existing version In addition to that we now wait until the server applied parameter changes instead of assuming it did in time. This fixes a race condition where the parameter changes wouldn't be applied if the controller took more than a couple ms --- cmd/pro/provider/up.go | 63 +++++++++++++++++++++++++++- cmd/pro/provider/update/workspace.go | 24 ++--------- pkg/ide/vscode/open.go | 2 +- pkg/platform/instance.go | 56 ++++++++++++++++++++++++- 4 files changed, 121 insertions(+), 24 deletions(-) diff --git a/cmd/pro/provider/up.go b/cmd/pro/provider/up.go index f711600ac..bf0362f7b 100644 --- a/cmd/pro/provider/up.go +++ b/cmd/pro/provider/up.go @@ -2,19 +2,23 @@ package provider import ( "context" + "encoding/json" "fmt" "io" "os" "github.com/loft-sh/devpod/cmd/pro/flags" + "github.com/loft-sh/devpod/pkg/client/clientimplementation" "github.com/loft-sh/devpod/pkg/platform" "github.com/loft-sh/devpod/pkg/platform/client" "github.com/loft-sh/devpod/pkg/platform/remotecommand" "github.com/loft-sh/log" + "github.com/sirupsen/logrus" "github.com/spf13/cobra" managementv1 "github.com/loft-sh/api/v4/pkg/apis/management/v1" storagev1 "github.com/loft-sh/api/v4/pkg/apis/storage/v1" + corev1 "k8s.io/api/core/v1" ) // UpCmd holds the cmd flags: @@ -33,9 +37,15 @@ type streams struct { // NewUpCmd creates a new command func NewUpCmd(globalFlags *flags.GlobalFlags) *cobra.Command { + logLevel := logrus.InfoLevel + if os.Getenv(clientimplementation.DevPodDebug) == "true" || globalFlags.Debug { + logLevel = logrus.DebugLevel + } + cmd := &UpCmd{ GlobalFlags: globalFlags, - Log: log.GetInstance(), + Log: log.NewStreamLoggerWithFormat( /* we don't use stdout */ nil, + os.Stderr, logLevel, log.JSONFormat).ErrorStreamOnly(), streams: streams{ Stdin: os.Stdin, Stdout: os.Stdout, @@ -69,6 +79,24 @@ func (cmd *UpCmd) Run(ctx context.Context) error { instance, err := platform.FindInstanceInProject(ctx, baseClient, info.UID, info.ProjectName) if err != nil { return err + } else if instance == nil { + return fmt.Errorf("workspace %s not found in project %s. Looks like it does not exist anymore and you can delete it", info.ID, info.ProjectName) + } + + // Log current workspace information. This is both useful to the user to understand the workspace configuration + // and to us when we receive troubleshooting logs + printInstanceInfo(instance, cmd.Log) + + if instance.Spec.TemplateRef != nil && templateUpdateRequired(instance) { + cmd.Log.Info("Template update required") + oldInstance := instance.DeepCopy() + instance.Spec.TemplateRef.SyncOnce = true + + instance, err = platform.UpdateInstance(ctx, baseClient, oldInstance, instance, cmd.Log) + if err != nil { + return fmt.Errorf("update instance: %w", err) + } + cmd.Log.Info("Successfully updated template") } return cmd.up(ctx, instance, baseClient) @@ -85,10 +113,41 @@ func (cmd *UpCmd) up(ctx context.Context, workspace *managementv1.DevPodWorkspac return err } - _, err = remotecommand.ExecuteConn(ctx, conn, cmd.streams.Stdin, cmd.streams.Stdout, cmd.streams.Stderr, cmd.Log.ErrorStreamOnly()) + _, err = remotecommand.ExecuteConn(ctx, conn, cmd.streams.Stdin, cmd.streams.Stdout, cmd.streams.Stderr, cmd.Log) if err != nil { return fmt.Errorf("error executing: %w", err) } return nil } + +func templateUpdateRequired(instance *managementv1.DevPodWorkspaceInstance) bool { + var templateResolved, templateChangesAvailable bool + for _, condition := range instance.Status.Conditions { + if condition.Type == storagev1.InstanceTemplateResolved { + templateResolved = condition.Status == corev1.ConditionTrue + continue + } + + if condition.Type == storagev1.InstanceTemplateSynced { + templateChangesAvailable = condition.Status == corev1.ConditionFalse && + condition.Reason == "TemplateChangesAvailable" + continue + } + } + + return !templateResolved || templateChangesAvailable +} + +func printInstanceInfo(instance *managementv1.DevPodWorkspaceInstance, log log.Logger) { + workspaceConfig, _ := json.Marshal(struct { + Runner storagev1.RunnerRef + Template *storagev1.TemplateRef + Parameters string + }{ + Runner: instance.Spec.RunnerRef, + Template: instance.Spec.TemplateRef, + Parameters: instance.Spec.Parameters, + }) + log.Info("Starting pro workspace with configuration", string(workspaceConfig)) +} diff --git a/cmd/pro/provider/update/workspace.go b/cmd/pro/provider/update/workspace.go index 5cb54e9f4..55f2d47bf 100644 --- a/cmd/pro/provider/update/workspace.go +++ b/cmd/pro/provider/update/workspace.go @@ -17,7 +17,6 @@ import ( "github.com/loft-sh/log/terminal" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ) // WorkspaceCmd holds the cmd flags @@ -112,25 +111,10 @@ func (cmd *WorkspaceCmd) Run(ctx context.Context, stdin io.Reader, stdout io.Wri } func updateInstance(ctx context.Context, client client.Client, oldInstance *managementv1.DevPodWorkspaceInstance, newInstance *managementv1.DevPodWorkspaceInstance, log log.Logger) (*managementv1.DevPodWorkspaceInstance, error) { - managementClient, err := client.Management() - if err != nil { - return nil, err - } - - patch := ctrlclient.MergeFrom(oldInstance) - data, err := patch.Data(newInstance) - if err != nil { - return nil, err - } else if len(data) == 0 || string(data) == "{}" { - return newInstance, nil - } - - res, err := managementClient.Loft().ManagementV1(). - DevPodWorkspaceInstances(oldInstance.GetNamespace()). - Patch(ctx, oldInstance.GetName(), patch.Type(), data, metav1.PatchOptions{}) - if err != nil { - return nil, err + // This ensures the template is kept up to date with configuration changes + if newInstance.Spec.TemplateRef != nil { + newInstance.Spec.TemplateRef.SyncOnce = true } - return platform.WaitForInstance(ctx, client, res, log) + return platform.UpdateInstance(ctx, client, oldInstance, newInstance, log) } diff --git a/pkg/ide/vscode/open.go b/pkg/ide/vscode/open.go index dc44ce17f..18aae60da 100644 --- a/pkg/ide/vscode/open.go +++ b/pkg/ide/vscode/open.go @@ -109,7 +109,7 @@ func openViaCLI(ctx context.Context, workspace, folder string, newWindow bool, f // Needs to be separated by `=` because of windows folderUriArg := fmt.Sprintf("--folder-uri=vscode-remote://ssh-remote+%s.devpod/%s", workspace, folder) args = append(args, folderUriArg) - log.Debugf("Run %s command %s %s", flavor, codePath, strings.Join(args, " ")) + log.Debugf("Run %s command %s %s", flavor.DisplayName(), codePath, strings.Join(args, " ")) out, err = exec.CommandContext(ctx, codePath, args...).CombinedOutput() if err != nil { return command.WrapCommandError(out, err) diff --git a/pkg/platform/instance.go b/pkg/platform/instance.go index 516003e0e..af2678eff 100644 --- a/pkg/platform/instance.go +++ b/pkg/platform/instance.go @@ -17,8 +17,10 @@ import ( "github.com/loft-sh/devpod/pkg/platform/client" "github.com/loft-sh/devpod/pkg/platform/project" "github.com/loft-sh/log" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ) type WorkspaceInfo struct { @@ -159,6 +161,32 @@ func DialInstance(baseClient client.Client, workspace *managementv1.DevPodWorksp return conn, nil } +// UpdateInstance diffs two versions of a DevPodWorkspaceInstance, applies changes via a patch to reduce conflicts. +// Afterwards it waits until the instance is ready to be used. +func UpdateInstance(ctx context.Context, client client.Client, oldInstance *managementv1.DevPodWorkspaceInstance, newInstance *managementv1.DevPodWorkspaceInstance, log log.Logger) (*managementv1.DevPodWorkspaceInstance, error) { + managementClient, err := client.Management() + if err != nil { + return nil, err + } + + patch := ctrlclient.MergeFrom(oldInstance) + data, err := patch.Data(newInstance) + if err != nil { + return nil, err + } else if len(data) == 0 || string(data) == "{}" { + return newInstance, nil + } + + res, err := managementClient.Loft().ManagementV1(). + DevPodWorkspaceInstances(oldInstance.GetNamespace()). + Patch(ctx, oldInstance.GetName(), patch.Type(), data, metav1.PatchOptions{}) + if err != nil { + return nil, err + } + + return WaitForInstance(ctx, client, res, log) +} + func WaitForInstance(ctx context.Context, client client.Client, instance *managementv1.DevPodWorkspaceInstance, log log.Logger) (*managementv1.DevPodWorkspaceInstance, error) { managementClient, err := client.Management() if err != nil { @@ -182,8 +210,18 @@ func WaitForInstance(ctx context.Context, client client.Client, instance *manage return false, nil } + if !isTemplateSynced(updatedInstance) { + log.Debugf("Workspace template is not ready yet") + for _, cond := range updatedInstance.Status.Conditions { + if cond.Status != corev1.ConditionTrue { + log.Debugf("%s is %s (%s): %s", cond.Type, cond.Status, cond.Reason, cond.Message) + } + } + return false, nil + } + if !isRunnerReady(updatedInstance, storagev1.BuiltinRunnerName) { - log.Debugf("Runner is not ready yet, waiting until its ready", name, status.Phase) + log.Debugf("Runner is not ready yet", name, status.Phase) return false, nil } @@ -218,3 +256,19 @@ func isRunnerReady(workspace *managementv1.DevPodWorkspaceInstance, builtinRunne return workspace.GetAnnotations() != nil && workspace.GetAnnotations()[storagev1.DevPodWorkspaceRunnerEndpointAnnotation] != "" } + +func isTemplateSynced(workspace *managementv1.DevPodWorkspaceInstance) bool { + // We're still waiting for the sync to happen + // The controller will remove this field once it's done syncing + if workspace.Spec.TemplateRef != nil && workspace.Spec.TemplateRef.SyncOnce { + return false + } + + for _, condition := range workspace.Status.Conditions { + if condition.Type == storagev1.InstanceTemplateResolved { + return condition.Status == corev1.ConditionTrue + } + } + + return false +}