On Tue, Mar 08, 2011 at 04:11:31PM +0100, Michael Hanselmann wrote: > The cfgupgrade tool was designed to be idempotent, that means it could > be run several times and still give produce the correct result. Ganeti > 2.4 moved the file containing the RAPI users to a separate directory > (…/lib/ganeti/rapi/users). If it exists, cfgupgrade would automatically > move an existing file from …/lib/ganeti/rapi_users and replace it with a > symlink. > > Unfortunately one of the checks for this was incorrect and, when run > multiple times, replaces the users file at the new location with a > symlink created during a previous run. > > In addition the “--dry-run” parameter to cfgupgrade was not respected. > Unittests are updated for all these cases. > > Signed-off-by: Michael Hanselmann <[email protected]> > --- > test/cfgupgrade_unittest.py | 74 +++++++++++++++++++++++++++++++++++++++++- > tools/cfgupgrade | 30 +++++++++++------ > 2 files changed, 91 insertions(+), 13 deletions(-) > > diff --git a/tools/cfgupgrade b/tools/cfgupgrade > index 2222920..e1d5463 100755 > --- a/tools/cfgupgrade > +++ b/tools/cfgupgrade > @@ -185,17 +185,25 @@ def main(): > raise Error("Configuration version %d.%d.%d not supported by this tool" % > (config_major, config_minor, config_revision)) > > - if os.path.isfile(options.RAPI_USERS_FILE_PRE24): > - logging.info("Found pre-2.4 RAPI users file at %s, renaming to %s", > - options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE) > - utils.RenameFile(options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE, > - mkdir=True, mkdir_mode=0750) > - > - # Create a symlink for RAPI users file > - if not os.path.islink(options.RAPI_USERS_FILE_PRE24): > - logging.info("Creating symlink from %s to %s", > - options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE) > - os.symlink(options.RAPI_USERS_FILE, options.RAPI_USERS_FILE_PRE24) > + if not options.dry_run: > + if (os.path.isfile(options.RAPI_USERS_FILE_PRE24) and > + not os.path.islink(options.RAPI_USERS_FILE_PRE24)): > + if os.path.exists(options.RAPI_USERS_FILE): > + raise Error("Found pre-2.4 RAPI users file at %s, but another file" > + " already exists at %s" % > + (options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE)) > + logging.info("Found pre-2.4 RAPI users file at %s, renaming to %s", > + options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE) > + utils.RenameFile(options.RAPI_USERS_FILE_PRE24, > options.RAPI_USERS_FILE, > + mkdir=True, mkdir_mode=0750) > + > + # Create a symlink for RAPI users file > + if (not (os.path.islink(options.RAPI_USERS_FILE_PRE24) or > + os.path.isfile(options.RAPI_USERS_FILE_PRE24)) and > + os.path.isfile(options.RAPI_USERS_FILE)): > + logging.info("Creating symlink from %s to %s", > + options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE) > + os.symlink(options.RAPI_USERS_FILE, options.RAPI_USERS_FILE_PRE24)
Please move just the utils.RenameFile under if not options.dry_run; the rest (checks + messages) should still execute, even in the dry_run case. I'm also thinking that utils.RenameFile should always be followed by os.symlink, but it's also good as it is. thanks, iustin
