pr comments

coder · f0ssel · Sep 23, 2022 · Sep 19, 2022 · Sep 19, 2022 · Sep 19, 2022
commit 634fb64cdca97c324fcb1b5de4a2bc9059053fe5
diff --git a/agent/apphealth.go b/agent/apphealth.go
@@ -24,125 +24,130 @@ type WorkspaceAppHealthReporter func(ctx context.Context)
 
 // NewWorkspaceAppHealthReporter creates a WorkspaceAppHealthReporter that reports app health to coderd.
 func NewWorkspaceAppHealthReporter(logger slog.Logger, workspaceAgentApps WorkspaceAgentApps, postWorkspaceAgentAppHealth PostWorkspaceAgentAppHealth) WorkspaceAppHealthReporter {
-	return func(ctx context.Context) {
-		r := retry.New(time.Second, 30*time.Second)
-		for {
-			err := func() error {
-				apps, err := workspaceAgentApps(ctx)
-				if err != nil {
-					if xerrors.Is(err, context.Canceled) {
-						return nil
-					}
-					return xerrors.Errorf("getting workspace apps: %w", err)
-				}
-
-				// no need to run this loop if no apps for this workspace.
-				if len(apps) == 0 {
-					return nil
-				}
+	runHealthcheckLoop := func(ctx context.Context) error {
+		apps, err := workspaceAgentApps(ctx)
+		if err != nil {
+			if xerrors.Is(err, context.Canceled) {
+				return nil
+			}
+			return xerrors.Errorf("getting workspace apps: %w", err)
+		}
 
-				hasHealthchecksEnabled := false
-				health := make(map[string]codersdk.WorkspaceAppHealth, 0)
-				for _, app := range apps {
-					health[app.Name] = app.Health
-					if !hasHealthchecksEnabled && app.Health != codersdk.WorkspaceAppHealthDisabled {
-						hasHealthchecksEnabled = true
-					}
-				}
+		// no need to run this loop if no apps for this workspace.
+		if len(apps) == 0 {
+			return nil
+		}
 
-				// no need to run this loop if no health checks are configured.
-				if !hasHealthchecksEnabled {
-					return nil
-				}
+		hasHealthchecksEnabled := false
+		health := make(map[string]codersdk.WorkspaceAppHealth, 0)
+		for _, app := range apps {
+			health[app.Name] = app.Health
+			if !hasHealthchecksEnabled && app.Health != codersdk.WorkspaceAppHealthDisabled {
+				hasHealthchecksEnabled = true
+			}
+		}
 
-				// run a ticker for each app health check.
-				var mu sync.RWMutex
-				failures := make(map[string]int, 0)
-				for _, app := range apps {
-					if shouldStartTicker(app) {
-						t := time.NewTicker(time.Duration(app.Healthcheck.Interval) * time.Second)
-						go func() {
-							for {
-								select {
-								case <-ctx.Done():
-									return
-								case <-t.C:
-									// we set the http timeout to the healthcheck interval to prevent getting too backed up.
-									client := &http.Client{
-										Timeout: time.Duration(app.Healthcheck.Interval) * time.Second,
-									}
-									err := func() error {
-										req, err := http.NewRequestWithContext(ctx, http.MethodGet, app.Healthcheck.URL, nil)
-										if err != nil {
-											return err
-										}
-										res, err := client.Do(req)
-										if err != nil {
-											return err
-										}
-										// successful healthcheck is a non-5XX status code
-										res.Body.Close()
-										if res.StatusCode >= http.StatusInternalServerError {
-											return xerrors.Errorf("error status code: %d", res.StatusCode)
-										}
-
-										return nil
-									}()
-									if err != nil {
-										mu.Lock()
-										if failures[app.Name] < int(app.Healthcheck.Threshold) {
-											// increment the failure count and keep status the same.
-											// we will change it when we hit the threshold.
-											failures[app.Name]++
-										} else {
-											// set to unhealthy if we hit the failure threshold.
-											// we stop incrementing at the threshold to prevent the failure value from increasing forever.
-											health[app.Name] = codersdk.WorkspaceAppHealthUnhealthy
-										}
-										mu.Unlock()
-									} else {
-										mu.Lock()
-										// we only need one successful health check to be considered healthy.
-										health[app.Name] = codersdk.WorkspaceAppHealthHealthy
-										failures[app.Name] = 0
-										mu.Unlock()
-									}
-
-									t.Reset(time.Duration(app.Healthcheck.Interval))
-								}
-							}
-						}()
-					}
-				}
+		// no need to run this loop if no health checks are configured.
+		if !hasHealthchecksEnabled {
+			return nil
+		}
 
-				mu.Lock()
-				lastHealth := copyHealth(health)
-				mu.Unlock()
-				reportTicker := time.NewTicker(time.Second)
-				// every second we check if the health values of the apps have changed
-				// and if there is a change we will report the new values.
+		// run a ticker for each app health check.
+		var mu sync.RWMutex
+		failures := make(map[string]int, 0)
+		for _, nextApp := range apps {
+			if !shouldStartTicker(nextApp) {
+				continue
+			}
+			app := nextApp
+			t := time.NewTicker(time.Duration(app.Healthcheck.Interval) * time.Second)
+			go func() {
 				for {
 					select {
 					case <-ctx.Done():
+						return
+					case <-t.C:
+					}
+					// we set the http timeout to the healthcheck interval to prevent getting too backed up.
+					client := &http.Client{
+						Timeout: time.Duration(app.Healthcheck.Interval) * time.Second,
+					}
+					err := func() error {
+						req, err := http.NewRequestWithContext(ctx, http.MethodGet, app.Healthcheck.URL, nil)
+						if err != nil {
+							return err
+						}
+						res, err := client.Do(req)
+						if err != nil {
+							return err
+						}
+						// successful healthcheck is a non-5XX status code
+						res.Body.Close()
+						if res.StatusCode >= http.StatusInternalServerError {
+							return xerrors.Errorf("error status code: %d", res.StatusCode)
+						}
+
 						return nil
-					case <-reportTicker.C:
-						mu.RLock()
-						changed := healthChanged(lastHealth, health)
-						mu.RUnlock()
-						if changed {
-							mu.Lock()
-							lastHealth = copyHealth(health)
-							mu.Unlock()
-							err := postWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{
-								Healths: health,
-							})
-							if err != nil {
-								logger.Error(ctx, "failed to report workspace app stat", slog.Error(err))
-							}
+					}()
+					if err != nil {
+						mu.Lock()
+						if failures[app.Name] < int(app.Healthcheck.Threshold) {
+							// increment the failure count and keep status the same.
+							// we will change it when we hit the threshold.
+							failures[app.Name]++
+						} else {
+							// set to unhealthy if we hit the failure threshold.
+							// we stop incrementing at the threshold to prevent the failure value from increasing forever.
+							health[app.Name] = codersdk.WorkspaceAppHealthUnhealthy
 						}
+						mu.Unlock()
+					} else {
+						mu.Lock()
+						// we only need one successful health check to be considered healthy.
+						health[app.Name] = codersdk.WorkspaceAppHealthHealthy
+						failures[app.Name] = 0
+						mu.Unlock()
 					}
+
+					t.Reset(time.Duration(app.Healthcheck.Interval))
 				}
 			}()
+		}
+
+		mu.Lock()
+		lastHealth := copyHealth(health)
+		mu.Unlock()
+		reportTicker := time.NewTicker(time.Second)
+		// every second we check if the health values of the apps have changed
+		// and if there is a change we will report the new values.
+		for {
+			select {
+			case <-ctx.Done():
+				return nil
+			case <-reportTicker.C:
+				mu.RLock()
+				changed := healthChanged(lastHealth, health)
+				mu.RUnlock()
+				if !changed {
+					continue
+				}
+
+				mu.Lock()
+				lastHealth = copyHealth(health)
+				mu.Unlock()
+				err := postWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{
+					Healths: health,
+				})
+				if err != nil {
+					logger.Error(ctx, "failed to report workspace app stat", slog.Error(err))
+				}
+			}
+		}
+	}
+
+	return func(ctx context.Context) {
+		for r := retry.New(time.Second, 30*time.Second); r.Wait(ctx); {
+			err := runHealthcheckLoop(ctx)
 			if err != nil {
 				if xerrors.Is(err, context.Canceled) || xerrors.Is(err, context.DeadlineExceeded) {
 					return
@@ -167,7 +172,7 @@ func healthChanged(old map[string]codersdk.WorkspaceAppHealth, new map[string]co
 	for name, newValue := range new {
 		oldValue, found := old[name]
 		if !found {
-			panic("workspace app lengths are not equal")
+			return true
 		}
 		if newValue != oldValue {
 			return true

diff --git a/agent/apphealth_test.go b/agent/apphealth_test.go
@@ -146,7 +146,8 @@ func setupAppReporter(ctx context.Context, t *testing.T, apps []codersdk.Workspa
 
 	var mu sync.Mutex
 	workspaceAgentApps := func(context.Context) ([]codersdk.WorkspaceApp, error) {
-		return apps, nil
+		var newApps []codersdk.WorkspaceApp
+		return append(newApps, apps...), nil
 	}
 	postWorkspaceAgentAppHealth := func(_ context.Context, req codersdk.PostWorkspaceAppHealthsRequest) error {
 		for name, health := range req.Healths {

diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go
@@ -437,7 +437,7 @@ func TestWorkspaceAgentAppHealth(t *testing.T) {
 		},
 	})
 	require.Error(t, err)
-	// app.HealthEnabled == false
+	// healcheck disabled
 	err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{
 		Healths: map[string]codersdk.WorkspaceAppHealth{
 			"code-server": codersdk.WorkspaceAppHealthInitializing,