add must-gather command by sanchezl · Pull Request #22430 · openshift/origin

sanchezl · 2019-03-28T19:07:26Z

Launch a pod to gather debugging information 

This command will launch a pod in a temporary namespace on your cluster that gathers debugging
information, using a copy of the active client config context, and then downloads the gathered
information.

Usage:
  oc adm must-gather [flags]

Examples:
  # gather default information using the default image and command, writing into ./must-gather.local.<rand>
  oc adm must-gather
  
  # gather default information with a specific local folder to copy to
  oc adm must-gather --dest-dir=/local/directory
  
  # gather default information using a specific image, command, and pod-dir
  oc adm must-gather --image=my/image:tag --source-dir=/pod/directory -- myspecial-command.sh

Options:
      --dest-dir='': Set a specific directory on the local machine to write gathered data to.
      --image='': Set a specific to use - by default the image will be looked up for OpenShift's must-gather
      --node-name='': Set a specific node to use - by default a random master will be used

Picking up from #22405

Create a temporary namespace
Create a cluster role binding for the default service account for the temporary namespace.
Create a pod that runs the 'gather' command in that image.
Wait for completion of that command.
Look up the image to use for gathering information
Copy the gathered data locally
Delete the temporary namespace.
Delete the cluster role binding.

https://jira.coreos.com/browse/MSTR-351

deads2k · 2019-03-29T12:49:45Z

@sanchezl add a phase-in plan for the description please.

soltysh

Left you some initial comments....

soltysh · 2019-03-29T14:20:30Z

