etcd_migrate: Add /usr/local/bin to path for oadm #4633
Conversation
There are cases where ansible may not keep the expected environment PATH. This change adds /usr/local/bin to the PATH on the oadm call. Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1464010
roles/etcd_migrate/tasks/migrate.yml
Outdated
| @@ -42,6 +44,7 @@ | |||
| --lease-duration 1h | |||
| environment: | |||
| ETCDCTL_API: 3 | |||
| PATH: "/usr/local/bin:{{ ansible_env.PATH }}" | |||
There was a problem hiding this comment.
is the PATH always set on AH with the proper path to the binary
There was a problem hiding this comment.
It is:
root:
# echo $PATH
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/.local/bin:/root/bin
cloud-user:
$ echo $PATH
/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/home/cloud-user/.local/bin:/home/cloud-user/bin
However, it is not found which leads us to believe that the PATH is being overwritten.
|
Added |
|
@ingvagabund / @kwoodson if either of you guys are good with the addition please 👍 with a review. @sdodson not sure if CI tests this or not. |
| @@ -42,6 +44,7 @@ | |||
| --lease-duration 1h | |||
| environment: | |||
| ETCDCTL_API: 3 | |||
| PATH: "/usr/local/bin:/var/usrlocal/bin:{{ ansible_env.PATH }}" | |||
There was a problem hiding this comment.
Is it really /var/usrlocal/bin? Not /var/usr/local/bin
There was a problem hiding this comment.
@ingvagabund it is:
[root@atomic-00 ~]# ls -la /var/usr/local/bin
ls: cannot access /var/usr/local/bin: No such file or directory
[root@atomic-00 ~]# ls -la /var/usrlocal/bin/
total 0
drwxr-xr-x. 2 root root 6 Jun 28 15:52 .
drwxr-xr-x. 11 root root 114 May 22 22:50 .
There was a problem hiding this comment.
should {{ openshift.common.admin_binary }} be used instead of oadm in the command? It has the absolute path for the oadm executable
There was a problem hiding this comment.
It is set to /usr/local/bin/oadm if the deployment is containerized. Based on grep -rn admin_binary I was not able to find any assignment for the AH. Plus, the fact does not cover /var/usrlocal/bin case.
There was a problem hiding this comment.
Perhaps, create a separate PR that extends the openshift_facts module and sets the openshift.common.admin_binary to the proper location. @ashcrow when does the /var/usrlocal/bin applies and when /usr/local/bin? Is there any rule to that? Optionally, the module could check if /var/usrlocal/bin/oadm or /usr/local/bin/oadm exists and then set the admin_binary variable properly.
There was a problem hiding this comment.
/var/usrlocal/bin should not really be used on Atomic. It is a detail of how /usr/local is made writeable. Checked on the last version of RHELAH:
# rpm-ostree status
State: idle
Deployments:
● rhel-atomic-host-ostree:rhel-atomic-host/7/x86_64/standard
Version: 7.3.6 (2017-06-23 16:20:45)
Commit: e073a47baa605a99632904e4e05692064302afd8769a15290d8ebe8dbfd3c81b
# ls -l /usr/local
lrwxrwxrwx. 3 root root 15 Feb 19 2015 /usr/local -> ../var/usrlocal
# stat /usr/local/bin/oadm
File: ‘/usr/local/bin/oadm’ -> ‘openshift’
Size: 9 Blocks: 0 IO Block: 4096 symbolic link
Device: fd00h/64768d Inode: 4294213 Links: 1
Access: (0777/lrwxrwxrwx) Uid: ( 0/ root) Gid: ( 0/ root)
Context: unconfined_u:object_r:var_t:s0
Access: 2017-06-29 09:30:24.375797982 +0000
Modify: 2017-06-26 17:33:32.993767546 +0000
Change: 2017-06-26 17:33:32.993767546 +0000
Birth: -
There was a problem hiding this comment.
I don't think it will hurt anything having /var/usrlocal/bin in the path. Normally I'd go ahead and remove it since it should be the same as /usr/local/bin but I'd rather not re-ci for it.
There was a problem hiding this comment.
sure that is not a blocker, we can go forward and merge this. My observation was about setting $PATH instead of using {{ openshift.common.admin_binary }} which should already have the correct path to oadm.
There was a problem hiding this comment.
Oh I see what you mean. I did do a quick dependency check and the role does not depend on openshift_facts however the playbook does.
@ingvagabund would it be safe to use openshift.common.admin_binary in the role?
|
aos-ci-test |
There are cases where
ansiblemay not keep the expected environmentPATH. This change adds/usr/local/binand/var/usrlocal/binto thePATHon theoadmcall.Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1464010