- Notifications
You must be signed in to change notification settings - Fork1k
chore: implement oom/ood processing component#16436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
44 commits Select commitHold shift + click to select a range
6c6240b chore: add workspace reached resource threshold notification
DanielleMaywoodb3081de chore: split out into two notifications
DanielleMaywooda9c8676 chore: update golden files
DanielleMaywood1a84f96 chore: begin impl of processing logic for oom/ood
DanielleMaywood78ede46 chore: appease the linter for now
DanielleMaywood0d2b970 chore: use latest changes to #247, start debounce logic
DanielleMaywood0df2fd5 chore: add more tests
DanielleMaywood854d81a chore: remove mock db for workspace monitor agentapi test
DanielleMaywood9d9d7b4 chore: remove todo comment
DanielleMaywood4c21ce7 Merge branch 'main' into dm-internal-247
DanielleMaywood944fdb5 chore: rewrite ood notification
DanielleMaywood6444176 chore: updaten golden file
DanielleMaywoodbc87268 chore: silly me
DanielleMaywoodd2265f6 chore: rename test
DanielleMaywood82a9852 Merge branch 'dm-internal-247' into dm-internal-248
DanielleMaywood62621d4 chore: add more tests, fix broken sql query
DanielleMaywood81f43d3 Merge branch 'main' into dm-internal-248
DanielleMaywood7522b37 chore: update to match main
DanielleMaywood69c4f42 chore: run 'make gen'
DanielleMaywood44ebf65 chore: run 'make fmt'
DanielleMaywood714e743 chore: remove cruft
DanielleMaywood7cf5212 chore: align interface
DanielleMaywoodd08e713 chore: add another test
DanielleMaywooded42eae chore: improve volume monitor test
DanielleMaywood1b0d0d2 chore: rename fields
DanielleMaywood51b16c6 Merge branch 'main' into dm-internal-248
DanielleMaywood4e43bab chore: align with other branch
DanielleMaywood4e144ae Merge branch 'main' into dm-internal-248
DanielleMaywoodda25ecc chore: bump migration number
DanielleMaywoodfe1e805 chore: add test and align better
DanielleMaywoodabbd522 chore: appease linter
DanielleMaywood456989e Merge branch 'main' into dm-internal-248
DanielleMaywood1550cc6 chore: update rbac
DanielleMaywood7998f89 chore: handle missing datapoints
DanielleMaywoodbda8f29 chore: add tests for unknown state on memory monitor
DanielleMaywood9d662a3 chore: add tests for missing datapoints in volume monitors
DanielleMaywoodbff48dc chore: add default debounce of 5 minutes
DanielleMaywoodc343a70 chore: implement feedback
DanielleMaywoodbabc48f chore: feedback
DanielleMaywood01ca549 chore: feedback
DanielleMaywooda975810 chore: forgot to run the linter
DanielleMaywoodee35d85 chore: use percentages for alert config
DanielleMaywoodd2fa8df Merge branch 'main' into dm-internal-248
DanielleMaywood27d78d1 chore: fmt and bump migration number
DanielleMaywoodFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
28 changes: 25 additions & 3 deletionscoderd/agentapi/api.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -17,10 +17,12 @@ import ( | ||
| "cdr.dev/slog" | ||
| agentproto "github.com/coder/coder/v2/agent/proto" | ||
| "github.com/coder/coder/v2/coderd/agentapi/resourcesmonitor" | ||
| "github.com/coder/coder/v2/coderd/appearance" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| "github.com/coder/coder/v2/coderd/database/pubsub" | ||
| "github.com/coder/coder/v2/coderd/externalauth" | ||
| "github.com/coder/coder/v2/coderd/notifications" | ||
| "github.com/coder/coder/v2/coderd/prometheusmetrics" | ||
| "github.com/coder/coder/v2/coderd/tracing" | ||
| "github.com/coder/coder/v2/coderd/workspacestats" | ||
| @@ -29,6 +31,7 @@ import ( | ||
| "github.com/coder/coder/v2/codersdk/agentsdk" | ||
| "github.com/coder/coder/v2/tailnet" | ||
| tailnetproto "github.com/coder/coder/v2/tailnet/proto" | ||
| "github.com/coder/quartz" | ||
| ) | ||
| // API implements the DRPC agent API interface from agent/proto. This struct is | ||
| @@ -59,7 +62,9 @@ type Options struct { | ||
| Ctx context.Context | ||
| Log slog.Logger | ||
| Clock quartz.Clock | ||
DanielleMaywood marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| Database database.Store | ||
| NotificationsEnqueuer notifications.Enqueuer | ||
| Pubsub pubsub.Pubsub | ||
| DerpMapFn func() *tailcfg.DERPMap | ||
| TailnetCoordinator *atomic.Pointer[tailnet.Coordinator] | ||
| @@ -82,6 +87,10 @@ type Options struct { | ||
| } | ||
| func New(opts Options) *API { | ||
| if opts.Clock == nil { | ||
| opts.Clock = quartz.NewReal() | ||
| } | ||
| api := &API{ | ||
| opts: opts, | ||
| mu: sync.Mutex{}, | ||
| @@ -104,9 +113,22 @@ func New(opts Options) *API { | ||
| } | ||
| api.ResourcesMonitoringAPI = &ResourcesMonitoringAPI{ | ||
| AgentID: opts.AgentID, | ||
| WorkspaceID: opts.WorkspaceID, | ||
| Clock: opts.Clock, | ||
| Database: opts.Database, | ||
| NotificationsEnqueuer: opts.NotificationsEnqueuer, | ||
| Debounce: 5 * time.Minute, | ||
DanielleMaywood marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| Config: resourcesmonitor.Config{ | ||
| NumDatapoints: 20, | ||
| CollectionInterval: 10 * time.Second, | ||
| Alert: resourcesmonitor.AlertConfig{ | ||
| MinimumNOKsPercent: 20, | ||
| ConsecutiveNOKsPercent: 50, | ||
| }, | ||
| }, | ||
| } | ||
| api.StatsAPI = &StatsAPI{ | ||
207 changes: 198 additions & 9 deletionscoderd/agentapi/resources_monitoring.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -4,20 +4,35 @@ import ( | ||
| "context" | ||
| "database/sql" | ||
| "errors" | ||
| "fmt" | ||
| "time" | ||
| "golang.org/x/xerrors" | ||
| "cdr.dev/slog" | ||
| "github.com/google/uuid" | ||
| "github.com/coder/coder/v2/agent/proto" | ||
| "github.com/coder/coder/v2/coderd/agentapi/resourcesmonitor" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| "github.com/coder/coder/v2/coderd/database/dbauthz" | ||
| "github.com/coder/coder/v2/coderd/database/dbtime" | ||
| "github.com/coder/coder/v2/coderd/notifications" | ||
| "github.com/coder/quartz" | ||
| ) | ||
| type ResourcesMonitoringAPI struct { | ||
| AgentID uuid.UUID | ||
| WorkspaceID uuid.UUID | ||
| Log slog.Logger | ||
| Clock quartz.Clock | ||
| Database database.Store | ||
| NotificationsEnqueuer notifications.Enqueuer | ||
| Debounce time.Duration | ||
| Config resourcesmonitor.Config | ||
| } | ||
| func (a *ResourcesMonitoringAPI) GetResourcesMonitoringConfiguration(ctx context.Context, _ *proto.GetResourcesMonitoringConfigurationRequest) (*proto.GetResourcesMonitoringConfigurationResponse, error) { | ||
| @@ -33,8 +48,8 @@ func (a *ResourcesMonitoringAPI) GetResourcesMonitoringConfiguration(ctx context | ||
| return &proto.GetResourcesMonitoringConfigurationResponse{ | ||
| Config: &proto.GetResourcesMonitoringConfigurationResponse_Config{ | ||
| CollectionIntervalSeconds:int32(a.Config.CollectionInterval.Seconds()), | ||
| NumDatapoints:a.Config.NumDatapoints, | ||
| }, | ||
| Memory: func() *proto.GetResourcesMonitoringConfigurationResponse_Memory { | ||
| if memoryErr != nil { | ||
| @@ -60,8 +75,182 @@ func (a *ResourcesMonitoringAPI) GetResourcesMonitoringConfiguration(ctx context | ||
| } | ||
| func (a *ResourcesMonitoringAPI) PushResourcesMonitoringUsage(ctx context.Context, req *proto.PushResourcesMonitoringUsageRequest) (*proto.PushResourcesMonitoringUsageResponse, error) { | ||
| var err error | ||
| if memoryErr := a.monitorMemory(ctx, req.Datapoints); memoryErr != nil { | ||
| err = errors.Join(err, xerrors.Errorf("monitor memory: %w", memoryErr)) | ||
| } | ||
| if volumeErr := a.monitorVolumes(ctx, req.Datapoints); volumeErr != nil { | ||
| err = errors.Join(err, xerrors.Errorf("monitor volume: %w", volumeErr)) | ||
| } | ||
| return &proto.PushResourcesMonitoringUsageResponse{}, err | ||
| } | ||
| func (a *ResourcesMonitoringAPI) monitorMemory(ctx context.Context, datapoints []*proto.PushResourcesMonitoringUsageRequest_Datapoint) error { | ||
| monitor, err := a.Database.FetchMemoryResourceMonitorsByAgentID(ctx, a.AgentID) | ||
DanielleMaywood marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| if err != nil { | ||
| // It is valid for an agent to not have a memory monitor, so we | ||
| // do not want to treat it as an error. | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil | ||
| } | ||
| return xerrors.Errorf("fetch memory resource monitor: %w", err) | ||
| } | ||
| if !monitor.Enabled { | ||
| return nil | ||
| } | ||
| usageDatapoints := make([]*proto.PushResourcesMonitoringUsageRequest_Datapoint_MemoryUsage, 0, len(datapoints)) | ||
| for _, datapoint := range datapoints { | ||
| usageDatapoints = append(usageDatapoints, datapoint.Memory) | ||
| } | ||
| usageStates := resourcesmonitor.CalculateMemoryUsageStates(monitor, usageDatapoints) | ||
| oldState := monitor.State | ||
| newState := resourcesmonitor.NextState(a.Config, oldState, usageStates) | ||
| debouncedUntil, shouldNotify := monitor.Debounce(a.Debounce, a.Clock.Now(), oldState, newState) | ||
| //nolint:gocritic // We need to be able to update the resource monitor here. | ||
| err = a.Database.UpdateMemoryResourceMonitor(dbauthz.AsResourceMonitor(ctx), database.UpdateMemoryResourceMonitorParams{ | ||
| AgentID: a.AgentID, | ||
| State: newState, | ||
| UpdatedAt: dbtime.Time(a.Clock.Now()), | ||
| DebouncedUntil: dbtime.Time(debouncedUntil), | ||
| }) | ||
| if err != nil { | ||
| return xerrors.Errorf("update workspace monitor: %w", err) | ||
| } | ||
| if !shouldNotify { | ||
| return nil | ||
| } | ||
| workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get workspace by id: %w", err) | ||
| } | ||
| _, err = a.NotificationsEnqueuer.EnqueueWithData( | ||
| // nolint:gocritic // We need to be able to send the notification. | ||
| dbauthz.AsNotifier(ctx), | ||
| workspace.OwnerID, | ||
| notifications.TemplateWorkspaceOutOfMemory, | ||
| map[string]string{ | ||
| "workspace": workspace.Name, | ||
| "threshold": fmt.Sprintf("%d%%", monitor.Threshold), | ||
| }, | ||
| map[string]any{ | ||
| // NOTE(DanielleMaywood): | ||
| // When notifications are enqueued, they are checked to be | ||
| // unique within a single day. This means that if we attempt | ||
| // to send two OOM notifications for the same workspace on | ||
| // the same day, the enqueuer will prevent us from sending | ||
| // a second one. We are inject a timestamp to make the | ||
| // notifications appear different enough to circumvent this | ||
| // deduplication logic. | ||
| "timestamp": a.Clock.Now(), | ||
| }, | ||
| "workspace-monitor-memory", | ||
| ) | ||
| if err != nil { | ||
| return xerrors.Errorf("notify workspace OOM: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
| func (a *ResourcesMonitoringAPI) monitorVolumes(ctx context.Context, datapoints []*proto.PushResourcesMonitoringUsageRequest_Datapoint) error { | ||
| volumeMonitors, err := a.Database.FetchVolumesResourceMonitorsByAgentID(ctx, a.AgentID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get or insert volume monitor: %w", err) | ||
| } | ||
| outOfDiskVolumes := make([]map[string]any, 0) | ||
dannykopping marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| for _, monitor := range volumeMonitors { | ||
| if !monitor.Enabled { | ||
| continue | ||
| } | ||
| usageDatapoints := make([]*proto.PushResourcesMonitoringUsageRequest_Datapoint_VolumeUsage, 0, len(datapoints)) | ||
| for _, datapoint := range datapoints { | ||
| var usage *proto.PushResourcesMonitoringUsageRequest_Datapoint_VolumeUsage | ||
| for _, volume := range datapoint.Volumes { | ||
| if volume.Volume == monitor.Path { | ||
| usage = volume | ||
| break | ||
| } | ||
| } | ||
| usageDatapoints = append(usageDatapoints, usage) | ||
| } | ||
| usageStates := resourcesmonitor.CalculateVolumeUsageStates(monitor, usageDatapoints) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. On second thought, given that we're having to do this for more memory & volumes, I think we should seriously consider updating the agent to send back that bool to indicate enabled but failed to collect; that's a 1:1 with your "unknown" logic here. This can be in a follow-up. | ||
| oldState := monitor.State | ||
| newState := resourcesmonitor.NextState(a.Config, oldState, usageStates) | ||
| debouncedUntil, shouldNotify := monitor.Debounce(a.Debounce, a.Clock.Now(), oldState, newState) | ||
| if shouldNotify { | ||
| outOfDiskVolumes = append(outOfDiskVolumes, map[string]any{ | ||
| "path": monitor.Path, | ||
| "threshold": fmt.Sprintf("%d%%", monitor.Threshold), | ||
| }) | ||
| } | ||
| //nolint:gocritic // We need to be able to update the resource monitor here. | ||
| if err := a.Database.UpdateVolumeResourceMonitor(dbauthz.AsResourceMonitor(ctx), database.UpdateVolumeResourceMonitorParams{ | ||
| AgentID: a.AgentID, | ||
| Path: monitor.Path, | ||
| State: newState, | ||
| UpdatedAt: dbtime.Time(a.Clock.Now()), | ||
| DebouncedUntil: dbtime.Time(debouncedUntil), | ||
| }); err != nil { | ||
| return xerrors.Errorf("update workspace monitor: %w", err) | ||
| } | ||
| } | ||
| if len(outOfDiskVolumes) == 0 { | ||
| return nil | ||
| } | ||
| workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID) | ||
| if err != nil { | ||
| return xerrors.Errorf("get workspace by id: %w", err) | ||
| } | ||
| if _, err := a.NotificationsEnqueuer.EnqueueWithData( | ||
| // nolint:gocritic // We need to be able to send the notification. | ||
| dbauthz.AsNotifier(ctx), | ||
| workspace.OwnerID, | ||
| notifications.TemplateWorkspaceOutOfDisk, | ||
| map[string]string{ | ||
| "workspace": workspace.Name, | ||
| }, | ||
| map[string]any{ | ||
| "volumes": outOfDiskVolumes, | ||
| // NOTE(DanielleMaywood): | ||
| // When notifications are enqueued, they are checked to be | ||
| // unique within a single day. This means that if we attempt | ||
| // to send two OOM notifications for the same workspace on | ||
| // the same day, the enqueuer will prevent us from sending | ||
| // a second one. We are inject a timestamp to make the | ||
| // notifications appear different enough to circumvent this | ||
| // deduplication logic. | ||
| "timestamp": a.Clock.Now(), | ||
| }, | ||
| "workspace-monitor-volumes", | ||
| ); err != nil { | ||
| return xerrors.Errorf("notify workspace OOD: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.