+
+	mustGatherExample = templates.Examples(`
+		# gather default information using the default image and command, writing into ./must-gather.local.<rand>
+		  oc adm must-gather


We try not to embed the name in the examples, use %[1]s instead and then
Example: fmt.Sprintf(mustGatherExample, fullName)

We try not to embed the name in the examples, use %[1]s instead and then
Example: fmt.Sprintf(mustGatherExample, fullName)

@soltysh I don't think we're gaining much with that, do you? Clayton came to a similar conclusion.

I know tools that are consuming us, and just trying to be good citizen and allow that consumption. Also I'm slowly accepting the fact that both kubectl and oc are primitives that folks use to build their opinionated flows on top (see https://www.youtube.com/watch?v=ytu3aUCwlSg for example how AirBnB is wrapping kubectl just for that). But that's not a strong requirement, yet it doesn't cost us that much to have it done this way.

soltysh · 2019-03-29T14:22:14Z

+)
+
+type MustGatherFlags struct {
+	ConfigFlags *genericclioptions.ConfigFlags


You should not need this, since you're part of oc this is implicit.

No longer applicable.

soltysh · 2019-03-29T14:23:31Z

+}
+
+func NewMustGatherCommand(restClientGetter genericclioptions.RESTClientGetter, streams genericclioptions.IOStreams) *cobra.Command {
+	f := NewMustGatherFlags(streams)


Nit: o is the preferred var name for option structs, this applies to entire file.

soltysh · 2019-03-29T14:24:27Z

+		Long:    mustGatherLong,
+		Example: mustGatherExample,
+		Run: func(cmd *cobra.Command, args []string) {
+			kcmdutil.CheckErr(f.Complete(cmd, restClientGetter, args).RunMustGather())


kcmdutil.CheckErr(o.Complete(f, cmd, args)) kcmdutil.CheckErr(o.Run())

is preferred.

soltysh · 2019-03-29T14:25:04Z

+	}
+
+	cmd.Flags().StringVar(&f.NodeName, "node-name", f.NodeName, "Set a specific node to use - by default a random master will be used")
+	cmd.Flags().StringVar(&f.Image, "image", f.Image, "Set a specific to use - by default the image will be looked up for OpenShift's must-gather")


Set a specific image to use, by default the OpenShift's must-gather image will be used.

soltysh · 2019-03-29T14:29:27Z

+}
+
+func (f *MustGatherFlags) Complete(cmd *cobra.Command, restClientGetter genericclioptions.RESTClientGetter, args []string) *MustGatherOptions {
+	o := &MustGatherOptions{


Something smells here. You should have only one MustGatherOptions struct and work from there through Complete -> Validate (if needed) -> Run. That is the required flow for all commands.

soltysh · 2019-03-29T14:30:22Z

+	return nil
+}
+
+func (o *MustGatherOptions) newPod(node, ns string) *corev1.Pod {


ns is not being used

It's used now.

deads2k · 2019-03-29T19:25:43Z

Picking up from #22405

Create a temporary namespace

Create a secret an upload the local kubeconfig content into it

Look up the image to use for gathering information

Create a pod that runs the 'gather' command in that image

Wait for completion of that command

Copy the gathered data locally

Let's get this into a Jira story. Add deleting the namespace. Mark the command hidden and deprecated and see about starting to phase this in with @soltysh and reasonable tests on subsets.

Redorder the pod creation and the image lookup. Use a floating tag on quay to start this. Describe a way to recognize from the client that the command has finished.

deads2k · 2019-03-29T19:26:27Z

Create a secret an upload the local kubeconfig content into it

If we didn't do this, could we use a cluster-reader + secret/metrics/debug/health reader assigned to a service account with an audience.

@enj

soltysh · 2019-04-01T10:51:15Z

Let's get this into a Jira story. Add deleting the namespace. Mark the command hidden and deprecated and see about starting to phase this in with @soltysh and reasonable tests on subsets.

Hidden 👍, but why deprecated, better experimental. Let's make it clear it's a moving target.

deads2k · 2019-04-01T12:10:30Z

but why deprecated, better experimenta

sure

sferich888 · 2019-04-01T14:02:34Z

Updated openshift/must-gather#65 as it will be needed in tandem with this command.

deads2k · 2019-04-01T22:16:30Z

+	if len(o.Image) == 0 {
+		// TODO lookup cluster specific default
+		//		Image: "quay.io/openshift-release-dev/ocp-v4.0-art-dev:v4.0.5-1-ose-must-gather",
+		o.Image = "sanchezl/ocp-v4.0-art-dev:v4.0.5-1-ose-must-gather"


quay.io/openshift/origin-must-gather:latest I'd think

deads2k · 2019-04-01T22:17:09Z

+
+	// TODO This command will:
+	// [x] Create a temporary namespace
+	// [x] Create a secret an upload the local kubeconfig content into it


let's not do this and grant an aggregator cluster-role to the service account in this namespace

Simplest possible thing to start. Grant it cluster-admin to land

Went with cluster-admin for now. Opened #22430 as a reminder to fix in the future.

deads2k · 2019-04-01T22:17:24Z

+
+	fmt.Fprintf(o.Out, "Created ns/%s\n", ns.Name)
+
+	kubeConfigSecret, err := o.newKubeConfigSecret(ns.Name)


let's stop doin this

deads2k · 2019-04-01T22:17:47Z

+					Command: []string{"openshift-must-gather", "inspect", "clusteroperator"},
+					Env: []corev1.EnvVar{
+						{
+							Name:  "KUBECONFIG",


deads2k · 2019-04-01T22:18:34Z

+					VolumeMounts: []corev1.VolumeMount{
+						{
+							Name:      "kubeconfig",
+							MountPath: "/etc/kubernetes",


ha, this is clever. Was this me or you?

Me. It's gone now.

deads2k · 2019-04-01T22:19:16Z

@sanchezl take out of WIP, add a test-cmd test for it test/cmd/admin.sh I'd think.

smarterclayton · 2019-04-02T21:24:17Z

What happens when launching pods won't work. How do I must-gather then?

sferich888 · 2019-04-02T23:47:00Z

must-gather can be run as a CLI plug-in (or standalone binary) too. The benefit of an image is that it let's us ship the binary as part of a release (connect but independent), with oc we don't have the same mechanism (thus why a plug-in was not considered).

In short this is a short comming of this implementation (by using an image and the oc shim to call start the image and exfiltrate data), however if your cluster is in that bad of shape your going to likely need host level access to debug issues that are likely not cluster related.

sanchezl · 2019-04-03T01:56:32Z

/test verify

soltysh

A few more fresh comments + some old ones are still not addressed.

soltysh · 2019-04-10T12:51:36Z

+	if err != nil {
+		return err
+	}
+	if podPhase != corev1.PodRunning {


This is still not addressed.

soltysh · 2019-04-10T12:57:03Z

+	}
+	if len(o.Image) == 0 {
+		// TODO lookup cluster specific default
+		o.Image = "quay.io/openshift/origin-must-gather:v4.0"


Set the default in NewMustGatherOptions, this way the default will be nicely presented in the command's help.

This isn't the real default, see #22528.

deads2k · 2019-04-10T15:31:20Z

verify failure is real again because you're relying on a new package. Whitelist all of k8s.io/apimachinery to save trouble

sanchezl · 2019-04-11T15:28:22Z

/retest

sferich888 · 2019-04-12T01:40:05Z

/retest

soltysh · 2019-04-12T10:36:24Z

+	if err != nil {
+		return err
+	}
+	if phase != corev1.PodRunning {


The switch from pending to running in most cases should be fast, but why wait for pending and then check its status after wait, and not just wait for running in wait.PollImmediate ?

It might go from Pending to Failed (or even Success or Completed). With a RestartPolicy: Never, I would be waiting for a transition that might never happen if I look for Running specifically. This way we fail ASAP, instead of timing out the wait.PollIntermediate call.

Right but the transition from Pending to Running might not be as fast as this check and you will fail then, b/c pod is still in pending. Maybe you should have a more explicit checks in place then.

My bad, I misread the condition in wait.PollImmediate, it's 👍

soltysh · 2019-04-12T10:36:48Z

+	err := wait.PollImmediate(time.Second, 10*time.Minute, func() (bool, error) {
+		var err error
+		if pod, err = o.Client.CoreV1().Pods(pod.Namespace).Get(pod.Name, metav1.GetOptions{}); err != nil {
+			klog.Error(err)


Don't log the error here, if you care, log it at higher debug levels, it's not an error per se.

removed in PR #22561

deads2k · 2019-04-12T12:37:26Z

I see that fake test. Start an e2e one

/lgtm
/retest

deads2k · 2019-04-12T12:37:44Z

I see that fake test. Start an e2e one

/lgtm
/retest

This is how bad I want this in a beta.

openshift-ci-robot · 2019-04-12T12:45:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/oc/OWNERS~~ [deads2k]
~~test/cmd/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deads2k · 2019-04-12T13:57:12Z

ci-operator failure

/retest

deads2k · 2019-04-13T13:00:45Z

/test all

sanchezl · 2019-04-13T23:12:49Z

/test e2e-aws

sanchezl · 2019-04-15T01:50:50Z

/retest

openshift-bot · 2019-04-15T13:14:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-04-15T14:59:01Z

@sanchezl: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`5089636`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sanchezl requested review from deads2k, smarterclayton and soltysh March 28, 2019 19:07

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2019

openshift-ci-robot requested a review from enj March 28, 2019 19:07

sanchezl force-pushed the oc_adm_must-gather branch from 14351d3 to 613c04f Compare March 28, 2019 19:58

soltysh self-assigned this Mar 29, 2019

soltysh suggested changes Mar 29, 2019

View reviewed changes

sanchezl force-pushed the oc_adm_must-gather branch 2 times, most recently from b7b6398 to 97f4ee5 Compare April 1, 2019 20:48

deads2k reviewed Apr 1, 2019

View reviewed changes

sanchezl force-pushed the oc_adm_must-gather branch 2 times, most recently from 41b7d1d to 87bd873 Compare April 2, 2019 18:32

sanchezl changed the title ~~[WIP] add must-gather command~~ add must-gather command Apr 2, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2019

sanchezl force-pushed the oc_adm_must-gather branch 2 times, most recently from f5b2970 to 113afe2 Compare April 9, 2019 21:14

soltysh suggested changes Apr 10, 2019

View reviewed changes

sanchezl force-pushed the oc_adm_must-gather branch from 113afe2 to dc0df0f Compare April 10, 2019 19:05

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 10, 2019

sanchezl force-pushed the oc_adm_must-gather branch 3 times, most recently from 5a7b7af to 9d89771 Compare April 11, 2019 05:57

sanchezl force-pushed the oc_adm_must-gather branch 2 times, most recently from a2b8706 to da8f59d Compare April 12, 2019 00:36

add must-gather command

5089636

sanchezl force-pushed the oc_adm_must-gather branch from da8f59d to 5089636 Compare April 12, 2019 02:30

soltysh suggested changes Apr 12, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2019

sanchezl mentioned this pull request Apr 15, 2019

doc oc adm must-gather openshift/openshift-docs#14436

Closed

deads2k merged commit 5089636 into openshift:master Apr 15, 2019

sttts mentioned this pull request Apr 15, 2019

get must-gather image from imagestream #22528

Merged


		fmt.Fprintf(o.Out, "Created ns/%s\n", ns.Name)

		kubeConfigSecret, err := o.newKubeConfigSecret(ns.Name)

Conversation

sanchezl commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Mar 29, 2019

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 29, 2019

Uh oh!

deads2k commented Mar 29, 2019

Uh oh!

soltysh commented Apr 1, 2019

Uh oh!

deads2k commented Apr 1, 2019

Uh oh!

sferich888 commented Apr 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 1, 2019

Uh oh!

smarterclayton commented Apr 2, 2019

Uh oh!

sferich888 commented Apr 2, 2019

Uh oh!

sanchezl commented Apr 3, 2019

Uh oh!

soltysh left a comment

sanchezl commented Mar 28, 2019 •

edited

Loading