Merge pull request #16379 from juanvallejo/jvallejo/fix-debug-on-depl…

…oyment-w-0-replicas

Automatic merge from submit-queue.

UPSTREAM: 53606: use deployment pod template if 0 replicas

Fixes #14286
Fixes #14915

Prevent `oc debug` from hanging indefinitely when dealing
with a deployment that has been scaled down to 0.

cc @openshift/cli-review @smarterclayton

pkg/cmd/util/clientcmd/factory.go

@@ -29,10 +29,8 @@ import (
 	"k8s.io/kubernetes/pkg/printers"
 
 	deployapi "github.com/openshift/origin/pkg/apps/apis/apps"
-	deployutil "github.com/openshift/origin/pkg/apps/util"
 	buildapi "github.com/openshift/origin/pkg/build/apis/build"
 	"github.com/openshift/origin/pkg/cmd/util"
-	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 )
 
 // New creates a default Factory for commands that should share identical server
@@ -148,87 +146,6 @@ func (f *Factory) ExtractFileContents(obj runtime.Object) (map[string][]byte, bo
 	}
 }
 
-// ApproximatePodTemplateForObject returns a pod template object for the provided source.
-// It may return both an error and a object. It attempt to return the best possible template
-// available at the current time.
-func (f *Factory) ApproximatePodTemplateForObject(object runtime.Object) (*api.PodTemplateSpec, error) {
-	switch t := object.(type) {
-	case *imageapi.ImageStreamTag:
-		// create a minimal pod spec that uses the image referenced by the istag without any introspection
-		// it possible that we could someday do a better job introspecting it
-		return &api.PodTemplateSpec{
-			Spec: api.PodSpec{
-				RestartPolicy: api.RestartPolicyNever,
-				Containers: []api.Container{
-					{Name: "container-00", Image: t.Image.DockerImageReference},
-				},
-			},
-		}, nil
-	case *imageapi.ImageStreamImage:
-		// create a minimal pod spec that uses the image referenced by the istag without any introspection
-		// it possible that we could someday do a better job introspecting it
-		return &api.PodTemplateSpec{
-			Spec: api.PodSpec{
-				RestartPolicy: api.RestartPolicyNever,
-				Containers: []api.Container{
-					{Name: "container-00", Image: t.Image.DockerImageReference},
-				},
-			},
-		}, nil
-	case *deployapi.DeploymentConfig:
-		fallback := t.Spec.Template
-
-		kc, err := f.ClientSet()
-		if err != nil {
-			return fallback, err
-		}
-
-		latestDeploymentName := deployutil.LatestDeploymentNameForConfig(t)
-		deployment, err := kc.Core().ReplicationControllers(t.Namespace).Get(latestDeploymentName, metav1.GetOptions{})
-		if err != nil {
-			return fallback, err
-		}
-
-		fallback = deployment.Spec.Template
-
-		pods, err := kc.Core().Pods(deployment.Namespace).List(metav1.ListOptions{LabelSelector: labels.SelectorFromSet(deployment.Spec.Selector).String()})
-		if err != nil {
-			return fallback, err
-		}
-
-		for i := range pods.Items {
-			pod := &pods.Items[i]
-			if fallback == nil || pod.CreationTimestamp.Before(fallback.CreationTimestamp) {
-				fallback = &api.PodTemplateSpec{
-					ObjectMeta: pod.ObjectMeta,
-					Spec:       pod.Spec,
-				}
-			}
-		}
-		return fallback, nil
-
-	default:
-		pod, err := f.AttachablePodForObject(object, 1*time.Minute)
-		if pod != nil {
-			return &api.PodTemplateSpec{
-				ObjectMeta: pod.ObjectMeta,
-				Spec:       pod.Spec,
-			}, err
-		}
-		switch t := object.(type) {
-		case *api.ReplicationController:
-			return t.Spec.Template, err
-		case *extensions.ReplicaSet:
-			return &t.Spec.Template, err
-		case *extensions.DaemonSet:
-			return &t.Spec.Template, err
-		case *batch.Job:
-			return &t.Spec.Template, err
-		}
-		return nil, err
-	}
-}
-
 func (f *Factory) PodForResource(resource string, timeout time.Duration) (string, error) {
 	sortBy := func(pods []*v1.Pod) sort.Interface { return sort.Reverse(controller.ActivePods(pods)) }
 	namespace, _, err := f.DefaultNamespace()

pkg/cmd/util/clientcmd/factory_object_mapping.go

@@ -31,6 +31,7 @@ import (
 	appsmanualclient "github.com/openshift/origin/pkg/apps/client/internalversion"
 	deploycmd "github.com/openshift/origin/pkg/apps/cmd"
 	appsclient "github.com/openshift/origin/pkg/apps/generated/internalclientset"
+	deployutil "github.com/openshift/origin/pkg/apps/util"
 	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
 	authorizationreaper "github.com/openshift/origin/pkg/authorization/reaper"
 	buildapi "github.com/openshift/origin/pkg/build/apis/build"
@@ -39,6 +40,7 @@ import (
 	buildclient "github.com/openshift/origin/pkg/build/generated/internalclientset"
 	buildutil "github.com/openshift/origin/pkg/build/util"
 	configcmd "github.com/openshift/origin/pkg/config/cmd"
+	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/oc/cli/describe"
 	userapi "github.com/openshift/origin/pkg/user/apis/user"
 	authenticationreaper "github.com/openshift/origin/pkg/user/reaper"
@@ -351,6 +353,74 @@ func (f *ring1Factory) StatusViewer(mapping *meta.RESTMapping) (kubectl.StatusVi
 	return f.kubeObjectMappingFactory.StatusViewer(mapping)
 }
 
+// ApproximatePodTemplateForObject returns a pod template object for the provided source.
+// It may return both an error and a object. It attempt to return the best possible template
+// available at the current time.
+func (f *ring1Factory) ApproximatePodTemplateForObject(object runtime.Object) (*kapi.PodTemplateSpec, error) {
+	switch t := object.(type) {
+	case *imageapi.ImageStreamTag:
+		// create a minimal pod spec that uses the image referenced by the istag without any introspection
+		// it possible that we could someday do a better job introspecting it
+		return &kapi.PodTemplateSpec{
+			Spec: kapi.PodSpec{
+				RestartPolicy: kapi.RestartPolicyNever,
+				Containers: []kapi.Container{
+					{Name: "container-00", Image: t.Image.DockerImageReference},
+				},
+			},
+		}, nil
+	case *imageapi.ImageStreamImage:
+		// create a minimal pod spec that uses the image referenced by the istag without any introspection
+		// it possible that we could someday do a better job introspecting it
+		return &kapi.PodTemplateSpec{
+			Spec: kapi.PodSpec{
+				RestartPolicy: kapi.RestartPolicyNever,
+				Containers: []kapi.Container{
+					{Name: "container-00", Image: t.Image.DockerImageReference},
+				},
+			},
+		}, nil
+	case *deployapi.DeploymentConfig:
+		fallback := t.Spec.Template
+
+		kc, err := f.clientAccessFactory.ClientSet()
+		if err != nil {
+			return fallback, err
+		}
+
+		latestDeploymentName := deployutil.LatestDeploymentNameForConfig(t)
+		deployment, err := kc.Core().ReplicationControllers(t.Namespace).Get(latestDeploymentName, metav1.GetOptions{})
+		if err != nil {
+			return fallback, err
+		}
+
+		fallback = deployment.Spec.Template
+
+		pods, err := kc.Core().Pods(deployment.Namespace).List(metav1.ListOptions{LabelSelector: labels.SelectorFromSet(deployment.Spec.Selector).String()})
+		if err != nil {
+			return fallback, err
+		}
+
+		// If we have any pods available, find the newest
+		// pod with regards to our most recent deployment.
+		// If the fallback PodTemplateSpec is nil, prefer
+		// the newest pod available.
+		for i := range pods.Items {
+			pod := &pods.Items[i]
+			if fallback == nil || pod.CreationTimestamp.Before(fallback.CreationTimestamp) {
+				fallback = &kapi.PodTemplateSpec{
+					ObjectMeta: pod.ObjectMeta,
+					Spec:       pod.Spec,
+				}
+			}
+		}
+		return fallback, nil
+
+	default:
+		return f.kubeObjectMappingFactory.ApproximatePodTemplateForObject(object)
+	}
+}
+
 func (f *ring1Factory) AttachablePodForObject(object runtime.Object, timeout time.Duration) (*kapi.Pod, error) {
 	switch t := object.(type) {
 	case *deployapi.DeploymentConfig:

test/cmd/debug.sh

@@ -33,6 +33,45 @@ os::cmd::expect_success_and_not_text "oc debug -f examples/hello-openshift/hello
 os::cmd::expect_success_and_not_text "oc debug -f examples/hello-openshift/hello-pod.json -o yaml -- /bin/env" 'tty'
 # TODO: write a test that emulates a TTY to verify the correct defaulting of what the pod is created
 
+# Ensure debug does not depend on a container actually existing for the selected resource.
+# The command should not hang waiting for an attachable pod. Timeout each cmd after 10s.
+os::cmd::expect_success 'oc create -f test/integration/testdata/test-replication-controller.yaml'
+os::cmd::expect_success 'oc scale --replicas=0 rc/test-replication-controller'
+os::cmd::expect_success_and_text "oc debug --request-timeout=10s -c ruby-helloworld --one-container rc/test-replication-controller -o jsonpath='{.metadata.name}'" 'test-replication-controller-debug'
+
+os::cmd::expect_success 'oc scale --replicas=0 dc/test-deployment-config'
+os::cmd::expect_success_and_text "oc debug --request-timeout=10s -c ruby-helloworld --one-container dc/test-deployment-config -o jsonpath='{.metadata.name}'" 'test-deployment-config'
+
+os::cmd::expect_success 'oc create -f - >> cat << __EOF__
+apiVersion: extensions/v1beta1
+kind: Deployment
+metadata:
+  name: test-deployment
+  labels:
+    deployment: test-deployment
+spec:
+  replicas: 0
+  selector:
+    matchLabels:
+      deployment: test-deployment
+  template:
+    metadata:
+      labels:
+        deployment: test-deployment
+      name: test-deployment
+    spec:
+      containers:
+      - name: ruby-helloworld
+        image: openshift/origin-pod
+        imagePullPolicy: IfNotPresent
+        resources: {}
+status: {}
+__EOF__'
+os::cmd::expect_success_and_text "oc debug --request-timeout=10s -c ruby-helloworld --one-container deploy/test-deployment -o jsonpath='{.metadata.name}'" 'test-deployment-debug'
+
+# re-scale existing resources
+os::cmd::expect_success 'oc scale --replicas=1 dc/test-deployment-config'
+
 os::cmd::expect_success 'oc create -f examples/image-streams/image-streams-centos7.json'
 os::cmd::try_until_success 'oc get imagestreamtags wildfly:latest'
 os::cmd::expect_success_and_text "oc debug istag/wildfly:latest -o yaml" 'image: openshift/wildfly-101-centos7'