[test] Fix flakes and speed up prebuild tests (#17996)

Author: WVerlaek

test/pkg/integration/workspace.go

@@ -546,14 +546,19 @@ func stopWsF(t *testing.T, instanceID string, workspaceID string, api *Component
 type WaitForWorkspaceOpt func(*waitForWorkspaceOpts)
 
 type waitForWorkspaceOpts struct {
-	CanFail bool
+	CanFail        bool
+	WaitForStopped bool
 }
 
 // WorkspaceCanFail doesn't fail the test if the workspace fails to start
 func WorkspaceCanFail(o *waitForWorkspaceOpts) {
 	o.CanFail = true
 }
 
+func WaitForStopped(o *waitForWorkspaceOpts) {
+	o.WaitForStopped = true
+}
+
 // WaitForWorkspace waits until a workspace is running. Fails the test if the workspace
 // fails or does not become RUNNING before the context is canceled.
 func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string, workspaceID string, api *ComponentAPI, opts ...WaitForWorkspaceOpt) (lastStatus *wsmanapi.WorkspaceStatus, err error) {
@@ -562,6 +567,34 @@ func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string,
 		o(&cfg)
 	}
 
+	checkStatus := func(status *wsmanapi.WorkspaceStatus) (done bool, err error) {
+		if status == nil {
+			return false, nil
+		}
+		if !cfg.CanFail && status.Conditions != nil && status.Conditions.Failed != "" {
+			return true, xerrors.Errorf("workspace instance %s failed: %s", instanceID, status.Conditions.Failed)
+		}
+
+		switch status.Phase {
+		case wsmanapi.WorkspacePhase_RUNNING:
+			if !cfg.WaitForStopped {
+				// Done.
+				return true, nil
+			}
+		case wsmanapi.WorkspacePhase_STOPPING:
+			if !cfg.WaitForStopped {
+				return true, ErrWorkspaceInstanceStopping
+			}
+		case wsmanapi.WorkspacePhase_STOPPED:
+			if !cfg.WaitForStopped {
+				return true, ErrWorkspaceInstanceStopped
+			} else {
+				return true, nil
+			}
+		}
+		return false, nil
+	}
+
 	done := make(chan *wsmanapi.WorkspaceStatus)
 	errStatus := make(chan error)
 	reboot := make(chan struct{}, 1)
@@ -595,7 +628,11 @@ func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string,
 			close(done)
 		}()
 		for {
-			t.Logf("check if the status of workspace is in the running phase: %s", instanceID)
+			waitForPhase := "running"
+			if cfg.WaitForStopped {
+				waitForPhase = "stopped"
+			}
+			t.Logf("check if the status of workspace is in the %s phase: %s", waitForPhase, instanceID)
 			resp, err := sub.Recv()
 			if err != nil {
 				if st, ok := status.FromError(err); ok && st.Code() == codes.Unavailable {
@@ -630,30 +667,15 @@ func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string,
 
 			t.Logf("status: %s, %s", s.Id, s.Phase)
 
-			if cfg.CanFail {
-				if s.Phase == wsmanapi.WorkspacePhase_STOPPING {
-					return
-				}
-				if s.Phase == wsmanapi.WorkspacePhase_STOPPED {
-					return
-				}
-			} else {
-				if s.Conditions.Failed != "" {
-					errStatus <- xerrors.Errorf("workspace instance %s failed: %s", instanceID, s.Conditions.Failed)
-					return
-				} else if s.Phase == wsmanapi.WorkspacePhase_STOPPING || s.Phase == wsmanapi.WorkspacePhase_STOPPED {
-					errStatus <- xerrors.Errorf("workspace instance %s is %s", instanceID, s.Phase)
-					return
-				}
-			}
-			if s.Phase != wsmanapi.WorkspacePhase_RUNNING {
-				// we're still starting
-				continue
+			done2, err := checkStatus(s)
+			if err != nil {
+				errStatus <- err
+				return
 			}
 
-			// all is well, the workspace is running
-			t.Logf("confirmed that the worksapce is running: %s, %s", s.Id, s.Phase)
-			return
+			if done2 {
+				return
+			}
 		}
 	}()
 
@@ -675,30 +697,18 @@ func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string,
 				return nil, false, nil
 			}
 		}
