UPSTREAM: 53606: use deployment pod template if 0 replicas #16379

juanvallejo · 2017-09-15T15:52:51Z

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

cc @openshift/cli-review @smarterclayton

smarterclayton · 2017-09-15T17:30:31Z

pkg/oc/cli/cmd/debug.go

-	template, err := f.ApproximatePodTemplateForObject(infos[0].Object)
-	if err != nil && template == nil {
-		return fmt.Errorf("cannot debug %s: %v", infos[0].Name, err)
+	var template *kapi.PodTemplateSpec


Why wouldn't we always use ApproximatePodTemplateForObject for debug?

ApproximatePodTemplateForObject shouldn't wait for pods under DC

With a deployment object, the default case was being hit here, as its type resolved to *extensions.Deployment. It then got stuck in the call to f.AttachablePodForObject. I could add a new case to the switch for *extensions.Deployment with the same logic as the *deployapi.DeploymentConfig case. wdyt?

https://github.com/openshift/origin/blob/master/pkg/cmd/util/clientcmd/factory.go#L209

There should be an "approximatepodtemplateforobject" for RCs and RSs and Deployments and StatefulSets and Daemonsets upstream that doesn't depend on pods existing. So I would expect to always be using approximate pod template and for it to never block.

Without adding the case for *extensions.Deployment, current behavior hits the default case in https://github.com/openshift/origin/blob/master/pkg/cmd/util/clientcmd/factory.go#L209. Then AttachablePodForObject method is called on factory_object_mapping upstream with a timeout of 1 min.

Here is where the blocking seems to be happening.

Another possible solution could be to skip the call to AttachablePodForObject altogether if there are 0 replicas, and add a new type switch case for *extensions.Deployment here that just returns t.Spec.Template

Don't call AttachablePodForObject ever. ApproximatePodTemplateforObject should always be used for debug all the way down, and nothing in APTFO should ever wait for a container to start (it doesn't have to). attach requires a running pod, debug does not.

0xmichalis · 2017-09-17T08:10:00Z

/retest

0xmichalis · 2017-09-17T08:35:37Z

/retest

juanvallejo · 2017-09-18T15:38:30Z

@smarterclayton

There should be an "approximatepodtemplateforobject" for RCs and RSs and Deployments and StatefulSets and Daemonsets upstream that doesn't depend on pods existing. So I would expect to always be using approximate pod template and for it to never block.

Thanks, added ApproximatePodTemplateForObject to upstream factory to handle upstream types. Also removed use of AttachablePodForObject from this method downstream.

juanvallejo · 2017-09-18T15:56:19Z

/retest

juanvallejo · 2017-09-19T14:19:22Z

/retest

juanvallejo · 2017-09-20T20:42:49Z

/retest

juanvallejo · 2017-09-22T15:18:29Z

ping @smarterclayton

fabianofranz · 2017-09-22T19:07:02Z

/unassign
/assign @smarterclayton

smarterclayton · 2017-09-23T16:49:33Z

Test case for test-cmd that uses an RC scaled to zero and a DC scaled to zero to prove it works.

juanvallejo · 2017-09-25T19:09:27Z

@smarterclayton

Test case for test-cmd that uses an RC scaled to zero and a DC scaled to zero to prove it works.

Thanks, done

smarterclayton · 2017-10-01T22:59:03Z

pkg/cmd/util/clientcmd/factory_object_mapping.go

