Skip to content

Commit 025dd77

Browse files
dmjbdmjb
committed
Use status files to retrieve PID
This follows on from the previous PRs which migrated PIDs from the PID files to status files.
1 parent 8396023 commit 025dd77

File tree

6 files changed

+148
-127
lines changed

6 files changed

+148
-127
lines changed

‎pkg/transport/proxy/manager.go‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,34 +9,6 @@ import (
99
// We may want to move these operations behind an interface. For now, they
1010
// have been moved to this package to keep proxy-related logic grouped together.
1111

12-
// StopProcess stops the proxy process associated with the container
13-
func StopProcess(containerBaseName string) {
14-
if containerBaseName == "" {
15-
logger.Warnf("Warning: Could not find base container name in labels")
16-
return
17-
}
18-
19-
// Try to read the PID file and kill the process
20-
pid, err := process.ReadPIDFile(containerBaseName)
21-
if err != nil {
22-
logger.Errorf("No PID file found for %s, proxy may not be running in detached mode", containerBaseName)
23-
return
24-
}
25-
26-
// PID file found, try to kill the process
27-
logger.Infof("Stopping proxy process (PID: %d)...", pid)
28-
if err := process.KillProcess(pid); err != nil {
29-
logger.Warnf("Warning: Failed to kill proxy process: %v", err)
30-
} else {
31-
logger.Info("Proxy process stopped")
32-
}
33-
34-
// Remove the PID file
35-
if err := process.RemovePIDFile(containerBaseName); err != nil {
36-
logger.Warnf("Warning: Failed to remove PID file: %v", err)
37-
}
38-
}
39-
4012
// IsRunning checks if the proxy process is running
4113
func IsRunning(containerBaseName string) bool {
4214
logger.Debugf("Checking if proxy process is running for container %s", containerBaseName)

‎pkg/workloads/manager.go‎

Lines changed: 53 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ func (d *defaultManager) stopRemoteWorkload(ctx context.Context, name string, ru
266266

267267
// Stop proxy if running
268268
if runConfig.BaseName != "" {
269-
d.stopProxyIfNeeded(ctx, name, runConfig.BaseName)
269+
d.stopProxyIfNeeded(name, runConfig.BaseName)
270270
}
271271

272272
// For remote workloads, we only need to clean up client configurations
@@ -497,7 +497,7 @@ func (d *defaultManager) deleteRemoteWorkload(ctx context.Context, name string,
497497

498498
// Stop proxy if running
499499
if runConfig.BaseName != "" {
500-
d.stopProxyIfNeeded(ctx, name, runConfig.BaseName)
500+
d.stopProxyIfNeeded(name, runConfig.BaseName)
501501
}
502502

503503
// Clean up associated resources
@@ -532,7 +532,7 @@ func (d *defaultManager) deleteContainerWorkload(ctx context.Context, name strin
532532

533533
// Stop proxy if running
534534
if container.IsRunning() {
535-
d.stopProxyIfNeeded(ctx, name, baseName)
535+
d.stopProxyIfNeeded(name, baseName)
536536
}
537537

538538
// Remove the container
@@ -569,16 +569,39 @@ func (d *defaultManager) getWorkloadContainer(ctx context.Context, name string)
569569
return &container, nil
570570
}
571571

572+
// stopProcess stops the proxy process associated with the container
573+
func (*defaultManager) stopProcess(name string) {
574+
if name == "" {
575+
logger.Warnf("Warning: Could not find base container name in labels")
576+
return
577+
}
578+
579+
// Try to read the PID file and kill the process
580+
pid, err := process.ReadPIDFile(name)
581+
if err != nil {
582+
logger.Errorf("No PID file found for %s, proxy may not be running in detached mode", name)
583+
return
584+
}
585+
586+
// PID file found, try to kill the process
587+
logger.Infof("Stopping proxy process (PID: %d)...", pid)
588+
if err := process.KillProcess(pid); err != nil {
589+
logger.Warnf("Warning: Failed to kill proxy process: %v", err)
590+
} else {
591+
logger.Info("Proxy process stopped")
592+
}
593+
594+
// Clean up PID file after successful kill
595+
if err := process.RemovePIDFile(name); err != nil {
596+
logger.Warnf("Warning: Failed to remove PID file: %v", err)
597+
}
598+
}
599+
572600
// stopProxyIfNeeded stops the proxy process if the workload has a base name
573-
func (d *defaultManager) stopProxyIfNeeded(ctx context.Context, name, baseName string) {
601+
func (d *defaultManager) stopProxyIfNeeded(name, baseName string) {
574602
logger.Infof("Removing proxy process for %s...", name)
575603
if baseName != "" {
576-
proxy.StopProcess(baseName)
577-
// TODO: refactor the StopProcess function to stop dealing explicitly with PID files.
578-
// Note that this is not a blocker for k8s since this code path is not called there.
579-
if err := d.statuses.ResetWorkloadPID(ctx, baseName); err != nil {
580-
logger.Warnf("Warning: Failed to reset workload %s PID: %v", name, err)
581-
}
604+
d.stopProcess(baseName)
582605
}
583606
}
584607

@@ -788,32 +811,40 @@ func (d *defaultManager) restartContainerWorkload(ctx context.Context, name stri
788811
workloadName = name
789812
}
790813

791-
// Get workload state information using the original name
792-
workloadState, err := d.getWorkloadState(ctx, name)
793-
if err != nil {
814+
// Get workload status using the status manager
815+
workload, err := d.statuses.GetWorkload(ctx, name)
816+
if err != nil && !errors.Is(err, rt.ErrWorkloadNotFound) {
794817
return err
795818
}
796819

797-
// Check if already running - use container name for this check
798-
if d.isWorkloadAlreadyRunning(containerName, workloadState) {
820+
// Check if already running - compare status to WorkloadStatusRunning
821+
if err == nil && workload.Status == rt.WorkloadStatusRunning {
822+
logger.Infof("Container %s is already running", containerName)
799823
return nil
800824
}
801825

802826
// Load runner configuration from state
803-
mcpRunner, err := d.loadRunnerFromState(ctx, workloadState.BaseName)
827+
mcpRunner, err := d.loadRunnerFromState(ctx, workloadName)
804828
if err != nil {
805-
return fmt.Errorf("failed to load state for %s: %v", workloadState.BaseName, err)
829+
return fmt.Errorf("failed to load state for %s: %v", workloadName, err)
806830
}
807831

808832
// Set workload status to starting - use the workload name for status operations
809833
if err := d.statuses.SetWorkloadStatus(ctx, workloadName, rt.WorkloadStatusStarting, ""); err != nil {
810834
logger.Warnf("Failed to set workload %s status to starting: %v", workloadName, err)
811835
}
812-
logger.Infof("Loaded configuration from state for %s", workloadState.BaseName)
836+
logger.Infof("Loaded configuration from state for %s", workloadName)
813837

814-
// Stop container if running but proxy is not - use the container name for runtime operations
815-
if err := d.stopContainerIfNeeded(ctx, containerName, workloadName, workloadState); err != nil {
816-
return err
838+
// Stop container if needed - since workload is not in running status, check if container needs stopping
839+
if container.IsRunning() {
840+
logger.Infof("Container %s is running but workload is not in running state. Stopping container...", containerName)
841+
if err := d.runtime.StopWorkload(ctx, containerName); err != nil {
842+
if statusErr := d.statuses.SetWorkloadStatus(ctx, workloadName, rt.WorkloadStatusError, ""); statusErr != nil {
843+
logger.Warnf("Failed to set workload %s status to error: %v", workloadName, statusErr)
844+
}
845+
return fmt.Errorf("failed to stop container %s: %v", containerName, err)
846+
}
847+
logger.Infof("Container %s stopped", containerName)
817848
}
818849

819850
// Start the workload with background context to avoid timeout cancellation
@@ -830,42 +861,6 @@ type workloadState struct {
830861
ProxyRunning bool
831862
}
832863

833-
// getWorkloadState retrieves the current state of a workload
834-
func (d *defaultManager) getWorkloadState(ctx context.Context, name string) (*workloadState, error) {
835-
workloadSt := &workloadState{}
836-
837-
// Try to find the container
838-
container, err := d.runtime.GetWorkloadInfo(ctx, name)
839-
if err != nil {
840-
if errors.Is(err, rt.ErrWorkloadNotFound) {
841-
logger.Warnf("Warning: Failed to find container: %v", err)
842-
logger.Warnf("Trying to find state with name %s directly...", name)
843-
// Try to use the provided name as the base name
844-
workloadSt.BaseName = name
845-
workloadSt.Running = false
846-
} else {
847-
return nil, fmt.Errorf("failed to find workload %s: %v", name, err)
848-
}
849-
} else {
850-
// Verify exact name match to prevent Docker prefix matching false positives
851-
if container.Name != name {
852-
logger.Warnf("Warning: Found container %s but requested %s (prefix match)", container.Name, name)
853-
// Treat as if container not found
854-
workloadSt.BaseName = name
855-
workloadSt.Running = false
856-
} else {
857-
// Container found with exact name, check if it's running and get the base name
858-
workloadSt.Running = container.IsRunning()
859-
workloadSt.BaseName = labels.GetContainerBaseName(container.Labels)
860-
}
861-
}
862-
863-
// Check if the proxy process is running
864-
workloadSt.ProxyRunning = proxy.IsRunning(workloadSt.BaseName)
865-
866-
return workloadSt, nil
867-
}
868-
869864
// getRemoteWorkloadState retrieves the current state of a remote workload
870865
func (d *defaultManager) getRemoteWorkloadState(ctx context.Context, name, baseName string) *workloadState {
871866
workloadSt := &workloadState{
@@ -897,25 +892,6 @@ func (*defaultManager) isWorkloadAlreadyRunning(name string, workloadSt *workloa
897892
return false
898893
}
899894

900-
// stopContainerIfNeeded stops the container if it's running but proxy is not
901-
func (d *defaultManager) stopContainerIfNeeded(
902-
ctx context.Context, containerName, workloadName string, workloadSt *workloadState,
903-
) error {
904-
if !workloadSt.Running {
905-
return nil
906-
}
907-
908-
logger.Infof("Container %s is running but proxy is not. Stopping container...", containerName)
909-
if err := d.runtime.StopWorkload(ctx, containerName); err != nil {
910-
if statusErr := d.statuses.SetWorkloadStatus(ctx, workloadName, rt.WorkloadStatusError, ""); statusErr != nil {
911-
logger.Warnf("Failed to set workload %s status to error: %v", workloadName, statusErr)
912-
}
913-
return fmt.Errorf("failed to stop container %s: %v", containerName, err)
914-
}
915-
logger.Infof("Container %s stopped", containerName)
916-
return nil
917-
}
918-
919895
// startWorkload starts the workload in either foreground or background mode
920896
func (d *defaultManager) startWorkload(ctx context.Context, name string, mcpRunner *runner.Runner, foreground bool) error {
921897
logger.Infof("Starting tooling server %s...", name)
@@ -1016,7 +992,7 @@ func (d *defaultManager) stopSingleContainerWorkload(ctx context.Context, worklo
1016992

1017993
name := labels.GetContainerBaseName(workload.Labels)
1018994
// Stop the proxy process
1019-
proxy.StopProcess(name)
995+
d.stopProcess(name)
1020996
// TODO: refactor the StopProcess function to stop dealing explicitly with PID files.
1021997
// Note that this is not a blocker for k8s since this code path is not called there.
1022998
if err := d.statuses.ResetWorkloadPID(ctx, name); err != nil {

0 commit comments

Comments
 (0)