Skip to content

Add V2 namespace and physical container resources - #223

Open
David Negstad (danegsta) wants to merge 29 commits into
mainfrom
danegsta-v2-namespace-support
Open

Add V2 namespace and physical container resources#223
David Negstad (danegsta) wants to merge 29 commits into
mainfrom
danegsta-v2-namespace-support

Conversation

@danegsta

@danegstaDavid Negstad (danegsta) commented Jul 24, 2026

Copy link
Copy Markdown
Member

Why

This starts the V2 resource model so new DCP resources can require namespaces, support namespace-scoped cleanup, and avoid repeating direct runtime lifecycle logic across V1 controllers.

V1 resources remain cluster-scoped and continue to work without namespaces. The new V2 model gives us a place to build cleaner physical runtime primitives first, then layer higher-level logical resources on top.

What changed

  • Adds the V2 API group with Namespace, PhysicalContainerImage, and PhysicalContainer resources.
  • Adds V2 namespace cleanup and side-effect blocking for missing, terminating, or inactive namespaces.
  • Adds physical container/image controllers with queued long-running work, in-memory progress data, Ready conditions, runtime event watches, referenced image watches, and port mapping status.
  • Defines physical-resource ownership semantics: existing runtime IDs are always retained, while persistent and replaceExisting apply only to newly created runtime objects.
  • Moves shared container API fragments into pkg/commonapi and decouples container orchestrator packages from V1/V2 API packages.
  • Adds shared cleanup ordering metadata and updates shutdown cleanup paths for V2 resources.
  • Adds docs/v2-resource-plan.md with V2 guidelines and the follow-up roadmap.

Notes for reviewers

This is intentionally the initial V2 foundation rather than a complete V2 application model. Physical resources represent concrete runtime objects; future logical resources will own persistence/reuse policy and delegate runtime work to these physical primitives.

The V1 JSON API remains compatible. Shared fields added to common fragments are optional, and V1 resources remain cluster-scoped.

Validation

  • make generate
  • make test-prereqs
  • Focused Go tests for changed API/controller/helper packages
  • Targeted V2 integration tests
  • Manual Docker smoke test for V2 namespace cleanup

Introduce the initial V2 namespace model, physical container/image APIs and controllers, shared common API fragments, namespace cleanup support, and V2 roadmap documentation.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074

CopilotAI left a comment

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.

Pull request overview

Introduces the initial namespaced V2 resource model and physical container primitives while retaining V1 compatibility.

Changes:

  • Adds V2 namespace, image, and container APIs and controllers.
  • Moves reusable container definitions into commonapi.
  • Extends cleanup, orchestration, generation, and integration coverage.

The namespace-finalization race, V1 lifecycle-key regression, and missing reference/protocol validation require changes.

Reviewed changes

Copilot reviewed 98 out of 103 changed files in this pull request and generated 4 comments.

