On 12/23/2012 02:42 PM, Lucas Meneghel Rodrigues wrote:
On 12/21/2012 01:41 PM, Cleber Rosa wrote:A utility designed to perform a complete autotest database creation and initialization at once.Currently, to get an autotest server running one needs to either run the "install-autotest-server.sh" script, or follow a long list of steps. This tool will allow users of say, autotest-server distro specific packages to just run a command and have it setup.This is awesome, slowly we're making the install process a lot better. I have some comments below:Signed-off-by: Cleber Rosa <[email protected]> --- contrib/install-autotest-server.sh | 50 +----installation_support/autotest-database-turnkey | 279 +++++++++++++++++++++++++installation_support/common.py | 14 ++ 3 files changed, 302 insertions(+), 41 deletions(-) create mode 100755 installation_support/autotest-database-turnkey create mode 100644 installation_support/common.pydiff --git a/contrib/install-autotest-server.sh b/contrib/install-autotest-server.shindex 83dbe2b..1c2d8c3 100755 --- a/contrib/install-autotest-server.sh +++ b/contrib/install-autotest-server.sh @@ -324,11 +324,12 @@ chmod 775 $ATHOME } check_mysql_password() { -print_log "INFO" "Verifying MySQL root password" +print_log "INFO" "Setting MySQL root password" mysqladmin -u root password $MYSQLPW > /dev/null 2>&1-DB=$(echo "use autotest_web;" | mysql --user=root --password=$MYSQLPW 2>&1)-if [ "$(echo $DB | grep 'Access denied')" != "" ] +print_log "INFO" "Verifying MySQL root password"+$ATHOME/installation_support/autotest-database-turnkey --check-credentials --root-password=$MYSQLPW+if [ $? != 0 ] then print_log "ERROR" "MySQL already has a different root password" exit 1 @@ -336,15 +337,12 @@ fi } create_autotest_database() { -if [ "$(echo $DB | grep 'Unknown database')" != "" ] +print_log "INFO" "Creating MySQL databases for autotest"+$ATHOME/installation_support/autotest-database-turnkey -s --root-password=$MYSQLPW -p $MYSQLPW > /dev/null 2>&1+if [ $? != 0 ] then - print_log "INFO" "Creating MySQL databases for autotest" - cat << SQLEOF | mysql --user=root --password=$MYSQLPW >> $LOG -create database autotest_web;-grant all privileges on autotest_web.* TO 'autotest'@'localhost' identified by "$MYSQLPW";-grant SELECT on autotest_web.* TO 'nobody'@'%'; -grant SELECT on autotest_web.* TO 'nobody'@'localhost'; -SQLEOF + print_log "ERROR" "Error creating MySQL database" + exit 1 fi } @@ -388,32 +386,6 @@ else fi } -configure_autotest() { -print_log "INFO" "Setting up the autotest configuration files" - -# TODO: notify_email in [SCHEDULER] section of global_config.ini - -cat << EOF | su - autotest >> $LOG 2>&1-/usr/local/bin/substitute please_set_this_password "$MYSQLPW" $ATHOME/global_config.ini-EOF -} - -setup_databse_schema() {-TABLES=$(echo "use autotest_web; show tables;" | mysql --user=root --password=$MYSQLPW 2>&1)- -if [ "$(echo $TABLES | grep tko_test_view_outer_joins)" = "" ] -then - print_log "INFO" "Setting up the database schemas" - cat << EOF | su - autotest >> $LOG 2>&1 -yes yes | $ATHOME/database/migrate.py --database=AUTOTEST_WEB sync -yes no | /usr/local/autotest/frontend/manage.py syncdb -/usr/local/autotest/frontend/manage.py syncdb -EOF -else - print_log "INFO" "Database schemas are already in place" -fi -} - restart_mysql_deb() { print_log "INFO" "Re-starting MySQL server" service mysql restart >> $LOG @@ -601,8 +573,6 @@ full_install() { create_autotest_database build_external_packages configure_webserver_rh - configure_autotest - setup_databse_schema restart_mysql_rh patch_python27_bug build_web_rpc_client @@ -622,8 +592,6 @@ full_install() { create_autotest_database build_external_packages configure_webserver_deb - configure_autotest - setup_databse_schema restart_mysql_deb build_web_rpc_client import_testsdiff --git a/installation_support/autotest-database-turnkey b/installation_support/autotest-database-turnkeynew file mode 100755 index 0000000..a981b9a --- /dev/null +++ b/installation_support/autotest-database-turnkey @@ -0,0 +1,279 @@ +#!/usr/bin/env python + + +"""+This script attemps to make it trivial to create the Autotest server database+and populate with the needed schema, all in one simple command +""" + + +import os +import re +import logging +import optparse + +import MySQLdb +import django.core.management + +try: + import autotest.common as common +except ImportError: + import common + +from autotest.client.shared import global_config +from autotest.database.migrate import MigrationManager +from autotest.database.database_connection import DatabaseConnection +from autotest.frontend import setup_django_environment + + +DB_ADMIN_USER = 'root' + ++def database_already_exists(database, user, password, hostname='localhost'):+ ''' + Detects if the given password exist by trying to connect to it + ''' + try:+ connection = MySQLdb.connect(user=user, passwd=password, db=database,+ host=hostname) + return True + except MySQLdb.OperationalError: + return False + + +def admin_credentials_valid(password, hostname='localhost'): + ''' + Checks if the database admin (root) credential is valid + ''' + try:+ connection = MySQLdb.connect(user=DB_ADMIN_USER, passwd=password)+ return True + except: + return FalseCatching all exceptions here might mask the fact that there are other problems going on with the database rather than just invalid credentials, as I see. Isn't it better to catch a more specific error and handle other problems differently (maybe even terminating the script, and saying something about the error)?
Maybe the name of the function is a bit misleading, but the idea is to check if we can connect to the database using the given root password. That function is used as a sanity check on other functions (create_database, database_setup) and also when calling:
# autotest-database-turnkey --root-password=<password> --check-credentialsAnd in all these case, a proper error message is displayed and the script is terminated.
BTW, I decided not to have many error conditions out of these functions for the sole reason that it would complicate the handling by scripts. Currently, it exits with either 0 or -1. If you think it'd be necessary or desirable to have more fine grained error codes, let me know.
+ +def create_database(database, root_password='',+ hostname='localhost', conn=None, transaction=False):+ if conn is None: + if not admin_credentials_valid(root_password, hostname): + logging.error("Failed to logon as the database admin user") + return False+ conn = MySQLdb.connect(user=DB_ADMIN_USER, passwd=root_password)+ + if database_already_exists(database, DB_ADMIN_USER, root_password, + hostname): + logging.info("Database already exists, doing nothing") + return True + + if transaction: + conn.begin() + + curs = conn.cursor() + try: + curs.execute("CREATE DATABASE %s" % database) + except MySQLdb.ProgrammingError, MySQLdb.OperationalError: + conn.rollback() + return False + + if transaction: + conn.commit() + + return True + + +def database_setup(database, user, password, root_password, + hostname='localhost'): + ''' + Attempts to create database AND users AND set privileges + ''' + # To actually give the privileges, we use the root user + if not admin_credentials_valid(root_password, hostname): + logging.error("Failed to logon as the database admin user") + return False + + conn = MySQLdb.connect(user=DB_ADMIN_USER, passwd=root_password) + conn.begin() + curs = conn.cursor() + + if not create_database(database, hostname=hostname, + root_password=root_password, conn=conn): + conn.rollback() + return False + + + GRANT_CMDS = ["grant all privileges on %(database)s.* TO "+ "'%(user)s'@'localhost' identified by '%(password)s'",+ "grant SELECT on %(database)s.* TO 'nobody'@'%%'",+ "grant SELECT on %(database)s.* TO 'nobody'@'localhost'"]+ for cmd in GRANT_CMDS: + cmd_ = cmd % locals()^ What's the purpose of passing locals() to the db command?
Because every command is a different line that needs parameter expansion, and the parameters are not always the same. Either I had to do that or have a more lines of code for individual parameter expansion. I guess it's just a matter of style and I'm not sure what's the downside of this approach.
+ try: + curs.execute(cmd_) + except: + conn.rollback() + return False + + conn.commit() + return True + + +def set_global_config_value(section, key, value): + ''' + Sets a value on the configuration file ++ It does so by reading all lines an rewriting the one needed. This is+ far from efficient and should only be used to perform changes to a + handful of configuration values + '''global_config.ini is a .ini file, that can be parsed and modified using ConfigParser (which seems much simpler and robust), so I didn't quite understand why to do things this way. I'm taking your word that this was really necessary... but I'd like to read some words on the why.
^ Well, using ConfigParser was the obvious thing to do, and actually the first thing that I did. But there's a caveat: ConfigParser won't preserve comments and does not keep the original order of sections and key/values.
So, for users of an installation based on the contrib script, they would get no comments in the config file, and a completely different file when doing a 'git diff'.
_______________________________________________ Autotest-kernel mailing list [email protected] https://www.redhat.com/mailman/listinfo/autotest-kernel
