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.

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

Log error when podman client cannot be initialized #6538

Merged
merged 11 commits into from
Feb 3, 2023

Conversation

valaparthvi
Copy link
Contributor

Signed-off-by: Parthvi Vala pvala@redhat.com

What type of PR is this:
/kind bug

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6501

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

  1. export PODMAN_CMD=echo
  2. odo dev --platform podman -v 2

@netlify
Copy link

netlify bot commented Jan 25, 2023

Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 3b22e20
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63dc9e7908287d00088dfe24

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 25, 2023
@openshift-ci openshift-ci bot requested review from kadel and rm3l January 25, 2023 12:06
@odo-robot
Copy link

odo-robot bot commented Jan 25, 2023

OpenShift Tests on commit c68826c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jan 25, 2023

NoCluster Tests on commit c68826c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jan 25, 2023

Validate Tests on commit c68826c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jan 25, 2023

Windows Tests (OCP) on commit c68826c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jan 25, 2023

Kubernetes Tests on commit c68826c finished successfully.
View logs: TXT HTML

@valaparthvi valaparthvi changed the title Log error when podman client cannot be initialized WIP: Log error when podman client cannot be initialized Jan 26, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jan 26, 2023
@odo-robot
Copy link

odo-robot bot commented Jan 26, 2023

Unit Tests on commit c68826c finished successfully.
View logs: TXT HTML

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jan 27, 2023
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jan 31, 2023
@valaparthvi valaparthvi changed the title WIP: Log error when podman client cannot be initialized Log error when podman client cannot be initialized Jan 31, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jan 31, 2023
@odo-robot
Copy link

odo-robot bot commented Jan 31, 2023

OpenShift Unauthenticated Tests on commit c68826c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jan 31, 2023

Kubernetes Docs Tests on commit bd3d61f finished successfully.
View logs: TXT HTML

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Another point: I find the error message (executable "echo" not found) to be misleading for the end-user and might not be accurate. The echo executable does exist in my PATH, but the output of the command is not the one expected by our Podman Client.

$ ODO_EXPERIMENTAL_MODE=t PODMAN_CMD=echo odo describe component --platform podman -v 3     
I0131 17:30:43.309683 3703568 version.go:28] executing [echo version --format json]
 ✗  unable to access podman. Do you have podman client installed? Cause: executable "echo" not found

For example, if I try to reproduce the steps provided in #6501 using a real Podman binary in my PATH:

$ which podman-v3     
/home/asoro/.local/bin/podman-v3

$ ODO_EXPERIMENTAL_MODE=t PODMAN_CMD=podman-v3 odo describe component --platform podman -v 3
I0131 17:31:21.622000 3705354 version.go:28] executing [podman-v3 version --format json]
 ✗  unable to access podman. Do you have podman client installed? Cause: executable "podman-v3" not found

But if I run the same Podman command to retrieve the version:

$ podman-v3 version --format json
Cannot connect to Podman. Please verify your connection to the Linux system using `podman system connection list`, or try `podman machine init` and `podman machine start` to manage a new Linux VM
Error: unable to connect to Podman socket: Get "http://d/v3.4.4/libpod/_ping": dial unix ///run/user/1000/podman/podman.sock: connect: no such file or directory

I think odo should display the full command output if there is an error with the command executed. What do you think?

pkg/podman/errors.go Outdated Show resolved Hide resolved
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

I think you raised good points, but I think they might be out of scope for this PR. The errors "executable %q not found" is raised when you initiliaze a new Podman Client. This PR is more about logging error when there is one. I'd be happy to work on it in a separate PR.

Okay, we can do that in a separate PR. But in this case, I'd leave the original issue (#6501) open, as it was more about odo reporting a wrong (and unhelpful) error.
We can say that this PR relates to #6501, but, to me, it does not close it yet.

Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi
Copy link
Contributor Author

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
Unrelated errors.

@openshift-ci
Copy link

openshift-ci bot commented Feb 1, 2023

@valaparthvi: Overrode contexts on behalf of valaparthvi: Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests, Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

In response to this:

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
Unrelated errors.

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.

@valaparthvi valaparthvi requested a review from rm3l February 1, 2023 13:00
@valaparthvi
Copy link
Contributor Author

I think you raised good points, but I think they might be out of scope for this PR. The errors "executable %q not found" is raised when you initiliaze a new Podman Client. This PR is more about logging error when there is one. I'd be happy to work on it in a separate PR.

Okay, we can do that in a separate PR. But in this case, I'd leave the original issue (#6501) open, as it was more about odo reporting a wrong (and unhelpful) error. We can say that this PR relates to #6501, but, to me, it does not close it yet.

I've updated the NewPodmanCli to return the actual error. But, this will still not work for odo describe component where it logs warning because it simply checks if the podmanClient is initialized.

pkg/podman/podman.go Outdated Show resolved Hide resolved
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Added a few comments.

pkg/podman/errors.go Outdated Show resolved Hide resolved
valaparthvi and others added 3 commits February 3, 2023 10:37
Co-authored-by: Armel Soro <armel@rm3l.org>
Co-authored-by: Armel Soro <armel@rm3l.org>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@valaparthvi valaparthvi requested a review from rm3l February 3, 2023 05:52
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Thanks Parthvi!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 3, 2023
@openshift-merge-robot openshift-merge-robot merged commit d1fa996 into redhat-developer:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

odo experimental feature --run-on podman fails with wrong error saying 'unable to access podman'
3 participants