-		if desc != nil && desc.Status != nil {
-			switch desc.Status.Phase {
-			case wsmanapi.WorkspacePhase_RUNNING:
-				return desc.Status, false, nil
-			case wsmanapi.WorkspacePhase_STOPPING:
-				if !cfg.CanFail {
-					return nil, false, ErrWorkspaceInstanceStopping
-				}
-			case wsmanapi.WorkspacePhase_STOPPED:
-				if !cfg.CanFail {
-					return nil, false, ErrWorkspaceInstanceStopped
-				}
-			}
-		}
-		return nil, true, nil
+
+		done, err := checkStatus(desc.Status)
+		return desc.Status, done, err
 	}
 
 	ticker := time.NewTicker(1 * time.Second)
 	for {
 		select {
 		case <-ticker.C:
 			// For in case missed the status change
-			desc, cont, err := handle()
-			if cont {
+			desc, done, err := handle()
+			if !done {
 				continue
 			} else if err != nil {
 				return nil, err
@@ -707,8 +717,8 @@ func WaitForWorkspaceStart(t *testing.T, ctx context.Context, instanceID string,
 			}
 		case <-reboot:
 			// Consider workspace state changes during subscriber reboot
-			desc, cont, err := handle()
-			if cont {
+			desc, done, err := handle()
+			if !done {
 				continue
 			} else if err != nil {
 				return nil, err
@@ -1008,17 +1018,12 @@ func DeleteWorkspace(ctx context.Context, api *ComponentAPI, instanceID string)
 		Id: instanceID,
 	})
 	if err != nil {
+		s, ok := status.FromError(err)
+		if ok && s.Code() == codes.NotFound {
+			// Workspace is already gone.
+			return nil
+		}
 		return err
 	}
-
-	if err == nil {
-		return nil
-	}
-
-	s, ok := status.FromError(err)
-	if ok && s.Code() == codes.NotFound {
-		return nil
-	}
-
-	return err
+	return nil
 }

test/tests/components/ws-manager/content_test.go

@@ -242,7 +242,7 @@ func TestMissingBackup(t *testing.T) {
 						}
 						w.Spec.FeatureFlags = test.FF
 						return nil
-					}), integration.WithWaitWorkspaceForOpts(integration.WorkspaceCanFail))
+					}), integration.WithWaitWorkspaceForOpts(integration.WorkspaceCanFail, integration.WaitForStopped))
 					if err != nil {
 						t.Fatal(err)
 					}

test/tests/components/ws-manager/prebuild_test.go

@@ -42,7 +42,7 @@ func TestPrebuildWorkspaceTaskSuccess(t *testing.T) {
 					CheckoutLocation: "empty",
 					WorkspaceRoot:    "/workspace/empty",
 					Task: []gitpod.TasksItems{
-						{Init: "echo \"some output\" > someFile; sleep 10; exit 0;"},
+						{Init: "echo \"some output\" > someFile; exit 0;"},
 					},
 				},
 			}
@@ -85,7 +85,7 @@ func TestPrebuildWorkspaceTaskSuccess(t *testing.T) {
 						}
 						req.Spec.WorkspaceLocation = test.CheckoutLocation
 						return nil
-					}))
+					}), integration.WithWaitWorkspaceForOpts(integration.WaitForStopped))
 					if err != nil {
 						t.Fatalf("cannot launch a workspace: %q", err)
 					}
@@ -95,18 +95,16 @@ func TestPrebuildWorkspaceTaskSuccess(t *testing.T) {
 							t.Errorf("cannot stop workspace: %q", err)
 						}
 					})
-					_, err = integration.WaitForWorkspace(ctx, api, ws.Req.Id, func(status *wsmanapi.WorkspaceStatus) bool {
-						if status.Phase != wsmanapi.WorkspacePhase_STOPPED {
-							return false
-						}
-						if status.Conditions.HeadlessTaskFailed != "" {
-							t.Logf("Conditions: %v", status.Conditions)
-							t.Fatal("unexpected HeadlessTaskFailed condition")
-						}
-						return true
-					})
-					if err != nil {
-						t.Fatalf("failed for wait workspace stop: %q", err)
+
+					if ws.LastStatus == nil {
+						t.Fatal("workspace status is nil")
+					}
+					if ws.LastStatus.Phase != wsmanapi.WorkspacePhase_STOPPED {
+						t.Fatalf("unexpected workspace phase: %v", ws.LastStatus.Phase)
+					}
+					if ws.LastStatus.Conditions != nil && ws.LastStatus.Conditions.HeadlessTaskFailed != "" {
+						t.Logf("Conditions: %v", ws.LastStatus.Conditions)
+						t.Fatalf("unexpected HeadlessTaskFailed condition: %v", ws.LastStatus.Conditions.HeadlessTaskFailed)
 					}
 				})
 			}
