On Thu, Dec 27, 2012 at 11:36 AM, Cleber Rosa <[email protected]> wrote:
> 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.py
>>>
>>> diff --git a/contrib/install-autotest-server.sh
>>> b/contrib/install-autotest-server.sh
>>> index 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_tests
>>> diff --git a/installation_support/autotest-database-turnkey
>>> b/installation_support/autotest-database-turnkey
>>> new 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 False
>>
>>
>> Catching 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-credentials
>
> And 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.
Fair enough, this looks fine, thanks for the explanation.
>
>>
>>> +
>>> +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.
Makes sense, it looks fine.
>
>>
>>> + 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'.
Ooooh, I've completely missed that one. Looks great, thanks for the explanation.
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
