-
Notifications
You must be signed in to change notification settings - Fork 10
fix(nebius): sweep orphaned validation CI resources and make TerminateInstance idempotent #156
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hakhandelwal11
wants to merge
3
commits into
main
Choose a base branch
from
BREV-12143/nebius-ci-orphan-sweep
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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 |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package validation | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| v1 "github.com/brevdev/cloud/v1" | ||
| ) | ||
|
|
||
| // CIRunIDLabel re-exports v1.CIRunIDLabel so the sweeper and create paths share one value. | ||
| const CIRunIDLabel = v1.CIRunIDLabel | ||
|
|
||
| // SweepOpts configures SweepOrphanedInstances. | ||
| type SweepOpts struct { | ||
| // RunID is the CIRunIDLabel label value to match. It is REQUIRED and must be unique per CI run. | ||
| RunID string | ||
| // Logf, if set, receives human-readable progress lines. Defaults to fmt.Printf. | ||
| Logf func(format string, args ...any) | ||
| } | ||
|
|
||
| // SweepResult reports what a sweep scanned and did. | ||
| type SweepResult struct { | ||
| Scanned int | ||
| Matched []v1.Instance | ||
| Deleted []v1.CloudProviderInstanceID | ||
| Failed map[v1.CloudProviderInstanceID]error | ||
| } | ||
|
|
||
| // SweepOrphanedInstances lists every instance visible to client and terminates exactly the ones | ||
| // whose CIRunIDLabel label equals opts.RunID — i.e. the VMs created by this CI run. | ||
| func SweepOrphanedInstances(ctx context.Context, client v1.CloudClient, opts SweepOpts) (SweepResult, error) { | ||
| res := SweepResult{Failed: map[v1.CloudProviderInstanceID]error{}} | ||
| logf := opts.Logf | ||
| if logf == nil { | ||
| logf = func(format string, args ...any) { fmt.Printf(format+"\n", args...) } | ||
| } | ||
|
|
||
| // A run ID is mandatory: without it there is nothing safe to match on. | ||
| if opts.RunID == "" { | ||
| return res, errors.New("SweepOrphanedInstances: RunID is required") | ||
| } | ||
|
|
||
| instances, err := client.ListInstances(ctx, v1.ListInstancesArgs{}) | ||
| if err != nil { | ||
| return res, fmt.Errorf("failed to list instances: %w", err) | ||
| } | ||
| res.Scanned = len(instances) | ||
|
|
||
| for i := range instances { | ||
| inst := instances[i] | ||
| if inst.Tags[CIRunIDLabel] != opts.RunID { | ||
| continue | ||
| } | ||
| res.Matched = append(res.Matched, inst) | ||
| } | ||
|
|
||
| logf("[sweep] scanned=%d matched=%d runID=%q", res.Scanned, len(res.Matched), opts.RunID) | ||
|
|
||
| for i := range res.Matched { | ||
| inst := res.Matched[i] | ||
| logf("[sweep] terminating cloudID=%s name=%q owner=%s location=%s", | ||
| inst.CloudID, inst.Name, inst.CloudCredRefID, inst.Location) | ||
| if err := terminateWithTimeout(ctx, client, inst.CloudID); err != nil { | ||
| res.Failed[inst.CloudID] = err | ||
| logf("[sweep] FAILED to terminate cloudID=%s: %v", inst.CloudID, err) | ||
| continue | ||
| } | ||
| res.Deleted = append(res.Deleted, inst.CloudID) | ||
| } | ||
| return res, nil | ||
| } | ||
|
|
||
| // terminateWithTimeout terminates an instance in a single attempt on an 8-min context, treating a | ||
| // not-found result as success (the instance is already gone). | ||
| func terminateWithTimeout(ctx context.Context, client v1.CloudCreateTerminateInstance, id v1.CloudProviderInstanceID) error { | ||
| termCtx, cancel := context.WithTimeout(ctx, 8*time.Minute) | ||
| defer cancel() | ||
| err := client.TerminateInstance(termCtx, id) | ||
| if err == nil || errors.Is(err, v1.ErrInstanceNotFound) || errors.Is(err, v1.ErrResourceNotFound) { | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
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 |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| package validation | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
| "time" | ||
|
|
||
| v1 "github.com/brevdev/cloud/v1" | ||
| ) | ||
|
|
||
| // fakeSweepClient is a CloudClient whose ListInstances returns a canned set and whose | ||
| // TerminateInstance records the IDs it was asked to delete. All other methods come from | ||
| // NotImplCloudClient and return ErrNotImplemented (the sweeper never calls them). | ||
| type fakeSweepClient struct { | ||
| v1.NotImplCloudClient | ||
| instances []v1.Instance | ||
| terminated []v1.CloudProviderInstanceID | ||
| } | ||
|
|
||
| var _ v1.CloudClient = (*fakeSweepClient)(nil) | ||
|
|
||
| func (c *fakeSweepClient) ListInstances(context.Context, v1.ListInstancesArgs) ([]v1.Instance, error) { | ||
| return c.instances, nil | ||
| } | ||
|
|
||
| func (c *fakeSweepClient) TerminateInstance(_ context.Context, id v1.CloudProviderInstanceID) error { | ||
| c.terminated = append(c.terminated, id) | ||
| return nil | ||
| } | ||
|
|
||
| func matchedIDs(res SweepResult) []v1.CloudProviderInstanceID { | ||
| ids := make([]v1.CloudProviderInstanceID, 0, len(res.Matched)) | ||
| for _, m := range res.Matched { | ||
| ids = append(ids, m.CloudID) | ||
| } | ||
| return ids | ||
| } | ||
|
|
||
| func assertSameSet(t *testing.T, got, want []v1.CloudProviderInstanceID) { | ||
| t.Helper() | ||
| gm := map[v1.CloudProviderInstanceID]bool{} | ||
| for _, g := range got { | ||
| gm[g] = true | ||
| } | ||
| wm := map[v1.CloudProviderInstanceID]bool{} | ||
| for _, w := range want { | ||
| wm[w] = true | ||
| } | ||
| for w := range wm { | ||
| if !gm[w] { | ||
| t.Fatalf("missing %q: got %v want %v", w, got, want) | ||
| } | ||
| } | ||
| for g := range gm { | ||
| if !wm[g] { | ||
| t.Fatalf("unexpected %q: got %v want %v", g, got, want) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TestSweepOrphanedInstances_OnlyMatchesRunID is the safety-critical test: the sweep terminates | ||
| // exactly the VMs carrying this run's ci-run-id label and nothing else. | ||
| func TestSweepOrphanedInstances_OnlyMatchesRunID(t *testing.T) { | ||
| old := time.Now().Add(-10 * 24 * time.Hour) | ||
| fresh := time.Now().Add(-1 * time.Minute) | ||
|
|
||
| instances := []v1.Instance{ | ||
| // This run's VMs — must be deleted, regardless of age. | ||
| {CloudID: "A-thisrun", CloudCredRefID: "validation-test", CreatedAt: fresh, Tags: v1.Tags{CIRunIDLabel: "run-1"}}, | ||
| {CloudID: "B-thisrun-int", CloudCredRefID: "integration-test-ref", CreatedAt: fresh, Tags: v1.Tags{CIRunIDLabel: "run-1"}}, | ||
| // Another run's VM — must NOT be deleted (different run id). | ||
| {CloudID: "C-otherrun", CloudCredRefID: "validation-test", CreatedAt: old, Tags: v1.Tags{CIRunIDLabel: "run-2"}}, | ||
| // Legacy CI VM with no run label (predates this feature) — must NOT be deleted (manual cleanup). | ||
| {CloudID: "D-legacy-nolabel", CloudCredRefID: "validation-test", CreatedAt: old}, | ||
| // Production VM that reused a CI credential ref but has no run label — must NOT be deleted. | ||
| {CloudID: "E-prod-reused-cred", CloudCredRefID: "validation-test", CreatedAt: old}, | ||
| // Ordinary production VM — must NOT be deleted. | ||
| {CloudID: "F-prod", CloudCredRefID: "cloud-cred-abc123", CreatedAt: old}, | ||
| } | ||
|
|
||
| fc := &fakeSweepClient{instances: instances} | ||
| res, err := SweepOrphanedInstances(context.Background(), fc, SweepOpts{RunID: "run-1", Logf: t.Logf}) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| want := []v1.CloudProviderInstanceID{"A-thisrun", "B-thisrun-int"} | ||
| assertSameSet(t, matchedIDs(res), want) | ||
| assertSameSet(t, res.Deleted, want) | ||
| assertSameSet(t, fc.terminated, want) | ||
| } | ||
|
|
||
| // TestSweepOrphanedInstances_RequiresRunID pins the safety invariant: without a run ID the sweep | ||
| // refuses to run and terminates nothing. | ||
| func TestSweepOrphanedInstances_RequiresRunID(t *testing.T) { | ||
| fc := &fakeSweepClient{instances: []v1.Instance{ | ||
| {CloudID: "x", CloudCredRefID: "validation-test", Tags: v1.Tags{CIRunIDLabel: "run-1"}}, | ||
| }} | ||
| if _, err := SweepOrphanedInstances(context.Background(), fc, SweepOpts{RunID: ""}); err == nil { | ||
| t.Error("expected an error when RunID is empty") | ||
| } | ||
| if len(fc.terminated) != 0 { | ||
| t.Errorf("empty RunID must terminate nothing, terminated: %v", fc.terminated) | ||
| } | ||
| } |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not pass the instance-id of the instance created during the test so we don't have to search through all instances to find a specific one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patelspratik,
Quick clarification: the value the sweep matches on there is
ci-run-id— a per-run label stamped on every resource a CI run creates, not an instance id. So it isn't looking up one specific instance; it selects every instance carrying this run's label.On pushing that match into the list call itself: we could set
ListInstancesArgs.TagFiltersto{ci-run-id: [runID]}soListInstancesreturns only this run's instances. We've kept the explicit match deliberately, because provider support forTagFiltersis inconsistent, only some providers apply it, while others ignore it or discard the arguments entirely. Since this is a delete path, relying on the filter is unsafe: against a provider that ignores it,ListInstanceswould return every instance and the sweep would terminate non-matching — including production — resources. The explicitinst.Tags[ci-run-id] == RunIDcheck keeps that guarantee independent of any provider's implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, how does this prevent concurrent runs from being deleted by each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patelspratik , Each workflow run gets a unique
github.run_id, which we stamp as theci-run-idlabel on every resource that run creates. The sweep only matchesci-run-idto its ownCI_RUN_ID, so a run can only ever delete resources it created. Two concurrent runs have differentrun_idsand therefore different labels so run A's sweep does list run B's resources in the shared tenant, but skips them on the mismatch, and vice versa. So they're isolated by the per-run label.