From d0b25cbc04669c3a77856d4b7542846a9808d6eb Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 7 Nov 2022 23:07:57 +0000 Subject: [PATCH 1/2] fix: Use app slugs instead of the display name to report health All applications without display names were reporting broken health. --- agent/apphealth.go | 24 +++++++++++---------- agent/apphealth_test.go | 14 ++++++------ coderd/workspaceagents.go | 8 +++---- coderd/workspaceagents_test.go | 39 +++++++++++++--------------------- codersdk/workspaceapps.go | 2 +- 5 files changed, 40 insertions(+), 47 deletions(-) diff --git a/agent/apphealth.go b/agent/apphealth.go index 39de7ce5673c5..38926c00016c9 100644 --- a/agent/apphealth.go +++ b/agent/apphealth.go @@ -8,6 +8,8 @@ import ( "golang.org/x/xerrors" + "github.com/google/uuid" + "cdr.dev/slog" "github.com/coder/coder/codersdk" "github.com/coder/retry" @@ -31,9 +33,9 @@ func NewWorkspaceAppHealthReporter(logger slog.Logger, apps []codersdk.Workspace } hasHealthchecksEnabled := false - health := make(map[string]codersdk.WorkspaceAppHealth, 0) + health := make(map[uuid.UUID]codersdk.WorkspaceAppHealth, 0) for _, app := range apps { - health[app.DisplayName] = app.Health + health[app.ID] = app.Health if !hasHealthchecksEnabled && app.Health != codersdk.WorkspaceAppHealthDisabled { hasHealthchecksEnabled = true } @@ -46,7 +48,7 @@ func NewWorkspaceAppHealthReporter(logger slog.Logger, apps []codersdk.Workspace // run a ticker for each app health check. var mu sync.RWMutex - failures := make(map[string]int, 0) + failures := make(map[uuid.UUID]int, 0) for _, nextApp := range apps { if !shouldStartTicker(nextApp) { continue @@ -85,21 +87,21 @@ func NewWorkspaceAppHealthReporter(logger slog.Logger, apps []codersdk.Workspace }() if err != nil { mu.Lock() - if failures[app.DisplayName] < int(app.Healthcheck.Threshold) { + if failures[app.ID] < int(app.Healthcheck.Threshold) { // increment the failure count and keep status the same. // we will change it when we hit the threshold. - failures[app.DisplayName]++ + failures[app.ID]++ } 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.DisplayName] = codersdk.WorkspaceAppHealthUnhealthy + health[app.ID] = codersdk.WorkspaceAppHealthUnhealthy } mu.Unlock() } else { mu.Lock() // we only need one successful health check to be considered healthy. - health[app.DisplayName] = codersdk.WorkspaceAppHealthHealthy - failures[app.DisplayName] = 0 + health[app.ID] = codersdk.WorkspaceAppHealthHealthy + failures[app.ID] = 0 mu.Unlock() } @@ -155,7 +157,7 @@ func shouldStartTicker(app codersdk.WorkspaceApp) bool { return app.Healthcheck.URL != "" && app.Healthcheck.Interval > 0 && app.Healthcheck.Threshold > 0 } -func healthChanged(old map[string]codersdk.WorkspaceAppHealth, new map[string]codersdk.WorkspaceAppHealth) bool { +func healthChanged(old map[uuid.UUID]codersdk.WorkspaceAppHealth, new map[uuid.UUID]codersdk.WorkspaceAppHealth) bool { for name, newValue := range new { oldValue, found := old[name] if !found { @@ -169,8 +171,8 @@ func healthChanged(old map[string]codersdk.WorkspaceAppHealth, new map[string]co return false } -func copyHealth(h1 map[string]codersdk.WorkspaceAppHealth) map[string]codersdk.WorkspaceAppHealth { - h2 := make(map[string]codersdk.WorkspaceAppHealth, 0) +func copyHealth(h1 map[uuid.UUID]codersdk.WorkspaceAppHealth) map[uuid.UUID]codersdk.WorkspaceAppHealth { + h2 := make(map[uuid.UUID]codersdk.WorkspaceAppHealth, 0) for k, v := range h1 { h2[k] = v } diff --git a/agent/apphealth_test.go b/agent/apphealth_test.go index 6904a870d2812..51887afd6472b 100644 --- a/agent/apphealth_test.go +++ b/agent/apphealth_test.go @@ -27,12 +27,12 @@ func TestAppHealth(t *testing.T) { defer cancel() apps := []codersdk.WorkspaceApp{ { - DisplayName: "app1", + Slug: "app1", Healthcheck: codersdk.Healthcheck{}, Health: codersdk.WorkspaceAppHealthDisabled, }, { - DisplayName: "app2", + Slug: "app2", Healthcheck: codersdk.Healthcheck{ // URL: We don't set the URL for this test because the setup will // create a httptest server for us and set it for us. @@ -69,7 +69,7 @@ func TestAppHealth(t *testing.T) { defer cancel() apps := []codersdk.WorkspaceApp{ { - DisplayName: "app2", + Slug: "app2", Healthcheck: codersdk.Healthcheck{ // URL: We don't set the URL for this test because the setup will // create a httptest server for us and set it for us. @@ -102,7 +102,7 @@ func TestAppHealth(t *testing.T) { defer cancel() apps := []codersdk.WorkspaceApp{ { - DisplayName: "app2", + Slug: "app2", Healthcheck: codersdk.Healthcheck{ // URL: We don't set the URL for this test because the setup will // create a httptest server for us and set it for us. @@ -137,7 +137,7 @@ func TestAppHealth(t *testing.T) { defer cancel() apps := []codersdk.WorkspaceApp{ { - DisplayName: "app2", + Slug: "app2", Healthcheck: codersdk.Healthcheck{ // URL: We don't set the URL for this test because the setup will // create a httptest server for us and set it for us. @@ -185,9 +185,9 @@ func setupAppReporter(ctx context.Context, t *testing.T, apps []codersdk.Workspa } postWorkspaceAgentAppHealth := func(_ context.Context, req codersdk.PostWorkspaceAppHealthsRequest) error { mu.Lock() - for name, health := range req.Healths { + for id, health := range req.Healths { for i, app := range apps { - if app.DisplayName != name { + if app.ID != id { continue } app.Health = health diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index b101789e15145..3a2ff65da1790 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -913,10 +913,10 @@ func (api *API) postWorkspaceAppHealth(rw http.ResponseWriter, r *http.Request) } var newApps []database.WorkspaceApp - for name, newHealth := range req.Healths { + for id, newHealth := range req.Healths { old := func() *database.WorkspaceApp { for _, app := range apps { - if app.DisplayName == name { + if app.ID == id { return &app } } @@ -926,7 +926,7 @@ func (api *API) postWorkspaceAppHealth(rw http.ResponseWriter, r *http.Request) if old == nil { httpapi.Write(r.Context(), rw, http.StatusNotFound, codersdk.Response{ Message: "Error setting workspace app health", - Detail: xerrors.Errorf("workspace app name %s not found", name).Error(), + Detail: xerrors.Errorf("workspace app name %s not found", id).Error(), }) return } @@ -934,7 +934,7 @@ func (api *API) postWorkspaceAppHealth(rw http.ResponseWriter, r *http.Request) if old.HealthcheckUrl == "" { httpapi.Write(r.Context(), rw, http.StatusNotFound, codersdk.Response{ Message: "Error setting workspace app health", - Detail: xerrors.Errorf("health checking is disabled for workspace app %s", name).Error(), + Detail: xerrors.Errorf("health checking is disabled for workspace app %s", id).Error(), }) return } diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 84ab9933234a5..a0b610379b0bc 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -555,9 +555,8 @@ func TestWorkspaceAgentListeningPorts(t *testing.T) { // should not exist in the response. _, appLPort := generateUnfilteredPort(t) app := &proto.App{ - Slug: "test-app", - DisplayName: "test-app", - Url: fmt.Sprintf("http://localhost:%d", appLPort), + Slug: "test-app", + Url: fmt.Sprintf("http://localhost:%d", appLPort), } // Generate a filtered port that should not exist in the response. @@ -624,11 +623,10 @@ func TestWorkspaceAgentAppHealth(t *testing.T) { authToken := uuid.NewString() apps := []*proto.App{ { - Slug: "code-server", - DisplayName: "code-server", - Command: "some-command", - Url: "http://localhost:3000", - Icon: "/code.svg", + Slug: "code-server", + Command: "some-command", + Url: "http://localhost:3000", + Icon: "/code.svg", }, { Slug: "code-server-2", @@ -683,31 +681,24 @@ func TestWorkspaceAgentAppHealth(t *testing.T) { // empty err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{}) require.Error(t, err) - // invalid name - err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{ - Healths: map[string]codersdk.WorkspaceAppHealth{ - "bad-name": codersdk.WorkspaceAppHealthDisabled, - }, - }) - require.Error(t, err) - // healcheck disabled + // healthcheck disabled err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{ - Healths: map[string]codersdk.WorkspaceAppHealth{ - "code-server": codersdk.WorkspaceAppHealthInitializing, + Healths: map[uuid.UUID]codersdk.WorkspaceAppHealth{ + metadata.Apps[0].ID: codersdk.WorkspaceAppHealthInitializing, }, }) require.Error(t, err) // invalid value err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{ - Healths: map[string]codersdk.WorkspaceAppHealth{ - "code-server-2": codersdk.WorkspaceAppHealth("bad-value"), + Healths: map[uuid.UUID]codersdk.WorkspaceAppHealth{ + metadata.Apps[1].ID: codersdk.WorkspaceAppHealth("bad-value"), }, }) require.Error(t, err) // update to healthy err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{ - Healths: map[string]codersdk.WorkspaceAppHealth{ - "code-server-2": codersdk.WorkspaceAppHealthHealthy, + Healths: map[uuid.UUID]codersdk.WorkspaceAppHealth{ + metadata.Apps[1].ID: codersdk.WorkspaceAppHealthHealthy, }, }) require.NoError(t, err) @@ -716,8 +707,8 @@ func TestWorkspaceAgentAppHealth(t *testing.T) { require.EqualValues(t, codersdk.WorkspaceAppHealthHealthy, metadata.Apps[1].Health) // update to unhealthy err = agentClient.PostWorkspaceAgentAppHealth(ctx, codersdk.PostWorkspaceAppHealthsRequest{ - Healths: map[string]codersdk.WorkspaceAppHealth{ - "code-server-2": codersdk.WorkspaceAppHealthUnhealthy, + Healths: map[uuid.UUID]codersdk.WorkspaceAppHealth{ + metadata.Apps[1].ID: codersdk.WorkspaceAppHealthUnhealthy, }, }) require.NoError(t, err) diff --git a/codersdk/workspaceapps.go b/codersdk/workspaceapps.go index 3cee425ebfe03..6a4f22e121265 100644 --- a/codersdk/workspaceapps.go +++ b/codersdk/workspaceapps.go @@ -54,5 +54,5 @@ type Healthcheck struct { // @typescript-ignore PostWorkspaceAppHealthsRequest type PostWorkspaceAppHealthsRequest struct { // Healths is a map of the workspace app name and the health of the app. - Healths map[string]WorkspaceAppHealth + Healths map[uuid.UUID]WorkspaceAppHealth } From a597d89b4a1321977e19f5ca70189fdf3c0f7ad9 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 7 Nov 2022 17:20:01 -0600 Subject: [PATCH 2/2] Update agent/apphealth.go Co-authored-by: Dean Sheather --- agent/apphealth.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/apphealth.go b/agent/apphealth.go index 38926c00016c9..911bc0d0bb4f8 100644 --- a/agent/apphealth.go +++ b/agent/apphealth.go @@ -7,7 +7,6 @@ import ( "time" "golang.org/x/xerrors" - "github.com/google/uuid" "cdr.dev/slog"