Show a summary per file
FileDescription
MakefileGenerates API artifacts for V1 and V2.
api/v1/common_types.goRe-exports shared API definitions.
api/v1/container_exec_types.goUses shared environment types.
api/v1/container_network_connection_types.goUses shared networking types.
api/v1/container_types.goMigrates container fragments to commonapi.
api/v1/container_types_test.goUpdates V1 container tests.
api/v1/executable_replica_set_types.goUses shared environment definitions.
api/v1/executable_types.goUses shared environment definitions.
api/v1/executable_types_test.goUpdates shared-type assertions.
api/v1/groupversion_info.goShares resource-creation state.
api/v1/service_types.goUses shared protocol definitions.
api/v1/zz_generated.deepcopy.goRegenerates V1 deep-copy code.
api/v1/zz_generated.model_name.goRegenerates V1 model names.
api/v2/common_types.goAdds V2 metadata helpers and validation.
api/v2/common_types_test.goTests V2 metadata validation.
api/v2/doc.goDeclares the V2 API package.
api/v2/groupversion_info.goRegisters the V2 API group.
api/v2/namespace_types.goDefines the V2 Namespace resource.
api/v2/namespace_types_test.goTests Namespace validation.
api/v2/physical_container_image_types.goDefines physical image APIs.
api/v2/physical_container_image_types_test.goTests physical image validation.
api/v2/physical_container_types.goDefines physical container APIs.
api/v2/physical_container_types_test.goTests physical container validation.
api/v2/zz_generated.deepcopy.goAdds V2 deep-copy implementations.
api/v2/zz_generated.model_name.goAdds V2 model names.
controllers/container_common.goAdapts shared container definitions.
controllers/container_controller.goConverts V1 specs to runtime options.
controllers/container_exec_controller.goMigrates shared environment types.
controllers/container_network_tunnel_proxy_controller.goUpdates shared runtime types.
controllers/container_watcher.goGeneralizes container event handling.
controllers/controller_common.goAdds namespace and status helpers.
controllers/executable_controller.goMigrates shared environment types.
controllers/executable_run_info.goUses common controller setters.
controllers/executable_start_result.goUses common controller setters.
controllers/namespace_controller.goImplements namespace lifecycle cleanup.
controllers/object_state_map.goAdds deferred operation tracking.
controllers/object_state_map_test.goTests deferred state behavior.
controllers/physical_container_controller.goImplements physical container reconciliation.
controllers/physical_container_data.goStores physical container progress.
controllers/physical_container_image_controller.goImplements image reconciliation.
controllers/physical_container_image_data.goStores image operation progress.
controllers/running_container_data.goUpdates shared runtime state.
controllers/service_controller_metrics.goUses shared protocol types.
controllers/volume_controller.goAdopts common setter conventions.
docs/v2-resource-plan.mdDocuments the V2 architecture roadmap.
internal/apiserver/admin_http_handler_test.goUpdates shutdown tests.
internal/apiserver/apiserver.goRegisters V2 API resources.
internal/appmgmt/shutdown.goIncludes V2 resources in shutdown.
internal/containers/container_orchestrator.goDecouples orchestrator options from APIs.
internal/containers/containers_common.goAdds API-neutral container types.
internal/containers/image_layers.goUses shared build definitions.
internal/containers/image_layers_test.goUpdates image-layer tests.
internal/containers/image_orchestrator.goAdds image operation options.
internal/containers/test_container_orchestrator_client.goUpdates orchestrator test client.
internal/dcp/commands/cleanup_test.goCovers revised cleanup metadata.
internal/dcpclient/client.goAdds V2 namespace workload resolution.
internal/dcpclient/client_test.goTests V2 workload resolution.
internal/dcpclient/scheme.goRegisters the V2 scheme.
internal/dcpctrl/commands/run_controllers.goStarts the V2 controllers.
internal/dcpproc/dcpproc_test.goUpdates shared-type usage.
internal/dcptun/image_builder.goUses API-neutral image options.
internal/dcptun/tunnel_test.goUpdates shared-type usage.
internal/docker/cli_orchestrator.goImplements API-neutral Docker options.
internal/docker/cli_orchestrator_test.goTests revised Docker options.
internal/exerunners/ide_requests_responses.goUses shared environment types.
internal/exerunners/ide_requests_responses_test.goUpdates IDE protocol tests.
internal/exerunners/process_executable_runner.goUses shared environment types.
internal/networking/networking.goUses shared protocol definitions.
internal/networking/networking_test.goUpdates networking tests.
internal/podman/cli_orchestrator.goImplements API-neutral Podman options.
internal/podman/cli_orchestrator_test.goTests revised Podman options.
internal/proxy/netproxy.goUses shared protocol definitions.
internal/proxy/proxy_api.goUses shared protocol definitions.
internal/proxy/proxy_test.goUpdates proxy tests.
internal/resourcecleanup/resources.goAdds ordered V2 cleanup metadata.
internal/resourcecleanup/resources_test.goTests cleanup ordering.
internal/templating/value_templates.goUses shared container fragments.
internal/testutil/ctrlutil/test_container_orchestrator.goSupports V2 controller testing.
internal/testutil/ctrlutil/test_proxy.goUses shared protocols.
pkg/commonapi/annotations.goAdds common annotation definitions.
pkg/commonapi/annotations_test.goTests common annotations.
pkg/commonapi/container_deepcopy.goAdds shared deep-copy helpers.
pkg/commonapi/container_types.goDefines shared container API fragments.
pkg/commonapi/resource_creation.goShares shutdown creation state.
pkg/commonapi/types.goAdds common workload identifiers.
pkg/generated/openapi/zz_generated.openapi.goRegenerates V1/V2 OpenAPI schemas.
pkg/kubeconfig/kubeconfig.goIncludes V2 API configuration.
test/integration/advanced_test_env.goRegisters V2 test resources.
test/integration/container_controller_test.goMigrates shared container types.
test/integration/container_network_connection_test.goMigrates network types.
test/integration/container_network_tunnel_proxy_test.goMigrates tunnel test types.
test/integration/controllers_common_test.goAdds V2 controller test helpers.
test/integration/endpoints_test.goMigrates shared protocol types.
test/integration/executable_controller_test.goMigrates environment types.
test/integration/executable_replica_set_controller_test.goMigrates environment types.
test/integration/resource_harvesting_test.goIncludes V2 cleanup behavior.
test/integration/service_controller_test.goMigrates shared service types.
test/integration/service_controller_test_not_darwin.goMigrates platform service tests.
test/integration/standard_test_env.goStarts V2 reconcilers in tests.
test/integration/test_env_common.goAdds V2 controller selection flags.
test/integration/v2_namespace_controller_test.goTests namespace activation and cleanup.
test/integration/v2_physical_container_controller_test.goTests physical container lifecycle behavior.
test/integration/v2_physical_container_image_controller_test.goTests image pull and build behavior.
Files not reviewed (4)
  • api/v1/zz_generated.deepcopy.go: Generated file
  • api/v1/zz_generated.model_name.go: Generated file
  • api/v2/zz_generated.deepcopy.go: Generated file
  • api/v2/zz_generated.model_name.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadpkg/commonapi/container_types.go Outdated
Comment threadcontrollers/namespace_controller.go
Comment threadapi/v2/physical_container_types.go Outdated
Comment threadpkg/commonapi/container_types.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Copilot Code Review

Independent review of the code before reading the PR description, then reconciled against it. This is a review without approval — 19 findings below (5 ❌ error, 9 ⚠️ warning, 5 💡 suggestion), posted as inline comments.

I deliberately skipped the 4 findings already raised by the earlier automated review (containerPortEnd lifecycle key, namespace-finalization race, imageRef name validation, port protocol validation) to avoid duplicate feedback. Note that finding on api/v1/container_types.go:499 is a second, independent cause of the lifecycle-key regression that the existing containerPortEnd fix would not address.

Holistic assessment: ⚠️ Needs Changes

Motivation is sound: a namespaced V2 model with physical primitives gives DCP one place to implement runtime lifecycle instead of duplicating orchestrator calls across V1 controllers, and docs/v2-resource-plan.md lays out a credible roadmap.

Approach bundles two largely independent changes: (1) a mechanical move of shared container API fragments into pkg/commonapi plus a rework of the orchestrator CreateContainerOptions boundary, touching ~70 existing files; and (2) the genuinely new V2 API group + three controllers. The new controllers follow existing repo patterns closely (ReconcilerBase, ObjectStateMap, resiliency.WorkQueue, ContainerWatcher), which is good.

I'd suggest splitting this PR. The api/v1 -> pkg/commonapi move is self-contained and independently reviewable/revertable; separating it would make the V1 regression risk far easier to reason about, and would let the V2 foundation be evaluated on its own.

The blocking concerns are concentrated in the V1 blast radius and the steady-state cost of the new controllers, not in the V2 design itself:

  • The commonapi move silently changes the V1 persistent-container lifecycle key (two independent causes), causing existing persistent containers to be removed and recreated on upgrade.
  • Both new controllers requeue unconditionally every ~5-7s forever, each cycle shelling out to docker inspect.
  • V2-created containers carry no creator-process labels, so the harvester can never reclaim them after a crash — and removeOnDeletion defaults to false, so they also survive a clean shutdown.

Where a human reviewer should focus / where I'm less confident:

  • The lifecycle-key finding is the one I'd most want a second opinion on. I verified empirically that gob embeds struct name + field list in the hashed byte stream, and that container_controller.go:445 removes+recreates on mismatch — but whether the resulting churn is acceptable in practice is a product call.
  • The api/v1 source-break + OpenAPI $ref rename: I don't know whether anything outside this repo consumes api/v1 as a Go package or generates clients from the DCP OpenAPI document. Someone who owns the Aspire/DCP client contract should confirm.
  • The reconcile-frequency cost depends on typical container counts; I did not measure it.

Validation performed locally on this branch

CheckResult
go build ./api/... ./controllers/... ./pkg/... ./internal/resourcecleanup/...
make lint✅ 0 issues
make generate✅ no drift in generated files
go test ./api/... ./pkg/commonapi/... ./controllers/... ./internal/dcpclient/... ./internal/resourcecleanup/...
go test ./test/integration/... (full suite)

No go.mod changes, so no new external dependencies.

Highest-risk files for human attention

FileRisk categories
api/v1/container_types.go, api/v1/common_types.go, api/v1/groupversion_info.goPublic API
api/v2/*_types.go, pkg/commonapi/container_types.goPublic API
pkg/generated/openapi/zz_generated.openapi.goPublic API, Network Communications
controllers/physical_container{,_image}_controller.goController Code, Concurrency and Resiliency, Process Manipulation
controllers/namespace_controller.go, controllers/object_state_map.goController Code, Concurrency and Resiliency
controllers/container_controller.go, container_common.go, running_container_data.goController Code, Process Manipulation
internal/containers/*, internal/docker/cli_orchestrator.go, internal/podman/cli_orchestrator.goProcess Manipulation, Program Invocation
internal/dcpctrl/commands/run_controllers.go, internal/appmgmt/shutdown.go, internal/resourcecleanup/resources.goProgram Invocation, Controller Code
internal/apiserver/apiserver.go, internal/dcpclient/*Public API, Network Communications
pkg/kubeconfig/kubeconfig.goSecurity

Comment threadapi/v1/container_types.go Outdated
Comment threadcontrollers/physical_container_controller.go Outdated
Comment threadcontrollers/physical_container_image_controller.go Outdated
Comment threadcontrollers/physical_container_controller.go Outdated
Comment threadapi/v2/physical_container_types.go Outdated
Comment threadpkg/commonapi/container_types.go Outdated
Comment threadcontrollers/namespace_controller.go Outdated
Comment threadcontrollers/physical_container_controller.go Outdated
Comment threadcontrollers/physical_container_controller.go Outdated
Comment threadcontrollers/physical_container_controller.go
Comment threadcontrollers/controller_common.go
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Invert PhysicalContainer removeOnDeletion into preserveOnDeletion so the
default behavior is to remove the runtime container when the resource is
deleted, with opt-in preservation.
Also fold in the remaining review fixes:
- Preserve legacy V1 gob encodings when computing container lifecycle keys
so existing persistent containers are not recreated on upgrade, using the
new port encoding only when a port range is specified.
- Reject unsupported container port protocols during validation.
- Validate PhysicalContainer imageRef as a resource name, and bring V2
container name and label validation to parity with V1.
- Require an Active, finalized namespace before V2 child resources perform
side effects.
- Expand V1 port ranges in the integration test helper like production does.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Each API version now owns its own container shapes instead of sharing
versioned fragments through pkg/commonapi. api/v1 is restored to gob
parity with main so existing container lifecycle keys stay valid, and
api/v2 owns its own copies including the containerPortEnd range feature.
pkg/commonapi is reduced to trivially simple cross-cutting types
(EnvVar, Label, PortProtocol), exposed in api/v1 as type aliases that
preserve gob identity. The lifecycle_hash_types.go workaround is removed
and replaced by a golden lifecycle key regression test that catches type
name, field list, and encoder registration order changes.
Orchestrator-neutral container types live in the internal/containers
orchestrator files that use them, so internal/containers still imports
neither api/v1 nor api/v2 and controllers convert at the boundary.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
calculateAnnotationsSize is only used within pkg/commonapi, and
annotationsSizeInfo has no callers at all. Both were unexported in
api/v1 before the move, so promoting them to exported pkg/ API added
public surface without a consumer.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Add DCP creator and persistence labels when the V2 controller creates runtime containers so startup harvesting can remove abandoned non-preserved containers after a DCP crash. Keep adopted runtime containers unstamped and map the harvester persistence label to PreserveOnDeletion.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Add API server validation for V2 PhysicalContainer and PhysicalContainerImage creation so children cannot be admitted after their DCP namespace is missing or terminating. This keeps namespace cleanup from racing finalizer removal.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
The custom V2 resource storage registration dropped tilt's generic
subresource branch, so any future V2 type implementing
ObjectWithGenericSubResource would silently lose those endpoints.
Also make the API-server-only PhysicalContainer test apply the finalizer
and Active phase that the namespace controller normally applies, so it
passes the new namespace lifecycle admission gate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
The API server admission gate duplicated ensureNamespace in
controllers/controller_common.go, which already rejects children whose
namespace is missing, terminating, unfinalized, or not Active, and does
so before any runtime work happens. The gate only prevented an inert API
object from being created, at the cost of forking tilt's resource
registration path.
Keep the NamespaceFinalizer constant on the API type, which is a useful
cleanup independent of the gate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Both V2 controllers passed LongDelay to SaveChangesWithDelay, which sets
additionalReconciliationNeeded before the noChange early return, so every
object requeued every ~5-7s for the life of the process. Each cycle shelled
out to the container runtime to inspect a container or image that had not
changed.
Follow the V1 pattern instead and let steady states go quiet:
- Add a MonitoringDelay tier (30s +/- 5s) for resources in a steady state
where reconciliation only guards against missed runtime events.
- PhysicalContainer reconciles at MonitoringDelay while Running and at
StandardDelay otherwise. applyInspectedPhysicalContainerStatus requests
reconciliation only for running and transitional states; exited and dead
containers go quiet and rely on the container event watch.
- Terminal file copy and start failures no longer request reconciliation,
which previously spun with no possible progress.
- PhysicalContainerImage reconciles at StandardDelay. Pulls now retry with
bounded exponential backoff inside the queued operation rather than by
re-entering the pull path on every reconcile, and a recorded pull or build
failure is terminal. Transient reconciliation failures stay retryable so a
namespace read error cannot strand an image.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Picks up the cel-spec repository move from google/cel-spec to cel-expr/cel-spec.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
The pull retry budget was a fixed 30 seconds, which meant a misspelled or
unreachable image took that long to report failure. Shorten the default and
let callers override or disable retries per image.
Add PhysicalContainerImageSpec.PullRetryLimit: unset uses a default of 3
retries, and zero disables retries so the pull fails on the first error.
The backoff is also tightened to 1s initial, 5s maximum interval, and a 15s
overall budget, so the default worst case is about 7 seconds instead of 30.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Do not run create validation from V2 update validation so status updates can proceed during shutdown while spec immutability checks still reject spec changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
PhysicalContainerImage had no finalizer, so deletion removed the object
immediately and the reconcile triggered by the completing pull or build
returned at the NotFound branch before the in-memory state and its queued
deferred op were discarded. Both leaked for the process lifetime, and the
DeletionTimestamp branch was unreachable because tilt-apiserver only starts
a two-phase delete when finalizers are present.
Register a finalizer so deletion is observed, cancel the in-flight pull or
build instead of letting it run to completion against a deleted resource,
and discard the state. The image itself is left in the container runtime,
since it is a shared artifact that outlives the resource describing it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Namespace deletion previously logged cleanup errors and requeued without
surfacing anything in status, so a namespace stuck in Terminating gave no
indication of what it was waiting for or why.
The cleanup handlers now return the number of remaining resources instead of
a bool, and the CleanupComplete condition reports either the failing error or
the resource kinds still pending deletion.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
PortProtocol moved to pkg/commonapi but api/v1 kept TCP/UDP aliases, leaving
two spellings for the same constant with the alias in the majority of uses.
Dropping the aliases lets internal/proxy, internal/networking, and
pkg/kubeconfig stop importing api/v1 entirely; they referenced it only to
name a port protocol. Removing the local alias also lets deepcopy-gen
recognize ContainerPort and ServiceSpec as shallow-copyable.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
The golden container spec pointed Build.Dockerfile at the POSIX-absolute
literal "/src/Dockerfile.golden". GetLifecycleKey hashes the Dockerfile
contents when the file is readable and falls back to hashing the resolved
path otherwise, and that fallback is not portable: filepath.IsAbs requires
a volume name on Windows, so Windows joined the literal against the build
context and hashed "\src\src\Dockerfile.golden" while POSIX hashed the
literal unchanged. The test therefore failed on Windows only, with the same
actual key on every run.
Point the fixture at a real file under t.TempDir() instead. A temp path is
absolute on every platform, so the IsAbs branch is skipped and no filepath
function contributes to the key, and the read succeeds so the key covers the
Dockerfile contents rather than the error fallback. The constant is
recomputed because the fixture inputs deliberately changed.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
V2 has no log surface: AdditionalTypes is empty and no V2 resource exposes
a log subresource, so output from a V2 runtime container is unreachable
through the V2 API. The roadmap did not mention logs at all.
Record the gap, summarize the V1 subresource mechanism, and capture the open
question of whether log streams belong to physical resources, logical
resources, or both. Note the evidence that bears on it: the orchestrator
layer defines only stdout and stderr because that is what a runtime
produces, while the V1 API adds startup_stdout, startup_stderr, and system
as DCP-level concepts, and the image pull/build output that V1 surfaces as
startup_* belongs to PhysicalContainerImage in V2.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
The V2 types register unconditionally with the API server and carried no
stability notice, even though the roadmap plans a breaking change to
PhysicalContainer that replaces direct runtime network and volume names with
references to V2 resources. Record that V2 is unstable in the package doc so
the planned churn is not a surprise to callers.
ResolveNamespaceWorkloadID has no production caller: V2 has no state store or
lifecycle key, so nothing consumes a workload ID yet and the namespace
annotation is validated without affecting behavior. Add a TODO marking it as
groundwork for V2 persistent resources rather than leaving it to read as dead
code.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Make the golden lifecycle key test independent of the ambient environment.
GetLifecycleKey hashes os.Getenv(Source) for env type build secrets, so using
NPM_TOKEN as the fixture source made the key depend on whether that variable
happens to be set, failing the test on any machine that sets it. Point the
fixture at a name that is never set and recompute the constant. t.Setenv is not
usable here because the test is parallel.
Add Namespace.ValidateUpdate so annotation validation, including the workload ID
annotation, applies to updates and not just creates. It deliberately omits the
ResourceCreationProhibited gate, which is creation-only and would otherwise block
the namespace controller from writing cleanup status or removing its finalizer
during shutdown.
Also document three behaviors that are load bearing but not evident from the
code: why runtime side effects of namespaced V2 resources are gated in the
controllers rather than at admission, why CleanupAllResources must run before the
controller host shuts down, and that WorkQueue drops queued items once its
lifetime context is done.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Clear the golden fixture's environment-backed build secret explicitly instead
of relying on a variable name that is unlikely to be set. This requires the
test to run serially because testing.T.Setenv cannot be used by a parallel
test.
Also refine the ensureNamespace documentation to distinguish normal runtime
work from deletion cleanup and record the accepted race where an inert child
API record can outlive namespace cleanup until the in-memory server exits.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
@danegsta
David Negstad (danegsta) marked this pull request as ready for review July 31, 2026 21:12
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Comment threadcontrollers/physical_container_controller.go
Comment threadapi/v2/container_types.go Outdated
Comment threadMakefile
Comment threadapi/v1/lifecycle_key_golden_test.go Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074

CopilotAI left a comment

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.

Pull request overview

Copilot reviewed 102 out of 108 changed files in this pull request and generated 3 comments.

Files not reviewed (4)
  • api/v1/zz_generated.deepcopy.go: Generated file
  • api/v1/zz_generated.model_name.go: Generated file
  • api/v2/zz_generated.deepcopy.go: Generated file
  • api/v2/zz_generated.model_name.go: Generated file

Comment threadcontrollers/physical_container_image_controller.go
Comment threadcontrollers/physical_container_image_controller.go
Comment threadpkg/commonapi/doc.go Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bcfab76-f43d-4564-a714-303ce7779074
Sign up for freeto 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.

3 participants

@danegsta@karolz-ms