Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1771552: Bug 1771553: Bug 1771549: Bug 1798549: oc debug #277

Merged
merged 2 commits into from Feb 13, 2020
Merged

Bug 1771552: Bug 1771553: Bug 1771549: Bug 1798549: oc debug #277

merged 2 commits into from Feb 13, 2020

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Jan 31, 2020 •
edited

/assign @soltysh

1 commit:

  • Bug 1771552
    In case of bad command passed to oc debug, user will now get information about why the command failed to run, and also will not have to wait the Timeout (15 min) to get the output.

2 commit:

  • Bug 1771553
    If container exit code != 0, debug cmd should report non-zero
  • Bug 1771549
    oc debug pod logs should go to stderr if container exit code != 0
  • Bug 1798549
    oc debug node/nodename should fail quickly if debug pod image pull fails

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 31, 2020
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 31, 2020
@sallyom sallyom changed the title Bug: 1771552: oc debug should not hang on bad cmd Bug 1771552: oc debug should not hang on bad cmd Jan 31, 2020
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1771552: oc debug should not hang on bad cmd

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 31, 2020
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 1, 2020
@sallyom sallyom changed the title Bug 1771552: oc debug should not hang on bad cmd Bug 1771552: Bug 1771553: oc debug should not hang on bad cmd Feb 1, 2020
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: oc debug should not hang on bad cmd

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.

@sallyom
Copy link
Contributor Author

sallyom commented Feb 1, 2020

/retest

@sallyom sallyom changed the title Bug 1771552: Bug 1771553: oc debug should not hang on bad cmd Bug 1771552: Bug 1771553: Bug 1771549: oc debug Feb 1, 2020
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

1 similar comment
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

@sallyom
Copy link
Contributor Author

sallyom commented Feb 3, 2020

/retest

soltysh
soltysh requested changes Feb 3, 2020
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left you comments.

pkg/cli/debug/debug.go Outdated Show resolved Hide resolved
pkg/helpers/conditions/conditions.go Outdated Show resolved Hide resolved
pkg/helpers/conditions/conditions.go Outdated Show resolved Hide resolved
pkg/cli/debug/debug.go Outdated Show resolved Hide resolved
pkg/cli/debug/debug.go Outdated Show resolved Hide resolved
pkg/cli/debug/debug.go Outdated Show resolved Hide resolved
@sallyom sallyom force-pushed the node-debug branch 2 times, most recently from 7707ef9 to c8a4604 Compare February 3, 2020 15:23
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

2 similar comments
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

@sallyom
Copy link
Contributor Author

sallyom commented Feb 5, 2020

/retest

soltysh
soltysh requested changes Feb 11, 2020
View reviewed changes
pkg/helpers/conditions/conditions.go Outdated Show resolved Hide resolved
pkg/helpers/conditions/conditions.go Outdated Show resolved Hide resolved
pkg/cli/debug/debug.go Outdated Show resolved Hide resolved
@sallyom sallyom force-pushed the node-debug branch 2 times, most recently from b050832 to 5ee97d7 Compare February 11, 2020 19:11
@openshift-ci-robot
Copy link

@sallyom: This pull request references Bugzilla bug 1771552, which is valid.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: oc debug

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.

@sallyom
Copy link
Contributor Author

sallyom commented Feb 11, 2020 •
edited

new output:

$ oc debug node/nodename -- false
Starting pod/sotest-lf2xg-w-b-hwwj9copenshift-gce-develinternal-debug ...
To use host binaries, run `chroot /host`

Removing debug pod ...
error: non-zero exit code from debug container
$ echo $?
1
$ oc debug node/nodename -- /bin/foo
Starting pod/sotest-lf2xg-w-b-hwwj9copenshift-gce-develinternal-debug ...
To use host binaries, run `chroot /host`

Removing debug pod ...
error: container create failed: time="2020-02-11T19:31:53Z" level=error msg="container_linux.go:346: starting container process caused \"exec: \\\"/bin/foo\\\": stat /bin/foo: no such file or directory\""
container_linux.go:346: starting container process caused "exec: \"/bin/foo\": stat /bin/foo: no such file or directory"
$ echo $?
1
$ oc debug node/nodename -- ls -al /foo
Starting pod/sotest-lf2xg-w-b-hwwj9copenshift-gce-develinternal-debug ...
To use host binaries, run `chroot /host`

Removing debug pod ...
error: ls: cannot access /foo: No such file or directory
$ echo $?
1
$ oc debug node/nodename -- ls
Starting pod/sotest-lf2xg-w-b-hwwj9copenshift-gce-develinternal-debug ...
To use host binaries, run `chroot /host`

Removing debug pod ...
error: Back-off pulling image "registry.redhat.io/rhel7/foo"
$ echo $?
1
$ oc debug node/nodename -- true
Starting pod/sotest-lf2xg-w-b-hwwj9copenshift-gce-develinternal-debug ...
To use host binaries, run `chroot /host`

Removing debug pod ...
$ echo $?
0

@sallyom sallyom changed the title Bug 1771552: Bug 1771553: Bug 1771549: oc debug Bug 1771552: Bug 1771553: Bug 1771549: Bug 1798549: oc debug Feb 11, 2020
@sallyom sallyom force-pushed the node-debug branch 2 times, most recently from 4e196e2 to dda61ac Compare February 11, 2020 20:51
soltysh
soltysh requested changes Feb 12, 2020
View reviewed changes
pkg/cli/debug/debug.go
@@ -453,20 +453,14 @@ func (o *DebugOptions) RunDebug() error {
}
return fmt.Errorf(msg)
// switch to logging output
case err == krun.ErrPodCompleted, err == conditions.ErrContainerTerminated, !o.Attach.Stdin:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal for this switch:

case err == krun.ErrPodCompleted, err == conditions.ErrContainerTerminated, !o.Attach.Stdin:
	return o.logsOptions(pod)
case err == conditions.ErrNonZeroExitCode:
	o.logsOptions(pod)
	return conditions.ErrNonZeroExitCode
case err != nil:
	return err
case !o.Attach.Stdin:
	return o.logsOptions(pod)

oh, and please change logsOptions to getLogs or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated - but handled the error from geLogs(pod) - or should I not? :

case err == conditions.ErrNonZeroExitCode:
	if err = o.getLogs(pod); err != nil {
            return err
         }
	return conditions.ErrNonZeroExitCode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 👍

pkg/cli/debug/debug.go Show resolved Hide resolved
@sallyom
Bug 1771553: oc debug node return container exit code
c8195c2 
Bug 1771549: oc debug pod logs to stderr if container exit code != 0
Bug 1798549: oc debug fail quickly on errimagepull
soltysh
soltysh approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020
@openshift-bot
Copy link
Contributor

/retest

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

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit d906590 into openshift:master Feb 13, 2020
@openshift-ci-robot
Copy link

@sallyom: All pull requests linked via external trackers have merged. Bugzilla bug 1771552 has been moved to the MODIFIED state.

In response to this:

Bug 1771552: Bug 1771553: Bug 1771549: Bug 1798549: oc debug

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sallyom @openshift-ci-robot @openshift-bot @soltysh @openshift-merge-robot