DEVPROD-24927 Terminate debug hosts when setting is off #9807
Conversation
| return hosts, nil | ||
| } | ||
|
|
||
| // FindDebugHostsForProject finds all debug hosts associated with a project that are eligible for termination. |
There was a problem hiding this comment.
I feel like the name should reflect that these hosts are eligible for termination but I'm okay with leaving it as-is
| // The nil check is necessary because MongoDB may return hosts where the struct field is nil | ||
| // even if the database field exists. | ||
| var debugHostsForProject []Host | ||
| for _, h := range hosts { |
There was a problem hiding this comment.
I'm okay with leaving this as-is but I could see this query being pretty inefficient in terms of speed and in terms of always returning all latest debug hosts and having to programatically filter them out.
I wonder if we could just implant the project id on creation, maybe another field like RunningTaskProject (we could utilize the same field but I wouldn't due to it being mentally a different type of data), and then query on that field rather than all this logic. It would make an index on project id pretty useful and relevant for this query (we could do a composite index on project id, debug key, and user host key to keep the index size small).
| continue | ||
| } | ||
|
|
||
| // Check if task belongs to the target project |
There was a problem hiding this comment.
| // Check if task belongs to the target project | |
| // Check if task belongs to the target project. |
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| assert.NoError(t, db.ClearCollections(Collection, task.Collection)) |
There was a problem hiding this comment.
| assert.NoError(t, db.ClearCollections(Collection, task.Collection)) | |
| require.NoError(t, db.ClearCollections(Collection, task.Collection)) |
| } | ||
|
|
||
| func TestFindDebugHostsForProject(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
nit: to use t.Context() rather than creating a context
|
|
||
| // Test empty projectId validation | ||
| found, err := FindDebugHostsForProject(ctx, "") | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
| assert.Error(t, err) | |
| assert.ErrorContains(t, err, "projectId cannot be empty") |
| } | ||
| require.NoError(t, task3.Insert(ctx)) | ||
|
|
||
| // Create hosts with various configurations |
There was a problem hiding this comment.
nit: periods at the end of the comments
| // Create hosts with various configurations | |
| // Create hosts with various configurations. |
| section == model.ProjectPageGeneralSection && | ||
| utility.FromBoolTPtr(mergedSection.DebugSpawnHostsDisabled) && | ||
| !utility.FromBoolTPtr(mergedBeforeRef.DebugSpawnHostsDisabled) { | ||
|
|
There was a problem hiding this comment.
This logic looks fine but I think we should break it up in to separate booleans or a helper function(s). It's a bit hard to reason about. This whole function has a similar vibe, I wonder if we could break this whole logic off to a separate function to keep it easier to maintain and test
| } | ||
|
|
||
| // NewSpawnHostTerminationJobWithSource returns a job to terminate a spawn host with a custom source. | ||
| func NewSpawnHostTerminationJobWithSource(h *host.Host, user, ts string, source evergreen.ModifySpawnHostSource) amboy.Job { |
There was a problem hiding this comment.
I think it's inconsistent that we export NewSpawnHostTerminationJob with a source of evergreen.ModifySpawnHostManual but for events originating from evergreen.ModifySpawnHostProjectSettings, they have to use this WithSource version. I think we should make both use cases provide a source or export two functions that embed the specific sources
| if err := mgr.TerminateInstance(ctx, h, user, "user requested spawn host termination"); err != nil { | ||
| reason := "user requested spawn host termination" | ||
| if j.CloudHostModification.Source == evergreen.ModifySpawnHostProjectSettings { | ||
| reason = "project disabled debug spawn hosts" |
There was a problem hiding this comment.
We have an existing test for this job here, could you add a second one that tests on this new source?
2 participants
DEVPROD-24927
Description
when the debug spawn host setting is turned off, it will terminate the hosts
Testing
staging and unit test