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

ceph_salt_deployment: use --prefix /usr with "pip install" #221

Merged
merged 2 commits into from
Apr 3, 2020
Merged

ceph_salt_deployment: use --prefix /usr with "pip install" #221

merged 2 commits into from
Apr 3, 2020

Conversation

smithfarm
Copy link
Collaborator

@smithfarm smithfarm commented Apr 3, 2020 •
edited
Loading

Deployment fails with:

master: ++ ceph-salt config /Ceph_Cluster/Minions add master.octopus.com
master: /tmp/vagrant-shell: line 143: ceph-salt: command not found

Yet, after subsequently SSHing in:

master:~ # type ceph-salt
ceph-salt is /usr/local/bin/ceph-salt
master:~ # echo $PATH
/sbin:/usr/sbin:/usr/local/sbin:/root/bin:/usr/local/bin:/usr/bin:/bin
master:~ # ceph-salt
Usage: ceph-salt [OPTIONS] COMMAND [ARGS]...

...

The reason is that ceph_salt_deployment.sh is not running in a login shell, so /usr/local/bin does not get added to the PATH.

For whatever reason, pip install does not behave this way in Leap 15.2 and earlier. This issue is only seen in --os=tumbleweed deployments.

Fixes: #220

@smithfarm smithfarm mentioned this pull request Apr 3, 2020
Closed
smithfarm added 2 commits April 3, 2020 16:23
@smithfarm
templates: before running deepsea/ceph-salt, echo "$PATH"
99f5c66 
This will help debug issues like this one:

Deployment fails with:

    master: ++ ceph-salt config /Ceph_Cluster/Minions add master.octopus.com
    master: /tmp/vagrant-shell: line 143: ceph-salt: command not found

Yet, after subsequently SSHing in:

    master:~ # type ceph-salt
    ceph-salt is /usr/local/bin/ceph-salt
    master:~ # echo $PATH
    /sbin:/usr/sbin:/usr/local/sbin:/root/bin:/usr/local/bin:/usr/bin:/bin
    master:~ # ceph-salt
    Usage: ceph-salt [OPTIONS] COMMAND [ARGS]...

    ...

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
ceph_salt_deployment: use --prefix /usr with "pip install"
6ae338c 
On tumbleweed, "pip install" puts executables in "/usr/local/bin" but
this is not in the PATH

Fixes: SUSE#220
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm smithfarm mentioned this pull request Apr 3, 2020
Closed
ricardoasmarques
ricardoasmarques approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@ricardoasmarques ricardoasmarques left a comment

Choose a reason for hiding this comment

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

lgtm

@smithfarm smithfarm merged commit 5c85aa7 into SUSE:master Apr 3, 2020
@smithfarm smithfarm deleted the wip-220 branch April 3, 2020 16:13
@toabctl
Copy link
Contributor

toabctl commented Apr 7, 2020

Wouldn't it be nicer to have /usr/local/bin in PATH when running ceph_salt_deployment.sh ? Now this pollutes the filesystem and will overwrite a packaged ceph-salt installation...

@smithfarm
Copy link
Collaborator Author

Wouldn't it be nicer to have /usr/local/bin in PATH when running ceph_salt_deployment.sh ? Now this pollutes the filesystem and will overwrite a packaged ceph-salt installation...

Maybe there is a misunderstanding? I see neither the commit message nor the issue emphasized that this PR is merely extending the current behavior (pip install . installs stuff in /usr/bin) from Leap 15.2 to Tumbleweed.

