CRI: stop sandbox before removing it

pkg/kubelet/api/v1alpha1/runtime/api.proto

@@ -8,30 +8,41 @@ service RuntimeService {
     // Version returns the runtime name, runtime version and runtime API version
     rpc Version(VersionRequest) returns (VersionResponse) {}
 
-    // RunPodSandbox creates and starts a pod-level sandbox. Runtimes should ensure
-    // the sandbox is in ready state.
+    // RunPodSandbox creates and starts a pod-level sandbox. Runtimes must ensure
+    // the sandbox is in the ready state on success.
     rpc RunPodSandbox(RunPodSandboxRequest) returns (RunPodSandboxResponse) {}
-    // StopPodSandbox stops the running sandbox. If there are any running
-    // containers in the sandbox, they should be forcibly terminated.
+    // StopPodSandbox stops any running process that is part of the sandbox and
+    // reclaims network resources (e.g., IP addresses) allocated to the sandbox.
+    // If there are any running containers in the sandbox, they must be forcibly
+    // terminated.
+    // This call is idempotent, and must not return an error if all relevant
+    // resources have already been reclaimed. kubelet will call StopPodSandbox
+    // at least once before calling RemovePodSandbox. It will also attempt to
+    // reclaim resources eagerly, as soon as a sandbox is not needed. Hence,
+    // multiple StopPodSandbox calls are expected.
     rpc StopPodSandbox(StopPodSandboxRequest) returns (StopPodSandboxResponse) {}
-    // RemovePodSandbox removes the sandbox. If there are any running containers in the
-    // sandbox, they should be forcibly removed.
-    // It should return success if the sandbox has already been removed.
+    // RemovePodSandbox removes the sandbox. If there are any running containers
+    // in the sandbox, they must be forcibly terminated and removed.
+    // This call is idempotent, and must not return an error if the sandbox has
+    // already been removed.
     rpc RemovePodSandbox(RemovePodSandboxRequest) returns (RemovePodSandboxResponse) {}
     // PodSandboxStatus returns the status of the PodSandbox.
     rpc PodSandboxStatus(PodSandboxStatusRequest) returns (PodSandboxStatusResponse) {}
-    // ListPodSandbox returns a list of Sandbox.
+    // ListPodSandbox returns a list of PodSandboxes.
     rpc ListPodSandbox(ListPodSandboxRequest) returns (ListPodSandboxResponse) {}
 
     // CreateContainer creates a new container in specified PodSandbox
     rpc CreateContainer(CreateContainerRequest) returns (CreateContainerResponse) {}
     // StartContainer starts the container.
     rpc StartContainer(StartContainerRequest) returns (StartContainerResponse) {}
     // StopContainer stops a running container with a grace period (i.e., timeout).
+    // This call is idempotent, and must not return an error if the container has
+    // already been stopped.
     rpc StopContainer(StopContainerRequest) returns (StopContainerResponse) {}
     // RemoveContainer removes the container. If the container is running, the
-    // container should be forcibly removed.
-    // It should return success if the container has already been removed.
+    // container must be forcibly removed.
+    // This call is idempotent, and must not return an error if the container has
+    // already been removed.
     rpc RemoveContainer(RemoveContainerRequest) returns (RemoveContainerResponse) {}
     // ListContainers lists all containers by filters.
     rpc ListContainers(ListContainersRequest) returns (ListContainersResponse) {}
@@ -61,7 +72,8 @@ service ImageService {
     // PullImage pulls an image with authentication config.
     rpc PullImage(PullImageRequest) returns (PullImageResponse) {}
     // RemoveImage removes the image.
-    // It should return success if the image has already been removed.
+    // This call is idempotent, and must not return an error if the image has
+    // already been removed.
     rpc RemoveImage(RemoveImageRequest) returns (RemoveImageResponse) {}
 }
 
@@ -75,10 +87,10 @@ message VersionResponse {
     optional string version = 1;
     // The name of the container runtime.
     optional string runtime_name = 2;
-    // The version of the container runtime. The string should be
+    // The version of the container runtime. The string must be
     // semver-compatible.
     optional string runtime_version = 3;
-    // The API version of the container runtime. The string should be
+    // The API version of the container runtime. The string must be
     // semver-compatible.
     optional string runtime_api_version = 4;
 }

pkg/kubelet/kuberuntime/kuberuntime_gc.go

@@ -131,6 +131,13 @@ func (cgc *containerGC) removeOldestN(containers []containerGCInfo, toRemove int
 // removeSandbox removes the sandbox by sandboxID.
 func (cgc *containerGC) removeSandbox(sandboxID string) {
 	glog.V(4).Infof("Removing sandbox %q", sandboxID)
+	// In normal cases, kubelet should've already called StopPodSandbox before
+	// GC kicks in. To guard against the rare cases where this is not true, try
+	// stopping the sandbox before removing it.
+	if err := cgc.client.StopPodSandbox(sandboxID); err != nil {
+		glog.Errorf("Failed to stop sandbox %q before removing: %v", sandboxID, err)
+		return
+	}
 	if err := cgc.client.RemovePodSandbox(sandboxID); err != nil {
 		glog.Errorf("Failed to remove sandbox %q: %v", sandboxID, err)
 	}