Skip to content

fix(nebius): sweep orphaned validation CI resources and make TerminateInstance idempotent - #156

Open
hakhandelwal11 wants to merge 3 commits into
mainfrom
BREV-12143/nebius-ci-orphan-sweep
Open

hakhandelwal11 wants to merge 3 commits into
mainfrom
BREV-12143/nebius-ci-orphan-sweep

Conversation

@hakhandelwal11

@hakhandelwal11 hakhandelwal11 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Problem

The Nebius validation/integration CI provisions real VMs, each with an isolated VPC, subnet, and boot disk. On failure paths these resources are never terminated and leak indefinitely.

Fix

Source fix (P0)

  • registerInstanceCleanup schedules termination via t.Cleanup before any assertion, on a fresh context.Background() with bounded retry. A terminate that ultimately fails is surfaced as t.Errorf("LEAKED INSTANCE …") — a red test, never swallowed.
  • Idempotent terminate: TerminateInstance maps gRPC NotFoundv1.ErrInstanceNotFound, wrapping both the sentinel and the raw error so errors.Is matches and the gRPC status code / message survive for callers that classify on them.

Backstop (P1)

  • Stamp a per-run ci-run-id label on every VM and its network/subnet/disk at create time.
  • A new if: always() step runs TestSweepOrphans, which first sweeps VMs via SweepOrphanedInstances (terminate cascades their attached network/disk), then sweepStandaloneResources reaps the leftover network/subnet/disk in dependency order (disk → subnet → VPC). Both match only ci-run-id == CI_RUN_ID.
  • Pagination: every list call is fully paginated (PageSize=1000 + PageToken) across all projects in the tenant — the SDK filter is client-side, and resources can land in any project/region, so a single page/project isn't sufficient.

Prod-safety analysis

  • Deletion is gated on a single label prod never sets. Every delete checks labels[ci-run-id] == CI_RUN_ID. In the shared tenant the sweep lists prod and other-run resources but skips everything without a matching ci-run-id.
  • Sub-resource stamping is narrowed to ci-run-id only (ciRunLabels). In production attrs.Tags never contains ci-run-id, so ciRunLabels returns nil and prod VPC/subnet/disk labels are byte-identical to before. The VM's own labels (full attrs.Tags) are unchanged.

@hakhandelwal11
hakhandelwal11 requested a review from a team as a code owner September 15, 2026 11:15
@hakhandelwal11 hakhandelwal11 self-assigned this Sep 15, 2026
Comment thread internal/validation/suite.go Outdated
return res, errors.New("SweepOrphanedInstances: RunID is required")
}

instances, err := client.ListInstances(ctx, v1.ListInstancesArgs{})

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.TagFilters to {ci-run-id: [runID]} so ListInstances returns only this run's instances. We've kept the explicit match deliberately, because provider support for TagFilters is 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, ListInstances would return every instance and the sweep would terminate non-matching — including production — resources. The explicit inst.Tags[ci-run-id] == RunID check keeps that guarantee independent of any provider's implementation.

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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 the ci-run-id label on every resource that run creates. The sweep only matches ci-run-id to its own CI_RUN_ID, so a run can only ever delete resources it created. Two concurrent runs have different run_ids and 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.

Comment thread .github/workflows/validation-nebius.yml Outdated
@hakhandelwal11
hakhandelwal11 force-pushed the BREV-12143/nebius-ci-orphan-sweep branch from accc87b to 1d9de38 Compare September 19, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants