From 9e0aafd93f8a13c8263e8db8072511fad0527699 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 14:56:52 -0700 Subject: [PATCH 1/8] Add crash monitors for V2 physical resources Add lifecycle monitors for V2 containers, processes, networks, and removable volumes. Support explicit monitor targets for retained containers and processes, retain volumes by default, and bound in-use volume cleanup retries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- api/v2/common_types.go | 29 ++ api/v2/physical_container_types.go | 17 ++ api/v2/physical_container_types_test.go | 65 +++++ api/v2/physical_container_volume_types.go | 5 +- .../physical_container_volume_types_test.go | 2 +- api/v2/physical_process_types.go | 17 ++ api/v2/physical_process_types_test.go | 64 +++++ api/v2/zz_generated.deepcopy.go | 12 + controllers/physical_container_controller.go | 34 ++- .../physical_container_network_controller.go | 35 ++- .../physical_container_volume_controller.go | 39 ++- controllers/physical_process_controller.go | 25 +- .../physical_resource_invalid_state_test.go | 2 +- internal/dcpctrl/commands/run_controllers.go | 2 + .../commands/container_resource_test.go | 207 ++++++++++++++ internal/dcpproc/commands/network.go | 247 +++++++++++++++++ internal/dcpproc/commands/root.go | 14 +- internal/dcpproc/commands/volume.go | 255 ++++++++++++++++++ internal/dcpproc/dcpproc_api.go | 52 ++++ internal/dcpproc/dcpproc_api_test.go | 38 +++ pkg/generated/openapi/zz_generated.openapi.go | 34 ++- plan/v2-resource-plan.md | 14 +- test/integration/advanced_test_env.go | 2 + test/integration/standard_test_env.go | 2 + .../v2_physical_container_controller_test.go | 41 +++ ...sical_container_network_controller_test.go | 10 + ...sical_container_network_durability_test.go | 8 +- ...ysical_container_volume_controller_test.go | 76 ++++-- ...ysical_container_volume_durability_test.go | 17 +- .../v2_physical_process_controller_test.go | 35 ++- 30 files changed, 1311 insertions(+), 89 deletions(-) create mode 100644 internal/dcpproc/commands/container_resource_test.go create mode 100644 internal/dcpproc/commands/network.go create mode 100644 internal/dcpproc/commands/volume.go diff --git a/api/v2/common_types.go b/api/v2/common_types.go index 2beb286c..459df3b7 100644 --- a/api/v2/common_types.go +++ b/api/v2/common_types.go @@ -6,6 +6,7 @@ package v2 import ( + "math" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -100,6 +101,34 @@ func ValidateNamespacedResourceMetadata(obj metav1.Object) field.ErrorList { return errorList } +func validateRetainedResourceMonitor( + retainRuntimeResource bool, + monitorPID *int64, + monitorTimestamp metav1.MicroTime, + monitorPath *field.Path, +) field.ErrorList { + errorList := field.ErrorList{} + monitorTimestampSet := !monitorTimestamp.IsZero() + + if monitorPID != nil && (*monitorPID <= 0 || *monitorPID > math.MaxUint32) { + errorList = append(errorList, field.Invalid(monitorPath.Child("monitorPID"), *monitorPID, "monitorPID must be between 1 and 4294967295")) + } + if !retainRuntimeResource && monitorPID != nil { + errorList = append(errorList, field.Forbidden(monitorPath.Child("monitorPID"), "monitorPID can only be set for retained runtime resources")) + } + if !retainRuntimeResource && monitorTimestampSet { + errorList = append(errorList, field.Forbidden(monitorPath.Child("monitorTimestamp"), "monitorTimestamp can only be set for retained runtime resources")) + } + if monitorPID != nil && !monitorTimestampSet { + errorList = append(errorList, field.Required(monitorPath.Child("monitorTimestamp"), "monitorTimestamp must be set when monitorPID is set")) + } + if monitorPID == nil && monitorTimestampSet { + errorList = append(errorList, field.Required(monitorPath.Child("monitorPID"), "monitorPID must be set when monitorTimestamp is set")) + } + + return errorList +} + func validateSameNamespaceResourceReference( reference string, namespace string, diff --git a/api/v2/physical_container_types.go b/api/v2/physical_container_types.go index 477d8e2d..5ca7f033 100644 --- a/api/v2/physical_container_types.go +++ b/api/v2/physical_container_types.go @@ -177,6 +177,14 @@ type PhysicalContainerConfig struct { // RetainRuntimeContainer keeps a runtime container created by this resource in place when the resource is deleted. RetainRuntimeContainer bool `json:"retainRuntimeContainer,omitempty"` + // MonitorPID optionally scopes a retained runtime container to another process lifetime. + // When set, monitorTimestamp must also be set and retainRuntimeContainer must be true. + // The container is stopped but not removed when the monitored process exits. + MonitorPID *int64 `json:"monitorPID,omitempty"` + + // MonitorTimestamp identifies the process in monitorPID and guards against PID reuse. + MonitorTimestamp metav1.MicroTime `json:"monitorTimestamp,omitempty"` + // ImageRef identifies a PhysicalContainerImage in the same namespace using or /. // Cross-namespace references are not supported. ImageRef string `json:"imageRef,omitempty"` @@ -363,6 +371,15 @@ func (pc *PhysicalContainer) Validate(ctx context.Context) field.ErrorList { container := pc.Spec.Container containerPath := specPath.Child("container") + errorList = append( + errorList, + validateRetainedResourceMonitor( + container.RetainRuntimeContainer, + container.MonitorPID, + container.MonitorTimestamp, + containerPath, + )..., + ) errorList = append(errorList, validateSameNamespaceResourceReference(container.ImageRef, pc.Namespace, containerPath.Child("imageRef"))...) if container.ContainerName != "" && !validContainerNameRegexp.MatchString(container.ContainerName) { errorList = append(errorList, field.Invalid(containerPath.Child("containerName"), container.ContainerName, fmt.Sprintf("containerName must match regex '%s'", validContainerName))) diff --git a/api/v2/physical_container_types_test.go b/api/v2/physical_container_types_test.go index d8c2502a..a57c7bd0 100644 --- a/api/v2/physical_container_types_test.go +++ b/api/v2/physical_container_types_test.go @@ -8,6 +8,7 @@ package v2 import ( "context" "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -16,6 +17,9 @@ import ( ) func TestPhysicalContainerValidate(t *testing.T) { + monitorPID := int64(42) + invalidMonitorPID := int64(0) + monitorTimestamp := metav1.NewMicroTime(time.Now().UTC()) testCases := []struct { name string container PhysicalContainer @@ -31,6 +35,18 @@ func TestPhysicalContainerValidate(t *testing.T) { Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ImageRef: "test-image"}}, }, }, + { + name: "valid retained container monitor", + container: PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-container", Namespace: "test-namespace"}, + Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ + ImageRef: "test-image", + RetainRuntimeContainer: true, + MonitorPID: &monitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + }, { name: "valid existing container", container: PhysicalContainer{ @@ -214,6 +230,55 @@ func TestPhysicalContainerValidate(t *testing.T) { }, expectedError: "spec.container.containerName", }, + { + name: "monitor requires retained container", + container: PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-container", Namespace: "test-namespace"}, + Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ + ImageRef: "test-image", + MonitorPID: &monitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.container.monitorPID", + }, + { + name: "monitor pid requires timestamp", + container: PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-container", Namespace: "test-namespace"}, + Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ + ImageRef: "test-image", + RetainRuntimeContainer: true, + MonitorPID: &monitorPID, + }}, + }, + expectedError: "spec.container.monitorTimestamp", + }, + { + name: "monitor timestamp requires pid", + container: PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-container", Namespace: "test-namespace"}, + Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ + ImageRef: "test-image", + RetainRuntimeContainer: true, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.container.monitorPID", + }, + { + name: "monitor pid must be valid", + container: PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-container", Namespace: "test-namespace"}, + Spec: PhysicalContainerSpec{Container: &PhysicalContainerConfig{ + ImageRef: "test-image", + RetainRuntimeContainer: true, + MonitorPID: &invalidMonitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.container.monitorPID", + }, { name: "invalid container port range size", container: PhysicalContainer{ diff --git a/api/v2/physical_container_volume_types.go b/api/v2/physical_container_volume_types.go index d040b47b..f839a2c4 100644 --- a/api/v2/physical_container_volume_types.go +++ b/api/v2/physical_container_volume_types.go @@ -98,8 +98,9 @@ type PhysicalContainerVolumeConfig struct { // VolumeName is the runtime name to use when creating a new volume. VolumeName string `json:"volumeName,omitempty"` - // RetainRuntimeVolume keeps the created runtime volume in place when this resource is deleted. - RetainRuntimeVolume bool `json:"retainRuntimeVolume,omitempty"` + // RemoveRuntimeVolumeOnDelete removes the created runtime volume when this resource is deleted. + // Created runtime volumes are retained by default. + RemoveRuntimeVolumeOnDelete bool `json:"removeRuntimeVolumeOnDelete,omitempty"` // ReplaceExisting removes an existing runtime volume with volumeName before creating a new one. // Replacement retries non-forced removal while the existing volume is in use and never removes attached containers. diff --git a/api/v2/physical_container_volume_types_test.go b/api/v2/physical_container_volume_types_test.go index f4f8c627..18efda0b 100644 --- a/api/v2/physical_container_volume_types_test.go +++ b/api/v2/physical_container_volume_types_test.go @@ -156,7 +156,7 @@ func TestPhysicalContainerVolumeValidateUpdateRejectsSpecChanges(t *testing.T) { }, } newVolume := oldVolume.DeepCopy() - newVolume.Spec.Volume.RetainRuntimeVolume = true + newVolume.Spec.Volume.RemoveRuntimeVolumeOnDelete = true errorList := newVolume.ValidateUpdate(context.Background(), oldVolume) diff --git a/api/v2/physical_process_types.go b/api/v2/physical_process_types.go index fa71d042..94292f35 100644 --- a/api/v2/physical_process_types.go +++ b/api/v2/physical_process_types.go @@ -97,6 +97,14 @@ type PhysicalProcessConfig struct { // RetainRuntimeProcess keeps a process launched by this resource running when the resource is deleted. RetainRuntimeProcess bool `json:"retainRuntimeProcess,omitempty"` + // MonitorPID optionally scopes a retained runtime process to another process lifetime. + // When set, monitorTimestamp must also be set and retainRuntimeProcess must be true. + // The retained process is stopped when the monitored process exits. + MonitorPID *int64 `json:"monitorPID,omitempty"` + + // MonitorTimestamp identifies the process in monitorPID and guards against PID reuse. + MonitorTimestamp metav1.MicroTime `json:"monitorTimestamp,omitempty"` + // ExecutablePath is the executable path or name to launch. ExecutablePath string `json:"executablePath"` @@ -230,6 +238,15 @@ func (pp *PhysicalProcess) Validate(ctx context.Context) field.ErrorList { processConfig := pp.Spec.Process processPath := specPath.Child("process") + errorList = append( + errorList, + validateRetainedResourceMonitor( + processConfig.RetainRuntimeProcess, + processConfig.MonitorPID, + processConfig.MonitorTimestamp, + processPath, + )..., + ) if strings.TrimSpace(processConfig.ExecutablePath) == "" { errorList = append(errorList, field.Required(processPath.Child("executablePath"), "executablePath must be set")) } diff --git a/api/v2/physical_process_types_test.go b/api/v2/physical_process_types_test.go index ce170c7d..5795371e 100644 --- a/api/v2/physical_process_types_test.go +++ b/api/v2/physical_process_types_test.go @@ -9,6 +9,7 @@ import ( "context" "math" "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -18,6 +19,8 @@ func TestPhysicalProcessValidate(t *testing.T) { validPID := int64(42) zeroPID := int64(0) largePID := int64(math.MaxUint32) + 1 + monitorPID := int64(43) + monitorTimestamp := metav1.NewMicroTime(time.Now().UTC()) testCases := []struct { name string process PhysicalProcess @@ -43,6 +46,18 @@ func TestPhysicalProcessValidate(t *testing.T) { Spec: PhysicalProcessSpec{PID: &validPID}, }, }, + { + name: "valid retained process monitor", + process: PhysicalProcess{ + ObjectMeta: metav1.ObjectMeta{Name: "test-process", Namespace: "test-namespace"}, + Spec: PhysicalProcessSpec{Process: &PhysicalProcessConfig{ + ExecutablePath: "test-command", + RetainRuntimeProcess: true, + MonitorPID: &monitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + }, { name: "missing namespace", process: PhysicalProcess{ @@ -101,6 +116,55 @@ func TestPhysicalProcessValidate(t *testing.T) { }, expectedError: "spec.process.executablePath", }, + { + name: "monitor requires retained process", + process: PhysicalProcess{ + ObjectMeta: metav1.ObjectMeta{Name: "test-process", Namespace: "test-namespace"}, + Spec: PhysicalProcessSpec{Process: &PhysicalProcessConfig{ + ExecutablePath: "test-command", + MonitorPID: &monitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.process.monitorPID", + }, + { + name: "monitor pid requires timestamp", + process: PhysicalProcess{ + ObjectMeta: metav1.ObjectMeta{Name: "test-process", Namespace: "test-namespace"}, + Spec: PhysicalProcessSpec{Process: &PhysicalProcessConfig{ + ExecutablePath: "test-command", + RetainRuntimeProcess: true, + MonitorPID: &monitorPID, + }}, + }, + expectedError: "spec.process.monitorTimestamp", + }, + { + name: "monitor timestamp requires pid", + process: PhysicalProcess{ + ObjectMeta: metav1.ObjectMeta{Name: "test-process", Namespace: "test-namespace"}, + Spec: PhysicalProcessSpec{Process: &PhysicalProcessConfig{ + ExecutablePath: "test-command", + RetainRuntimeProcess: true, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.process.monitorPID", + }, + { + name: "monitor pid must be valid", + process: PhysicalProcess{ + ObjectMeta: metav1.ObjectMeta{Name: "test-process", Namespace: "test-namespace"}, + Spec: PhysicalProcessSpec{Process: &PhysicalProcessConfig{ + ExecutablePath: "test-command", + RetainRuntimeProcess: true, + MonitorPID: &zeroPID, + MonitorTimestamp: monitorTimestamp, + }}, + }, + expectedError: "spec.process.monitorPID", + }, } for _, testCase := range testCases { diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index 72d8e1f4..c36cb4b7 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -295,6 +295,12 @@ func (in *PhysicalContainer) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PhysicalContainerConfig) DeepCopyInto(out *PhysicalContainerConfig) { *out = *in + if in.MonitorPID != nil { + in, out := &in.MonitorPID, &out.MonitorPID + *out = new(int64) + **out = **in + } + in.MonitorTimestamp.DeepCopyInto(&out.MonitorTimestamp) if in.Command != nil { in, out := &in.Command, &out.Command *out = make([]string, len(*in)) @@ -946,6 +952,12 @@ func (in *PhysicalProcess) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PhysicalProcessConfig) DeepCopyInto(out *PhysicalProcessConfig) { *out = *in + if in.MonitorPID != nil { + in, out := &in.MonitorPID, &out.MonitorPID + *out = new(int64) + **out = **in + } + in.MonitorTimestamp.DeepCopyInto(&out.MonitorTimestamp) if in.Args != nil { in, out := &in.Args, &out.Args *out = make([]string, len(*in)) diff --git a/controllers/physical_container_controller.go b/controllers/physical_container_controller.go index d2af86c9..daa8a19e 100644 --- a/controllers/physical_container_controller.go +++ b/controllers/physical_container_controller.go @@ -974,21 +974,43 @@ func (r *PhysicalContainerReconciler) createPhysicalContainer( r.queuePhysicalContainerDataResult(container, stateKey, data) } -// Starts a container monitor process that removes the runtime container if this DCP instance terminates unexpectedly. -// Containers the resource does not own past its own lifetime (RetainRuntimeContainer) are left alone. +// Starts the monitor process selected by the runtime container's retention policy. // Failures are logged but not surfaced, because the monitor is a best-effort reliability enhancement; // the container harvester reclaims orphaned containers in a later session. func (r *PhysicalContainerReconciler) runPhysicalContainerLifecycleMonitor(container *apiv2.PhysicalContainer, containerID string, log logr.Logger) { - if container.Spec.Container == nil || container.Spec.Container.RetainRuntimeContainer || containerID == "" { + containerConfig := container.Spec.Container + if containerConfig == nil || containerID == "" { return } - if r.processExecutor == nil { - log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainer cleanup monitor") + if !containerConfig.RetainRuntimeContainer { + if r.processExecutor == nil { + log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainer cleanup monitor") + return + } + dcpproc.RunContainerWatcher(r.processExecutor, containerID, log) return } - dcpproc.RunContainerWatcher(r.processExecutor, containerID, log) + monitor, found, monitorErr := dcpproc.MonitorTargetFromFields(containerConfig.MonitorPID, containerConfig.MonitorTimestamp) + if monitorErr != nil { + log.Error(monitorErr, "Could not start retained PhysicalContainer lifecycle monitor") + return + } + if !found { + return + } + if r.processExecutor == nil { + log.Error(errors.New("process executor is not configured"), "Could not start retained PhysicalContainer lifecycle monitor") + return + } + dcpproc.RunContainerWatcherForMonitorWithOptions( + r.processExecutor, + monitor, + containerID, + dcpproc.ContainerWatcherOptions{StopOnly: true}, + log, + ) } func (r *PhysicalContainerReconciler) removePhysicalContainerForReplacement(ctx context.Context, containerName string, log logr.Logger) error { diff --git a/controllers/physical_container_network_controller.go b/controllers/physical_container_network_controller.go index 4e67baac..4a57fe40 100644 --- a/controllers/physical_container_network_controller.go +++ b/controllers/physical_container_network_controller.go @@ -26,7 +26,9 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/internal/containers" + "github.com/microsoft/dcp/internal/dcpproc" "github.com/microsoft/dcp/pkg/commonapi" + "github.com/microsoft/dcp/pkg/process" "github.com/microsoft/dcp/pkg/resiliency" ) @@ -60,9 +62,10 @@ type physicalContainerNetworkDataInitializerFunc = stateInitializerFunc[ type PhysicalContainerNetworkReconciler struct { *ReconcilerBase[apiv2.PhysicalContainerNetwork, *apiv2.PhysicalContainerNetwork] - orchestrator containers.NetworkAttachmentOrchestrator - networkData *ObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork] - operationQueue *resiliency.WorkQueue + orchestrator containers.NetworkAttachmentOrchestrator + processExecutor process.Executor + networkData *ObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork] + operationQueue *resiliency.WorkQueue } func NewPhysicalContainerNetworkReconciler( @@ -71,12 +74,14 @@ func NewPhysicalContainerNetworkReconciler( noCacheClient ctrl_client.Reader, log logr.Logger, orchestrator containers.NetworkAttachmentOrchestrator, + processExecutor process.Executor, ) *PhysicalContainerNetworkReconciler { return &PhysicalContainerNetworkReconciler{ - ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerNetwork](client, noCacheClient, log, lifetimeCtx), - orchestrator: orchestrator, - networkData: NewObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork](), - operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), + ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerNetwork](client, noCacheClient, log, lifetimeCtx), + orchestrator: orchestrator, + processExecutor: processExecutor, + networkData: NewObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork](), + operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), } } @@ -615,9 +620,25 @@ func handlePhysicalContainerNetworkCreated( networkID := data.networkID log.V(1).Info("Runtime network created; saving network status", "NetworkID", networkID) + reconciler.runPhysicalContainerNetworkLifecycleMonitor(network, networkID, log) return reconciler.applyRuntimeNetworkStatus(ctx, network, data, networkID, log) } +func (r *PhysicalContainerNetworkReconciler) runPhysicalContainerNetworkLifecycleMonitor( + network *apiv2.PhysicalContainerNetwork, + networkID string, + log logr.Logger, +) { + if network.Spec.Network == nil || network.Spec.Network.RetainRuntimeNetwork || networkID == "" { + return + } + if r.processExecutor == nil { + log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainerNetwork cleanup monitor") + return + } + dcpproc.RunNetworkWatcher(r.processExecutor, networkID, log) +} + func handlePhysicalContainerNetworkCreateFailure( ctx context.Context, reconciler *PhysicalContainerNetworkReconciler, diff --git a/controllers/physical_container_volume_controller.go b/controllers/physical_container_volume_controller.go index b14766f6..7eaa7e10 100644 --- a/controllers/physical_container_volume_controller.go +++ b/controllers/physical_container_volume_controller.go @@ -24,6 +24,8 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/internal/containers" + "github.com/microsoft/dcp/internal/dcpproc" + "github.com/microsoft/dcp/pkg/process" "github.com/microsoft/dcp/pkg/resiliency" ) @@ -54,9 +56,10 @@ type physicalContainerVolumeDataInitializerFunc = stateInitializerFunc[ type PhysicalContainerVolumeReconciler struct { *ReconcilerBase[apiv2.PhysicalContainerVolume, *apiv2.PhysicalContainerVolume] - orchestrator containers.VolumeOrchestrator - volumeData *ObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume] - operationQueue *resiliency.WorkQueue + orchestrator containers.VolumeOrchestrator + processExecutor process.Executor + volumeData *ObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume] + operationQueue *resiliency.WorkQueue } func NewPhysicalContainerVolumeReconciler( @@ -65,12 +68,14 @@ func NewPhysicalContainerVolumeReconciler( noCacheClient ctrl_client.Reader, log logr.Logger, orchestrator containers.VolumeOrchestrator, + processExecutor process.Executor, ) *PhysicalContainerVolumeReconciler { return &PhysicalContainerVolumeReconciler{ - ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerVolume](client, noCacheClient, log, lifetimeCtx), - orchestrator: orchestrator, - volumeData: NewObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume](), - operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), + ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerVolume](client, noCacheClient, log, lifetimeCtx), + orchestrator: orchestrator, + processExecutor: processExecutor, + volumeData: NewObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume](), + operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), } } @@ -488,9 +493,25 @@ func handlePhysicalContainerVolumeCreated( } log.V(1).Info("Runtime volume created; saving volume status", "VolumeID", data.volumeID) + reconciler.runPhysicalContainerVolumeLifecycleMonitor(volume, data.volumeID, log) return reconciler.applyRuntimeVolumeStatus(ctx, volume, data, data.volumeID, log) } +func (r *PhysicalContainerVolumeReconciler) runPhysicalContainerVolumeLifecycleMonitor( + volume *apiv2.PhysicalContainerVolume, + volumeID string, + log logr.Logger, +) { + if volume.Spec.Volume == nil || !volume.Spec.Volume.RemoveRuntimeVolumeOnDelete || volumeID == "" { + return + } + if r.processExecutor == nil { + log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainerVolume cleanup monitor") + return + } + dcpproc.RunVolumeWatcher(r.processExecutor, volumeID, log) +} + func handlePhysicalContainerVolumeCreateFailure( ctx context.Context, reconciler *PhysicalContainerVolumeReconciler, @@ -611,7 +632,7 @@ func (r *PhysicalContainerVolumeReconciler) beginPhysicalContainerVolumeRemoval( log logr.Logger, ) objectChange { volumeConfig := volume.Spec.Volume - if volumeConfig == nil || volumeConfig.RetainRuntimeVolume { + if volumeConfig == nil || !volumeConfig.RemoveRuntimeVolumeOnDelete { r.volumeData.DeleteByNamespacedName(volume.NamespacedName()) return deleteFinalizer(volume, physicalContainerVolumeFinalizer, log) } @@ -972,7 +993,7 @@ func physicalContainerVolumeCreationLabels(volume *apiv2.PhysicalContainerVolume volumeConfig := volume.Spec.Volume creationLabels := physicalResourceCreationLabels( volumeConfig.Labels, - volumeConfig.RetainRuntimeVolume, + !volumeConfig.RemoveRuntimeVolumeOnDelete, volume.UID, log, ) diff --git a/controllers/physical_process_controller.go b/controllers/physical_process_controller.go index cbc2ce72..d327a6dd 100644 --- a/controllers/physical_process_controller.go +++ b/controllers/physical_process_controller.go @@ -604,9 +604,7 @@ func (r *PhysicalProcessReconciler) launchPhysicalProcess( data.progress = physicalResourceProgressRunning data.failureMessage = "" data.retryAfter = time.Time{} - if !processConfig.RetainRuntimeProcess { - dcpproc.RunProcessWatcher(r.processExecutor, handle, log) - } + r.runPhysicalProcessLifecycleMonitor(processConfig, handle, log) r.queuePhysicalProcessDataResult(physicalProcess, stateKey, data) if startWaitForExit != nil { startWaitForExit() @@ -614,6 +612,27 @@ func (r *PhysicalProcessReconciler) launchPhysicalProcess( log.V(1).Info("Physical process launched", "PID", handle.Pid, "ExecutablePath", processConfig.ExecutablePath) } +func (r *PhysicalProcessReconciler) runPhysicalProcessLifecycleMonitor( + processConfig *apiv2.PhysicalProcessConfig, + handle process.ProcessHandle, + log logr.Logger, +) { + if !processConfig.RetainRuntimeProcess { + dcpproc.RunProcessWatcher(r.processExecutor, handle, log) + return + } + + monitor, found, monitorErr := dcpproc.MonitorTargetFromFields(processConfig.MonitorPID, processConfig.MonitorTimestamp) + if monitorErr != nil { + log.Error(monitorErr, "Could not start retained PhysicalProcess lifecycle monitor") + return + } + if !found { + return + } + dcpproc.RunProcessWatcherForMonitor(r.processExecutor, monitor, handle, log) +} + func (r *PhysicalProcessReconciler) queuePhysicalProcessDataResult( physicalProcess *apiv2.PhysicalProcess, stateKey physicalProcessDataStateKey, diff --git a/controllers/physical_resource_invalid_state_test.go b/controllers/physical_resource_invalid_state_test.go index 16f18240..91be9f73 100644 --- a/controllers/physical_resource_invalid_state_test.go +++ b/controllers/physical_resource_invalid_state_test.go @@ -165,7 +165,7 @@ func TestInvalidPhysicalContainerVolumeStillHandlesDeletion(t *testing.T) { DeletionTimestamp: &now, }, Spec: apiv2.PhysicalContainerVolumeSpec{ - Volume: &apiv2.PhysicalContainerVolumeConfig{RetainRuntimeVolume: true}, + Volume: &apiv2.PhysicalContainerVolumeConfig{}, }, } data := &physicalContainerVolumeData{ diff --git a/internal/dcpctrl/commands/run_controllers.go b/internal/dcpctrl/commands/run_controllers.go index 95eacce3..93030ca7 100644 --- a/internal/dcpctrl/commands/run_controllers.go +++ b/internal/dcpctrl/commands/run_controllers.go @@ -293,6 +293,7 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), containerOrchestrator, + processExecutor, ) if err = physicalContainerNetworkCtrl.SetupWithManager(mgr, defaultControllerName); err != nil { log.Error(err, "Unable to set up PhysicalContainerNetwork controller") @@ -305,6 +306,7 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), containerOrchestrator, + processExecutor, ) if err = physicalContainerVolumeCtrl.SetupWithManager(mgr, defaultControllerName); err != nil { log.Error(err, "Unable to set up PhysicalContainerVolume controller") diff --git a/internal/dcpproc/commands/container_resource_test.go b/internal/dcpproc/commands/container_resource_test.go new file mode 100644 index 00000000..e5d67472 --- /dev/null +++ b/internal/dcpproc/commands/container_resource_test.go @@ -0,0 +1,207 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/cenkalti/backoff/v4" + "github.com/stretchr/testify/require" + + "github.com/microsoft/dcp/internal/containers" + ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" + "github.com/microsoft/dcp/pkg/testutil" +) + +func TestCleanupNetworkDisconnectsContainersWithoutRemovingThem(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + + log := testutil.NewLogForTesting(t.Name()) + orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) + require.NoError(t, orchestratorErr) + defer func() { + require.NoError(t, orchestrator.Close()) + }() + + createdNetworkID, createNetworkErr := orchestrator.CreateNetwork(ctx, containers.CreateNetworkOptions{ + Name: "cleanup-network", + }) + require.NoError(t, createNetworkErr) + createdContainerID, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ + Name: "cleanup-network-container", + Image: "cleanup-network-image", + Networks: []containers.CreateContainerNetworkOptions{{Name: createdNetworkID}}, + }) + require.NoError(t, createContainerErr) + + require.NoError(t, doCleanupNetwork(ctx, createdNetworkID, log, orchestrator)) + + _, inspectNetworkErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{createdNetworkID}, + }) + require.ErrorIs(t, inspectNetworkErr, containers.ErrNotFound) + inspectedContainers, inspectContainerErr := orchestrator.InspectContainers(ctx, containers.InspectContainersOptions{ + Containers: []string{createdContainerID}, + }) + require.NoError(t, inspectContainerErr) + require.Len(t, inspectedContainers, 1) +} + +func TestCleanupVolumeDoesNotForceRemoval(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + + log := testutil.NewLogForTesting(t.Name()) + orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) + require.NoError(t, orchestratorErr) + defer func() { + require.NoError(t, orchestrator.Close()) + }() + + const createdVolumeID = "cleanup-volume" + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + createdContainerID, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ + Name: "cleanup-volume-container", + Image: "cleanup-volume-image", + VolumeMounts: []containers.CreateContainerVolumeMount{{ + Type: containers.NamedVolumeMount, + Source: createdVolumeID, + Target: "/data", + }}, + }) + require.NoError(t, createContainerErr) + + cleanupErr := doCleanupVolume(ctx, createdVolumeID, orchestrator) + require.Error(t, cleanupErr) + + inspectedVolumes, inspectVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{createdVolumeID}, + }) + require.NoError(t, inspectVolumeErr) + require.Len(t, inspectedVolumes, 1) + _, removeContainerErr := orchestrator.RemoveContainers(ctx, containers.RemoveContainersOptions{ + Containers: []string{createdContainerID}, + Force: true, + }) + require.NoError(t, removeContainerErr) + require.NoError(t, doCleanupVolume(ctx, createdVolumeID, orchestrator)) + _, inspectRemovedVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{createdVolumeID}, + }) + require.True(t, errors.Is(inspectRemovedVolumeErr, containers.ErrNotFound)) +} + +func TestCleanupVolumeWaitsForContainerCleanup(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + + log := testutil.NewLogForTesting(t.Name()) + orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) + require.NoError(t, orchestratorErr) + defer func() { + require.NoError(t, orchestrator.Close()) + }() + + const ( + createdVolumeID = "cleanup-volume-after-container" + createdContainerID = "cleanup-volume-after-container-container" + ) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ + Name: createdContainerID, + Image: "cleanup-volume-after-container-image", + VolumeMounts: []containers.CreateContainerVolumeMount{{ + Type: containers.NamedVolumeMount, + Source: createdVolumeID, + Target: "/data", + }}, + }) + require.NoError(t, createContainerErr) + + orderedOrchestrator := &removeContainerAfterVolumeAttemptOrchestrator{ + TestContainerOrchestrator: orchestrator, + containerID: createdContainerID, + } + require.NoError(t, cleanupVolumeAfterMonitorExit( + ctx, + createdVolumeID, + backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), + log, + orderedOrchestrator, + )) + require.Equal(t, 2, orchestrator.RemoveVolumeCallCount(createdVolumeID)) + + _, inspectRemovedVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{createdVolumeID}, + }) + require.ErrorIs(t, inspectRemovedVolumeErr, containers.ErrNotFound) +} + +func TestCleanupVolumeStopsRetryingWhileContainerRemains(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + + log := testutil.NewLogForTesting(t.Name()) + orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) + require.NoError(t, orchestratorErr) + defer func() { + require.NoError(t, orchestrator.Close()) + }() + + const createdVolumeID = "bounded-cleanup-volume" + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ + Name: "bounded-cleanup-volume-container", + Image: "bounded-cleanup-volume-image", + VolumeMounts: []containers.CreateContainerVolumeMount{{ + Type: containers.NamedVolumeMount, + Source: createdVolumeID, + Target: "/data", + }}, + }) + require.NoError(t, createContainerErr) + + cleanupErr := cleanupVolumeAfterMonitorExit( + ctx, + createdVolumeID, + backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), + log, + orchestrator, + ) + require.ErrorIs(t, cleanupErr, containers.ErrObjectInUse) + require.Equal(t, 2, orchestrator.RemoveVolumeCallCount(createdVolumeID)) +} + +type removeContainerAfterVolumeAttemptOrchestrator struct { + *ctrl_testutil.TestContainerOrchestrator + containerID string +} + +func (orchestrator *removeContainerAfterVolumeAttemptOrchestrator) RemoveVolumes( + ctx context.Context, + options containers.RemoveVolumesOptions, +) ([]string, error) { + removedVolumes, removeVolumeErr := orchestrator.TestContainerOrchestrator.RemoveVolumes(ctx, options) + if errors.Is(removeVolumeErr, containers.ErrObjectInUse) { + _, removeContainerErr := orchestrator.TestContainerOrchestrator.RemoveContainers( + ctx, + containers.RemoveContainersOptions{ + Containers: []string{orchestrator.containerID}, + Force: true, + }, + ) + return removedVolumes, errors.Join(removeVolumeErr, removeContainerErr) + } + return removedVolumes, removeVolumeErr +} diff --git a/internal/dcpproc/commands/network.go b/internal/dcpproc/commands/network.go new file mode 100644 index 00000000..2dad72c9 --- /dev/null +++ b/internal/dcpproc/commands/network.go @@ -0,0 +1,247 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "context" + "errors" + "fmt" + "math/rand" + "time" + + "github.com/go-logr/logr" + "github.com/spf13/cobra" + + cmds "github.com/microsoft/dcp/internal/commands" + "github.com/microsoft/dcp/internal/containers" + container_flags "github.com/microsoft/dcp/internal/containers/flags" + container_runtimes "github.com/microsoft/dcp/internal/containers/runtimes" + "github.com/microsoft/dcp/pkg/logger" + "github.com/microsoft/dcp/pkg/process" +) + +const defaultNetworkPollInterval = 30 * time.Second + +var ( + networkID string + networkPollInterval time.Duration +) + +func NewNetworkCommand(log logr.Logger) (*cobra.Command, error) { + networkCmd := &cobra.Command{ + Use: "monitor-container-network", + Short: "Ensures that a container network is removed when the monitored process exits", + Long: `Ensures that a container network is removed when the monitored process exits. + +This command is used to ensure that container networks are properly cleaned up when +DCP terminates unexpectedly. Attached containers are disconnected without being removed.`, + RunE: monitorNetwork(log), + SilenceUsage: true, + Args: cobra.NoArgs, + } + + flagErr := addMonitorFlags(networkCmd) + if flagErr != nil { + return nil, flagErr + } + + networkCmd.Flags().StringVar(&networkID, "networkID", "", "The network ID or name to monitor and clean up when DCP exits") + flagErr = networkCmd.MarkFlagRequired("networkID") + if flagErr != nil { + return nil, flagErr + } + + networkCmd.Flags().DurationVar( + &networkPollInterval, + "networkPollInterval", + defaultNetworkPollInterval, + "How often to poll the network status to check if it has been removed. Default is 30 seconds.", + ) + flagErr = networkCmd.Flags().MarkHidden("networkPollInterval") + if flagErr != nil { + return nil, flagErr + } + + container_flags.EnsureRuntimeFlag(networkCmd.Flags()) + + return networkCmd, nil +} + +func monitorNetwork(log logr.Logger) func(cmd *cobra.Command, _ []string) error { + return func(cmd *cobra.Command, _ []string) error { + if networkID == "" { + return errors.New("network ID or name must be specified with --networkID") + } + + log = log.WithName("ContainerNetworkMonitor"). + WithValues( + "MonitorPID", monitorPid, + "Network", networkID, + ) + if resourceId != "" { + log = log.WithValues(logger.RESOURCE_LOG_STREAM_ID, resourceId) + } + + processExecutor := process.NewOSExecutor(log.WithName("ProcessExecutor")) + defer processExecutor.Dispose() + orchestrator, orchestratorErr := container_runtimes.FindAvailableContainerRuntime( + cmd.Context(), + log.WithName("ContainerOrchestrator").WithValues("ContainerRuntime", container_flags.GetRuntimeFlagValue()), + processExecutor, + ) + if orchestratorErr != nil { + log.Error(orchestratorErr, "Unable to ensure container network cleanup") + return orchestratorErr + } + + monitorCtx, monitorCtxCancel, monitorCtxErr := cmds.MonitorPid( + cmd.Context(), + process.NewHandle(monitorPid, monitorProcessStartTime), + monitorInterval, + log, + ) + defer monitorCtxCancel() + if monitorCtxErr != nil { + if isMonitorProcessGoneErr(monitorCtxErr) { + log.Info("Monitored process already exited, cleaning up container network", "Reason", monitorCtxErr) + return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) + } + + log.Error(monitorCtxErr, "Process could not be monitored") + return monitorCtxErr + } + + networkRemovedCh := pollNetworkRemoved(monitorCtx, networkID, orchestrator, log) + select { + case <-networkRemovedCh: + return nil + case <-monitorCtx.Done(): + log.Info("Monitored process exited, cleaning up container network") + return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) + } + } +} + +func doCleanupNetwork( + ctx context.Context, + networkID string, + log logr.Logger, + orchestrator containers.NetworkAttachmentOrchestrator, +) error { + inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{networkID}, + }) + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + if errors.Is(inspectErr, containers.ErrNotFound) { + return nil + } + return fmt.Errorf("inspect container network before removal: %w", inspectErr) + } + if len(inspectedNetworks) == 0 { + return nil + } + + network := inspectedNetworks[0] + if orchestrator.IsBuiltInNetwork(network.Name) { + log.Info("Skipping cleanup of built-in container network", "NetworkName", network.Name) + return nil + } + + listedContainers, listErr := orchestrator.ListContainers(ctx, containers.ListContainersOptions{ + All: true, + Filters: containers.ListContainersFilters{ + NetworkFilters: []string{network.Id}, + }, + }) + if listErr != nil { + _, confirmErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{networkID}, + }) + if errors.Is(confirmErr, containers.ErrNotFound) { + return nil + } + return fmt.Errorf("list containers attached to container network: %w", errors.Join(listErr, confirmErr)) + } + + attachedContainerIDs := make(map[string]struct{}, len(network.Containers)+len(listedContainers)) + for _, attachedContainer := range network.Containers { + attachedContainerIDs[attachedContainer.Id] = struct{}{} + } + for _, listedContainer := range listedContainers { + attachedContainerIDs[listedContainer.Id] = struct{}{} + } + + var disconnectErrors error + for containerID := range attachedContainerIDs { + disconnectErr := orchestrator.DisconnectNetwork(ctx, containers.DisconnectNetworkOptions{ + Network: network.Id, + Container: containerID, + Force: true, + }) + if disconnectErr != nil && !errors.Is(disconnectErr, containers.ErrNotFound) { + disconnectErrors = errors.Join(disconnectErrors, disconnectErr) + } + } + if disconnectErrors != nil { + return fmt.Errorf("disconnect all containers from container network: %w", disconnectErrors) + } + + _, removeErr := orchestrator.RemoveNetworks(ctx, containers.RemoveNetworksOptions{ + Networks: []string{networkID}, + }) + if removeErr == nil { + return nil + } + + _, confirmErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{networkID}, + }) + if errors.Is(confirmErr, containers.ErrNotFound) { + return nil + } + return fmt.Errorf("remove container network: %w", errors.Join(removeErr, confirmErr)) +} + +func pollNetworkRemoved( + ctx context.Context, + networkID string, + orchestrator containers.InspectNetworks, + log logr.Logger, +) <-chan struct{} { + networkRemovedCh := make(chan struct{}) + go func() { + defer close(networkRemovedCh) + timer := time.NewTimer(containerResourcePollDelay(networkPollInterval)) + defer timer.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-timer.C: + inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{networkID}, + }) + if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedNetworks) == 0) { + return + } + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + log.Error(inspectErr, "Failed to inspect container network") + } + timer.Reset(containerResourcePollDelay(networkPollInterval)) + } + } + }() + return networkRemovedCh +} + +func containerResourcePollDelay(pollInterval time.Duration) time.Duration { + jitterRange := pollInterval / 20 + if jitterRange <= 0 { + return pollInterval + } + return pollInterval + time.Duration(rand.Int63n(int64(jitterRange))) +} diff --git a/internal/dcpproc/commands/root.go b/internal/dcpproc/commands/root.go index e9d6f84c..9b0db5bd 100644 --- a/internal/dcpproc/commands/root.go +++ b/internal/dcpproc/commands/root.go @@ -23,7 +23,7 @@ func NewRootCmd(log *logger.Logger) (*cobra.Command, error) { rootCmd := &cobra.Command{ SilenceErrors: true, Use: "dcpproc", - Short: "Monitors dcp and cleans up orphaned resources (processes or containers)", + Short: "Monitors dcp and cleans up orphaned resources", Long: `DCP is a developer tool for running multi-service applications. It integrates your code, emulators and containers to give you a development environment @@ -56,6 +56,18 @@ func NewRootCmd(log *logger.Logger) (*cobra.Command, error) { rootCmd.AddCommand(cmd) } + if cmd, err = NewNetworkCommand(log.Logger); err != nil { + return nil, fmt.Errorf("could not set up 'monitor-container-network' command: %w", err) + } else { + rootCmd.AddCommand(cmd) + } + + if cmd, err = NewVolumeCommand(log.Logger); err != nil { + return nil, fmt.Errorf("could not set up 'monitor-container-volume' command: %w", err) + } else { + rootCmd.AddCommand(cmd) + } + if cmd, err = NewStopProcessTreeCommand(log.Logger); err != nil { return nil, fmt.Errorf("could not set up 'stop-process-tree' command: %w", err) } else { diff --git a/internal/dcpproc/commands/volume.go b/internal/dcpproc/commands/volume.go new file mode 100644 index 00000000..2f130d7c --- /dev/null +++ b/internal/dcpproc/commands/volume.go @@ -0,0 +1,255 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/cenkalti/backoff/v4" + "github.com/go-logr/logr" + "github.com/spf13/cobra" + + cmds "github.com/microsoft/dcp/internal/commands" + "github.com/microsoft/dcp/internal/containers" + container_flags "github.com/microsoft/dcp/internal/containers/flags" + container_runtimes "github.com/microsoft/dcp/internal/containers/runtimes" + "github.com/microsoft/dcp/pkg/logger" + "github.com/microsoft/dcp/pkg/process" + "github.com/microsoft/dcp/pkg/resiliency" +) + +const ( + defaultVolumePollInterval = 30 * time.Second + volumeCleanupRetryInitialInterval = 500 * time.Millisecond + volumeCleanupRetryMaxInterval = 5 * time.Second + volumeCleanupRetryTimeout = 30 * time.Second + volumeCleanupRetryRandomizationFactor = 0.1 + volumeCleanupRetryBackoffMultiplier = 2.0 +) + +var ( + volumeID string + volumePollInterval time.Duration +) + +func NewVolumeCommand(log logr.Logger) (*cobra.Command, error) { + volumeCmd := &cobra.Command{ + Use: "monitor-container-volume", + Short: "Ensures that a container volume is removed when the monitored process exits", + Long: `Ensures that a container volume is removed when the monitored process exits. + +This command is used to ensure that container volumes are properly cleaned up when +DCP terminates unexpectedly. Volumes are never force-removed.`, + RunE: monitorVolume(log), + SilenceUsage: true, + Args: cobra.NoArgs, + } + + flagErr := addMonitorFlags(volumeCmd) + if flagErr != nil { + return nil, flagErr + } + + volumeCmd.Flags().StringVar(&volumeID, "volumeID", "", "The volume ID or name to monitor and clean up when DCP exits") + flagErr = volumeCmd.MarkFlagRequired("volumeID") + if flagErr != nil { + return nil, flagErr + } + + volumeCmd.Flags().DurationVar( + &volumePollInterval, + "volumePollInterval", + defaultVolumePollInterval, + "How often to poll the volume status to check if it has been removed. Default is 30 seconds.", + ) + flagErr = volumeCmd.Flags().MarkHidden("volumePollInterval") + if flagErr != nil { + return nil, flagErr + } + + container_flags.EnsureRuntimeFlag(volumeCmd.Flags()) + + return volumeCmd, nil +} + +func monitorVolume(log logr.Logger) func(cmd *cobra.Command, _ []string) error { + return func(cmd *cobra.Command, _ []string) error { + if volumeID == "" { + return errors.New("volume ID or name must be specified with --volumeID") + } + + log = log.WithName("ContainerVolumeMonitor"). + WithValues( + "MonitorPID", monitorPid, + "Volume", volumeID, + ) + if resourceId != "" { + log = log.WithValues(logger.RESOURCE_LOG_STREAM_ID, resourceId) + } + + processExecutor := process.NewOSExecutor(log.WithName("ProcessExecutor")) + defer processExecutor.Dispose() + orchestrator, orchestratorErr := container_runtimes.FindAvailableContainerRuntime( + cmd.Context(), + log.WithName("ContainerOrchestrator").WithValues("ContainerRuntime", container_flags.GetRuntimeFlagValue()), + processExecutor, + ) + if orchestratorErr != nil { + log.Error(orchestratorErr, "Unable to ensure container volume cleanup") + return orchestratorErr + } + + monitorCtx, monitorCtxCancel, monitorCtxErr := cmds.MonitorPid( + cmd.Context(), + process.NewHandle(monitorPid, monitorProcessStartTime), + monitorInterval, + log, + ) + defer monitorCtxCancel() + if monitorCtxErr != nil { + if isMonitorProcessGoneErr(monitorCtxErr) { + log.Info("Monitored process already exited, cleaning up container volume", "Reason", monitorCtxErr) + return cleanupVolumeAfterMonitorExit( + cmd.Context(), + volumeID, + newVolumeCleanupBackoff(), + log, + orchestrator, + ) + } + + log.Error(monitorCtxErr, "Process could not be monitored") + return monitorCtxErr + } + + volumeRemovedCh := pollVolumeRemoved(monitorCtx, volumeID, orchestrator, log) + select { + case <-volumeRemovedCh: + return nil + case <-monitorCtx.Done(): + log.Info("Monitored process exited, cleaning up container volume") + return cleanupVolumeAfterMonitorExit( + cmd.Context(), + volumeID, + newVolumeCleanupBackoff(), + log, + orchestrator, + ) + } + } +} + +func cleanupVolumeAfterMonitorExit( + ctx context.Context, + volumeID string, + retryPolicy backoff.BackOff, + log logr.Logger, + orchestrator containers.VolumeOrchestrator, +) error { + waitingForContainerCleanup := false + return resiliency.Retry(ctx, retryPolicy, func() error { + cleanupErr := doCleanupVolume(ctx, volumeID, orchestrator) + if cleanupErr == nil { + return nil + } + if !errors.Is(cleanupErr, containers.ErrObjectInUse) { + return resiliency.Permanent(cleanupErr) + } + + if !waitingForContainerCleanup { + log.Info( + "Container volume is still in use; waiting for container cleanup before retrying removal", + "Timeout", + volumeCleanupRetryTimeout, + ) + waitingForContainerCleanup = true + } else { + log.V(1).Info("Container volume is still in use; retrying after backoff") + } + return cleanupErr + }) +} + +func newVolumeCleanupBackoff() *backoff.ExponentialBackOff { + return backoff.NewExponentialBackOff( + backoff.WithInitialInterval(volumeCleanupRetryInitialInterval), + backoff.WithMaxInterval(volumeCleanupRetryMaxInterval), + backoff.WithMaxElapsedTime(volumeCleanupRetryTimeout), + backoff.WithRandomizationFactor(volumeCleanupRetryRandomizationFactor), + backoff.WithMultiplier(volumeCleanupRetryBackoffMultiplier), + ) +} + +func doCleanupVolume( + ctx context.Context, + volumeID string, + orchestrator containers.VolumeOrchestrator, +) error { + inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{volumeID}, + }) + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + if errors.Is(inspectErr, containers.ErrNotFound) { + return nil + } + return fmt.Errorf("inspect container volume before removal: %w", inspectErr) + } + if len(inspectedVolumes) == 0 { + return nil + } + + _, removeErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ + Volumes: []string{volumeID}, + Force: false, + }) + if removeErr == nil { + return nil + } + + _, confirmErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{volumeID}, + }) + if errors.Is(confirmErr, containers.ErrNotFound) { + return nil + } + return fmt.Errorf("remove container volume: %w", errors.Join(removeErr, confirmErr)) +} + +func pollVolumeRemoved( + ctx context.Context, + volumeID string, + orchestrator containers.InspectVolumes, + log logr.Logger, +) <-chan struct{} { + volumeRemovedCh := make(chan struct{}) + go func() { + defer close(volumeRemovedCh) + timer := time.NewTimer(containerResourcePollDelay(volumePollInterval)) + defer timer.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-timer.C: + inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{volumeID}, + }) + if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedVolumes) == 0) { + return + } + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + log.Error(inspectErr, "Failed to inspect container volume") + } + timer.Reset(containerResourcePollDelay(volumePollInterval)) + } + } + }() + return volumeRemovedCh +} diff --git a/internal/dcpproc/dcpproc_api.go b/internal/dcpproc/dcpproc_api.go index d5c8a0e4..cda0d504 100644 --- a/internal/dcpproc/dcpproc_api.go +++ b/internal/dcpproc/dcpproc_api.go @@ -143,6 +143,58 @@ func RunContainerWatcherForMonitorWithOptions( } } +// RunNetworkWatcher starts a monitor that removes a container network if the current process exits. +// Failures are logged because the monitor is a best-effort reliability enhancement. +func RunNetworkWatcher( + pe process.Executor, + networkID string, + log logr.Logger, +) { + if _, found := os.LookupEnv(DCP_DISABLE_MONITOR_PROCESS); found { + return + } + + log = log.WithValues("NetworkID", networkID) + monitorPid := process.Uint32_ToPidT(uint32(os.Getpid())) + monitorIdentityTime := process.ProcessIdentityTime(monitorPid) + cmdArgs := []string{ + "monitor-container-network", + "--networkID", networkID, + } + cmdArgs = append(cmdArgs, getMonitorCmdArgs(process.NewHandle(monitorPid, monitorIdentityTime))...) + + startErr := startDcpProc(pe, cmdArgs) + if startErr != nil { + log.Error(startErr, "Failed to start container network monitor") + } +} + +// RunVolumeWatcher starts a monitor that removes a container volume if the current process exits. +// Failures are logged because the monitor is a best-effort reliability enhancement. +func RunVolumeWatcher( + pe process.Executor, + volumeID string, + log logr.Logger, +) { + if _, found := os.LookupEnv(DCP_DISABLE_MONITOR_PROCESS); found { + return + } + + log = log.WithValues("VolumeID", volumeID) + monitorPid := process.Uint32_ToPidT(uint32(os.Getpid())) + monitorIdentityTime := process.ProcessIdentityTime(monitorPid) + cmdArgs := []string{ + "monitor-container-volume", + "--volumeID", volumeID, + } + cmdArgs = append(cmdArgs, getMonitorCmdArgs(process.NewHandle(monitorPid, monitorIdentityTime))...) + + startErr := startDcpProc(pe, cmdArgs) + if startErr != nil { + log.Error(startErr, "Failed to start container volume monitor") + } +} + // Runs stop-process-tree command to stop the process tree rooted at the given process. func StopProcessTree( ctx context.Context, diff --git a/internal/dcpproc/dcpproc_api_test.go b/internal/dcpproc/dcpproc_api_test.go index 22d98892..5cf163ce 100644 --- a/internal/dcpproc/dcpproc_api_test.go +++ b/internal/dcpproc/dcpproc_api_test.go @@ -169,6 +169,44 @@ func TestRunContainerWatcherForMonitorWithStopOnly(t *testing.T) { require.Contains(t, dcpProc.Cmd.Args, "--stop-only", "Should include --stop-only flag") } +func TestRunNetworkWatcher(t *testing.T) { + log := testutil.NewLogForTesting(t.Name()) + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + pe := internal_testutil.NewTestProcessExecutor(ctx) + dcppaths.EnableTestPathProbing() + + testNetworkID := "test-network-123" + RunNetworkWatcher(pe, testNetworkID, log) + + dcpProc, dcpProcErr := findRunningDcp(pe) + require.NoError(t, dcpProcErr) + require.Equal(t, "monitor-container-network", dcpProc.Cmd.Args[1]) + require.Equal(t, "--networkID", dcpProc.Cmd.Args[2]) + require.Equal(t, testNetworkID, dcpProc.Cmd.Args[3]) + require.Equal(t, "--monitor", dcpProc.Cmd.Args[4]) + require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[5]) +} + +func TestRunVolumeWatcher(t *testing.T) { + log := testutil.NewLogForTesting(t.Name()) + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + pe := internal_testutil.NewTestProcessExecutor(ctx) + dcppaths.EnableTestPathProbing() + + testVolumeID := "test-volume-123" + RunVolumeWatcher(pe, testVolumeID, log) + + dcpProc, dcpProcErr := findRunningDcp(pe) + require.NoError(t, dcpProcErr) + require.Equal(t, "monitor-container-volume", dcpProc.Cmd.Args[1]) + require.Equal(t, "--volumeID", dcpProc.Cmd.Args[2]) + require.Equal(t, testVolumeID, dcpProc.Cmd.Args[3]) + require.Equal(t, "--monitor", dcpProc.Cmd.Args[4]) + require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[5]) +} + func TestStopProcessTree(t *testing.T) { log := testutil.NewLogForTesting(t.Name()) ctx, cancel := testutil.GetTestContext(t, 20*time.Second) diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index b3669a12..481f3530 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -4808,6 +4808,19 @@ func schema_microsoft_dcp_api_v2_PhysicalContainerConfig(ref common.ReferenceCal Format: "", }, }, + "monitorPID": { + SchemaProps: spec.SchemaProps{ + Description: "MonitorPID optionally scopes a retained runtime container to another process lifetime. When set, monitorTimestamp must also be set and retainRuntimeContainer must be true. The container is stopped but not removed when the monitored process exits.", + Type: []string{"integer"}, + Format: "int64", + }, + }, + "monitorTimestamp": { + SchemaProps: spec.SchemaProps{ + Description: "MonitorTimestamp identifies the process in monitorPID and guards against PID reuse.", + Ref: ref(metav1.MicroTime{}.OpenAPIModelName()), + }, + }, "imageRef": { SchemaProps: spec.SchemaProps{ Description: "ImageRef identifies a PhysicalContainerImage in the same namespace using or /. Cross-namespace references are not supported.", @@ -4973,7 +4986,7 @@ func schema_microsoft_dcp_api_v2_PhysicalContainerConfig(ref common.ReferenceCal }, }, Dependencies: []string{ - v2.ContainerNetworkConnectionConfig{}.OpenAPIModelName(), v2.ContainerPort{}.OpenAPIModelName(), v2.CreateFileSystem{}.OpenAPIModelName(), v2.VolumeMount{}.OpenAPIModelName(), commonapi.EnvVar{}.OpenAPIModelName(), commonapi.Label{}.OpenAPIModelName()}, + v2.ContainerNetworkConnectionConfig{}.OpenAPIModelName(), v2.ContainerPort{}.OpenAPIModelName(), v2.CreateFileSystem{}.OpenAPIModelName(), v2.VolumeMount{}.OpenAPIModelName(), commonapi.EnvVar{}.OpenAPIModelName(), commonapi.Label{}.OpenAPIModelName(), metav1.MicroTime{}.OpenAPIModelName()}, } } @@ -5992,9 +6005,9 @@ func schema_microsoft_dcp_api_v2_PhysicalContainerVolumeConfig(ref common.Refere Format: "", }, }, - "retainRuntimeVolume": { + "removeRuntimeVolumeOnDelete": { SchemaProps: spec.SchemaProps{ - Description: "RetainRuntimeVolume keeps the created runtime volume in place when this resource is deleted.", + Description: "RemoveRuntimeVolumeOnDelete removes the created runtime volume when this resource is deleted. Created runtime volumes are retained by default.", Type: []string{"boolean"}, Format: "", }, @@ -6249,6 +6262,19 @@ func schema_microsoft_dcp_api_v2_PhysicalProcessConfig(ref common.ReferenceCallb Format: "", }, }, + "monitorPID": { + SchemaProps: spec.SchemaProps{ + Description: "MonitorPID optionally scopes a retained runtime process to another process lifetime. When set, monitorTimestamp must also be set and retainRuntimeProcess must be true. The retained process is stopped when the monitored process exits.", + Type: []string{"integer"}, + Format: "int64", + }, + }, + "monitorTimestamp": { + SchemaProps: spec.SchemaProps{ + Description: "MonitorTimestamp identifies the process in monitorPID and guards against PID reuse.", + Ref: ref(metav1.MicroTime{}.OpenAPIModelName()), + }, + }, "executablePath": { SchemaProps: spec.SchemaProps{ Description: "ExecutablePath is the executable path or name to launch.", @@ -6316,7 +6342,7 @@ func schema_microsoft_dcp_api_v2_PhysicalProcessConfig(ref common.ReferenceCallb }, }, Dependencies: []string{ - commonapi.EnvVar{}.OpenAPIModelName()}, + commonapi.EnvVar{}.OpenAPIModelName(), metav1.MicroTime{}.OpenAPIModelName()}, } } diff --git a/plan/v2-resource-plan.md b/plan/v2-resource-plan.md index 9965af6c..3c96877e 100644 --- a/plan/v2-resource-plan.md +++ b/plan/v2-resource-plan.md @@ -79,29 +79,25 @@ This document tracks the intended direction for DCP V2 resources. The current V2 - `PhysicalContainerImage` provides source image pull and build workflows. The first runtime image ID successfully inspected by the controller is pinned for the resource lifetime and remains the only identity used for later inspection and dependent containers. If that exact image becomes unavailable, the resource reports it unavailable while retaining the published identity and metadata; it never silently pulls or builds a replacement. Delete and recreate the resource to realize a different image. - `PhysicalContainer` creates or tracks one runtime container, reports runtime status and port mappings, and references same-namespace `PhysicalContainerImage`, `PhysicalContainerVolume`, and `PhysicalContainerNetwork` resources. Container creation waits for every referenced physical resource to become ready. Bind mounts continue to use direct host paths, while named volume mounts resolve the referenced volume's observed runtime ID. - `PhysicalContainerNetwork` creates or references one runtime container network and reports its observed identity, driver, and address allocations. Its spec contains exactly one of top-level `networkID` or nested `network` creation config. Networks referenced by runtime ID are always retained. Created networks are retained when `network.retainRuntimeNetwork` is true; otherwise deletion enumerates running and stopped attachments, forcibly disconnects each container without removing it, and then removes the network. Name collisions are terminal unless `network.replaceExisting` is true, in which case the controller safely removes the specifically resolved network before creating its replacement. Runtime adapters classify their own built-in, non-removable networks, and replacement rejects them before disconnecting any attachments. -- `PhysicalContainerVolume` creates or references one runtime container volume and reports its observed identifier, driver, scope, mount point, and creation time. Its spec contains exactly one of top-level `volumeID` or nested `volume` creation config. Volumes referenced by runtime ID are always retained. Created volumes are retained when `volume.retainRuntimeVolume` is true; otherwise deletion retries non-forced removal until the runtime releases the volume. Removal deliberately does not use force because Podman force-removes attached containers. During namespace deletion, each volume retries removal for up to 30 seconds so an externally attached volume cannot block graceful namespace cleanup indefinitely. Name collisions are terminal unless `volume.replaceExisting` is true, in which case the controller safely removes the specifically resolved volume before creating its replacement. Caller-supplied `volume.labels` pass through to created volumes, with reserved persistence, creator-process, and internal resource UID labels set by the controller. +- `PhysicalContainerVolume` creates or references one runtime container volume and reports its observed identifier, driver, scope, mount point, and creation time. Its spec contains exactly one of top-level `volumeID` or nested `volume` creation config. Volumes referenced by runtime ID and newly created volumes are retained by default. Setting `volume.removeRuntimeVolumeOnDelete` opts a created volume into deletion and crash cleanup; deletion retries non-forced removal until the runtime releases the volume. Removal deliberately does not use force because Podman force-removes attached containers. During namespace deletion, each removable volume retries removal for up to 30 seconds so an externally attached volume cannot block graceful namespace cleanup indefinitely. Name collisions are terminal unless `volume.replaceExisting` is true, in which case the controller safely removes the specifically resolved volume before creating its replacement. Caller-supplied `volume.labels` pass through to created volumes, with reserved persistence, creator-process, and internal resource UID labels set by the controller. - `PhysicalProcess` launches or references one operating system process and reports its PID, PID-reuse identity timestamp, exit code when available, and lifecycle phase. Its spec contains exactly one of top-level `pid` or nested `process` creation config. Existing processes referenced by PID are observed and always retained when the resource is deleted. Created processes are stopped on deletion and namespace deletion unless `process.retainRuntimeProcess` is true. Deletion never blocks on runtime state: a resource that never took ownership of a running process drops its finalizer without stopping anything. The mutable top-level `stop` request can terminate either mode. Creation supports executable path, arguments, working directory, and environment without importing logical executable or IDE policy. - The physical resources use the shared `Pending`, `Ready`, `Unknown`, and `Failed` phases, specific `Ready` condition reasons, separate in-memory operation progress, and queued work where side effects can block. +- Created physical containers, networks, volumes, and processes launch best-effort monitor processes when they are configured to remove or stop their runtime object on Kubernetes resource deletion. Retained physical containers and processes can instead specify a monitor PID and identity timestamp; the retained runtime object is stopped, but not removed, when that process exits. Referenced runtime objects and retained resources without an explicit monitor do not launch cleanup monitors. ## Follow-up roadmap ### Physical resource layer -1. Decide how monitor processes should clean up physical resources after DCP crashes. - - Define how monitor processes are configured and launched for physical resources. - - Decide which physical resources require crash cleanup monitoring. - - Ensure cleanup behavior works when DCP exits unexpectedly and cannot rely on controller finalizers. - -2. Migrate V1 container-network tunnel proxy to V2 physical resources. +1. Migrate V1 container-network tunnel proxy to V2 physical resources. - Keep tunnel-specific behavior in the V1 controller, including dcptun image handling, server proxy process management, TLS, tunnel gRPC calls, status, and endpoint projection. - Delegate common runtime container lifecycle to V2 physical resources instead of creating and managing the proxy container directly through the orchestrator. -3. Migrate V1 container resource lifecycle to V2 physical resources. +2. Migrate V1 container resource lifecycle to V2 physical resources. - Keep V1-specific policy in the V1 controller, including lifecycle keys, persistent and existing container lookup, leases, compatibility status, and V1 API semantics. - Delegate common image/container/network/volume runtime lifecycle to V2 physical resources. - Avoid keeping repeated container creation, start, inspect, watch, stop, and remove logic in multiple V1 controllers. -4. Add logical resource controllers. +3. Add logical resource controllers. - Physical controllers preserve caller-supplied runtime labels and reserve the persistence, creator-process, and internal resource UID labels they need for harvesting and uncertain-create recovery. - Harvesting normally honors the controller-owned persistence label. Network harvesting is the exception: it intentionally ignores that label and removes orphaned networks after their creator exits so persistent networks cannot exhaust the runtime's finite default network allocations. - Build-created images receive persistent, creator-process, and internal UID labels through `build.labels`. Pulling resolves an expected named image and is not a runtime-object creation operation. diff --git a/test/integration/advanced_test_env.go b/test/integration/advanced_test_env.go index 7033e6b0..75b4eca2 100644 --- a/test/integration/advanced_test_env.go +++ b/test/integration/advanced_test_env.go @@ -266,6 +266,7 @@ func StartAdvancedTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), serverInfo.ContainerOrchestrator, + nil, ) if err = physicalContainerNetworkR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerNetworkReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerNetwork reconciler: %w", err) @@ -279,6 +280,7 @@ func StartAdvancedTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), serverInfo.ContainerOrchestrator, + nil, ) if err = physicalContainerVolumeR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerVolumeReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerVolume reconciler: %w", err) diff --git a/test/integration/standard_test_env.go b/test/integration/standard_test_env.go index 1bccdb2d..494eef79 100644 --- a/test/integration/standard_test_env.go +++ b/test/integration/standard_test_env.go @@ -292,6 +292,7 @@ func StartTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), serverInfo.ContainerOrchestrator, + pex, ) if err = physicalContainerNetworkR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerNetworkReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerNetwork reconciler: %w", err) @@ -305,6 +306,7 @@ func StartTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), serverInfo.ContainerOrchestrator, + pex, ) if err = physicalContainerVolumeR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerVolumeReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerVolume reconciler: %w", err) diff --git a/test/integration/v2_physical_container_controller_test.go b/test/integration/v2_physical_container_controller_test.go index aeff4498..b7ffaba7 100644 --- a/test/integration/v2_physical_container_controller_test.go +++ b/test/integration/v2_physical_container_controller_test.go @@ -9,6 +9,7 @@ import ( "context" "errors" std_slices "slices" + "strconv" "strings" "testing" "time" @@ -26,6 +27,7 @@ import ( internal_testutil "github.com/microsoft/dcp/internal/testutil" ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/commonapi" + "github.com/microsoft/dcp/pkg/osutil" "github.com/microsoft/dcp/pkg/testutil" ) @@ -1234,6 +1236,45 @@ func TestV2PhysicalContainerControllerPreservesCreatedContainerOnDeletion(t *tes require.Empty(t, physicalContainerMonitorProcesses(containerID)) } +func TestV2PhysicalContainerControllerScopesRetainedContainerToMonitorProcess(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() + + namespace := createActiveV2Namespace(t, ctx, "v2-pctr-retained-monitor") + image := createReadyV2PhysicalContainerImage(t, ctx, namespace.Name, "retained-monitor-image", "retained-monitor-image") + monitorPID := int64(12345) + monitorTimestamp := metav1.NewMicroTime(time.Now().Add(-time.Minute)) + container := &apiv2.PhysicalContainer{ + ObjectMeta: metav1.ObjectMeta{Name: "retained-monitor-container", Namespace: namespace.Name}, + Spec: apiv2.PhysicalContainerSpec{Container: &apiv2.PhysicalContainerConfig{ + ImageRef: image.Name, + ContainerName: "v2-pctr-retained-monitor", + RetainRuntimeContainer: true, + MonitorPID: &monitorPID, + MonitorTimestamp: monitorTimestamp, + }}, + } + require.NoError(t, client.Create(ctx, container)) + + updatedContainer := waitPhysicalContainerPhase(t, ctx, container.NamespacedName(), apiv2.PhysicalContainerPhaseRunning) + require.NotNil(t, updatedContainer.Spec.Container.MonitorPID) + require.Equal(t, monitorPID, *updatedContainer.Spec.Container.MonitorPID) + require.True(t, monitorTimestamp.Time.Equal(updatedContainer.Spec.Container.MonitorTimestamp.Time)) + containerID := updatedContainer.Status.ContainerID + removeRuntimeContainerOnCleanup(t, containerID) + var monitorProcesses []*internal_testutil.ProcessExecution + waitErr := wait.PollUntilContextCancel(ctx, waitPollInterval, pollImmediately, func(context.Context) (bool, error) { + monitorProcesses = physicalContainerMonitorProcesses(containerID) + return len(monitorProcesses) == 1, nil + }) + require.NoError(t, waitErr) + require.Len(t, monitorProcesses, 1) + require.Contains(t, monitorProcesses[0].Cmd.Args, "--stop-only") + require.Contains(t, monitorProcesses[0].Cmd.Args, strconv.FormatInt(monitorPID, 10)) + require.Contains(t, monitorProcesses[0].Cmd.Args, monitorTimestamp.Time.Format(osutil.RFC3339MiliTimestampFormat)) +} + func physicalContainerMonitorProcesses(containerID string) []*internal_testutil.ProcessExecution { return testProcessExecutor.FindAll([]string{"dcp", "monitor-container"}, "", func(processExecution *internal_testutil.ProcessExecution) bool { return std_slices.Contains(processExecution.Cmd.Args, containerID) diff --git a/test/integration/v2_physical_container_network_controller_test.go b/test/integration/v2_physical_container_network_controller_test.go index a2a4c373..03e575f1 100644 --- a/test/integration/v2_physical_container_network_controller_test.go +++ b/test/integration/v2_physical_container_network_controller_test.go @@ -21,6 +21,7 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/controllers" "github.com/microsoft/dcp/internal/containers" + internal_testutil "github.com/microsoft/dcp/internal/testutil" ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/commonapi" "github.com/microsoft/dcp/pkg/slices" @@ -81,6 +82,7 @@ func TestV2PhysicalContainerNetworkControllerCreatesNetwork(t *testing.T) { require.NotEqual(t, "caller-value", labels[controllers.CreatorProcessIdLabel]) require.NotEmpty(t, labels[controllers.CreatorProcessStartTimeLabel]) require.NotEqual(t, "caller-value", labels[controllers.CreatorProcessStartTimeLabel]) + require.Len(t, physicalContainerNetworkMonitorProcesses(updatedNetwork.Status.NetworkID), 1) } func TestV2PhysicalContainerNetworkControllerTracksExistingNetwork(t *testing.T) { @@ -111,6 +113,7 @@ func TestV2PhysicalContainerNetworkControllerTracksExistingNetwork(t *testing.T) // Tracking must not create anything: the only create is the one this test performed. require.Equal(t, 1, containerOrchestrator.CreateNetworkCallCount(networkName)) + require.Empty(t, physicalContainerNetworkMonitorProcesses(networkID)) } func TestV2PhysicalContainerNetworkControllerRemovesCreatedNetworkOnDeletion(t *testing.T) { @@ -269,6 +272,7 @@ func TestV2PhysicalContainerNetworkControllerPreservesCreatedNetworkOnDeletion(t updatedNetwork := waitPhysicalContainerNetworkPhase(t, ctx, network.NamespacedName(), apiv2.PhysicalContainerNetworkPhaseReady) networkID := updatedNetwork.Status.NetworkID require.Equal(t, "true", runtimeNetworkLabels(t, ctx, networkName)[controllers.PersistentLabel]) + require.Empty(t, physicalContainerNetworkMonitorProcesses(networkID)) require.NoError(t, client.Delete(ctx, network)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerNetwork](t, ctx, client, network) @@ -1000,6 +1004,12 @@ func waitCreateNetworkCallCount(t *testing.T, ctx context.Context, networkName s require.NoError(t, waitErr) } +func physicalContainerNetworkMonitorProcesses(networkID string) []*internal_testutil.ProcessExecution { + return testProcessExecutor.FindAll([]string{"dcp", "monitor-container-network"}, "", func(processExecution *internal_testutil.ProcessExecution) bool { + return slices.Contains(processExecution.Cmd.Args, networkID) + }) +} + func waitInspectNetworkCallCount( t *testing.T, ctx context.Context, diff --git a/test/integration/v2_physical_container_network_durability_test.go b/test/integration/v2_physical_container_network_durability_test.go index 7cc58d8f..9582de65 100644 --- a/test/integration/v2_physical_container_network_durability_test.go +++ b/test/integration/v2_physical_container_network_durability_test.go @@ -175,7 +175,7 @@ func TestV2PhysicalContainerNetworkControllerQueuesDeletionBeforeRemovingFinaliz WithStatusSubresource(&apiv2.PhysicalContainerNetwork{}). WithObjects(network). Build() - reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator) + reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator, nil) request := ctrl.Request{NamespacedName: network.NamespacedName()} reconcileDone := make(chan error, 1) @@ -267,7 +267,7 @@ func TestV2PhysicalContainerNetworkControllerRetriesUncertainCreateCleanup(t *te }, } - reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator) + reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator, nil) request := ctrl.Request{NamespacedName: network.NamespacedName()} waitErr := wait.PollUntilContextCancel(ctx, waitPollInterval, pollImmediately, func(ctx context.Context) (bool, error) { _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -417,6 +417,7 @@ func TestV2PhysicalContainerNetworkControllerRetainsTerminalCreateFailureUntilSt baseClient, log, orchestrator, + nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -514,6 +515,7 @@ func TestV2PhysicalContainerNetworkControllerRetainsBuiltInFailureUntilStatusIsD baseClient, log, orchestrator, + nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -609,6 +611,7 @@ func TestV2PhysicalContainerNetworkControllerAdoptsOwnedNetworkBeforeReplacement baseClient, log, orchestrator, + nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -685,6 +688,7 @@ func TestV2PhysicalContainerNetworkControllerRetainsCreatedNetworkUntilStatusIsD baseClient, log, orchestrator, + nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} diff --git a/test/integration/v2_physical_container_volume_controller_test.go b/test/integration/v2_physical_container_volume_controller_test.go index ff28fac3..b816f1e4 100644 --- a/test/integration/v2_physical_container_volume_controller_test.go +++ b/test/integration/v2_physical_container_volume_controller_test.go @@ -22,12 +22,14 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/controllers" "github.com/microsoft/dcp/internal/containers" + internal_testutil "github.com/microsoft/dcp/internal/testutil" ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/commonapi" + "github.com/microsoft/dcp/pkg/slices" "github.com/microsoft/dcp/pkg/testutil" ) -func TestV2PhysicalContainerVolumeControllerCreatesVolume(t *testing.T) { +func TestV2PhysicalContainerVolumeControllerCreatesRetainedVolumeByDefault(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() @@ -64,11 +66,12 @@ func TestV2PhysicalContainerVolumeControllerCreatesVolume(t *testing.T) { inspectedVolume := inspectRuntimeVolume(t, ctx, volumeName) require.Equal(t, "test-value", inspectedVolume.Labels["test-label"]) require.Equal(t, string(readyVolume.UID), inspectedVolume.Labels["com.microsoft.developer.usvc-dev.uid"]) - require.Equal(t, "false", inspectedVolume.Labels[controllers.PersistentLabel]) + require.Equal(t, "true", inspectedVolume.Labels[controllers.PersistentLabel]) require.NotEmpty(t, inspectedVolume.Labels[controllers.CreatorProcessIdLabel]) require.NotEqual(t, "caller-value", inspectedVolume.Labels[controllers.CreatorProcessIdLabel]) require.NotEmpty(t, inspectedVolume.Labels[controllers.CreatorProcessStartTimeLabel]) require.NotEqual(t, "caller-value", inspectedVolume.Labels[controllers.CreatorProcessStartTimeLabel]) + require.Empty(t, physicalContainerVolumeMonitorProcesses(readyVolume.Status.VolumeID)) } func TestV2PhysicalContainerVolumeControllerRetainsReferencedVolume(t *testing.T) { @@ -90,22 +93,23 @@ func TestV2PhysicalContainerVolumeControllerRetainsReferencedVolume(t *testing.T readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) require.Equal(t, volumeName, readyVolume.Status.VolumeID) require.Equal(t, 1, containerOrchestrator.CreateVolumeCallCount(volumeName)) + require.Empty(t, physicalContainerVolumeMonitorProcesses(volumeName)) require.NoError(t, client.Delete(ctx, volume)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerVolume](t, ctx, client, volume) require.NotNil(t, inspectRuntimeVolume(t, ctx, volumeName)) } -func TestV2PhysicalContainerVolumeControllerDeletesCreatedVolumesUnlessPersistent(t *testing.T) { +func TestV2PhysicalContainerVolumeControllerHonorsCreatedVolumeCleanupPolicy(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() namespace := createActiveV2Namespace(t, ctx, "v2-pcv-delete") - for _, persistent := range []bool{false, true} { - name := "deleted" - if persistent { - name = "persistent" + for _, removeRuntimeVolumeOnDelete := range []bool{false, true} { + name := "retained" + if removeRuntimeVolumeOnDelete { + name = "removed" } volumeName := "v2-pcv-" + name + "-runtime" removeRuntimeVolumeOnCleanup(t, volumeName) @@ -113,21 +117,27 @@ func TestV2PhysicalContainerVolumeControllerDeletesCreatedVolumesUnlessPersisten ObjectMeta: metav1.ObjectMeta{Name: name + "-volume", Namespace: namespace.Name}, Spec: apiv2.PhysicalContainerVolumeSpec{ Volume: &apiv2.PhysicalContainerVolumeConfig{ - VolumeName: volumeName, - RetainRuntimeVolume: persistent, + VolumeName: volumeName, + RemoveRuntimeVolumeOnDelete: removeRuntimeVolumeOnDelete, }, }, } require.NoError(t, client.Create(ctx, volume)) - waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) + readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) + monitorProcesses := physicalContainerVolumeMonitorProcesses(readyVolume.Status.VolumeID) + if removeRuntimeVolumeOnDelete { + require.Len(t, monitorProcesses, 1) + } else { + require.Empty(t, monitorProcesses) + } require.NoError(t, client.Delete(ctx, volume)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerVolume](t, ctx, client, volume) - if persistent { + if removeRuntimeVolumeOnDelete { + waitRuntimeVolumeMissing(t, ctx, volumeName) + } else { inspectedVolume := inspectRuntimeVolume(t, ctx, volumeName) require.Equal(t, "true", inspectedVolume.Labels[controllers.PersistentLabel]) - } else { - waitRuntimeVolumeMissing(t, ctx, volumeName) } } } @@ -142,7 +152,7 @@ func TestV2PhysicalContainerVolumeControllerWaitsForInUseVolume(t *testing.T) { removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "in-use-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) @@ -190,7 +200,7 @@ func TestV2PhysicalContainerVolumeControllerCleansUpOnNamespaceDeletion(t *testi removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "namespace-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) @@ -214,7 +224,7 @@ func TestV2PhysicalContainerVolumeControllerDoesNotDuplicateCreate(t *testing.T) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "single-create-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) waitCreateVolumeCallCount(t, ctx, volumeName, 1) @@ -243,7 +253,7 @@ func TestV2PhysicalContainerVolumeControllerWaitsForCreateBeforeDeletion(t *test volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "delete-during-create-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) waitCreateVolumeCallCount(t, ctx, volumeName, 1) @@ -275,7 +285,7 @@ func TestV2PhysicalContainerVolumeControllerAdoptsVolumeAfterUncertainCreateFail volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "uncertain-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) @@ -296,7 +306,7 @@ func TestV2PhysicalContainerVolumeControllerReportsTerminalNameCollision(t *test volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "collision-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) @@ -323,9 +333,8 @@ func TestV2PhysicalContainerVolumeControllerReplacesAndPersistsExistingVolume(t ObjectMeta: metav1.ObjectMeta{Name: "replacement-volume", Namespace: namespace.Name}, Spec: apiv2.PhysicalContainerVolumeSpec{ Volume: &apiv2.PhysicalContainerVolumeConfig{ - VolumeName: volumeName, - RetainRuntimeVolume: true, - ReplaceExisting: true, + VolumeName: volumeName, + ReplaceExisting: true, }, }, } @@ -506,7 +515,7 @@ func TestV2PhysicalContainerVolumeControllerAdoptsSameResourceVolumeAfterStateLo removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "state-loss-volume", Namespace: namespaceName}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) pendingVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhasePending) @@ -536,7 +545,7 @@ func TestV2PhysicalContainerVolumeControllerReportsExternalRemovalWithoutRecreat removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "missing-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) @@ -562,7 +571,7 @@ func TestV2PhysicalContainerVolumeControllerDoesNotChurnReadyStatus(t *testing.T removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "steady-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) @@ -603,7 +612,7 @@ func TestV2PhysicalContainerVolumeControllerRecoversFromRuntimeAndCreateFailures volumeName := "v2-pcv-recovery-runtime" volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "recovering-volume", Namespace: namespace.Name}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, serverInfo.Client.Create(ctx, volume)) failedVolume := waitPhysicalContainerVolumeReasonEx(t, ctx, serverInfo.Client, volume.NamespacedName(), apiv2.PhysicalContainerVolumeReasonCreateFailed) @@ -633,7 +642,7 @@ func TestV2PhysicalContainerVolumeControllerWaitsForNamespace(t *testing.T) { removeRuntimeVolumeOnCleanup(t, volumeName) volume := &apiv2.PhysicalContainerVolume{ ObjectMeta: metav1.ObjectMeta{Name: "wait-namespace-volume", Namespace: "v2-pcv-wait-namespace"}, - Spec: newPhysicalContainerVolumeSpec(volumeName), + Spec: newRemovablePhysicalContainerVolumeSpec(volumeName), } require.NoError(t, client.Create(ctx, volume)) waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhasePending) @@ -682,9 +691,12 @@ func waitPhysicalContainerVolumeReasonEx( }) } -func newPhysicalContainerVolumeSpec(volumeName string) apiv2.PhysicalContainerVolumeSpec { +func newRemovablePhysicalContainerVolumeSpec(volumeName string) apiv2.PhysicalContainerVolumeSpec { return apiv2.PhysicalContainerVolumeSpec{ - Volume: &apiv2.PhysicalContainerVolumeConfig{VolumeName: volumeName}, + Volume: &apiv2.PhysicalContainerVolumeConfig{ + VolumeName: volumeName, + RemoveRuntimeVolumeOnDelete: true, + }, } } @@ -709,6 +721,12 @@ func waitCreateVolumeCallCount(t *testing.T, ctx context.Context, volumeName str require.NoError(t, waitErr) } +func physicalContainerVolumeMonitorProcesses(volumeID string) []*internal_testutil.ProcessExecution { + return testProcessExecutor.FindAll([]string{"dcp", "monitor-container-volume"}, "", func(processExecution *internal_testutil.ProcessExecution) bool { + return slices.Contains(processExecution.Cmd.Args, volumeID) + }) +} + func waitInspectVolumeCallCount( t *testing.T, ctx context.Context, diff --git a/test/integration/v2_physical_container_volume_durability_test.go b/test/integration/v2_physical_container_volume_durability_test.go index a876774f..c13844b2 100644 --- a/test/integration/v2_physical_container_volume_durability_test.go +++ b/test/integration/v2_physical_container_volume_durability_test.go @@ -136,7 +136,7 @@ func TestV2PhysicalContainerVolumeControllerBoundsRemovalDuringNamespaceDeletion }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} currentVolume := waitPhysicalContainerVolumeConditionReason( @@ -187,7 +187,7 @@ func TestV2PhysicalContainerVolumeControllerRetriesBeforeNamespaceRemovalDeadlin }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} require.NoError(t, baseClient.Delete(ctx, volume)) @@ -240,7 +240,7 @@ func TestV2PhysicalContainerVolumeControllerDoesNotBoundDirectRemoval(t *testing }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} currentVolume := waitPhysicalContainerVolumeConditionReason( @@ -290,7 +290,7 @@ func TestV2PhysicalContainerVolumeControllerRetriesUncertainCreateCleanup(t *tes }, } - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} waitErr := wait.PollUntilContextCancel(ctx, waitPollInterval, pollImmediately, func(ctx context.Context) (bool, error) { _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -374,7 +374,7 @@ func TestV2PhysicalContainerVolumeControllerRetainsCreatedVolumeUntilStatusIsDur }, } orchestrator := newDurabilityTestContainerOrchestrator(t, ctx) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -441,7 +441,7 @@ func TestV2PhysicalContainerVolumeControllerRetainsTerminalFailureUntilStatusIsD } orchestrator := newDurabilityTestContainerOrchestrator(t, ctx) require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: volume.Spec.Volume.VolumeName})) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) request := ctrl.Request{NamespacedName: volume.NamespacedName()} _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -492,7 +492,10 @@ func durablePhysicalContainerVolume(namespace, name, volumeName string) *apiv2.P Finalizers: []string{apiv2.GroupName + "/physicalcontainervolume-reconciler"}, }, Spec: apiv2.PhysicalContainerVolumeSpec{ - Volume: &apiv2.PhysicalContainerVolumeConfig{VolumeName: volumeName}, + Volume: &apiv2.PhysicalContainerVolumeConfig{ + VolumeName: volumeName, + RemoveRuntimeVolumeOnDelete: true, + }, }, } } diff --git a/test/integration/v2_physical_process_controller_test.go b/test/integration/v2_physical_process_controller_test.go index f860e435..6d70d1e5 100644 --- a/test/integration/v2_physical_process_controller_test.go +++ b/test/integration/v2_physical_process_controller_test.go @@ -621,13 +621,17 @@ func TestV2PhysicalProcessControllerDeletesOrRetainsCreatedProcess(t *testing.T) dcppaths.EnableTestPathProbing() dcpPath, dcpPathErr := dcppaths.GetDcpExePath() require.NoError(t, dcpPathErr) + customMonitorPID := int64(12345) testCases := []struct { - name string - retain bool + name string + slug string + retain bool + customMonitorPID *int64 }{ - {name: "deletes", retain: false}, - {name: "retains", retain: true}, + {name: "deletes", slug: "deletes", retain: false}, + {name: "retains", slug: "retains", retain: true}, + {name: "retains with monitor", slug: "retains-with-monitor", retain: true, customMonitorPID: &customMonitorPID}, } for _, testCase := range testCases { @@ -636,19 +640,30 @@ func TestV2PhysicalProcessControllerDeletesOrRetainsCreatedProcess(t *testing.T) ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - namespace := createActiveV2Namespace(t, ctx, "v2-pproc-"+testCase.name) - executablePath := "v2-pproc-" + testCase.name + "-command" + namespace := createActiveV2Namespace(t, ctx, "v2-pproc-"+testCase.slug) + executablePath := "v2-pproc-" + testCase.slug + "-command" + monitorTimestamp := metav1.NewMicroTime(time.Now().Add(-time.Minute)) physicalProcess := &apiv2.PhysicalProcess{ - ObjectMeta: metav1.ObjectMeta{Name: testCase.name + "-process", Namespace: namespace.Name}, + ObjectMeta: metav1.ObjectMeta{Name: testCase.slug + "-process", Namespace: namespace.Name}, Spec: apiv2.PhysicalProcessSpec{ Process: &apiv2.PhysicalProcessConfig{ ExecutablePath: executablePath, RetainRuntimeProcess: testCase.retain, + MonitorPID: testCase.customMonitorPID, + MonitorTimestamp: monitorTimestamp, }, }, } + if testCase.customMonitorPID == nil { + physicalProcess.Spec.Process.MonitorTimestamp = metav1.MicroTime{} + } require.NoError(t, client.Create(ctx, physicalProcess)) runningProcess := waitPhysicalProcessPhase(t, ctx, physicalProcess.NamespacedName(), apiv2.PhysicalProcessPhaseRunning) + if testCase.customMonitorPID != nil { + require.NotNil(t, runningProcess.Spec.Process.MonitorPID) + require.Equal(t, *testCase.customMonitorPID, *runningProcess.Spec.Process.MonitorPID) + require.True(t, monitorTimestamp.Time.Equal(runningProcess.Spec.Process.MonitorTimestamp.Time)) + } pid, convertErr := process.Int64_ToPidT(*runningProcess.Status.PID) require.NoError(t, convertErr) monitorExecutions := testProcessExecutor.FindAll( @@ -656,7 +671,11 @@ func TestV2PhysicalProcessControllerDeletesOrRetainsCreatedProcess(t *testing.T) "", nil, ) - if testCase.retain { + if testCase.customMonitorPID != nil { + require.Len(t, monitorExecutions, 1) + require.Contains(t, monitorExecutions[0].Cmd.Args, strconv.FormatInt(*testCase.customMonitorPID, 10)) + require.Contains(t, monitorExecutions[0].Cmd.Args, monitorTimestamp.Time.Format(osutil.RFC3339MiliTimestampFormat)) + } else if testCase.retain { require.Empty(t, monitorExecutions) } else { require.Len(t, monitorExecutions, 1) From d30dc509e39d9dc2940646307d3c78321fce78cd Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 15:56:09 -0700 Subject: [PATCH 2/8] Address V2 crash monitor review feedback Make resource-removal polling cancellation-safe, start retained container monitors only after successful startup, and protect volume cleanup against same-name replacement using the physical resource UID. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/container_controller.go | 2 +- controllers/physical_container_controller.go | 30 ++++-- .../physical_container_volume_controller.go | 2 +- internal/containers/containers_common.go | 1 + internal/dcpproc/commands/container.go | 70 ++++---------- .../dcpproc/commands/container_resource.go | 52 +++++++++++ .../commands/container_resource_test.go | 93 +++++++++++++++++-- internal/dcpproc/commands/network.go | 62 +++++-------- internal/dcpproc/commands/volume.go | 89 ++++++++++-------- internal/dcpproc/dcpproc_api.go | 4 +- internal/dcpproc/dcpproc_api_test.go | 9 +- ...ysical_container_volume_controller_test.go | 2 + 12 files changed, 264 insertions(+), 152 deletions(-) create mode 100644 internal/dcpproc/commands/container_resource.go diff --git a/controllers/container_controller.go b/controllers/container_controller.go index 4334267f..9c3077cc 100644 --- a/controllers/container_controller.go +++ b/controllers/container_controller.go @@ -63,7 +63,7 @@ const ( dcpBuildLabel = "com.microsoft.developer.usvc-dev.build" groupVersionLabel = "com.microsoft.developer.usvc-dev.group-version" nameLabel = "com.microsoft.developer.usvc-dev.name" - uidLabel = "com.microsoft.developer.usvc-dev.uid" + uidLabel = containers.ResourceUIDLabel lifecycleKeyLabel = "com.microsoft.developer.usvc-dev.lifecycle-key" envLabel = "com.microsoft.developer.usvc-dev.env" mountsLabel = "com.microsoft.developer.usvc-dev.mountsLabel" diff --git a/controllers/physical_container_controller.go b/controllers/physical_container_controller.go index daa8a19e..aa8d84de 100644 --- a/controllers/physical_container_controller.go +++ b/controllers/physical_container_controller.go @@ -968,30 +968,39 @@ func (r *PhysicalContainerReconciler) createPhysicalContainer( data.progress = physicalContainerOperationCompleted data.failureMessage = "" data.retryAfter = time.Time{} - r.runPhysicalContainerLifecycleMonitor(container, containerID, log) + r.runPhysicalContainerCleanupMonitor(container, containerID, log) } r.queuePhysicalContainerDataResult(container, stateKey, data) } -// Starts the monitor process selected by the runtime container's retention policy. +// Starts a monitor that removes a non-retained runtime container if DCP exits. // Failures are logged but not surfaced, because the monitor is a best-effort reliability enhancement; // the container harvester reclaims orphaned containers in a later session. -func (r *PhysicalContainerReconciler) runPhysicalContainerLifecycleMonitor(container *apiv2.PhysicalContainer, containerID string, log logr.Logger) { +func (r *PhysicalContainerReconciler) runPhysicalContainerCleanupMonitor(container *apiv2.PhysicalContainer, containerID string, log logr.Logger) { containerConfig := container.Spec.Container - if containerConfig == nil || containerID == "" { + if containerConfig == nil || containerConfig.RetainRuntimeContainer || containerID == "" { return } - if !containerConfig.RetainRuntimeContainer { - if r.processExecutor == nil { - log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainer cleanup monitor") - return - } - dcpproc.RunContainerWatcher(r.processExecutor, containerID, log) + if r.processExecutor == nil { + log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainer cleanup monitor") return } + dcpproc.RunContainerWatcher(r.processExecutor, containerID, log) +} +// Starts a stop-only monitor for a retained runtime container after it has started. +// Failures are logged but not surfaced, because the monitor is a best-effort reliability enhancement. +func (r *PhysicalContainerReconciler) runRetainedPhysicalContainerLifecycleMonitor( + container *apiv2.PhysicalContainer, + containerID string, + log logr.Logger, +) { + containerConfig := container.Spec.Container + if containerConfig == nil || !containerConfig.RetainRuntimeContainer || containerID == "" { + return + } monitor, found, monitorErr := dcpproc.MonitorTargetFromFields(containerConfig.MonitorPID, containerConfig.MonitorTimestamp) if monitorErr != nil { log.Error(monitorErr, "Could not start retained PhysicalContainer lifecycle monitor") @@ -1176,6 +1185,7 @@ func (r *PhysicalContainerReconciler) startPhysicalContainer( data.state = physicalContainerStateStart data.progress = physicalContainerOperationCompleted data.failureMessage = "" + r.runRetainedPhysicalContainerLifecycleMonitor(container, data.containerID, log) } r.queuePhysicalContainerDataResult(container, stateKey, data) diff --git a/controllers/physical_container_volume_controller.go b/controllers/physical_container_volume_controller.go index 7eaa7e10..b1d75b77 100644 --- a/controllers/physical_container_volume_controller.go +++ b/controllers/physical_container_volume_controller.go @@ -509,7 +509,7 @@ func (r *PhysicalContainerVolumeReconciler) runPhysicalContainerVolumeLifecycleM log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainerVolume cleanup monitor") return } - dcpproc.RunVolumeWatcher(r.processExecutor, volumeID, log) + dcpproc.RunVolumeWatcher(r.processExecutor, volumeID, string(volume.UID), log) } func handlePhysicalContainerVolumeCreateFailure( diff --git a/internal/containers/containers_common.go b/internal/containers/containers_common.go index 7230fc9a..c2453b26 100644 --- a/internal/containers/containers_common.go +++ b/internal/containers/containers_common.go @@ -27,6 +27,7 @@ import ( const ( ContainerLogsHttpPath string = "/apis/usvc-dev.developer.microsoft.com/v1/containers/%s/log" ContainerHttpPath string = "/apis/usvc-dev.developer.microsoft.com/v1/containers/%s" + ResourceUIDLabel = "com.microsoft.developer.usvc-dev.uid" ) var ( diff --git a/internal/dcpproc/commands/container.go b/internal/dcpproc/commands/container.go index d0fd8090..9e8aad5b 100644 --- a/internal/dcpproc/commands/container.go +++ b/internal/dcpproc/commands/container.go @@ -9,7 +9,6 @@ import ( "context" "errors" "fmt" - "math/rand" "time" "github.com/go-logr/logr" @@ -126,16 +125,12 @@ func monitorContainer(log logr.Logger) func(cmd *cobra.Command, args []string) e } } - ctrRemovedCh := pollContainerRemoved(monitorCtx, containerID, co, log) - - select { - case <-ctrRemovedCh: - // Container was removed, we are done + if pollContainerRemoved(monitorCtx, containerID, co, log) { return nil - case <-monitorCtx.Done(): - log.Info("Monitored process exited, cleaning up container") - return doCleanupContainer(cmd.Context(), containerID, containerStopOnly, log, co) } + + log.Info("Monitored process exited, cleaning up container") + return doCleanupContainer(cmd.Context(), containerID, containerStopOnly, log, co) } } @@ -229,47 +224,20 @@ func doCleanupContainer( return nil } -func pollContainerRemoved(ctx context.Context, containerID string, co inspectStopRemoveContainers, log logr.Logger) <-chan struct{} { - ctrRemovedCh := make(chan struct{}) - - jitter := func() time.Duration { - // Up to 5% of the poll interval, to avoid all instances of dcpproc polling at the same exact time - return time.Duration(rand.Int63n(int64(containerPollInterval / 20.0))) - } - - go func() { - defer close(ctrRemovedCh) - // Use the configured poll interval (overridable via hidden flag for tests) - timer := time.NewTimer(containerPollInterval + jitter()) - defer timer.Stop() - - for { - select { - - case <-ctx.Done(): - return - - case <-timer.C: - // Poll the container status - _, inspectErr := co.InspectContainers(ctx, containers.InspectContainersOptions{ - Containers: []string{containerID}, - }) - - if inspectErr != nil { - if errors.Is(inspectErr, containers.ErrNotFound) { - // Container has been removed, we should exit, which will close the channel - // and notify the caller. - return - } else { - log.Error(inspectErr, "Failed to inspect container") - // May be transient error, continue polling - } - } - - timer.Reset(containerPollInterval + jitter()) +func pollContainerRemoved(ctx context.Context, containerID string, co inspectStopRemoveContainers, log logr.Logger) bool { + return pollContainerResourceRemoved( + ctx, + containerPollInterval, + func(ctx context.Context) (bool, error) { + _, inspectErr := co.InspectContainers(ctx, containers.InspectContainersOptions{ + Containers: []string{containerID}, + }) + if errors.Is(inspectErr, containers.ErrNotFound) { + return true, nil } - } - }() - - return ctrRemovedCh + return false, inspectErr + }, + "Failed to inspect container", + log, + ) } diff --git a/internal/dcpproc/commands/container_resource.go b/internal/dcpproc/commands/container_resource.go new file mode 100644 index 00000000..afeada08 --- /dev/null +++ b/internal/dcpproc/commands/container_resource.go @@ -0,0 +1,52 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package commands + +import ( + "context" + "math/rand" + "time" + + "github.com/go-logr/logr" +) + +func pollContainerResourceRemoved( + ctx context.Context, + pollInterval time.Duration, + inspect func(context.Context) (bool, error), + inspectFailureMessage string, + log logr.Logger, +) bool { + timer := time.NewTimer(containerResourcePollDelay(pollInterval)) + defer timer.Stop() + + for { + select { + case <-ctx.Done(): + return false + case <-timer.C: + removed, inspectErr := inspect(ctx) + if removed { + return true + } + if inspectErr != nil { + if ctx.Err() != nil { + return false + } + log.Error(inspectErr, inspectFailureMessage) + } + timer.Reset(containerResourcePollDelay(pollInterval)) + } + } +} + +func containerResourcePollDelay(pollInterval time.Duration) time.Duration { + jitterRange := pollInterval / 20 + if jitterRange <= 0 { + return pollInterval + } + return pollInterval + time.Duration(rand.Int63n(int64(jitterRange))) +} diff --git a/internal/dcpproc/commands/container_resource_test.go b/internal/dcpproc/commands/container_resource_test.go index e5d67472..f68d0e5c 100644 --- a/internal/dcpproc/commands/container_resource_test.go +++ b/internal/dcpproc/commands/container_resource_test.go @@ -67,8 +67,14 @@ func TestCleanupVolumeDoesNotForceRemoval(t *testing.T) { require.NoError(t, orchestrator.Close()) }() - const createdVolumeID = "cleanup-volume" - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + const ( + createdVolumeID = "cleanup-volume" + resourceUID = "cleanup-volume-resource" + ) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ + Name: createdVolumeID, + Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, + })) createdContainerID, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ Name: "cleanup-volume-container", Image: "cleanup-volume-image", @@ -80,7 +86,7 @@ func TestCleanupVolumeDoesNotForceRemoval(t *testing.T) { }) require.NoError(t, createContainerErr) - cleanupErr := doCleanupVolume(ctx, createdVolumeID, orchestrator) + cleanupErr := doCleanupVolume(ctx, createdVolumeID, resourceUID, orchestrator) require.Error(t, cleanupErr) inspectedVolumes, inspectVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ @@ -93,7 +99,7 @@ func TestCleanupVolumeDoesNotForceRemoval(t *testing.T) { Force: true, }) require.NoError(t, removeContainerErr) - require.NoError(t, doCleanupVolume(ctx, createdVolumeID, orchestrator)) + require.NoError(t, doCleanupVolume(ctx, createdVolumeID, resourceUID, orchestrator)) _, inspectRemovedVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ Volumes: []string{createdVolumeID}, }) @@ -115,8 +121,12 @@ func TestCleanupVolumeWaitsForContainerCleanup(t *testing.T) { const ( createdVolumeID = "cleanup-volume-after-container" createdContainerID = "cleanup-volume-after-container-container" + resourceUID = "cleanup-volume-after-container-resource" ) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ + Name: createdVolumeID, + Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, + })) _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ Name: createdContainerID, Image: "cleanup-volume-after-container-image", @@ -135,6 +145,7 @@ func TestCleanupVolumeWaitsForContainerCleanup(t *testing.T) { require.NoError(t, cleanupVolumeAfterMonitorExit( ctx, createdVolumeID, + resourceUID, backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), log, orderedOrchestrator, @@ -159,8 +170,14 @@ func TestCleanupVolumeStopsRetryingWhileContainerRemains(t *testing.T) { require.NoError(t, orchestrator.Close()) }() - const createdVolumeID = "bounded-cleanup-volume" - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: createdVolumeID})) + const ( + createdVolumeID = "bounded-cleanup-volume" + resourceUID = "bounded-cleanup-volume-resource" + ) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ + Name: createdVolumeID, + Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, + })) _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ Name: "bounded-cleanup-volume-container", Image: "bounded-cleanup-volume-image", @@ -175,6 +192,7 @@ func TestCleanupVolumeStopsRetryingWhileContainerRemains(t *testing.T) { cleanupErr := cleanupVolumeAfterMonitorExit( ctx, createdVolumeID, + resourceUID, backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), log, orchestrator, @@ -183,6 +201,67 @@ func TestCleanupVolumeStopsRetryingWhileContainerRemains(t *testing.T) { require.Equal(t, 2, orchestrator.RemoveVolumeCallCount(createdVolumeID)) } +func TestCleanupVolumePreservesSameNameReplacement(t *testing.T) { + t.Parallel() + ctx, cancel := testutil.GetTestContext(t, 20*time.Second) + defer cancel() + + log := testutil.NewLogForTesting(t.Name()) + orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) + require.NoError(t, orchestratorErr) + defer func() { + require.NoError(t, orchestrator.Close()) + }() + + const ( + createdVolumeID = "replaced-cleanup-volume" + originalUID = "original-resource" + replacementUID = "replacement-resource" + ) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ + Name: createdVolumeID, + Labels: map[string]string{containers.ResourceUIDLabel: originalUID}, + })) + _, removeOriginalErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ + Volumes: []string{createdVolumeID}, + }) + require.NoError(t, removeOriginalErr) + require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ + Name: createdVolumeID, + Labels: map[string]string{containers.ResourceUIDLabel: replacementUID}, + })) + + require.NoError(t, doCleanupVolume(ctx, createdVolumeID, originalUID, orchestrator)) + require.Equal(t, 1, orchestrator.RemoveVolumeCallCount(createdVolumeID)) + inspectedVolumes, inspectVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{createdVolumeID}, + }) + require.NoError(t, inspectVolumeErr) + require.Len(t, inspectedVolumes, 1) + require.Equal(t, replacementUID, inspectedVolumes[0].Labels[containers.ResourceUIDLabel]) +} + +func TestPollContainerResourceRemovedReturnsFalseWhenContextIsCancelled(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + inspectCalled := false + removed := pollContainerResourceRemoved( + ctx, + time.Hour, + func(context.Context) (bool, error) { + inspectCalled = true + return false, nil + }, + "Unexpected inspection failure", + testutil.NewLogForTesting(t.Name()), + ) + + require.False(t, removed) + require.False(t, inspectCalled) +} + type removeContainerAfterVolumeAttemptOrchestrator struct { *ctrl_testutil.TestContainerOrchestrator containerID string diff --git a/internal/dcpproc/commands/network.go b/internal/dcpproc/commands/network.go index 2dad72c9..9e13009e 100644 --- a/internal/dcpproc/commands/network.go +++ b/internal/dcpproc/commands/network.go @@ -9,7 +9,6 @@ import ( "context" "errors" "fmt" - "math/rand" "time" "github.com/go-logr/logr" @@ -114,14 +113,12 @@ func monitorNetwork(log logr.Logger) func(cmd *cobra.Command, _ []string) error return monitorCtxErr } - networkRemovedCh := pollNetworkRemoved(monitorCtx, networkID, orchestrator, log) - select { - case <-networkRemovedCh: + if pollNetworkRemoved(monitorCtx, networkID, orchestrator, log) { return nil - case <-monitorCtx.Done(): - log.Info("Monitored process exited, cleaning up container network") - return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) } + + log.Info("Monitored process exited, cleaning up container network") + return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) } } @@ -210,38 +207,23 @@ func pollNetworkRemoved( networkID string, orchestrator containers.InspectNetworks, log logr.Logger, -) <-chan struct{} { - networkRemovedCh := make(chan struct{}) - go func() { - defer close(networkRemovedCh) - timer := time.NewTimer(containerResourcePollDelay(networkPollInterval)) - defer timer.Stop() - - for { - select { - case <-ctx.Done(): - return - case <-timer.C: - inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{networkID}, - }) - if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedNetworks) == 0) { - return - } - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - log.Error(inspectErr, "Failed to inspect container network") - } - timer.Reset(containerResourcePollDelay(networkPollInterval)) +) bool { + return pollContainerResourceRemoved( + ctx, + networkPollInterval, + func(ctx context.Context) (bool, error) { + inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ + Networks: []string{networkID}, + }) + if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedNetworks) == 0) { + return true, nil } - } - }() - return networkRemovedCh -} - -func containerResourcePollDelay(pollInterval time.Duration) time.Duration { - jitterRange := pollInterval / 20 - if jitterRange <= 0 { - return pollInterval - } - return pollInterval + time.Duration(rand.Int63n(int64(jitterRange))) + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + return false, inspectErr + } + return false, nil + }, + "Failed to inspect container network", + log, + ) } diff --git a/internal/dcpproc/commands/volume.go b/internal/dcpproc/commands/volume.go index 2f130d7c..cfbde777 100644 --- a/internal/dcpproc/commands/volume.go +++ b/internal/dcpproc/commands/volume.go @@ -35,6 +35,7 @@ const ( var ( volumeID string + volumeResourceUID string volumePollInterval time.Duration ) @@ -62,6 +63,12 @@ DCP terminates unexpectedly. Volumes are never force-removed.`, return nil, flagErr } + volumeCmd.Flags().StringVar(&volumeResourceUID, "resourceUID", "", "The UID of the PhysicalContainerVolume that created the volume") + flagErr = volumeCmd.MarkFlagRequired("resourceUID") + if flagErr != nil { + return nil, flagErr + } + volumeCmd.Flags().DurationVar( &volumePollInterval, "volumePollInterval", @@ -83,11 +90,15 @@ func monitorVolume(log logr.Logger) func(cmd *cobra.Command, _ []string) error { if volumeID == "" { return errors.New("volume ID or name must be specified with --volumeID") } + if volumeResourceUID == "" { + return errors.New("physical container volume UID must be specified with --resourceUID") + } log = log.WithName("ContainerVolumeMonitor"). WithValues( "MonitorPID", monitorPid, "Volume", volumeID, + "ResourceUID", volumeResourceUID, ) if resourceId != "" { log = log.WithValues(logger.RESOURCE_LOG_STREAM_ID, resourceId) @@ -118,6 +129,7 @@ func monitorVolume(log logr.Logger) func(cmd *cobra.Command, _ []string) error { return cleanupVolumeAfterMonitorExit( cmd.Context(), volumeID, + volumeResourceUID, newVolumeCleanupBackoff(), log, orchestrator, @@ -128,33 +140,33 @@ func monitorVolume(log logr.Logger) func(cmd *cobra.Command, _ []string) error { return monitorCtxErr } - volumeRemovedCh := pollVolumeRemoved(monitorCtx, volumeID, orchestrator, log) - select { - case <-volumeRemovedCh: + if pollVolumeRemoved(monitorCtx, volumeID, volumeResourceUID, orchestrator, log) { return nil - case <-monitorCtx.Done(): - log.Info("Monitored process exited, cleaning up container volume") - return cleanupVolumeAfterMonitorExit( - cmd.Context(), - volumeID, - newVolumeCleanupBackoff(), - log, - orchestrator, - ) } + + log.Info("Monitored process exited, cleaning up container volume") + return cleanupVolumeAfterMonitorExit( + cmd.Context(), + volumeID, + volumeResourceUID, + newVolumeCleanupBackoff(), + log, + orchestrator, + ) } } func cleanupVolumeAfterMonitorExit( ctx context.Context, volumeID string, + resourceUID string, retryPolicy backoff.BackOff, log logr.Logger, orchestrator containers.VolumeOrchestrator, ) error { waitingForContainerCleanup := false return resiliency.Retry(ctx, retryPolicy, func() error { - cleanupErr := doCleanupVolume(ctx, volumeID, orchestrator) + cleanupErr := doCleanupVolume(ctx, volumeID, resourceUID, orchestrator) if cleanupErr == nil { return nil } @@ -189,6 +201,7 @@ func newVolumeCleanupBackoff() *backoff.ExponentialBackOff { func doCleanupVolume( ctx context.Context, volumeID string, + resourceUID string, orchestrator containers.VolumeOrchestrator, ) error { inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ @@ -203,6 +216,9 @@ func doCleanupVolume( if len(inspectedVolumes) == 0 { return nil } + if inspectedVolumes[0].Labels[containers.ResourceUIDLabel] != resourceUID { + return nil + } _, removeErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ Volumes: []string{volumeID}, @@ -224,32 +240,29 @@ func doCleanupVolume( func pollVolumeRemoved( ctx context.Context, volumeID string, + resourceUID string, orchestrator containers.InspectVolumes, log logr.Logger, -) <-chan struct{} { - volumeRemovedCh := make(chan struct{}) - go func() { - defer close(volumeRemovedCh) - timer := time.NewTimer(containerResourcePollDelay(volumePollInterval)) - defer timer.Stop() - - for { - select { - case <-ctx.Done(): - return - case <-timer.C: - inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{volumeID}, - }) - if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedVolumes) == 0) { - return - } - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - log.Error(inspectErr, "Failed to inspect container volume") - } - timer.Reset(containerResourcePollDelay(volumePollInterval)) +) bool { + return pollContainerResourceRemoved( + ctx, + volumePollInterval, + func(ctx context.Context) (bool, error) { + inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ + Volumes: []string{volumeID}, + }) + if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedVolumes) == 0) { + return true, nil } - } - }() - return volumeRemovedCh + if len(inspectedVolumes) > 0 && inspectedVolumes[0].Labels[containers.ResourceUIDLabel] != resourceUID { + return true, nil + } + if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { + return false, inspectErr + } + return false, nil + }, + "Failed to inspect container volume", + log, + ) } diff --git a/internal/dcpproc/dcpproc_api.go b/internal/dcpproc/dcpproc_api.go index cda0d504..a41133c5 100644 --- a/internal/dcpproc/dcpproc_api.go +++ b/internal/dcpproc/dcpproc_api.go @@ -174,18 +174,20 @@ func RunNetworkWatcher( func RunVolumeWatcher( pe process.Executor, volumeID string, + resourceUID string, log logr.Logger, ) { if _, found := os.LookupEnv(DCP_DISABLE_MONITOR_PROCESS); found { return } - log = log.WithValues("VolumeID", volumeID) + log = log.WithValues("VolumeID", volumeID, "ResourceUID", resourceUID) monitorPid := process.Uint32_ToPidT(uint32(os.Getpid())) monitorIdentityTime := process.ProcessIdentityTime(monitorPid) cmdArgs := []string{ "monitor-container-volume", "--volumeID", volumeID, + "--resourceUID", resourceUID, } cmdArgs = append(cmdArgs, getMonitorCmdArgs(process.NewHandle(monitorPid, monitorIdentityTime))...) diff --git a/internal/dcpproc/dcpproc_api_test.go b/internal/dcpproc/dcpproc_api_test.go index 5cf163ce..d45dc501 100644 --- a/internal/dcpproc/dcpproc_api_test.go +++ b/internal/dcpproc/dcpproc_api_test.go @@ -196,15 +196,18 @@ func TestRunVolumeWatcher(t *testing.T) { dcppaths.EnableTestPathProbing() testVolumeID := "test-volume-123" - RunVolumeWatcher(pe, testVolumeID, log) + testResourceUID := "test-resource-123" + RunVolumeWatcher(pe, testVolumeID, testResourceUID, log) dcpProc, dcpProcErr := findRunningDcp(pe) require.NoError(t, dcpProcErr) require.Equal(t, "monitor-container-volume", dcpProc.Cmd.Args[1]) require.Equal(t, "--volumeID", dcpProc.Cmd.Args[2]) require.Equal(t, testVolumeID, dcpProc.Cmd.Args[3]) - require.Equal(t, "--monitor", dcpProc.Cmd.Args[4]) - require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[5]) + require.Equal(t, "--resourceUID", dcpProc.Cmd.Args[4]) + require.Equal(t, testResourceUID, dcpProc.Cmd.Args[5]) + require.Equal(t, "--monitor", dcpProc.Cmd.Args[6]) + require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[7]) } func TestStopProcessTree(t *testing.T) { diff --git a/test/integration/v2_physical_container_volume_controller_test.go b/test/integration/v2_physical_container_volume_controller_test.go index b816f1e4..09977ada 100644 --- a/test/integration/v2_physical_container_volume_controller_test.go +++ b/test/integration/v2_physical_container_volume_controller_test.go @@ -127,6 +127,8 @@ func TestV2PhysicalContainerVolumeControllerHonorsCreatedVolumeCleanupPolicy(t * monitorProcesses := physicalContainerVolumeMonitorProcesses(readyVolume.Status.VolumeID) if removeRuntimeVolumeOnDelete { require.Len(t, monitorProcesses, 1) + require.Contains(t, monitorProcesses[0].Cmd.Args, "--resourceUID") + require.Contains(t, monitorProcesses[0].Cmd.Args, string(readyVolume.UID)) } else { require.Empty(t, monitorProcesses) } From d569482a8b9b73038bcd3bab964a8f98b40547cf Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 16:05:01 -0700 Subject: [PATCH 3/8] Keep V2 monitor changes out of V1 controller Restore the V1 container controller's private UID label declaration so the V2 volume monitor ownership fix does not create an unrelated V1 source change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/container_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/container_controller.go b/controllers/container_controller.go index 9c3077cc..4334267f 100644 --- a/controllers/container_controller.go +++ b/controllers/container_controller.go @@ -63,7 +63,7 @@ const ( dcpBuildLabel = "com.microsoft.developer.usvc-dev.build" groupVersionLabel = "com.microsoft.developer.usvc-dev.group-version" nameLabel = "com.microsoft.developer.usvc-dev.name" - uidLabel = containers.ResourceUIDLabel + uidLabel = "com.microsoft.developer.usvc-dev.uid" lifecycleKeyLabel = "com.microsoft.developer.usvc-dev.lifecycle-key" envLabel = "com.microsoft.developer.usvc-dev.env" mountsLabel = "com.microsoft.developer.usvc-dev.mountsLabel" From c0ca81048980464eaef722228b3a50c12ca8d7ee Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 16:10:31 -0700 Subject: [PATCH 4/8] Use resiliency retry for resource polling Replace the custom container-resource polling loop with the shared resiliency retry helper and a jittered constant-interval backoff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../dcpproc/commands/container_resource.go | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/internal/dcpproc/commands/container_resource.go b/internal/dcpproc/commands/container_resource.go index afeada08..419e1139 100644 --- a/internal/dcpproc/commands/container_resource.go +++ b/internal/dcpproc/commands/container_resource.go @@ -7,12 +7,17 @@ package commands import ( "context" - "math/rand" + "errors" "time" + "github.com/cenkalti/backoff/v4" "github.com/go-logr/logr" + + "github.com/microsoft/dcp/pkg/resiliency" ) +var errContainerResourceNotRemoved = errors.New("container resource has not been removed") + func pollContainerResourceRemoved( ctx context.Context, pollInterval time.Duration, @@ -20,33 +25,30 @@ func pollContainerResourceRemoved( inspectFailureMessage string, log logr.Logger, ) bool { - timer := time.NewTimer(containerResourcePollDelay(pollInterval)) - defer timer.Stop() + if ctx.Err() != nil { + return false + } - for { - select { - case <-ctx.Done(): - return false - case <-timer.C: - removed, inspectErr := inspect(ctx) - if removed { - return true - } - if inspectErr != nil { - if ctx.Err() != nil { - return false - } - log.Error(inspectErr, inspectFailureMessage) + pollBackoff := backoff.NewExponentialBackOff( + backoff.WithInitialInterval(pollInterval), + backoff.WithMaxInterval(pollInterval), + backoff.WithMaxElapsedTime(0), + backoff.WithRandomizationFactor(0.05), + backoff.WithMultiplier(1), + ) + pollErr := resiliency.Retry(ctx, pollBackoff, func() error { + removed, inspectErr := inspect(ctx) + if removed { + return nil + } + if inspectErr != nil { + if ctx.Err() != nil { + return inspectErr } - timer.Reset(containerResourcePollDelay(pollInterval)) + log.Error(inspectErr, inspectFailureMessage) + return inspectErr } - } -} - -func containerResourcePollDelay(pollInterval time.Duration) time.Duration { - jitterRange := pollInterval / 20 - if jitterRange <= 0 { - return pollInterval - } - return pollInterval + time.Duration(rand.Int63n(int64(jitterRange))) + return errContainerResourceNotRemoved + }) + return pollErr == nil } From 346f42a4a60d2964e01013bf22863a5cd19831ff Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 17:12:46 -0700 Subject: [PATCH 5/8] Narrow V2 crash monitoring scope Remove dedicated network and volume crash monitors while preserving normal controller deletion and retained-by-default volumes. Forward the selected container runtime through the existing container monitor API. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../physical_container_network_controller.go | 35 +-- .../physical_container_volume_controller.go | 35 +-- internal/containers/containers_common.go | 1 - internal/dcpctrl/commands/run_controllers.go | 2 - .../commands/container_resource_test.go | 249 ---------------- internal/dcpproc/commands/network.go | 229 --------------- internal/dcpproc/commands/root.go | 12 - internal/dcpproc/commands/volume.go | 268 ------------------ internal/dcpproc/dcpproc_api.go | 64 +---- internal/dcpproc/dcpproc_api_test.go | 45 ++- plan/v2-resource-plan.md | 4 +- test/integration/advanced_test_env.go | 2 - test/integration/standard_test_env.go | 2 - ...sical_container_network_controller_test.go | 10 - ...sical_container_network_durability_test.go | 8 +- ...ysical_container_volume_controller_test.go | 20 +- ...ysical_container_volume_durability_test.go | 12 +- 17 files changed, 51 insertions(+), 947 deletions(-) delete mode 100644 internal/dcpproc/commands/network.go delete mode 100644 internal/dcpproc/commands/volume.go diff --git a/controllers/physical_container_network_controller.go b/controllers/physical_container_network_controller.go index 4a57fe40..4e67baac 100644 --- a/controllers/physical_container_network_controller.go +++ b/controllers/physical_container_network_controller.go @@ -26,9 +26,7 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/internal/containers" - "github.com/microsoft/dcp/internal/dcpproc" "github.com/microsoft/dcp/pkg/commonapi" - "github.com/microsoft/dcp/pkg/process" "github.com/microsoft/dcp/pkg/resiliency" ) @@ -62,10 +60,9 @@ type physicalContainerNetworkDataInitializerFunc = stateInitializerFunc[ type PhysicalContainerNetworkReconciler struct { *ReconcilerBase[apiv2.PhysicalContainerNetwork, *apiv2.PhysicalContainerNetwork] - orchestrator containers.NetworkAttachmentOrchestrator - processExecutor process.Executor - networkData *ObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork] - operationQueue *resiliency.WorkQueue + orchestrator containers.NetworkAttachmentOrchestrator + networkData *ObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork] + operationQueue *resiliency.WorkQueue } func NewPhysicalContainerNetworkReconciler( @@ -74,14 +71,12 @@ func NewPhysicalContainerNetworkReconciler( noCacheClient ctrl_client.Reader, log logr.Logger, orchestrator containers.NetworkAttachmentOrchestrator, - processExecutor process.Executor, ) *PhysicalContainerNetworkReconciler { return &PhysicalContainerNetworkReconciler{ - ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerNetwork](client, noCacheClient, log, lifetimeCtx), - orchestrator: orchestrator, - processExecutor: processExecutor, - networkData: NewObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork](), - operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), + ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerNetwork](client, noCacheClient, log, lifetimeCtx), + orchestrator: orchestrator, + networkData: NewObjectStateMap[physicalContainerNetworkDataStateKey, physicalContainerNetworkData, *physicalContainerNetworkData, *apiv2.PhysicalContainerNetwork](), + operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), } } @@ -620,25 +615,9 @@ func handlePhysicalContainerNetworkCreated( networkID := data.networkID log.V(1).Info("Runtime network created; saving network status", "NetworkID", networkID) - reconciler.runPhysicalContainerNetworkLifecycleMonitor(network, networkID, log) return reconciler.applyRuntimeNetworkStatus(ctx, network, data, networkID, log) } -func (r *PhysicalContainerNetworkReconciler) runPhysicalContainerNetworkLifecycleMonitor( - network *apiv2.PhysicalContainerNetwork, - networkID string, - log logr.Logger, -) { - if network.Spec.Network == nil || network.Spec.Network.RetainRuntimeNetwork || networkID == "" { - return - } - if r.processExecutor == nil { - log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainerNetwork cleanup monitor") - return - } - dcpproc.RunNetworkWatcher(r.processExecutor, networkID, log) -} - func handlePhysicalContainerNetworkCreateFailure( ctx context.Context, reconciler *PhysicalContainerNetworkReconciler, diff --git a/controllers/physical_container_volume_controller.go b/controllers/physical_container_volume_controller.go index b1d75b77..199deb08 100644 --- a/controllers/physical_container_volume_controller.go +++ b/controllers/physical_container_volume_controller.go @@ -24,8 +24,6 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/internal/containers" - "github.com/microsoft/dcp/internal/dcpproc" - "github.com/microsoft/dcp/pkg/process" "github.com/microsoft/dcp/pkg/resiliency" ) @@ -56,10 +54,9 @@ type physicalContainerVolumeDataInitializerFunc = stateInitializerFunc[ type PhysicalContainerVolumeReconciler struct { *ReconcilerBase[apiv2.PhysicalContainerVolume, *apiv2.PhysicalContainerVolume] - orchestrator containers.VolumeOrchestrator - processExecutor process.Executor - volumeData *ObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume] - operationQueue *resiliency.WorkQueue + orchestrator containers.VolumeOrchestrator + volumeData *ObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume] + operationQueue *resiliency.WorkQueue } func NewPhysicalContainerVolumeReconciler( @@ -68,14 +65,12 @@ func NewPhysicalContainerVolumeReconciler( noCacheClient ctrl_client.Reader, log logr.Logger, orchestrator containers.VolumeOrchestrator, - processExecutor process.Executor, ) *PhysicalContainerVolumeReconciler { return &PhysicalContainerVolumeReconciler{ - ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerVolume](client, noCacheClient, log, lifetimeCtx), - orchestrator: orchestrator, - processExecutor: processExecutor, - volumeData: NewObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume](), - operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), + ReconcilerBase: NewReconcilerBase[apiv2.PhysicalContainerVolume](client, noCacheClient, log, lifetimeCtx), + orchestrator: orchestrator, + volumeData: NewObjectStateMap[physicalContainerVolumeDataStateKey, physicalContainerVolumeData, *physicalContainerVolumeData, *apiv2.PhysicalContainerVolume](), + operationQueue: resiliency.NewWorkQueue(lifetimeCtx, MaxConcurrentReconciles), } } @@ -493,25 +488,9 @@ func handlePhysicalContainerVolumeCreated( } log.V(1).Info("Runtime volume created; saving volume status", "VolumeID", data.volumeID) - reconciler.runPhysicalContainerVolumeLifecycleMonitor(volume, data.volumeID, log) return reconciler.applyRuntimeVolumeStatus(ctx, volume, data, data.volumeID, log) } -func (r *PhysicalContainerVolumeReconciler) runPhysicalContainerVolumeLifecycleMonitor( - volume *apiv2.PhysicalContainerVolume, - volumeID string, - log logr.Logger, -) { - if volume.Spec.Volume == nil || !volume.Spec.Volume.RemoveRuntimeVolumeOnDelete || volumeID == "" { - return - } - if r.processExecutor == nil { - log.Error(errors.New("process executor is not configured"), "Could not start PhysicalContainerVolume cleanup monitor") - return - } - dcpproc.RunVolumeWatcher(r.processExecutor, volumeID, string(volume.UID), log) -} - func handlePhysicalContainerVolumeCreateFailure( ctx context.Context, reconciler *PhysicalContainerVolumeReconciler, diff --git a/internal/containers/containers_common.go b/internal/containers/containers_common.go index c2453b26..7230fc9a 100644 --- a/internal/containers/containers_common.go +++ b/internal/containers/containers_common.go @@ -27,7 +27,6 @@ import ( const ( ContainerLogsHttpPath string = "/apis/usvc-dev.developer.microsoft.com/v1/containers/%s/log" ContainerHttpPath string = "/apis/usvc-dev.developer.microsoft.com/v1/containers/%s" - ResourceUIDLabel = "com.microsoft.developer.usvc-dev.uid" ) var ( diff --git a/internal/dcpctrl/commands/run_controllers.go b/internal/dcpctrl/commands/run_controllers.go index 93030ca7..95eacce3 100644 --- a/internal/dcpctrl/commands/run_controllers.go +++ b/internal/dcpctrl/commands/run_controllers.go @@ -293,7 +293,6 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), containerOrchestrator, - processExecutor, ) if err = physicalContainerNetworkCtrl.SetupWithManager(mgr, defaultControllerName); err != nil { log.Error(err, "Unable to set up PhysicalContainerNetwork controller") @@ -306,7 +305,6 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), containerOrchestrator, - processExecutor, ) if err = physicalContainerVolumeCtrl.SetupWithManager(mgr, defaultControllerName); err != nil { log.Error(err, "Unable to set up PhysicalContainerVolume controller") diff --git a/internal/dcpproc/commands/container_resource_test.go b/internal/dcpproc/commands/container_resource_test.go index f68d0e5c..e1b7dd2b 100644 --- a/internal/dcpproc/commands/container_resource_test.go +++ b/internal/dcpproc/commands/container_resource_test.go @@ -7,240 +7,14 @@ package commands import ( "context" - "errors" "testing" "time" - "github.com/cenkalti/backoff/v4" "github.com/stretchr/testify/require" - "github.com/microsoft/dcp/internal/containers" - ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/testutil" ) -func TestCleanupNetworkDisconnectsContainersWithoutRemovingThem(t *testing.T) { - t.Parallel() - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - - log := testutil.NewLogForTesting(t.Name()) - orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) - require.NoError(t, orchestratorErr) - defer func() { - require.NoError(t, orchestrator.Close()) - }() - - createdNetworkID, createNetworkErr := orchestrator.CreateNetwork(ctx, containers.CreateNetworkOptions{ - Name: "cleanup-network", - }) - require.NoError(t, createNetworkErr) - createdContainerID, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ - Name: "cleanup-network-container", - Image: "cleanup-network-image", - Networks: []containers.CreateContainerNetworkOptions{{Name: createdNetworkID}}, - }) - require.NoError(t, createContainerErr) - - require.NoError(t, doCleanupNetwork(ctx, createdNetworkID, log, orchestrator)) - - _, inspectNetworkErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{createdNetworkID}, - }) - require.ErrorIs(t, inspectNetworkErr, containers.ErrNotFound) - inspectedContainers, inspectContainerErr := orchestrator.InspectContainers(ctx, containers.InspectContainersOptions{ - Containers: []string{createdContainerID}, - }) - require.NoError(t, inspectContainerErr) - require.Len(t, inspectedContainers, 1) -} - -func TestCleanupVolumeDoesNotForceRemoval(t *testing.T) { - t.Parallel() - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - - log := testutil.NewLogForTesting(t.Name()) - orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) - require.NoError(t, orchestratorErr) - defer func() { - require.NoError(t, orchestrator.Close()) - }() - - const ( - createdVolumeID = "cleanup-volume" - resourceUID = "cleanup-volume-resource" - ) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ - Name: createdVolumeID, - Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, - })) - createdContainerID, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ - Name: "cleanup-volume-container", - Image: "cleanup-volume-image", - VolumeMounts: []containers.CreateContainerVolumeMount{{ - Type: containers.NamedVolumeMount, - Source: createdVolumeID, - Target: "/data", - }}, - }) - require.NoError(t, createContainerErr) - - cleanupErr := doCleanupVolume(ctx, createdVolumeID, resourceUID, orchestrator) - require.Error(t, cleanupErr) - - inspectedVolumes, inspectVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{createdVolumeID}, - }) - require.NoError(t, inspectVolumeErr) - require.Len(t, inspectedVolumes, 1) - _, removeContainerErr := orchestrator.RemoveContainers(ctx, containers.RemoveContainersOptions{ - Containers: []string{createdContainerID}, - Force: true, - }) - require.NoError(t, removeContainerErr) - require.NoError(t, doCleanupVolume(ctx, createdVolumeID, resourceUID, orchestrator)) - _, inspectRemovedVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{createdVolumeID}, - }) - require.True(t, errors.Is(inspectRemovedVolumeErr, containers.ErrNotFound)) -} - -func TestCleanupVolumeWaitsForContainerCleanup(t *testing.T) { - t.Parallel() - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - - log := testutil.NewLogForTesting(t.Name()) - orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) - require.NoError(t, orchestratorErr) - defer func() { - require.NoError(t, orchestrator.Close()) - }() - - const ( - createdVolumeID = "cleanup-volume-after-container" - createdContainerID = "cleanup-volume-after-container-container" - resourceUID = "cleanup-volume-after-container-resource" - ) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ - Name: createdVolumeID, - Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, - })) - _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ - Name: createdContainerID, - Image: "cleanup-volume-after-container-image", - VolumeMounts: []containers.CreateContainerVolumeMount{{ - Type: containers.NamedVolumeMount, - Source: createdVolumeID, - Target: "/data", - }}, - }) - require.NoError(t, createContainerErr) - - orderedOrchestrator := &removeContainerAfterVolumeAttemptOrchestrator{ - TestContainerOrchestrator: orchestrator, - containerID: createdContainerID, - } - require.NoError(t, cleanupVolumeAfterMonitorExit( - ctx, - createdVolumeID, - resourceUID, - backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), - log, - orderedOrchestrator, - )) - require.Equal(t, 2, orchestrator.RemoveVolumeCallCount(createdVolumeID)) - - _, inspectRemovedVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{createdVolumeID}, - }) - require.ErrorIs(t, inspectRemovedVolumeErr, containers.ErrNotFound) -} - -func TestCleanupVolumeStopsRetryingWhileContainerRemains(t *testing.T) { - t.Parallel() - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - - log := testutil.NewLogForTesting(t.Name()) - orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) - require.NoError(t, orchestratorErr) - defer func() { - require.NoError(t, orchestrator.Close()) - }() - - const ( - createdVolumeID = "bounded-cleanup-volume" - resourceUID = "bounded-cleanup-volume-resource" - ) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ - Name: createdVolumeID, - Labels: map[string]string{containers.ResourceUIDLabel: resourceUID}, - })) - _, createContainerErr := orchestrator.CreateContainer(ctx, containers.CreateContainerOptions{ - Name: "bounded-cleanup-volume-container", - Image: "bounded-cleanup-volume-image", - VolumeMounts: []containers.CreateContainerVolumeMount{{ - Type: containers.NamedVolumeMount, - Source: createdVolumeID, - Target: "/data", - }}, - }) - require.NoError(t, createContainerErr) - - cleanupErr := cleanupVolumeAfterMonitorExit( - ctx, - createdVolumeID, - resourceUID, - backoff.WithMaxRetries(backoff.NewConstantBackOff(time.Millisecond), 1), - log, - orchestrator, - ) - require.ErrorIs(t, cleanupErr, containers.ErrObjectInUse) - require.Equal(t, 2, orchestrator.RemoveVolumeCallCount(createdVolumeID)) -} - -func TestCleanupVolumePreservesSameNameReplacement(t *testing.T) { - t.Parallel() - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - - log := testutil.NewLogForTesting(t.Name()) - orchestrator, orchestratorErr := ctrl_testutil.NewTestContainerOrchestrator(ctx, log, ctrl_testutil.TcoOptionNone) - require.NoError(t, orchestratorErr) - defer func() { - require.NoError(t, orchestrator.Close()) - }() - - const ( - createdVolumeID = "replaced-cleanup-volume" - originalUID = "original-resource" - replacementUID = "replacement-resource" - ) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ - Name: createdVolumeID, - Labels: map[string]string{containers.ResourceUIDLabel: originalUID}, - })) - _, removeOriginalErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ - Volumes: []string{createdVolumeID}, - }) - require.NoError(t, removeOriginalErr) - require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{ - Name: createdVolumeID, - Labels: map[string]string{containers.ResourceUIDLabel: replacementUID}, - })) - - require.NoError(t, doCleanupVolume(ctx, createdVolumeID, originalUID, orchestrator)) - require.Equal(t, 1, orchestrator.RemoveVolumeCallCount(createdVolumeID)) - inspectedVolumes, inspectVolumeErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{createdVolumeID}, - }) - require.NoError(t, inspectVolumeErr) - require.Len(t, inspectedVolumes, 1) - require.Equal(t, replacementUID, inspectedVolumes[0].Labels[containers.ResourceUIDLabel]) -} - func TestPollContainerResourceRemovedReturnsFalseWhenContextIsCancelled(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) @@ -261,26 +35,3 @@ func TestPollContainerResourceRemovedReturnsFalseWhenContextIsCancelled(t *testi require.False(t, removed) require.False(t, inspectCalled) } - -type removeContainerAfterVolumeAttemptOrchestrator struct { - *ctrl_testutil.TestContainerOrchestrator - containerID string -} - -func (orchestrator *removeContainerAfterVolumeAttemptOrchestrator) RemoveVolumes( - ctx context.Context, - options containers.RemoveVolumesOptions, -) ([]string, error) { - removedVolumes, removeVolumeErr := orchestrator.TestContainerOrchestrator.RemoveVolumes(ctx, options) - if errors.Is(removeVolumeErr, containers.ErrObjectInUse) { - _, removeContainerErr := orchestrator.TestContainerOrchestrator.RemoveContainers( - ctx, - containers.RemoveContainersOptions{ - Containers: []string{orchestrator.containerID}, - Force: true, - }, - ) - return removedVolumes, errors.Join(removeVolumeErr, removeContainerErr) - } - return removedVolumes, removeVolumeErr -} diff --git a/internal/dcpproc/commands/network.go b/internal/dcpproc/commands/network.go deleted file mode 100644 index 9e13009e..00000000 --- a/internal/dcpproc/commands/network.go +++ /dev/null @@ -1,229 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See LICENSE in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -package commands - -import ( - "context" - "errors" - "fmt" - "time" - - "github.com/go-logr/logr" - "github.com/spf13/cobra" - - cmds "github.com/microsoft/dcp/internal/commands" - "github.com/microsoft/dcp/internal/containers" - container_flags "github.com/microsoft/dcp/internal/containers/flags" - container_runtimes "github.com/microsoft/dcp/internal/containers/runtimes" - "github.com/microsoft/dcp/pkg/logger" - "github.com/microsoft/dcp/pkg/process" -) - -const defaultNetworkPollInterval = 30 * time.Second - -var ( - networkID string - networkPollInterval time.Duration -) - -func NewNetworkCommand(log logr.Logger) (*cobra.Command, error) { - networkCmd := &cobra.Command{ - Use: "monitor-container-network", - Short: "Ensures that a container network is removed when the monitored process exits", - Long: `Ensures that a container network is removed when the monitored process exits. - -This command is used to ensure that container networks are properly cleaned up when -DCP terminates unexpectedly. Attached containers are disconnected without being removed.`, - RunE: monitorNetwork(log), - SilenceUsage: true, - Args: cobra.NoArgs, - } - - flagErr := addMonitorFlags(networkCmd) - if flagErr != nil { - return nil, flagErr - } - - networkCmd.Flags().StringVar(&networkID, "networkID", "", "The network ID or name to monitor and clean up when DCP exits") - flagErr = networkCmd.MarkFlagRequired("networkID") - if flagErr != nil { - return nil, flagErr - } - - networkCmd.Flags().DurationVar( - &networkPollInterval, - "networkPollInterval", - defaultNetworkPollInterval, - "How often to poll the network status to check if it has been removed. Default is 30 seconds.", - ) - flagErr = networkCmd.Flags().MarkHidden("networkPollInterval") - if flagErr != nil { - return nil, flagErr - } - - container_flags.EnsureRuntimeFlag(networkCmd.Flags()) - - return networkCmd, nil -} - -func monitorNetwork(log logr.Logger) func(cmd *cobra.Command, _ []string) error { - return func(cmd *cobra.Command, _ []string) error { - if networkID == "" { - return errors.New("network ID or name must be specified with --networkID") - } - - log = log.WithName("ContainerNetworkMonitor"). - WithValues( - "MonitorPID", monitorPid, - "Network", networkID, - ) - if resourceId != "" { - log = log.WithValues(logger.RESOURCE_LOG_STREAM_ID, resourceId) - } - - processExecutor := process.NewOSExecutor(log.WithName("ProcessExecutor")) - defer processExecutor.Dispose() - orchestrator, orchestratorErr := container_runtimes.FindAvailableContainerRuntime( - cmd.Context(), - log.WithName("ContainerOrchestrator").WithValues("ContainerRuntime", container_flags.GetRuntimeFlagValue()), - processExecutor, - ) - if orchestratorErr != nil { - log.Error(orchestratorErr, "Unable to ensure container network cleanup") - return orchestratorErr - } - - monitorCtx, monitorCtxCancel, monitorCtxErr := cmds.MonitorPid( - cmd.Context(), - process.NewHandle(monitorPid, monitorProcessStartTime), - monitorInterval, - log, - ) - defer monitorCtxCancel() - if monitorCtxErr != nil { - if isMonitorProcessGoneErr(monitorCtxErr) { - log.Info("Monitored process already exited, cleaning up container network", "Reason", monitorCtxErr) - return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) - } - - log.Error(monitorCtxErr, "Process could not be monitored") - return monitorCtxErr - } - - if pollNetworkRemoved(monitorCtx, networkID, orchestrator, log) { - return nil - } - - log.Info("Monitored process exited, cleaning up container network") - return doCleanupNetwork(cmd.Context(), networkID, log, orchestrator) - } -} - -func doCleanupNetwork( - ctx context.Context, - networkID string, - log logr.Logger, - orchestrator containers.NetworkAttachmentOrchestrator, -) error { - inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{networkID}, - }) - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - if errors.Is(inspectErr, containers.ErrNotFound) { - return nil - } - return fmt.Errorf("inspect container network before removal: %w", inspectErr) - } - if len(inspectedNetworks) == 0 { - return nil - } - - network := inspectedNetworks[0] - if orchestrator.IsBuiltInNetwork(network.Name) { - log.Info("Skipping cleanup of built-in container network", "NetworkName", network.Name) - return nil - } - - listedContainers, listErr := orchestrator.ListContainers(ctx, containers.ListContainersOptions{ - All: true, - Filters: containers.ListContainersFilters{ - NetworkFilters: []string{network.Id}, - }, - }) - if listErr != nil { - _, confirmErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{networkID}, - }) - if errors.Is(confirmErr, containers.ErrNotFound) { - return nil - } - return fmt.Errorf("list containers attached to container network: %w", errors.Join(listErr, confirmErr)) - } - - attachedContainerIDs := make(map[string]struct{}, len(network.Containers)+len(listedContainers)) - for _, attachedContainer := range network.Containers { - attachedContainerIDs[attachedContainer.Id] = struct{}{} - } - for _, listedContainer := range listedContainers { - attachedContainerIDs[listedContainer.Id] = struct{}{} - } - - var disconnectErrors error - for containerID := range attachedContainerIDs { - disconnectErr := orchestrator.DisconnectNetwork(ctx, containers.DisconnectNetworkOptions{ - Network: network.Id, - Container: containerID, - Force: true, - }) - if disconnectErr != nil && !errors.Is(disconnectErr, containers.ErrNotFound) { - disconnectErrors = errors.Join(disconnectErrors, disconnectErr) - } - } - if disconnectErrors != nil { - return fmt.Errorf("disconnect all containers from container network: %w", disconnectErrors) - } - - _, removeErr := orchestrator.RemoveNetworks(ctx, containers.RemoveNetworksOptions{ - Networks: []string{networkID}, - }) - if removeErr == nil { - return nil - } - - _, confirmErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{networkID}, - }) - if errors.Is(confirmErr, containers.ErrNotFound) { - return nil - } - return fmt.Errorf("remove container network: %w", errors.Join(removeErr, confirmErr)) -} - -func pollNetworkRemoved( - ctx context.Context, - networkID string, - orchestrator containers.InspectNetworks, - log logr.Logger, -) bool { - return pollContainerResourceRemoved( - ctx, - networkPollInterval, - func(ctx context.Context) (bool, error) { - inspectedNetworks, inspectErr := orchestrator.InspectNetworks(ctx, containers.InspectNetworksOptions{ - Networks: []string{networkID}, - }) - if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedNetworks) == 0) { - return true, nil - } - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - return false, inspectErr - } - return false, nil - }, - "Failed to inspect container network", - log, - ) -} diff --git a/internal/dcpproc/commands/root.go b/internal/dcpproc/commands/root.go index 9b0db5bd..9d4489c2 100644 --- a/internal/dcpproc/commands/root.go +++ b/internal/dcpproc/commands/root.go @@ -56,18 +56,6 @@ func NewRootCmd(log *logger.Logger) (*cobra.Command, error) { rootCmd.AddCommand(cmd) } - if cmd, err = NewNetworkCommand(log.Logger); err != nil { - return nil, fmt.Errorf("could not set up 'monitor-container-network' command: %w", err) - } else { - rootCmd.AddCommand(cmd) - } - - if cmd, err = NewVolumeCommand(log.Logger); err != nil { - return nil, fmt.Errorf("could not set up 'monitor-container-volume' command: %w", err) - } else { - rootCmd.AddCommand(cmd) - } - if cmd, err = NewStopProcessTreeCommand(log.Logger); err != nil { return nil, fmt.Errorf("could not set up 'stop-process-tree' command: %w", err) } else { diff --git a/internal/dcpproc/commands/volume.go b/internal/dcpproc/commands/volume.go deleted file mode 100644 index cfbde777..00000000 --- a/internal/dcpproc/commands/volume.go +++ /dev/null @@ -1,268 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See LICENSE in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -package commands - -import ( - "context" - "errors" - "fmt" - "time" - - "github.com/cenkalti/backoff/v4" - "github.com/go-logr/logr" - "github.com/spf13/cobra" - - cmds "github.com/microsoft/dcp/internal/commands" - "github.com/microsoft/dcp/internal/containers" - container_flags "github.com/microsoft/dcp/internal/containers/flags" - container_runtimes "github.com/microsoft/dcp/internal/containers/runtimes" - "github.com/microsoft/dcp/pkg/logger" - "github.com/microsoft/dcp/pkg/process" - "github.com/microsoft/dcp/pkg/resiliency" -) - -const ( - defaultVolumePollInterval = 30 * time.Second - volumeCleanupRetryInitialInterval = 500 * time.Millisecond - volumeCleanupRetryMaxInterval = 5 * time.Second - volumeCleanupRetryTimeout = 30 * time.Second - volumeCleanupRetryRandomizationFactor = 0.1 - volumeCleanupRetryBackoffMultiplier = 2.0 -) - -var ( - volumeID string - volumeResourceUID string - volumePollInterval time.Duration -) - -func NewVolumeCommand(log logr.Logger) (*cobra.Command, error) { - volumeCmd := &cobra.Command{ - Use: "monitor-container-volume", - Short: "Ensures that a container volume is removed when the monitored process exits", - Long: `Ensures that a container volume is removed when the monitored process exits. - -This command is used to ensure that container volumes are properly cleaned up when -DCP terminates unexpectedly. Volumes are never force-removed.`, - RunE: monitorVolume(log), - SilenceUsage: true, - Args: cobra.NoArgs, - } - - flagErr := addMonitorFlags(volumeCmd) - if flagErr != nil { - return nil, flagErr - } - - volumeCmd.Flags().StringVar(&volumeID, "volumeID", "", "The volume ID or name to monitor and clean up when DCP exits") - flagErr = volumeCmd.MarkFlagRequired("volumeID") - if flagErr != nil { - return nil, flagErr - } - - volumeCmd.Flags().StringVar(&volumeResourceUID, "resourceUID", "", "The UID of the PhysicalContainerVolume that created the volume") - flagErr = volumeCmd.MarkFlagRequired("resourceUID") - if flagErr != nil { - return nil, flagErr - } - - volumeCmd.Flags().DurationVar( - &volumePollInterval, - "volumePollInterval", - defaultVolumePollInterval, - "How often to poll the volume status to check if it has been removed. Default is 30 seconds.", - ) - flagErr = volumeCmd.Flags().MarkHidden("volumePollInterval") - if flagErr != nil { - return nil, flagErr - } - - container_flags.EnsureRuntimeFlag(volumeCmd.Flags()) - - return volumeCmd, nil -} - -func monitorVolume(log logr.Logger) func(cmd *cobra.Command, _ []string) error { - return func(cmd *cobra.Command, _ []string) error { - if volumeID == "" { - return errors.New("volume ID or name must be specified with --volumeID") - } - if volumeResourceUID == "" { - return errors.New("physical container volume UID must be specified with --resourceUID") - } - - log = log.WithName("ContainerVolumeMonitor"). - WithValues( - "MonitorPID", monitorPid, - "Volume", volumeID, - "ResourceUID", volumeResourceUID, - ) - if resourceId != "" { - log = log.WithValues(logger.RESOURCE_LOG_STREAM_ID, resourceId) - } - - processExecutor := process.NewOSExecutor(log.WithName("ProcessExecutor")) - defer processExecutor.Dispose() - orchestrator, orchestratorErr := container_runtimes.FindAvailableContainerRuntime( - cmd.Context(), - log.WithName("ContainerOrchestrator").WithValues("ContainerRuntime", container_flags.GetRuntimeFlagValue()), - processExecutor, - ) - if orchestratorErr != nil { - log.Error(orchestratorErr, "Unable to ensure container volume cleanup") - return orchestratorErr - } - - monitorCtx, monitorCtxCancel, monitorCtxErr := cmds.MonitorPid( - cmd.Context(), - process.NewHandle(monitorPid, monitorProcessStartTime), - monitorInterval, - log, - ) - defer monitorCtxCancel() - if monitorCtxErr != nil { - if isMonitorProcessGoneErr(monitorCtxErr) { - log.Info("Monitored process already exited, cleaning up container volume", "Reason", monitorCtxErr) - return cleanupVolumeAfterMonitorExit( - cmd.Context(), - volumeID, - volumeResourceUID, - newVolumeCleanupBackoff(), - log, - orchestrator, - ) - } - - log.Error(monitorCtxErr, "Process could not be monitored") - return monitorCtxErr - } - - if pollVolumeRemoved(monitorCtx, volumeID, volumeResourceUID, orchestrator, log) { - return nil - } - - log.Info("Monitored process exited, cleaning up container volume") - return cleanupVolumeAfterMonitorExit( - cmd.Context(), - volumeID, - volumeResourceUID, - newVolumeCleanupBackoff(), - log, - orchestrator, - ) - } -} - -func cleanupVolumeAfterMonitorExit( - ctx context.Context, - volumeID string, - resourceUID string, - retryPolicy backoff.BackOff, - log logr.Logger, - orchestrator containers.VolumeOrchestrator, -) error { - waitingForContainerCleanup := false - return resiliency.Retry(ctx, retryPolicy, func() error { - cleanupErr := doCleanupVolume(ctx, volumeID, resourceUID, orchestrator) - if cleanupErr == nil { - return nil - } - if !errors.Is(cleanupErr, containers.ErrObjectInUse) { - return resiliency.Permanent(cleanupErr) - } - - if !waitingForContainerCleanup { - log.Info( - "Container volume is still in use; waiting for container cleanup before retrying removal", - "Timeout", - volumeCleanupRetryTimeout, - ) - waitingForContainerCleanup = true - } else { - log.V(1).Info("Container volume is still in use; retrying after backoff") - } - return cleanupErr - }) -} - -func newVolumeCleanupBackoff() *backoff.ExponentialBackOff { - return backoff.NewExponentialBackOff( - backoff.WithInitialInterval(volumeCleanupRetryInitialInterval), - backoff.WithMaxInterval(volumeCleanupRetryMaxInterval), - backoff.WithMaxElapsedTime(volumeCleanupRetryTimeout), - backoff.WithRandomizationFactor(volumeCleanupRetryRandomizationFactor), - backoff.WithMultiplier(volumeCleanupRetryBackoffMultiplier), - ) -} - -func doCleanupVolume( - ctx context.Context, - volumeID string, - resourceUID string, - orchestrator containers.VolumeOrchestrator, -) error { - inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{volumeID}, - }) - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - if errors.Is(inspectErr, containers.ErrNotFound) { - return nil - } - return fmt.Errorf("inspect container volume before removal: %w", inspectErr) - } - if len(inspectedVolumes) == 0 { - return nil - } - if inspectedVolumes[0].Labels[containers.ResourceUIDLabel] != resourceUID { - return nil - } - - _, removeErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ - Volumes: []string{volumeID}, - Force: false, - }) - if removeErr == nil { - return nil - } - - _, confirmErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{volumeID}, - }) - if errors.Is(confirmErr, containers.ErrNotFound) { - return nil - } - return fmt.Errorf("remove container volume: %w", errors.Join(removeErr, confirmErr)) -} - -func pollVolumeRemoved( - ctx context.Context, - volumeID string, - resourceUID string, - orchestrator containers.InspectVolumes, - log logr.Logger, -) bool { - return pollContainerResourceRemoved( - ctx, - volumePollInterval, - func(ctx context.Context) (bool, error) { - inspectedVolumes, inspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{ - Volumes: []string{volumeID}, - }) - if errors.Is(inspectErr, containers.ErrNotFound) || (inspectErr == nil && len(inspectedVolumes) == 0) { - return true, nil - } - if len(inspectedVolumes) > 0 && inspectedVolumes[0].Labels[containers.ResourceUIDLabel] != resourceUID { - return true, nil - } - if inspectErr != nil && !errors.Is(inspectErr, containers.ErrIncomplete) { - return false, inspectErr - } - return false, nil - }, - "Failed to inspect container volume", - log, - ) -} diff --git a/internal/dcpproc/dcpproc_api.go b/internal/dcpproc/dcpproc_api.go index a41133c5..06b3ac56 100644 --- a/internal/dcpproc/dcpproc_api.go +++ b/internal/dcpproc/dcpproc_api.go @@ -17,6 +17,7 @@ import ( "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + container_flags "github.com/microsoft/dcp/internal/containers/flags" "github.com/microsoft/dcp/internal/dcppaths" internal_testutil "github.com/microsoft/dcp/internal/testutil" "github.com/microsoft/dcp/pkg/logger" @@ -136,6 +137,7 @@ func RunContainerWatcherForMonitorWithOptions( cmdArgs = append(cmdArgs, "--stop-only") } cmdArgs = append(cmdArgs, getMonitorCmdArgs(monitor)...) + cmdArgs = append(cmdArgs, getContainerRuntimeCmdArgs()...) startErr := startDcpProc(pe, cmdArgs) if startErr != nil { @@ -143,60 +145,6 @@ func RunContainerWatcherForMonitorWithOptions( } } -// RunNetworkWatcher starts a monitor that removes a container network if the current process exits. -// Failures are logged because the monitor is a best-effort reliability enhancement. -func RunNetworkWatcher( - pe process.Executor, - networkID string, - log logr.Logger, -) { - if _, found := os.LookupEnv(DCP_DISABLE_MONITOR_PROCESS); found { - return - } - - log = log.WithValues("NetworkID", networkID) - monitorPid := process.Uint32_ToPidT(uint32(os.Getpid())) - monitorIdentityTime := process.ProcessIdentityTime(monitorPid) - cmdArgs := []string{ - "monitor-container-network", - "--networkID", networkID, - } - cmdArgs = append(cmdArgs, getMonitorCmdArgs(process.NewHandle(monitorPid, monitorIdentityTime))...) - - startErr := startDcpProc(pe, cmdArgs) - if startErr != nil { - log.Error(startErr, "Failed to start container network monitor") - } -} - -// RunVolumeWatcher starts a monitor that removes a container volume if the current process exits. -// Failures are logged because the monitor is a best-effort reliability enhancement. -func RunVolumeWatcher( - pe process.Executor, - volumeID string, - resourceUID string, - log logr.Logger, -) { - if _, found := os.LookupEnv(DCP_DISABLE_MONITOR_PROCESS); found { - return - } - - log = log.WithValues("VolumeID", volumeID, "ResourceUID", resourceUID) - monitorPid := process.Uint32_ToPidT(uint32(os.Getpid())) - monitorIdentityTime := process.ProcessIdentityTime(monitorPid) - cmdArgs := []string{ - "monitor-container-volume", - "--volumeID", volumeID, - "--resourceUID", resourceUID, - } - cmdArgs = append(cmdArgs, getMonitorCmdArgs(process.NewHandle(monitorPid, monitorIdentityTime))...) - - startErr := startDcpProc(pe, cmdArgs) - if startErr != nil { - log.Error(startErr, "Failed to start container volume monitor") - } -} - // Runs stop-process-tree command to stop the process tree rooted at the given process. func StopProcessTree( ctx context.Context, @@ -248,6 +196,14 @@ func getMonitorCmdArgs(monitor process.ProcessHandle) []string { return cmdArgs } +func getContainerRuntimeCmdArgs() []string { + runtime := container_flags.GetRuntimeFlagValue() + if runtime == container_flags.UnknownRuntime { + return nil + } + return []string{container_flags.GetRuntimeFlag(), string(runtime)} +} + func startDcpProc(pe process.Executor, cmdArgs []string) error { dcpPath, dcpPathErr := dcppaths.GetDcpExePath() if dcpPathErr != nil { diff --git a/internal/dcpproc/dcpproc_api_test.go b/internal/dcpproc/dcpproc_api_test.go index d45dc501..5322ba09 100644 --- a/internal/dcpproc/dcpproc_api_test.go +++ b/internal/dcpproc/dcpproc_api_test.go @@ -10,13 +10,16 @@ import ( "fmt" "os" "os/exec" + "slices" "strconv" "testing" "time" + "github.com/spf13/pflag" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + container_flags "github.com/microsoft/dcp/internal/containers/flags" "github.com/microsoft/dcp/internal/dcppaths" internal_testutil "github.com/microsoft/dcp/internal/testutil" "github.com/microsoft/dcp/pkg/osutil" @@ -169,45 +172,29 @@ func TestRunContainerWatcherForMonitorWithStopOnly(t *testing.T) { require.Contains(t, dcpProc.Cmd.Args, "--stop-only", "Should include --stop-only flag") } -func TestRunNetworkWatcher(t *testing.T) { +func TestRunContainerWatcherPassesConfiguredRuntime(t *testing.T) { log := testutil.NewLogForTesting(t.Name()) ctx, cancel := testutil.GetTestContext(t, 20*time.Second) defer cancel() pe := internal_testutil.NewTestProcessExecutor(ctx) dcppaths.EnableTestPathProbing() - testNetworkID := "test-network-123" - RunNetworkWatcher(pe, testNetworkID, log) - - dcpProc, dcpProcErr := findRunningDcp(pe) - require.NoError(t, dcpProcErr) - require.Equal(t, "monitor-container-network", dcpProc.Cmd.Args[1]) - require.Equal(t, "--networkID", dcpProc.Cmd.Args[2]) - require.Equal(t, testNetworkID, dcpProc.Cmd.Args[3]) - require.Equal(t, "--monitor", dcpProc.Cmd.Args[4]) - require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[5]) -} - -func TestRunVolumeWatcher(t *testing.T) { - log := testutil.NewLogForTesting(t.Name()) - ctx, cancel := testutil.GetTestContext(t, 20*time.Second) - defer cancel() - pe := internal_testutil.NewTestProcessExecutor(ctx) - dcppaths.EnableTestPathProbing() + flagSet := pflag.NewFlagSet(t.Name(), pflag.ContinueOnError) + container_flags.EnsureRuntimeFlag(flagSet) + originalRuntime := container_flags.GetRuntimeFlagValue() + t.Cleanup(func() { + require.NoError(t, flagSet.Set(container_flags.RuntimeFlagName, string(originalRuntime))) + }) + require.NoError(t, flagSet.Set(container_flags.RuntimeFlagName, string(container_flags.PodmanRuntime))) - testVolumeID := "test-volume-123" - testResourceUID := "test-resource-123" - RunVolumeWatcher(pe, testVolumeID, testResourceUID, log) + RunContainerWatcher(pe, "test-container-123", log) dcpProc, dcpProcErr := findRunningDcp(pe) require.NoError(t, dcpProcErr) - require.Equal(t, "monitor-container-volume", dcpProc.Cmd.Args[1]) - require.Equal(t, "--volumeID", dcpProc.Cmd.Args[2]) - require.Equal(t, testVolumeID, dcpProc.Cmd.Args[3]) - require.Equal(t, "--resourceUID", dcpProc.Cmd.Args[4]) - require.Equal(t, testResourceUID, dcpProc.Cmd.Args[5]) - require.Equal(t, "--monitor", dcpProc.Cmd.Args[6]) - require.Equal(t, strconv.FormatInt(int64(os.Getpid()), 10), dcpProc.Cmd.Args[7]) + runtimeFlagIndex := slices.Index(dcpProc.Cmd.Args, container_flags.GetRuntimeFlag()) + require.GreaterOrEqual(t, runtimeFlagIndex, 0) + require.Less(t, runtimeFlagIndex+1, len(dcpProc.Cmd.Args)) + require.Equal(t, string(container_flags.PodmanRuntime), dcpProc.Cmd.Args[runtimeFlagIndex+1]) } func TestStopProcessTree(t *testing.T) { diff --git a/plan/v2-resource-plan.md b/plan/v2-resource-plan.md index 3c96877e..6a87b56a 100644 --- a/plan/v2-resource-plan.md +++ b/plan/v2-resource-plan.md @@ -79,10 +79,10 @@ This document tracks the intended direction for DCP V2 resources. The current V2 - `PhysicalContainerImage` provides source image pull and build workflows. The first runtime image ID successfully inspected by the controller is pinned for the resource lifetime and remains the only identity used for later inspection and dependent containers. If that exact image becomes unavailable, the resource reports it unavailable while retaining the published identity and metadata; it never silently pulls or builds a replacement. Delete and recreate the resource to realize a different image. - `PhysicalContainer` creates or tracks one runtime container, reports runtime status and port mappings, and references same-namespace `PhysicalContainerImage`, `PhysicalContainerVolume`, and `PhysicalContainerNetwork` resources. Container creation waits for every referenced physical resource to become ready. Bind mounts continue to use direct host paths, while named volume mounts resolve the referenced volume's observed runtime ID. - `PhysicalContainerNetwork` creates or references one runtime container network and reports its observed identity, driver, and address allocations. Its spec contains exactly one of top-level `networkID` or nested `network` creation config. Networks referenced by runtime ID are always retained. Created networks are retained when `network.retainRuntimeNetwork` is true; otherwise deletion enumerates running and stopped attachments, forcibly disconnects each container without removing it, and then removes the network. Name collisions are terminal unless `network.replaceExisting` is true, in which case the controller safely removes the specifically resolved network before creating its replacement. Runtime adapters classify their own built-in, non-removable networks, and replacement rejects them before disconnecting any attachments. -- `PhysicalContainerVolume` creates or references one runtime container volume and reports its observed identifier, driver, scope, mount point, and creation time. Its spec contains exactly one of top-level `volumeID` or nested `volume` creation config. Volumes referenced by runtime ID and newly created volumes are retained by default. Setting `volume.removeRuntimeVolumeOnDelete` opts a created volume into deletion and crash cleanup; deletion retries non-forced removal until the runtime releases the volume. Removal deliberately does not use force because Podman force-removes attached containers. During namespace deletion, each removable volume retries removal for up to 30 seconds so an externally attached volume cannot block graceful namespace cleanup indefinitely. Name collisions are terminal unless `volume.replaceExisting` is true, in which case the controller safely removes the specifically resolved volume before creating its replacement. Caller-supplied `volume.labels` pass through to created volumes, with reserved persistence, creator-process, and internal resource UID labels set by the controller. +- `PhysicalContainerVolume` creates or references one runtime container volume and reports its observed identifier, driver, scope, mount point, and creation time. Its spec contains exactly one of top-level `volumeID` or nested `volume` creation config. Volumes referenced by runtime ID and newly created volumes are retained by default. Setting `volume.removeRuntimeVolumeOnDelete` opts a created volume into controller-managed deletion. Deletion retries non-forced removal until the runtime releases the volume. Removal deliberately does not use force because Podman force-removes attached containers. During namespace deletion, each removable volume retries removal for up to 30 seconds so an externally attached volume cannot block graceful namespace cleanup indefinitely. Name collisions are terminal unless `volume.replaceExisting` is true, in which case the controller safely removes the specifically resolved volume before creating its replacement. Caller-supplied `volume.labels` pass through to created volumes, with reserved persistence, creator-process, and internal resource UID labels set by the controller. - `PhysicalProcess` launches or references one operating system process and reports its PID, PID-reuse identity timestamp, exit code when available, and lifecycle phase. Its spec contains exactly one of top-level `pid` or nested `process` creation config. Existing processes referenced by PID are observed and always retained when the resource is deleted. Created processes are stopped on deletion and namespace deletion unless `process.retainRuntimeProcess` is true. Deletion never blocks on runtime state: a resource that never took ownership of a running process drops its finalizer without stopping anything. The mutable top-level `stop` request can terminate either mode. Creation supports executable path, arguments, working directory, and environment without importing logical executable or IDE policy. - The physical resources use the shared `Pending`, `Ready`, `Unknown`, and `Failed` phases, specific `Ready` condition reasons, separate in-memory operation progress, and queued work where side effects can block. -- Created physical containers, networks, volumes, and processes launch best-effort monitor processes when they are configured to remove or stop their runtime object on Kubernetes resource deletion. Retained physical containers and processes can instead specify a monitor PID and identity timestamp; the retained runtime object is stopped, but not removed, when that process exits. Referenced runtime objects and retained resources without an explicit monitor do not launch cleanup monitors. +- Created physical containers and processes launch best-effort monitor processes when they are configured to remove or stop their runtime object on Kubernetes resource deletion. Retained physical containers and processes can instead specify a monitor PID and identity timestamp; the retained runtime object is stopped, but not removed, when that process exits. Referenced runtime objects and retained resources without an explicit monitor do not launch cleanup monitors. Abandoned networks and volumes remain eligible for workload-scoped cleanup. ## Follow-up roadmap diff --git a/test/integration/advanced_test_env.go b/test/integration/advanced_test_env.go index 75b4eca2..7033e6b0 100644 --- a/test/integration/advanced_test_env.go +++ b/test/integration/advanced_test_env.go @@ -266,7 +266,6 @@ func StartAdvancedTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), serverInfo.ContainerOrchestrator, - nil, ) if err = physicalContainerNetworkR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerNetworkReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerNetwork reconciler: %w", err) @@ -280,7 +279,6 @@ func StartAdvancedTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), serverInfo.ContainerOrchestrator, - nil, ) if err = physicalContainerVolumeR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerVolumeReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerVolume reconciler: %w", err) diff --git a/test/integration/standard_test_env.go b/test/integration/standard_test_env.go index 494eef79..1bccdb2d 100644 --- a/test/integration/standard_test_env.go +++ b/test/integration/standard_test_env.go @@ -292,7 +292,6 @@ func StartTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerNetworkReconciler"), serverInfo.ContainerOrchestrator, - pex, ) if err = physicalContainerNetworkR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerNetworkReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerNetwork reconciler: %w", err) @@ -306,7 +305,6 @@ func StartTestEnvironmentWithOptions( mgr.GetAPIReader(), log.WithName("PhysicalContainerVolumeReconciler"), serverInfo.ContainerOrchestrator, - pex, ) if err = physicalContainerVolumeR.SetupWithManager(mgr, instanceTag+"-PhysicalContainerVolumeReconciler"); err != nil { return nil, nil, fmt.Errorf("failed to initialize PhysicalContainerVolume reconciler: %w", err) diff --git a/test/integration/v2_physical_container_network_controller_test.go b/test/integration/v2_physical_container_network_controller_test.go index 03e575f1..a2a4c373 100644 --- a/test/integration/v2_physical_container_network_controller_test.go +++ b/test/integration/v2_physical_container_network_controller_test.go @@ -21,7 +21,6 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/controllers" "github.com/microsoft/dcp/internal/containers" - internal_testutil "github.com/microsoft/dcp/internal/testutil" ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/commonapi" "github.com/microsoft/dcp/pkg/slices" @@ -82,7 +81,6 @@ func TestV2PhysicalContainerNetworkControllerCreatesNetwork(t *testing.T) { require.NotEqual(t, "caller-value", labels[controllers.CreatorProcessIdLabel]) require.NotEmpty(t, labels[controllers.CreatorProcessStartTimeLabel]) require.NotEqual(t, "caller-value", labels[controllers.CreatorProcessStartTimeLabel]) - require.Len(t, physicalContainerNetworkMonitorProcesses(updatedNetwork.Status.NetworkID), 1) } func TestV2PhysicalContainerNetworkControllerTracksExistingNetwork(t *testing.T) { @@ -113,7 +111,6 @@ func TestV2PhysicalContainerNetworkControllerTracksExistingNetwork(t *testing.T) // Tracking must not create anything: the only create is the one this test performed. require.Equal(t, 1, containerOrchestrator.CreateNetworkCallCount(networkName)) - require.Empty(t, physicalContainerNetworkMonitorProcesses(networkID)) } func TestV2PhysicalContainerNetworkControllerRemovesCreatedNetworkOnDeletion(t *testing.T) { @@ -272,7 +269,6 @@ func TestV2PhysicalContainerNetworkControllerPreservesCreatedNetworkOnDeletion(t updatedNetwork := waitPhysicalContainerNetworkPhase(t, ctx, network.NamespacedName(), apiv2.PhysicalContainerNetworkPhaseReady) networkID := updatedNetwork.Status.NetworkID require.Equal(t, "true", runtimeNetworkLabels(t, ctx, networkName)[controllers.PersistentLabel]) - require.Empty(t, physicalContainerNetworkMonitorProcesses(networkID)) require.NoError(t, client.Delete(ctx, network)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerNetwork](t, ctx, client, network) @@ -1004,12 +1000,6 @@ func waitCreateNetworkCallCount(t *testing.T, ctx context.Context, networkName s require.NoError(t, waitErr) } -func physicalContainerNetworkMonitorProcesses(networkID string) []*internal_testutil.ProcessExecution { - return testProcessExecutor.FindAll([]string{"dcp", "monitor-container-network"}, "", func(processExecution *internal_testutil.ProcessExecution) bool { - return slices.Contains(processExecution.Cmd.Args, networkID) - }) -} - func waitInspectNetworkCallCount( t *testing.T, ctx context.Context, diff --git a/test/integration/v2_physical_container_network_durability_test.go b/test/integration/v2_physical_container_network_durability_test.go index 9582de65..7cc58d8f 100644 --- a/test/integration/v2_physical_container_network_durability_test.go +++ b/test/integration/v2_physical_container_network_durability_test.go @@ -175,7 +175,7 @@ func TestV2PhysicalContainerNetworkControllerQueuesDeletionBeforeRemovingFinaliz WithStatusSubresource(&apiv2.PhysicalContainerNetwork{}). WithObjects(network). Build() - reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator, nil) + reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator) request := ctrl.Request{NamespacedName: network.NamespacedName()} reconcileDone := make(chan error, 1) @@ -267,7 +267,7 @@ func TestV2PhysicalContainerNetworkControllerRetriesUncertainCreateCleanup(t *te }, } - reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator, nil) + reconciler := controllers.NewPhysicalContainerNetworkReconciler(ctx, baseClient, baseClient, log, orchestrator) request := ctrl.Request{NamespacedName: network.NamespacedName()} waitErr := wait.PollUntilContextCancel(ctx, waitPollInterval, pollImmediately, func(ctx context.Context) (bool, error) { _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -417,7 +417,6 @@ func TestV2PhysicalContainerNetworkControllerRetainsTerminalCreateFailureUntilSt baseClient, log, orchestrator, - nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -515,7 +514,6 @@ func TestV2PhysicalContainerNetworkControllerRetainsBuiltInFailureUntilStatusIsD baseClient, log, orchestrator, - nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -611,7 +609,6 @@ func TestV2PhysicalContainerNetworkControllerAdoptsOwnedNetworkBeforeReplacement baseClient, log, orchestrator, - nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} @@ -688,7 +685,6 @@ func TestV2PhysicalContainerNetworkControllerRetainsCreatedNetworkUntilStatusIsD baseClient, log, orchestrator, - nil, ) request := ctrl.Request{NamespacedName: network.NamespacedName()} diff --git a/test/integration/v2_physical_container_volume_controller_test.go b/test/integration/v2_physical_container_volume_controller_test.go index 09977ada..7e0fa02f 100644 --- a/test/integration/v2_physical_container_volume_controller_test.go +++ b/test/integration/v2_physical_container_volume_controller_test.go @@ -22,10 +22,8 @@ import ( apiv2 "github.com/microsoft/dcp/api/v2" "github.com/microsoft/dcp/controllers" "github.com/microsoft/dcp/internal/containers" - internal_testutil "github.com/microsoft/dcp/internal/testutil" ctrl_testutil "github.com/microsoft/dcp/internal/testutil/ctrlutil" "github.com/microsoft/dcp/pkg/commonapi" - "github.com/microsoft/dcp/pkg/slices" "github.com/microsoft/dcp/pkg/testutil" ) @@ -71,7 +69,6 @@ func TestV2PhysicalContainerVolumeControllerCreatesRetainedVolumeByDefault(t *te require.NotEqual(t, "caller-value", inspectedVolume.Labels[controllers.CreatorProcessIdLabel]) require.NotEmpty(t, inspectedVolume.Labels[controllers.CreatorProcessStartTimeLabel]) require.NotEqual(t, "caller-value", inspectedVolume.Labels[controllers.CreatorProcessStartTimeLabel]) - require.Empty(t, physicalContainerVolumeMonitorProcesses(readyVolume.Status.VolumeID)) } func TestV2PhysicalContainerVolumeControllerRetainsReferencedVolume(t *testing.T) { @@ -93,7 +90,6 @@ func TestV2PhysicalContainerVolumeControllerRetainsReferencedVolume(t *testing.T readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) require.Equal(t, volumeName, readyVolume.Status.VolumeID) require.Equal(t, 1, containerOrchestrator.CreateVolumeCallCount(volumeName)) - require.Empty(t, physicalContainerVolumeMonitorProcesses(volumeName)) require.NoError(t, client.Delete(ctx, volume)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerVolume](t, ctx, client, volume) @@ -123,15 +119,7 @@ func TestV2PhysicalContainerVolumeControllerHonorsCreatedVolumeCleanupPolicy(t * }, } require.NoError(t, client.Create(ctx, volume)) - readyVolume := waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) - monitorProcesses := physicalContainerVolumeMonitorProcesses(readyVolume.Status.VolumeID) - if removeRuntimeVolumeOnDelete { - require.Len(t, monitorProcesses, 1) - require.Contains(t, monitorProcesses[0].Cmd.Args, "--resourceUID") - require.Contains(t, monitorProcesses[0].Cmd.Args, string(readyVolume.UID)) - } else { - require.Empty(t, monitorProcesses) - } + waitPhysicalContainerVolumePhase(t, ctx, volume.NamespacedName(), apiv2.PhysicalContainerVolumePhaseReady) require.NoError(t, client.Delete(ctx, volume)) ctrl_testutil.WaitObjectDeleted[apiv2.PhysicalContainerVolume](t, ctx, client, volume) @@ -723,12 +711,6 @@ func waitCreateVolumeCallCount(t *testing.T, ctx context.Context, volumeName str require.NoError(t, waitErr) } -func physicalContainerVolumeMonitorProcesses(volumeID string) []*internal_testutil.ProcessExecution { - return testProcessExecutor.FindAll([]string{"dcp", "monitor-container-volume"}, "", func(processExecution *internal_testutil.ProcessExecution) bool { - return slices.Contains(processExecution.Cmd.Args, volumeID) - }) -} - func waitInspectVolumeCallCount( t *testing.T, ctx context.Context, diff --git a/test/integration/v2_physical_container_volume_durability_test.go b/test/integration/v2_physical_container_volume_durability_test.go index c13844b2..ee6a515a 100644 --- a/test/integration/v2_physical_container_volume_durability_test.go +++ b/test/integration/v2_physical_container_volume_durability_test.go @@ -136,7 +136,7 @@ func TestV2PhysicalContainerVolumeControllerBoundsRemovalDuringNamespaceDeletion }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} currentVolume := waitPhysicalContainerVolumeConditionReason( @@ -187,7 +187,7 @@ func TestV2PhysicalContainerVolumeControllerRetriesBeforeNamespaceRemovalDeadlin }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} require.NoError(t, baseClient.Delete(ctx, volume)) @@ -240,7 +240,7 @@ func TestV2PhysicalContainerVolumeControllerDoesNotBoundDirectRemoval(t *testing }, })) orchestrator.FailNextRemoveVolume(volumeName, errors.New("volume remains in use")) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} currentVolume := waitPhysicalContainerVolumeConditionReason( @@ -290,7 +290,7 @@ func TestV2PhysicalContainerVolumeControllerRetriesUncertainCreateCleanup(t *tes }, } - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, baseClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} waitErr := wait.PollUntilContextCancel(ctx, waitPollInterval, pollImmediately, func(ctx context.Context) (bool, error) { _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -374,7 +374,7 @@ func TestV2PhysicalContainerVolumeControllerRetainsCreatedVolumeUntilStatusIsDur }, } orchestrator := newDurabilityTestContainerOrchestrator(t, ctx) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} _, reconcileErr := reconciler.Reconcile(ctx, request) @@ -441,7 +441,7 @@ func TestV2PhysicalContainerVolumeControllerRetainsTerminalFailureUntilStatusIsD } orchestrator := newDurabilityTestContainerOrchestrator(t, ctx) require.NoError(t, orchestrator.CreateVolume(ctx, containers.CreateVolumeOptions{Name: volume.Spec.Volume.VolumeName})) - reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator, nil) + reconciler := controllers.NewPhysicalContainerVolumeReconciler(ctx, statusClient, baseClient, testutil.NewLogForTesting(t.Name()), orchestrator) request := ctrl.Request{NamespacedName: volume.NamespacedName()} _, reconcileErr := reconciler.Reconcile(ctx, request) From be68e85319055eaba26091a53c12f214d090bcbf Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 17:26:43 -0700 Subject: [PATCH 6/8] Restore container monitor polling Restore the pre-existing pollContainerRemoved behavior and remove the shared polling abstraction that is no longer needed after dropping network and volume crash monitors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/dcpproc/commands/container.go | 70 ++++++++++++++----- .../dcpproc/commands/container_resource.go | 54 -------------- .../commands/container_resource_test.go | 37 ---------- 3 files changed, 51 insertions(+), 110 deletions(-) delete mode 100644 internal/dcpproc/commands/container_resource.go delete mode 100644 internal/dcpproc/commands/container_resource_test.go diff --git a/internal/dcpproc/commands/container.go b/internal/dcpproc/commands/container.go index 9e8aad5b..d0fd8090 100644 --- a/internal/dcpproc/commands/container.go +++ b/internal/dcpproc/commands/container.go @@ -9,6 +9,7 @@ import ( "context" "errors" "fmt" + "math/rand" "time" "github.com/go-logr/logr" @@ -125,12 +126,16 @@ func monitorContainer(log logr.Logger) func(cmd *cobra.Command, args []string) e } } - if pollContainerRemoved(monitorCtx, containerID, co, log) { + ctrRemovedCh := pollContainerRemoved(monitorCtx, containerID, co, log) + + select { + case <-ctrRemovedCh: + // Container was removed, we are done return nil + case <-monitorCtx.Done(): + log.Info("Monitored process exited, cleaning up container") + return doCleanupContainer(cmd.Context(), containerID, containerStopOnly, log, co) } - - log.Info("Monitored process exited, cleaning up container") - return doCleanupContainer(cmd.Context(), containerID, containerStopOnly, log, co) } } @@ -224,20 +229,47 @@ func doCleanupContainer( return nil } -func pollContainerRemoved(ctx context.Context, containerID string, co inspectStopRemoveContainers, log logr.Logger) bool { - return pollContainerResourceRemoved( - ctx, - containerPollInterval, - func(ctx context.Context) (bool, error) { - _, inspectErr := co.InspectContainers(ctx, containers.InspectContainersOptions{ - Containers: []string{containerID}, - }) - if errors.Is(inspectErr, containers.ErrNotFound) { - return true, nil +func pollContainerRemoved(ctx context.Context, containerID string, co inspectStopRemoveContainers, log logr.Logger) <-chan struct{} { + ctrRemovedCh := make(chan struct{}) + + jitter := func() time.Duration { + // Up to 5% of the poll interval, to avoid all instances of dcpproc polling at the same exact time + return time.Duration(rand.Int63n(int64(containerPollInterval / 20.0))) + } + + go func() { + defer close(ctrRemovedCh) + // Use the configured poll interval (overridable via hidden flag for tests) + timer := time.NewTimer(containerPollInterval + jitter()) + defer timer.Stop() + + for { + select { + + case <-ctx.Done(): + return + + case <-timer.C: + // Poll the container status + _, inspectErr := co.InspectContainers(ctx, containers.InspectContainersOptions{ + Containers: []string{containerID}, + }) + + if inspectErr != nil { + if errors.Is(inspectErr, containers.ErrNotFound) { + // Container has been removed, we should exit, which will close the channel + // and notify the caller. + return + } else { + log.Error(inspectErr, "Failed to inspect container") + // May be transient error, continue polling + } + } + + timer.Reset(containerPollInterval + jitter()) } - return false, inspectErr - }, - "Failed to inspect container", - log, - ) + } + }() + + return ctrRemovedCh } diff --git a/internal/dcpproc/commands/container_resource.go b/internal/dcpproc/commands/container_resource.go deleted file mode 100644 index 419e1139..00000000 --- a/internal/dcpproc/commands/container_resource.go +++ /dev/null @@ -1,54 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See LICENSE in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -package commands - -import ( - "context" - "errors" - "time" - - "github.com/cenkalti/backoff/v4" - "github.com/go-logr/logr" - - "github.com/microsoft/dcp/pkg/resiliency" -) - -var errContainerResourceNotRemoved = errors.New("container resource has not been removed") - -func pollContainerResourceRemoved( - ctx context.Context, - pollInterval time.Duration, - inspect func(context.Context) (bool, error), - inspectFailureMessage string, - log logr.Logger, -) bool { - if ctx.Err() != nil { - return false - } - - pollBackoff := backoff.NewExponentialBackOff( - backoff.WithInitialInterval(pollInterval), - backoff.WithMaxInterval(pollInterval), - backoff.WithMaxElapsedTime(0), - backoff.WithRandomizationFactor(0.05), - backoff.WithMultiplier(1), - ) - pollErr := resiliency.Retry(ctx, pollBackoff, func() error { - removed, inspectErr := inspect(ctx) - if removed { - return nil - } - if inspectErr != nil { - if ctx.Err() != nil { - return inspectErr - } - log.Error(inspectErr, inspectFailureMessage) - return inspectErr - } - return errContainerResourceNotRemoved - }) - return pollErr == nil -} diff --git a/internal/dcpproc/commands/container_resource_test.go b/internal/dcpproc/commands/container_resource_test.go deleted file mode 100644 index e1b7dd2b..00000000 --- a/internal/dcpproc/commands/container_resource_test.go +++ /dev/null @@ -1,37 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See LICENSE in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -package commands - -import ( - "context" - "testing" - "time" - - "github.com/stretchr/testify/require" - - "github.com/microsoft/dcp/pkg/testutil" -) - -func TestPollContainerResourceRemovedReturnsFalseWhenContextIsCancelled(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - inspectCalled := false - removed := pollContainerResourceRemoved( - ctx, - time.Hour, - func(context.Context) (bool, error) { - inspectCalled = true - return false, nil - }, - "Unexpected inspection failure", - testutil.NewLogForTesting(t.Name()), - ) - - require.False(t, removed) - require.False(t, inspectCalled) -} From 12f42a6c73b7cef449e7242a35b5fd6eab7475e1 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Thu, 17 Sep 2026 18:33:52 -0700 Subject: [PATCH 7/8] Fix monitor timestamp assertions on Linux Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- test/integration/v2_physical_container_controller_test.go | 2 +- test/integration/v2_physical_process_controller_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/v2_physical_container_controller_test.go b/test/integration/v2_physical_container_controller_test.go index b7ffaba7..6578b413 100644 --- a/test/integration/v2_physical_container_controller_test.go +++ b/test/integration/v2_physical_container_controller_test.go @@ -1260,7 +1260,7 @@ func TestV2PhysicalContainerControllerScopesRetainedContainerToMonitorProcess(t updatedContainer := waitPhysicalContainerPhase(t, ctx, container.NamespacedName(), apiv2.PhysicalContainerPhaseRunning) require.NotNil(t, updatedContainer.Spec.Container.MonitorPID) require.Equal(t, monitorPID, *updatedContainer.Spec.Container.MonitorPID) - require.True(t, monitorTimestamp.Time.Equal(updatedContainer.Spec.Container.MonitorTimestamp.Time)) + require.True(t, osutil.Within(monitorTimestamp.Time, updatedContainer.Spec.Container.MonitorTimestamp.Time, 2*time.Microsecond)) containerID := updatedContainer.Status.ContainerID removeRuntimeContainerOnCleanup(t, containerID) var monitorProcesses []*internal_testutil.ProcessExecution diff --git a/test/integration/v2_physical_process_controller_test.go b/test/integration/v2_physical_process_controller_test.go index 6d70d1e5..10c9f1b3 100644 --- a/test/integration/v2_physical_process_controller_test.go +++ b/test/integration/v2_physical_process_controller_test.go @@ -662,7 +662,7 @@ func TestV2PhysicalProcessControllerDeletesOrRetainsCreatedProcess(t *testing.T) if testCase.customMonitorPID != nil { require.NotNil(t, runningProcess.Spec.Process.MonitorPID) require.Equal(t, *testCase.customMonitorPID, *runningProcess.Spec.Process.MonitorPID) - require.True(t, monitorTimestamp.Time.Equal(runningProcess.Spec.Process.MonitorTimestamp.Time)) + require.True(t, osutil.Within(monitorTimestamp.Time, runningProcess.Spec.Process.MonitorTimestamp.Time, 2*time.Microsecond)) } pid, convertErr := process.Int64_ToPidT(*runningProcess.Status.PID) require.NoError(t, convertErr) From 4524895fce02f644210db79e84ef13c684c883a4 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Fri, 18 Sep 2026 13:32:30 -0700 Subject: [PATCH 8/8] Preserve implicitly selected container runtime Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../containers/flags/container_runtime.go | 5 ++ internal/containers/runtimes/runtime.go | 5 ++ internal/containers/runtimes/runtime_test.go | 64 +++++++++++++++++++ internal/dcpproc/dcpproc_api_test.go | 9 +-- 4 files changed, 77 insertions(+), 6 deletions(-) create mode 100644 internal/containers/runtimes/runtime_test.go diff --git a/internal/containers/flags/container_runtime.go b/internal/containers/flags/container_runtime.go index 13a5d5e7..1a876d7e 100644 --- a/internal/containers/flags/container_runtime.go +++ b/internal/containers/flags/container_runtime.go @@ -40,6 +40,11 @@ func GetRuntimeFlagValue() RuntimeFlagValue { return runtime } +// SetRuntimeFlagValue sets the container runtime used by subsequent runtime consumers. +func SetRuntimeFlagValue(value RuntimeFlagValue) error { + return runtime.Set(string(value)) +} + func (rf *RuntimeFlagValue) Set(flagValue string) error { if flagValue == string(UnknownRuntime) || slices.ContainsFunc(supportedRuntimeNames, func(name string) bool { return name == strings.ToLower(flagValue) diff --git a/internal/containers/runtimes/runtime.go b/internal/containers/runtimes/runtime.go index fa1a76ea..54ad4f38 100644 --- a/internal/containers/runtimes/runtime.go +++ b/internal/containers/runtimes/runtime.go @@ -79,6 +79,11 @@ func FindAvailableContainerRuntime(ctx context.Context, log logr.Logger, executo return nil, errNoRuntimeFound } + selectedRuntimeErr := flags.SetRuntimeFlagValue(flags.RuntimeFlagValue(availableRuntime.orchestrator.Name())) + if selectedRuntimeErr != nil { + return nil, fmt.Errorf("record selected container runtime: %w", selectedRuntimeErr) + } + log.V(1).Info("Runtime status", "Runtime", availableRuntime.orchestrator.Name(), "Status", availableRuntime.status) return availableRuntime.orchestrator, nil diff --git a/internal/containers/runtimes/runtime_test.go b/internal/containers/runtimes/runtime_test.go new file mode 100644 index 00000000..7fe5b54d --- /dev/null +++ b/internal/containers/runtimes/runtime_test.go @@ -0,0 +1,64 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +package runtimes + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + + "github.com/microsoft/dcp/internal/containers" + "github.com/microsoft/dcp/internal/containers/flags" + "github.com/microsoft/dcp/pkg/process" +) + +type testContainerOrchestrator struct { + containers.ContainerOrchestrator + name string + status containers.ContainerRuntimeStatus +} + +func (o *testContainerOrchestrator) IsDefault() bool { + return false +} + +func (o *testContainerOrchestrator) Name() string { + return o.name +} + +func (o *testContainerOrchestrator) CheckStatus(context.Context, containers.CachedRuntimeStatusUsage) containers.ContainerRuntimeStatus { + return o.status +} + +func TestFindAvailableContainerRuntimeRecordsImplicitSelection(t *testing.T) { + originalRuntime := flags.GetRuntimeFlagValue() + originalSupportedRuntimes := supportedRuntimes + t.Cleanup(func() { + supportedRuntimes = originalSupportedRuntimes + require.NoError(t, flags.SetRuntimeFlagValue(originalRuntime)) + }) + + require.NoError(t, flags.SetRuntimeFlagValue(flags.UnknownRuntime)) + supportedRuntimes = map[flags.RuntimeFlagValue]ContainerOrchestratorFactory{ + flags.PodmanRuntime: func(logr.Logger, process.Executor) containers.ContainerOrchestrator { + return &testContainerOrchestrator{ + name: string(flags.PodmanRuntime), + status: containers.ContainerRuntimeStatus{ + Installed: true, + Running: true, + }, + } + }, + } + + orchestrator, findErr := FindAvailableContainerRuntime(context.Background(), logr.Discard(), nil) + + require.NoError(t, findErr) + require.Equal(t, string(flags.PodmanRuntime), orchestrator.Name()) + require.Equal(t, flags.PodmanRuntime, flags.GetRuntimeFlagValue()) +} diff --git a/internal/dcpproc/dcpproc_api_test.go b/internal/dcpproc/dcpproc_api_test.go index 5322ba09..e2947200 100644 --- a/internal/dcpproc/dcpproc_api_test.go +++ b/internal/dcpproc/dcpproc_api_test.go @@ -15,7 +15,6 @@ import ( "testing" "time" - "github.com/spf13/pflag" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -172,20 +171,18 @@ func TestRunContainerWatcherForMonitorWithStopOnly(t *testing.T) { require.Contains(t, dcpProc.Cmd.Args, "--stop-only", "Should include --stop-only flag") } -func TestRunContainerWatcherPassesConfiguredRuntime(t *testing.T) { +func TestRunContainerWatcherPassesSelectedRuntime(t *testing.T) { log := testutil.NewLogForTesting(t.Name()) ctx, cancel := testutil.GetTestContext(t, 20*time.Second) defer cancel() pe := internal_testutil.NewTestProcessExecutor(ctx) dcppaths.EnableTestPathProbing() - flagSet := pflag.NewFlagSet(t.Name(), pflag.ContinueOnError) - container_flags.EnsureRuntimeFlag(flagSet) originalRuntime := container_flags.GetRuntimeFlagValue() t.Cleanup(func() { - require.NoError(t, flagSet.Set(container_flags.RuntimeFlagName, string(originalRuntime))) + require.NoError(t, container_flags.SetRuntimeFlagValue(originalRuntime)) }) - require.NoError(t, flagSet.Set(container_flags.RuntimeFlagName, string(container_flags.PodmanRuntime))) + require.NoError(t, container_flags.SetRuntimeFlagValue(container_flags.PodmanRuntime)) RunContainerWatcher(pe, "test-container-123", log)