I double-checked it just now:

    master: ++ pip install .
    master: Processing /root/ceph-salt
    master: Collecting Click>=6.7 (from ceph-salt==15.2.0)
    master:   Downloading https://files.pythonhosted.org/packages/dd/c0/4d8f43a9b16e289f36478422031b8a63b54b6ac3b1ba605d602f10dd54d6/click-7.1.1-py2.py3-none-any.whl (82kB)
    master: Collecting configshell-fb>=1.1 (from ceph-salt==15.2.0)
    master:   Downloading https://files.pythonhosted.org/packages/c1/b6/86599b430f70b4c8a77e47188fa0bcecca62ecd2900e1011ff37107dc18e/configshell-fb-1.1.27.tar.gz (57kB)
    master: Collecting pycryptodomex>=3.4.6 (from ceph-salt==15.2.0)
    master:   Downloading https://files.pythonhosted.org/packages/bc/97/68a589e4913b5081e365920a9303a712ccc1e6f6f30cafba2f1fc3644f5e/pycryptodomex-3.9.7-cp36-cp36m-manylinux1_x86_64.whl (13.7MB)
    master: Requirement already satisfied: PyYAML>=5.1.2 in /usr/lib64/python3.6/site-packages (from ceph-salt==15.2.0) (5.1.2)
    master: Requirement already satisfied: salt>=2019.2.0 in /usr/lib/python3.6/site-packages (from ceph-salt==15.2.0) (2019.2.3)
    master: Requirement already satisfied: tornado<5.0,>=4.2.1 in /usr/lib64/python3.6/site-packages (from ceph-salt==15.2.0) (4.5.3)
    master: Requirement already satisfied: pyparsing>=2.0.2 in /usr/lib/python3.6/site-packages (from configshell-fb>=1.1->ceph-salt==15.2.0) (2.2.0)
    master: Requirement already satisfied: six in /usr/lib/python3.6/site-packages (from configshell-fb>=1.1->ceph-salt==15.2.0) (1.14.0) 
    master: Collecting urwid (from configshell-fb>=1.1->ceph-salt==15.2.0)
    master:   Downloading https://files.pythonhosted.org/packages/45/dd/d57924f77b0914f8a61c81222647888fbb583f89168a376ffeb5613b02a6/urwid-2.1.0.tar.gz (630kB)
    master: Installing collected packages: Click, urwid, configshell-fb, pycryptodomex, ceph-salt
    master:   Running setup.py install for urwid: started
    master:     Running setup.py install for urwid: finished with status 'done'
    master:   Running setup.py install for configshell-fb: started
    master:     Running setup.py install for configshell-fb: finished with status 'done'
    master:   Running setup.py install for ceph-salt: started
    master:     Running setup.py install for ceph-salt: finished with status 'done'
    master: Successfully installed Click-7.1.1 ceph-salt-15.2.0 configshell-fb-1.1.27 pycryptodomex-3.9.7 urwid-2.1.0
    master: You are using pip version 10.0.1, however version 20.0.2 is available.
    master: You should consider upgrading via the 'pip install --upgrade pip' command.

$ sesdev ssh octopus
Warning: Permanently added '192.168.121.149' (ECDSA) to the list of known hosts.
Have a lot of fun...
master:~ # type ceph-salt
ceph-salt is /usr/bin/ceph-salt

In other words, we have been "polluting the filesystem", as you put it, since the very beginning...

Now, I suppose we could change pip install --prefix /usr . to pip install --prefix /usr/local . - i.e. change the current behavior in Leap 15.2 to match that of Tumbleweed, but we would need to really be sure that /usr/local/bin will always be in the PATH - not just when running ceph_salt_deployment.sh` but also when SSHing into the cluster.

But is this really needed? sesdev never installs ceph-salt both from source and from RPM, only one or the other.

@toabctl
Copy link
Contributor

toabctl commented Apr 8, 2020

Maybe there is a misunderstanding? I see neither the commit message nor the issue emphasized that this PR is merely extending the current behavior (pip install . installs stuff in /usr/bin) from Leap 15.2 to Tumbleweed.

Ah. I wasn't aware of that. Thanks for clarification.

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

@ricardoasmarques ricardoasmarques ricardoasmarques approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

On tumbleweed, "pip install" puts executables in "/usr/local/bin" but this is not in the PATH
3 participants
@smithfarm @toabctl @ricardoasmarques