Skip to content

Commit

Permalink
fix: Use app slugs instead of the display name to report health
Browse files Browse the repository at this point in the history
All applications without display names were reporting broken health.
  • Loading branch information
kylecarbs committed Nov 7, 2022
1 parent 50ad4a8 commit d0b25cb
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 47 deletions.
24 changes: 13 additions & 11 deletions agent/apphealth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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()
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down
14 changes: 7 additions & 7 deletions agent/apphealth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions coderd/workspaceagents.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -926,15 +926,15 @@ 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
}

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
}
Expand Down
39 changes: 15 additions & 24 deletions coderd/workspaceagents_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion codersdk/workspaceapps.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit d0b25cb

Please sign in to comment.