+
+		for i := range pods.Items {
+			pod := &pods.Items[i]
+			if fallback == nil || pod.CreationTimestamp.Before(fallback.CreationTimestamp) {


Why wouldn't you return the first pod no matter what, instead of the fallback?

Are you looking for pods newer than the most recent deployment?

@smarterclayton Yeah, this is existing code that I moved from the clientcmd/factory to clientcmd/factory_object_mapping. Can update the logic to just return the first pod if any exists instead, if that is a better approach

Hrm, it's probably fine. I just wasn't sure why it is here. Add a comment to this section that describes the algorithm.

Thanks, done! Will go ahead and open PR for upstream changes
if everything else looks okay

juanvallejo · 2017-10-09T18:03:36Z

@smarterclayton friendly ping on #16379 (comment)

juanvallejo · 2017-10-10T14:12:01Z

Thanks, done!

juanvallejo · 2017-10-10T17:02:37Z

/retest

@smarterclayton

…mplate-factory-method Automatic merge from submit-queue (batch tested with PRs 53606, 49361). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. add ApproximatePodTemplateForObject factory method Makes it possible to get at a pod spec template even if an object is scaled to zero, for use with commands that care about pod templates. **Release note**: ```release-note NONE ``` Related downstream patch and use-case: openshift/origin#16379 cc @smarterclayton

juanvallejo · 2017-10-12T15:21:13Z

@smarterclayton upstream dep has merged, ptal

juanvallejo · 2017-10-18T17:43:19Z

/test extended_conformance_gce

juanvallejo · 2017-10-18T20:10:49Z

/test extended_conformance_gce
/test extended_conformance_install_update

juanvallejo · 2017-10-18T22:55:26Z

/retest

juanvallejo · 2017-10-19T14:17:44Z

/retest

juanvallejo · 2017-10-20T18:47:45Z

/test extended_conformance_gce

juanvallejo · 2017-10-20T21:20:01Z

/test extended_conformance_gce

juanvallejo · 2017-10-25T19:08:36Z

@smarterclayton friendly ping - wondering if you could tag this for merging

juanvallejo · 2017-10-25T20:39:52Z

/kind bug

smarterclayton · 2017-10-25T21:29:59Z

/lgtm

openshift-merge-robot · 2017-10-25T21:30:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-10-25T23:42:40Z

Automatic merge from submit-queue.

openshift-merge-robot assigned fabianofranz Sep 15, 2017

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 15, 2017

smarterclayton reviewed Sep 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch 2 times, most recently from edb4c59 to a26a37e Compare September 15, 2017 18:15

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2017

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch 2 times, most recently from cfcfe3d to 19c54c2 Compare September 18, 2017 17:41

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch 2 times, most recently from a63cf37 to dca2dc3 Compare September 20, 2017 14:51

openshift-ci-robot assigned smarterclayton and unassigned fabianofranz Sep 22, 2017

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from 7588e37 to c547f05 Compare September 25, 2017 19:08

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from eb8ee57 to 094634d Compare September 25, 2017 20:44

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2017

smarterclayton reviewed Oct 1, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from 094634d to c8f4eb6 Compare October 9, 2017 17:44

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from e9cf687 to 2463610 Compare October 10, 2017 14:10

openshift-merge-robot added vendor-update Touching vendor dir or related files needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 14, 2017

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from 2463610 to b0e827d Compare October 18, 2017 16:08

openshift-merge-robot added needs-upstream and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 18, 2017

0xmichalis removed the needs-upstream label Oct 18, 2017

juanvallejo added 2 commits October 20, 2017 16:13

use deployment pod template if 0 replicas

216de39

UPSTREAM: 53606: implement ApproximatePodTemplateObject upstream

0598c2d

juanvallejo force-pushed the jvallejo/fix-debug-on-deployment-w-0-replicas branch from b0e827d to 0598c2d Compare October 20, 2017 20:13

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 25, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2017

openshift-merge-robot merged commit 598a4ac into openshift:master Oct 25, 2017

juanvallejo deleted the jvallejo/fix-debug-on-deployment-w-0-replicas branch October 26, 2017 15:08

UPSTREAM: 53606: use deployment pod template if 0 replicas #16379

UPSTREAM: 53606: use deployment pod template if 0 replicas #16379

Conversation

juanvallejo commented Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Sep 17, 2017

0xmichalis commented Sep 17, 2017

juanvallejo commented Sep 18, 2017

juanvallejo commented Sep 18, 2017

juanvallejo commented Sep 19, 2017

juanvallejo commented Sep 20, 2017

juanvallejo commented Sep 22, 2017

fabianofranz commented Sep 22, 2017

smarterclayton commented Sep 23, 2017

juanvallejo commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Oct 9, 2017

juanvallejo commented Oct 10, 2017

juanvallejo commented Oct 10, 2017

juanvallejo commented Oct 12, 2017 • edited Loading

juanvallejo commented Oct 18, 2017

juanvallejo commented Oct 18, 2017

juanvallejo commented Oct 18, 2017

juanvallejo commented Oct 19, 2017

juanvallejo commented Oct 20, 2017

juanvallejo commented Oct 20, 2017

juanvallejo commented Oct 25, 2017

juanvallejo commented Oct 25, 2017

smarterclayton commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017

juanvallejo commented Sep 15, 2017 •

edited

Loading

juanvallejo Sep 15, 2017 •

edited

Loading

juanvallejo Oct 2, 2017 •

edited

Loading

juanvallejo commented Oct 12, 2017 •

edited

Loading