diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index ab583ccf8..4da3b781f 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -192,6 +192,12 @@ jobs: install-kind: false requires-secret: false + - label: down + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + # Up tests - label: up-workspaces diff --git a/cmd/down.go b/cmd/down.go new file mode 100644 index 000000000..9db680099 --- /dev/null +++ b/cmd/down.go @@ -0,0 +1,129 @@ +package cmd + +import ( + "context" + "fmt" + + "github.com/devsy-org/devsy/cmd/completion" + "github.com/devsy-org/devsy/cmd/flags" + client2 "github.com/devsy-org/devsy/pkg/client" + "github.com/devsy-org/devsy/pkg/client/clientimplementation" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/workspace" + "github.com/spf13/cobra" +) + +// DownCmd holds the down cmd flags. +type DownCmd struct { + *flags.GlobalFlags + client2.DeleteOptions +} + +// NewDownCmd creates a new down command that stops and deletes a workspace. +func NewDownCmd(flags *flags.GlobalFlags) *cobra.Command { + cmd := &DownCmd{ + GlobalFlags: flags, + } + downCmd := &cobra.Command{ + Use: "down [flags] [workspace-path|workspace-name]", + Short: "Stops and deletes an existing workspace", + RunE: func(cobraCmd *cobra.Command, args []string) error { + return cmd.Run(cobraCmd.Context(), args) + }, + ValidArgsFunction: func( + rootCmd *cobra.Command, args []string, toComplete string, + ) ([]string, cobra.ShellCompDirective) { + return completion.GetWorkspaceSuggestions( + rootCmd, + cmd.Context, + cmd.Provider, + args, + toComplete, + cmd.Owner, + ) + }, + } + + downCmd.Flags(). + BoolVar(&cmd.Force, "force", false, "Delete workspace even if it is not found remotely anymore") + downCmd.Flags(). + StringVar(&cmd.GracePeriod, "grace-period", "", "The amount of time to give the command to delete the workspace") + return downCmd +} + +// Run stops and then deletes the workspace. +func (cmd *DownCmd) Run(ctx context.Context, args []string) error { + devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider) + if err != nil { + return err + } + + if _, err := clientimplementation.DecodeOptionsFromEnv( + config.EnvFlagsDelete, + &cmd.DeleteOptions, + ); err != nil { + return fmt.Errorf("decode delete options: %w", err) + } + + if err := clientimplementation.DecodePlatformOptionsFromEnv(&cmd.Platform); err != nil { + return fmt.Errorf("decode platform options: %w", err) + } + + client, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: args, + Owner: cmd.Owner, + }) + if err != nil { + return err + } + + if err := cmd.stop(ctx, client); err != nil { + return err + } + + _, err = workspace.Delete(ctx, workspace.DeleteOptions{ + DevsyConfig: devsyConfig, + Args: args, + ClientDelete: cmd.DeleteOptions, + Force: cmd.Force, + Owner: cmd.Owner, + }) + if err != nil { + return err + } + + log.Infof("successfully stopped and deleted workspace") + return nil +} + +func (cmd *DownCmd) stop( + ctx context.Context, + client client2.BaseWorkspaceClient, +) error { + if !cmd.Platform.Enabled { + if err := client.Lock(ctx); err != nil { + return err + } + defer client.Unlock() + } + + status, err := client.Status(ctx, client2.StatusOptions{}) + if err != nil { + return err + } + + if status != client2.StatusRunning { + log.Infof("workspace is %s, skipping stop", status) + return nil + } + + // Single-machine stop is not needed here: workspace.Delete handles + // single-machine cleanup via deleteSingleMachine. + if err := client.Stop(ctx, client2.StopOptions{}); err != nil { + return fmt.Errorf("stop workspace: %w", err) + } + + return nil +} diff --git a/cmd/root.go b/cmd/root.go index a848d1bf6..f2fe2336d 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -117,6 +117,7 @@ func BuildRoot() *cobra.Command { rootCmd.AddCommand(NewSSHCmd(globalFlags)) rootCmd.AddCommand(NewVersionCmd()) rootCmd.AddCommand(NewStopCmd(globalFlags)) + rootCmd.AddCommand(NewDownCmd(globalFlags)) rootCmd.AddCommand(NewListCmd(globalFlags)) rootCmd.AddCommand(NewStatusCmd(globalFlags)) rootCmd.AddCommand(NewBuildCmd(globalFlags)) diff --git a/cmd/stop.go b/cmd/stop.go index 5337bde19..5fd2a0138 100644 --- a/cmd/stop.go +++ b/cmd/stop.go @@ -26,9 +26,8 @@ func NewStopCmd(flags *flags.GlobalFlags) *cobra.Command { GlobalFlags: flags, } stopCmd := &cobra.Command{ - Use: "stop [flags] [workspace-path|workspace-name]", - Aliases: []string{"down"}, - Short: "Stops an existing workspace", + Use: "stop [flags] [workspace-path|workspace-name]", + Short: "Stops an existing workspace", RunE: func(cobraCmd *cobra.Command, args []string) error { ctx := cobraCmd.Context() devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider) diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 8aa6a5ee5..eeb43b999 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -11,12 +11,12 @@ import ( _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" + _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/ide" _ "github.com/devsy-org/devsy/e2e/tests/integration" _ "github.com/devsy-org/devsy/e2e/tests/machine" _ "github.com/devsy-org/devsy/e2e/tests/machineprovider" _ "github.com/devsy-org/devsy/e2e/tests/provider" - _ "github.com/devsy-org/devsy/e2e/tests/readconfiguration" _ "github.com/devsy-org/devsy/e2e/tests/ssh" _ "github.com/devsy-org/devsy/e2e/tests/tunnel" _ "github.com/devsy-org/devsy/e2e/tests/up" diff --git a/e2e/framework/command.go b/e2e/framework/command.go index 5f2a53a43..9a15aa860 100644 --- a/e2e/framework/command.go +++ b/e2e/framework/command.go @@ -235,6 +235,15 @@ func (f *Framework) DevsyStop(ctx context.Context, workspace string) error { return nil } +func (f *Framework) DevsyDown(ctx context.Context, workspace string) error { + baseArgs := []string{"down", workspace} + err := f.ExecCommand(ctx, false, false, "", baseArgs) + if err != nil { + return fmt.Errorf("devsy down failed: %s", err.Error()) + } + return nil +} + func (f *Framework) DevsyProviderAdd(ctx context.Context, args ...string) error { baseArgs := []string{"provider", "add"} baseArgs = append(baseArgs, args...) diff --git a/e2e/tests/down/down.go b/e2e/tests/down/down.go new file mode 100644 index 000000000..250432cdf --- /dev/null +++ b/e2e/tests/down/down.go @@ -0,0 +1,105 @@ +package down + +import ( + "context" + "fmt" + "os" + "strings" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + docker "github.com/devsy-org/devsy/pkg/docker" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("testing down command", ginkgo.Label("down"), func() { + var dockerHelper *docker.DockerHelper + var initialDir string + + ginkgo.BeforeEach(func() { + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + + dockerHelper = &docker.DockerHelper{DockerCommand: "docker"} + }) + + ginkgo.It("down stops and deletes workspace", func(ctx context.Context) { + f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") + framework.ExpectNoError(err) + + name := "vscode-remote-try-python" + + err = f.DevsyUp(ctx, "https://github.com/microsoft/vscode-remote-try-python.git") + framework.ExpectNoError(err) + + // Verify workspace is running + status, err := f.DevsyStatus(ctx, name) + framework.ExpectNoError(err) + gomega.Expect(strings.ToUpper(status.State)).To(gomega.Equal("RUNNING")) + + // Get workspace UID for container lookup + workspace, err := f.FindWorkspace(ctx, name) + framework.ExpectNoError(err) + + containerIDs, err := dockerHelper.FindContainer(ctx, []string{ + fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + }) + framework.ExpectNoError(err) + gomega.Expect(containerIDs).NotTo(gomega.BeEmpty(), "container should exist before down") + + // Run devsy down + err = f.DevsyDown(ctx, name) + framework.ExpectNoError(err) + + // Verify container is gone + containerIDs, err = dockerHelper.FindContainer(ctx, []string{ + fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + }) + framework.ExpectNoError(err) + gomega.Expect(containerIDs).To(gomega.BeEmpty(), "container should be deleted after down") + + // Verify workspace no longer appears in list + _, err = f.FindWorkspace(ctx, name) + gomega.Expect(err).To(gomega.HaveOccurred(), "workspace should not be in list after down") + }, ginkgo.SpecTimeout(framework.TimeoutModerate())) + + ginkgo.It("stop only stops and does not delete workspace", func(ctx context.Context) { + f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") + framework.ExpectNoError(err) + + name := "vscode-remote-try-python" + ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, name) + + err = f.DevsyUp(ctx, "https://github.com/microsoft/vscode-remote-try-python.git") + framework.ExpectNoError(err) + + // Verify workspace is running + status, err := f.DevsyStatus(ctx, name) + framework.ExpectNoError(err) + gomega.Expect(strings.ToUpper(status.State)).To(gomega.Equal("RUNNING")) + + // Get workspace UID for container lookup + workspace, err := f.FindWorkspace(ctx, name) + framework.ExpectNoError(err) + + // Run devsy stop + err = f.DevsyStop(ctx, name) + framework.ExpectNoError(err) + + // Verify workspace still exists in list (just stopped, not deleted) + _, err = f.FindWorkspace(ctx, name) + framework.ExpectNoError(err) + + // Verify container still exists (stopped but not removed) + containerIDs, err := dockerHelper.FindContainer(ctx, []string{ + fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + }) + framework.ExpectNoError(err) + gomega.Expect(containerIDs).NotTo( + gomega.BeEmpty(), + "container should still exist after stop (only stopped, not deleted)", + ) + }, ginkgo.SpecTimeout(framework.TimeoutModerate())) +}) diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index 1b02226cc..606c1a724 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/pkg/devcontainer/feature/extend.go @@ -441,46 +441,93 @@ func getSortedFeatureSets( devContainer *config.DevContainerConfig, featureSets []*config.FeatureSet, ) ([]*config.FeatureSet, error) { - orderedFeatureSets, err := getOrderedFeatureSets(featureSets) - if err != nil { - return nil, err + if len(devContainer.OverrideFeatureInstallOrder) == 0 { + return getOrderedFeatureSets(featureSets) } - if len(devContainer.OverrideFeatureInstallOrder) == 0 { - return orderedFeatureSets, nil + featureLookup := buildFeatureLookupMap(featureSets) + priority := buildOverridePriority(devContainer.OverrideFeatureInstallOrder, featureLookup) + + if err := validateOverrideOrder(priority, featureSets, featureLookup); err != nil { + return nil, err } - return sortFeaturesByOverride(devContainer.OverrideFeatureInstallOrder, orderedFeatureSets), nil + return getOrderedFeatureSetsWithPriority(featureSets, priority) } -func sortFeaturesByOverride( +func buildOverridePriority( overrideOrder []string, - featureSets []*config.FeatureSet, -) []*config.FeatureSet { - orderedFeatures := make([]*config.FeatureSet, 0, len(featureSets)) - seen := make(map[string]bool) - - for _, overrideFeatureID := range overrideOrder { - feature := extractFeatureByID(featureSets, overrideFeatureID) - if feature == nil { - normalizedID := normalizeFeatureID(overrideFeatureID) - feature = extractFeatureByID(featureSets, normalizedID) + featureLookup map[string]*config.FeatureSet, +) map[string]int { + priority := make(map[string]int) + for i, id := range overrideOrder { + if _, exists := featureLookup[id]; exists { + priority[id] = i + continue } - - if feature != nil && !seen[feature.ConfigID] { - orderedFeatures = append(orderedFeatures, feature) - seen[feature.ConfigID] = true + normalizedID := normalizeFeatureID(id) + if _, exists := featureLookup[normalizedID]; exists { + priority[normalizedID] = i } } + return priority +} +func validateOverrideOrder( + priority map[string]int, + featureSets []*config.FeatureSet, + featureLookup map[string]*config.FeatureSet, +) error { for _, feature := range featureSets { - if !seen[feature.ConfigID] { - orderedFeatures = append(orderedFeatures, feature) - seen[feature.ConfigID] = true + featurePriority, featureHasPriority := priority[feature.ConfigID] + if !featureHasPriority { + continue + } + + for depID := range feature.Config.DependsOn { + depConfigID := resolveDepConfigID(depID, featureLookup) + if depConfigID == "" { + continue + } + depPriority, depHasPriority := priority[depConfigID] + if !depHasPriority { + continue + } + if featurePriority < depPriority { + return fmt.Errorf( + "overrideFeatureInstallOrder places %q (position %d)"+ + " before its dependency %q (position %d)", + feature.ConfigID, + featurePriority, + depConfigID, + depPriority, + ) + } } } + return nil +} - return orderedFeatures +func resolveDepConfigID(depID string, featureLookup map[string]*config.FeatureSet) string { + if _, exists := featureLookup[depID]; exists { + return depID + } + normalizedID := normalizeFeatureID(depID) + if _, exists := featureLookup[normalizedID]; exists { + return normalizedID + } + return "" +} + +func getOrderedFeatureSetsWithPriority( + features []*config.FeatureSet, + priority map[string]int, +) ([]*config.FeatureSet, error) { + dependencyGraph, err := buildFeatureDependencyGraph(features) + if err != nil { + return nil, err + } + return dependencyGraph.SortWithPriority(priority) } func extractFeatureByID(features []*config.FeatureSet, featureID string) *config.FeatureSet { diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index 4071aae0b..814e548d7 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -330,7 +330,7 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() { } } -func (suite *ExtendTestSuite) TestComputeFeatureOrder_WithOverride() { +func (suite *ExtendTestSuite) TestComputeFeatureOrder_OverrideViolatesDependsOn() { devContainer := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ OverrideFeatureInstallOrder: []string{"feature-a", "feature-b"}, @@ -347,17 +347,34 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_WithOverride() { {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } + _, err := getSortedFeatureSets(devContainer, features) + suite.Error(err) + suite.Contains(err.Error(), "overrideFeatureInstallOrder") + suite.Contains(err.Error(), "dependency") +} + +func (suite *ExtendTestSuite) TestComputeFeatureOrder_ValidOverride() { + devContainer := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + OverrideFeatureInstallOrder: []string{"feature-b", "feature-a"}, + }, + } + + features := []*config.FeatureSet{ + { + ConfigID: "feature-a", + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{"feature-b": map[string]any{}}, + }, + }, + {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + } + order, err := getSortedFeatureSets(devContainer, features) suite.Require().NoError(err) suite.Len(order, 2) - if order[0].ConfigID != "feature-a" || order[1].ConfigID != "feature-b" { - suite.Failf( - "Order mismatch", - "Expected [feature-a, feature-b], got [%s, %s]", - order[0].ConfigID, - order[1].ConfigID, - ) - } + suite.Equal("feature-b", order[0].ConfigID) + suite.Equal("feature-a", order[1].ConfigID) } func (suite *ExtendTestSuite) TestComputeFeatureOrder_PartialOverride() { @@ -387,28 +404,42 @@ func (suite *ExtendTestSuite) TestComputeFeatureOrder_PartialOverride() { } } -func (suite *ExtendTestSuite) TestApplyManualOrdering() { - automaticOrder := []*config.FeatureSet{ - {ConfigID: "feature-a"}, - {ConfigID: "feature-b"}, - {ConfigID: "feature-c"}, +func (suite *ExtendTestSuite) TestBuildOverridePriority() { + features := []*config.FeatureSet{ + {ConfigID: "feature-a", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: "feature-c", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, } + lookup := buildFeatureLookupMap(features) overrideOrder := []string{"feature-c", "feature-a"} - result := sortFeaturesByOverride(overrideOrder, automaticOrder) - expected := []string{"feature-c", "feature-a", "feature-b"} - suite.Len(result, 3) - for i, expectedID := range expected { - if result[i].ConfigID != expectedID { - suite.Failf( - "Position mismatch", - "Position %d: expected %s, got %s", - i, - expectedID, - result[i].ConfigID, - ) - } + priority := buildOverridePriority(overrideOrder, lookup) + + suite.Equal(0, priority["feature-c"]) + suite.Equal(1, priority["feature-a"]) + _, hasB := priority["feature-b"] + suite.False(hasB) +} + +func (suite *ExtendTestSuite) TestOverridePriorityAffectsSortOrder() { + devContainer := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + OverrideFeatureInstallOrder: []string{"feature-c", "feature-a"}, + }, } + + features := []*config.FeatureSet{ + {ConfigID: "feature-a", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: "feature-b", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + {ConfigID: "feature-c", Config: &config.FeatureConfig{DependsOn: config.DependsOnField{}}}, + } + + order, err := getSortedFeatureSets(devContainer, features) + suite.Require().NoError(err) + suite.Len(order, 3) + suite.Equal("feature-c", order[0].ConfigID) + suite.Equal("feature-a", order[1].ConfigID) + suite.Equal("feature-b", order[2].ConfigID) } func (suite *ExtendTestSuite) TestExtractFeatureByID() { diff --git a/pkg/devcontainer/graph/graph.go b/pkg/devcontainer/graph/graph.go index 208f2019f..a48ccd5c1 100644 --- a/pkg/devcontainer/graph/graph.go +++ b/pkg/devcontainer/graph/graph.go @@ -248,6 +248,22 @@ func (g *Graph[T]) Sort() ([]T, error) { return result, nil } +// SortWithPriority performs a topological sort where, within each round of +// nodes that have zero in-degree, nodes are ordered by the supplied priority +// map (lower value = installed first). Nodes not in the priority map sort +// alphabetically after prioritized nodes. +func (g *Graph[T]) SortWithPriority(priority map[string]int) ([]T, error) { + sortedIDs, err := g.sortNodeIDsWithPriority(priority) + if err != nil { + return nil, err + } + result := make([]T, len(sortedIDs)) + for i, id := range sortedIDs { + result[i] = g.nodes[id] + } + return result, nil +} + func (g *Graph[T]) SortNodeIDs() ([]string, error) { return g.sortNodeIDs() } @@ -350,3 +366,68 @@ func processNeighbors( } } } + +func (g *Graph[T]) sortNodeIDsWithPriority(priority map[string]int) ([]string, error) { + workingInDegree := copyInDegreeMap(g.inDegree) + ready := initializeQueue(workingInDegree) + sortedResult := make([]string, 0, len(g.nodes)) + + for len(ready) > 0 { + sortByPriority(ready, priority) + + current := ready[0] + ready = ready[1:] + sortedResult = append(sortedResult, current) + + processNeighborsUnordered(g.edges, current, workingInDegree, &ready) + } + + if len(sortedResult) != len(g.nodes) { + return nil, circularDependencyError(workingInDegree) + } + + return sortedResult, nil +} + +func sortByPriority(ids []string, priority map[string]int) { + sort.Slice(ids, func(i, j int) bool { + return comparePriority(ids[i], ids[j], priority) + }) +} + +func comparePriority(a, b string, priority map[string]int) bool { + pa, oka := priority[a] + pb, okb := priority[b] + if oka != okb { + return oka + } + if oka && pa != pb { + return pa < pb + } + return a < b +} + +func processNeighborsUnordered( + edges map[string][]string, + currentNode string, + inDegree map[string]int, + queue *[]string, +) { + for _, neighbor := range edges[currentNode] { + inDegree[neighbor]-- + if inDegree[neighbor] == 0 { + *queue = append(*queue, neighbor) + } + } +} + +func circularDependencyError(inDegree map[string]int) error { + remaining := []string{} + for id, deg := range inDegree { + if deg > 0 { + remaining = append(remaining, id) + } + } + sort.Strings(remaining) + return fmt.Errorf("circular dependency detected among nodes: %v", remaining) +}