Adds flag to inspect non-running pod #244
Conversation
openshift-ci-robot
added
the
size/S
ashishranjan738
force-pushed
the
non-running
branch
from
f3035f0
to
36b0736
Compare
ashishranjan738
force-pushed
the
non-running
branch
2 times, most recently
from
55f4b16
to
50a4da1
Compare
openshift-ci-robot
added
size/XS
Signed-off-by: Ashish Ranjan <ashishranjan738@gmail.com> The `oc adm inspect ns/<namespace>` skips the collection of logs from non-running pods. But in some scenarios the user may want to include this data (i.e: must-gather for OCS needs this). This commit adds a flag to include the logs from non-running pod.
ashishranjan738
force-pushed
the
non-running
branch
from
50a4da1
to
1fa8d82
Compare
openshift-ci-robot
added
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ashishranjan738 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
@soltysh any suggestions on the flag name for this PR ? |
I m thinking of changing to |
|
/test e2e-aws |
| @@ -17,7 +17,7 @@ import ( | |||
| ) | |||
|
|
|||
| func (o *InspectOptions) gatherPodData(destDir, namespace string, pod *corev1.Pod) error { | |||
| if pod.Status.Phase != corev1.PodRunning { | |||
| if pod.Status.Phase != corev1.PodRunning && !o.inspectNonRunningPod { | |||
There was a problem hiding this comment.
That's not sufficient, we scrape much more data, which rely on the pod running. You'd need a clear definition what you can scrape from running and what from other pods. The current condition changes are not sufficient.
| @@ -100,7 +101,7 @@ func NewCmdInspect(streams genericclioptions.IOStreams, parentCommandPath string | |||
|
|
|||
| cmd.Flags().StringVar(&o.destDir, "dest-dir", o.destDir, "Root directory used for storing all gathered cluster operator data. Defaults to $(PWD)/inspect.local.<rand>") | |||
| cmd.Flags().BoolVarP(&o.allNamespaces, "all-namespaces", "A", o.allNamespaces, "If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.") | |||
|
|
|||
| cmd.Flags().BoolVarP(&o.inspectNonRunningPod, "non-running-pods", "", o.inspectNonRunningPod, "If present, doesn't skips the collection of non running pods") | |||
There was a problem hiding this comment.
yup makes sense
|
@ashishranjan738 would #262 satisfy your needs? It's simpler. |
yeah but I have a follow-up comment on that PR |
sgtm, let's move the discussion to the other PR. |
|
@soltysh: Closed this PR. In response to this:
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. |
Signed-off-by: Ashish Ranjan ashishranjan738@gmail.com
The
oc adm inspect ns/<namespace>skips the collection of logs from non-running pods. But in some scenarios the user may want to include this data (i.e: must-gather for OCS needs this). This commit adds a--non-running-podsflag to include the logs from non-running pod.Fixes: #179