@@ -135,10 +133,10 @@ func TestPrebuildWorkspaceTaskFail(t *testing.T) {
 				req.Type = wsmanapi.WorkspaceType_PREBUILD
 				req.Spec.Envvars = append(req.Spec.Envvars, &wsmanapi.EnvironmentVariable{
 					Name:  "GITPOD_TASKS",
-					Value: `[{ "init": "echo \"some output\" > someFile; sleep 10; exit 1;" }]`,
+					Value: `[{ "init": "echo \"some output\" > someFile; exit 1;" }]`,
 				})
 				return nil
-			}))
+			}), integration.WithWaitWorkspaceForOpts(integration.WaitForStopped))
 			if err != nil {
 				t.Fatalf("cannot start workspace: %q", err)
 			}
@@ -150,18 +148,15 @@ func TestPrebuildWorkspaceTaskFail(t *testing.T) {
 				}
 			})
 
-			_, err = integration.WaitForWorkspace(ctx, api, ws.Req.Id, func(status *wsmanapi.WorkspaceStatus) bool {
-				if status.Phase != wsmanapi.WorkspacePhase_STOPPED {
-					return false
-				}
-				if status.Conditions.HeadlessTaskFailed == "" {
-					t.Logf("Conditions: %v", status.Conditions)
-					t.Fatal("expected HeadlessTaskFailed condition")
-				}
-				return true
-			})
-			if err != nil {
-				t.Fatalf("failed for wait workspace stop: %q", err)
+			if ws.LastStatus == nil {
+				t.Fatal("workspace status is nil")
+			}
+			if ws.LastStatus.Phase != wsmanapi.WorkspacePhase_STOPPED {
+				t.Fatalf("unexpected workspace phase: %v", ws.LastStatus.Phase)
+			}
+			if ws.LastStatus.Conditions == nil || ws.LastStatus.Conditions.HeadlessTaskFailed == "" {
+				t.Logf("Status: %v", ws.LastStatus)
+				t.Fatal("expected HeadlessTaskFailed condition")
 			}
 
 			return ctx
@@ -174,7 +169,7 @@ func TestPrebuildWorkspaceTaskFail(t *testing.T) {
 const (
 	prebuildLogPath string = "/workspace/.gitpod"
 	prebuildLog     string = "'🍊 This task ran as a workspace prebuild'"
-	initTask        string = "echo \"some output\" > someFile; sleep 10;"
+	initTask        string = "echo \"some output\" > someFile;"
 	regularPrefix   string = "ws-"
 )
 
@@ -227,25 +222,30 @@ func TestOpenWorkspaceFromPrebuildSerialOnly(t *testing.T) {
 						// create a prebuild and stop workspace
 						// TODO: change to use server API to launch the workspace, so we could run the integration test as the user code flow
 						//       which is client -> server -> ws-manager rather than client -> ws-manager directly
-						ws, prebuildStopWs, err := integration.LaunchWorkspaceDirectly(t, ctx, api, integration.WithRequestModifier(func(req *wsmanapi.StartWorkspaceRequest) error {
-							req.Type = wsmanapi.WorkspaceType_PREBUILD
-							req.Spec.Envvars = append(req.Spec.Envvars, &wsmanapi.EnvironmentVariable{
-								Name:  "GITPOD_TASKS",
-								Value: fmt.Sprintf(`[{ "init": %q }]`, initTask),
-							})
-							req.Spec.FeatureFlags = test.FF
-							req.Spec.Initializer = &csapi.WorkspaceInitializer{
-								Spec: &csapi.WorkspaceInitializer_Git{
-									Git: &csapi.GitInitializer{
-										RemoteUri:        test.ContextURL,
-										CheckoutLocation: test.CheckoutLocation,
-										Config:           &csapi.GitConfig{},
+						ws, prebuildStopWs, err := integration.LaunchWorkspaceDirectly(t, ctx, api,
+							integration.WithRequestModifier(func(req *wsmanapi.StartWorkspaceRequest) error {
+								req.Type = wsmanapi.WorkspaceType_PREBUILD
+								req.Spec.Envvars = append(req.Spec.Envvars, &wsmanapi.EnvironmentVariable{
+									Name:  "GITPOD_TASKS",
+									Value: fmt.Sprintf(`[{ "init": %q }]`, initTask),
+								})
+								req.Spec.FeatureFlags = test.FF
+								req.Spec.Initializer = &csapi.WorkspaceInitializer{
+									Spec: &csapi.WorkspaceInitializer_Git{
+										Git: &csapi.GitInitializer{
+											RemoteUri:        test.ContextURL,
+											CheckoutLocation: test.CheckoutLocation,
+											Config:           &csapi.GitConfig{},
+										},
 									},
-								},
-							}
-							req.Spec.WorkspaceLocation = test.CheckoutLocation
-							return nil
-						}))
+								}
+								req.Spec.WorkspaceLocation = test.CheckoutLocation
+								return nil
+							}),
+							// Wait for the prebuild to finish, as this can happen quickly before we
+							// would have time to observe the workspace to stop and get its status.
+							integration.WithWaitWorkspaceForOpts(integration.WaitForStopped),
+						)
 						if err != nil {
 							t.Fatalf("cannot launch a workspace: %q", err)
 						}
@@ -255,7 +255,7 @@ func TestOpenWorkspaceFromPrebuildSerialOnly(t *testing.T) {
 							}
 						}()
 
-						prebuildSnapshot, _, err = watchStopWorkspaceAndFindSnapshot(t, ctx, ws.Req.Id, ws.WorkspaceID, api)
+						prebuildSnapshot, _, err = findSnapshotFromStoppedWs(t, ctx, ws.LastStatus)
 						if err != nil {
 							_ = stopWorkspace(t, cfg, prebuildStopWs)
 							t.Fatalf("stop workspace and find snapshot error: %v", err)
@@ -472,9 +472,9 @@ func TestOpenWorkspaceFromOutdatedPrebuild(t *testing.T) {
 							req.Spec.WorkspaceLocation = test.CheckoutLocation
 							return nil
 						}),
-						// The init task only runs for a short duration, so it's possible we miss the Running state, as it quickly transitions to Stopping.
-						// Therefore ignore if we can't see the Running state.
-						integration.WithWaitWorkspaceForOpts(integration.WorkspaceCanFail))
+						// The init task only runs for a short duration, so it's possible we miss the Running state, as it quickly transitions to Stopping/Stopped.
+						// Therefore wait for the workspace to stop to get its last status.
+						integration.WithWaitWorkspaceForOpts(integration.WaitForStopped))
 					if err != nil {
 						t.Fatalf("cannot launch a workspace: %q", err)
 					}
@@ -484,7 +484,7 @@ func TestOpenWorkspaceFromOutdatedPrebuild(t *testing.T) {
 							t.Errorf("cannot stop workspace: %q", err)
 						}
 					}()
