Skip to content

Refactor the Create/Resume functions to avoid globals/reduce args#1258

Merged
jakubno merged 9 commits into
mainfrom
refactor-to-make-factory
Oct 1, 2025
Merged

Refactor the Create/Resume functions to avoid globals/reduce args#1258
jakubno merged 9 commits into
mainfrom
refactor-to-make-factory

Conversation

@djeebus
Copy link
Copy Markdown
Contributor

@djeebus djeebus commented Sep 26, 2025

This PR creates a Factory struct, which does a few things:

  • moves networkPool and devicePool to a central place, so they don't need to be passed into CreateSandbox and ResumeSandbox on every invocation.
  • moves the query for MetricsWriteFlagName into the Factory, simplifying the arguments.
  • move the check of the ALLOW_SANDBOX_INTERNET env var into the main function, and out of the global scope.

Note

Introduce sandbox.Factory to encapsulate network/device pools and feature flags, refactoring sandbox create/resume flows and updating builders/servers to use it while removing global config and excess args.

  • Core refactor:
    • Add sandbox.Factory holding network.Pool, nbd.DevicePool, featureflags.Client, and default internet access.
    • Convert sandbox.CreateSandbox/ResumeSandbox to Factory methods; eliminate global config.AllowSandboxInternet and per-call pool args.
    • Move metrics write flag resolution into Factory during ResumeSandbox.
  • Server wiring:
    • server.New simplified (no context/err return); ServiceConfig gains SandboxFactory and uses it when creating sandboxes.
    • Main initializes SandboxFactory (env-driven ALLOW_SANDBOX_INTERNET) and passes it to server and template manager.
  • Template build pipeline:
    • build.Builder, layer executors, and phase builders accept/use sandbox.Factory instead of pools; constructors updated accordingly.
  • Cleanup:
    • Remove internal/config AllowSandboxInternet.
    • Drop redundant feature flag checks and pool args across call sites.

Written by Cursor Bugbot for commit 5f5caa1. This will update automatically on new commits. Configure here.

cursor[bot]

This comment was marked as outdated.

@djeebus
Copy link
Copy Markdown
Contributor Author

djeebus commented Sep 26, 2025

Cursor's comment is true, but intentional. It felt like the right thing to do (the CLIs are used for testing, while the main binary is deployed and configured via env vars), but if that's the wrong move, let me know.

@djeebus
Copy link
Copy Markdown
Contributor Author

djeebus commented Sep 26, 2025

oh, the template server shouldn't have the hardcoded value, I'll fix that

Comment on lines -70 to -71
layerExecutor.networkPool,
layerExecutor.devicePool,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think layer executor doesn't need those anymore

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.

Good catch! Removed them and a bunch of other unused fields.

// Create sandbox for building template
userLogger.Debug("Creating base sandbox template layer")

// TODO: Temporarily set this based on global config, should be removed later (it should be passed as a parameter in build)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move the TODO? We're still not sending it as part of build

Comment thread packages/orchestrator/internal/template/server/main.go Outdated
@jakubno jakubno self-assigned this Sep 30, 2025
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Comment thread packages/orchestrator/internal/template/build/builder.go Outdated
index,
builder.metrics,
)
baseBuilder := base.New(bc, builder.logger, builder.proxy, builder.templateStorage, builder.artifactRegistry, layerExecutor, index, builder.metrics, builder.sandboxFactory)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep it on separate lines? 🙏🏻 it's easier to review

Comment thread packages/orchestrator/internal/sandbox/sandbox.go Outdated
Comment thread packages/orchestrator/cmd/build-template/main.go
@jakubno jakubno merged commit 8b1af8c into main Oct 1, 2025
26 checks passed
@jakubno jakubno deleted the refactor-to-make-factory branch October 1, 2025 09:44
ValentaTomas pushed a commit that referenced this pull request May 4, 2026
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