-
Notifications
You must be signed in to change notification settings - Fork 152
[TRAFODION-1839] Trafodion Installer Evolution #807
Conversation
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1309/ |
Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1309/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand everything needed in installer, but this is definitely a good start for the re-write. Few minor suggestions.
- /etc/hosts contains hostname info for all Trafodion nodes on **installer node** | ||
- python version 2.6/2.7, and python library `httplib2`, `prettytable` | ||
- Trafodion server package file is stored on **installer node** | ||
- Passwordless SSH login, two ways: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to clarify that one of these two options is needed, not both.
from ConfigParser import ConfigParser | ||
from collections import defaultdict | ||
|
||
__VERSION__ = 'v1.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coding a version number here is not a good idea. If we need one, it should be generated at time the installer is packaged.
DBCFG_TMP_FILE = INSTALLER_LOC + '/.db_config_temp' | ||
|
||
TMP_DIR = '/tmp/.install' | ||
MARK = '[ERR]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.install is a very generic name. It may possibly be used for other things and hence not secured properly, etc.
Maybe something like /tmp/trafodion_install_temp would be better.
|
||
def run_cmd_as_user(user, cmd): | ||
return run_cmd('sudo su - %s -c \'%s\'' % (user, cmd)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to use -n (non-interactive) option to sudo, so we never get password prompt, etc.
key_file = '/tmp/id_rsa' | ||
run_cmd('sudo rm -rf %s*' % key_file) | ||
run_cmd('sudo echo -e "y" | ssh-keygen -t rsa -N "" -f %s' % key_file) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use -q option to prevent ssh-keygen from prompting.
from common import run_cmd, cmd_output, err | ||
|
||
# not used | ||
EPEL_REPO = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not need to be hard-coded. Doing a yum install of "epel-release" package will get you the epel repo file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only need 'pdsh' dependency from epel repository, so I'm thinking to install 'pdsh' rpm directly, but not use epel repository. Is it acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly? I'm not sure what is meant, but I think it is best to install from epel repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same. Directly seems to imply that we're distributing the pdsh package?
run_cmd('mkdir -p %s' % TRAF_DIR) | ||
run_cmd('tar xf %s -C %s' % (TRAF_PACKAGE_FILE, TRAF_DIR)) | ||
|
||
print 'Trafodion package uncompressed successfully!' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "extracted" would make more sense to me than "uncompressed". But opinions may vary.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1313/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1313/ |
A huge change, need sometime to review it. Is previous installer still work together with this new one? I mean, one can still use the old installer as well this new installer? Or it is a replacement abruptly? |
The two installer will coexist for a short of time, both of them can be used. Once the new one is stable and acceptable by users, we can remove the old one. There're still features need to be implemented for the new installer, for example, elasticity. |
+1 to Ming. Would also like a chance to review but if we are not making switch yet that's different. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. +1
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1334/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1334/ |
is this good to be merged? |
new python version installer