-					prebuildSnapshot, _, err := watchStopWorkspaceAndFindSnapshot(t, ctx, ws.Req.Id, ws.WorkspaceID, api)
+					prebuildSnapshot, _, err := findSnapshotFromStoppedWs(t, ctx, ws.LastStatus)
 					if err != nil {
 						t.Fatalf("stop workspace and find snapshot error: %v", err)
 					}
@@ -690,18 +690,19 @@ func checkGitFolderPermission(t *testing.T, rsa *integration.RpcClient, workspac
 	}
 }
 
-func watchStopWorkspaceAndFindSnapshot(t *testing.T, ctx context.Context, instanceId string, workspaceID string, api *integration.ComponentAPI) (string, *wsmanapi.VolumeSnapshotInfo, error) {
-	ready := make(chan struct{}, 1)
-	lastStatus, err := integration.WaitForWorkspaceStop(t, ctx, ready, api, instanceId, workspaceID)
-	if err != nil {
-		return "", nil, err
+func findSnapshotFromStoppedWs(t *testing.T, ctx context.Context, lastStatus *wsmanapi.WorkspaceStatus) (string, *wsmanapi.VolumeSnapshotInfo, error) {
+	if lastStatus == nil {
+		return "", nil, fmt.Errorf("did not get last workspace status")
 	}
-	if lastStatus.Conditions != nil && lastStatus.Conditions.HeadlessTaskFailed != "" {
-		return "", nil, errors.New("unexpected HeadlessTaskFailed condition")
+	if lastStatus.Phase != wsmanapi.WorkspacePhase_STOPPED {
+		return "", nil, fmt.Errorf("workspace is not stopped: %s", lastStatus.Phase)
 	}
-	if lastStatus == nil || lastStatus.Conditions == nil {
+	if lastStatus.Conditions == nil {
 		return "", nil, nil
 	}
+	if lastStatus.Conditions.HeadlessTaskFailed != "" {
+		return "", nil, errors.New("unexpected HeadlessTaskFailed condition")
+	}
 	return lastStatus.Conditions.Snapshot, lastStatus.Conditions.VolumeSnapshot, nil
 }