General remarks: Consider adding another node to the mix - the QA is missing checks for global hooks running on both master and other nodes, and running on master despite not being executed on other nodes. You can allow the LUTestDelay to run hooks to achieve that effect - it can be run on arbitrary nodes, and being a test opcode can be modified as you wish.
Alternatively, unit test these cases? The main benefit of the QA is that we get to see how global hooks work with real files, and more complicated cases can be unit tested. On Fri, Nov 20, 2015 at 5:11 PM, 'Oleg Ponomarev' via ganeti-devel < [email protected]> wrote: > Test 3 main cases of global post hooks usage: > - successful LU execution; > - LU with the prerequisites failed; > - disappeared LU process. > All the tests are performed on the master node. > > Signed-off-by: Oleg Ponomarev <[email protected]> > --- > Makefile.am | 1 + > qa/ganeti-qa.py | 13 ++++++ > qa/qa-sample.json | 2 + > qa/qa_global_hooks.py | 120 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > qa/qa_utils.py | 11 +++++ > 5 files changed, 147 insertions(+) > create mode 100644 qa/qa_global_hooks.py > > diff --git a/Makefile.am b/Makefile.am > index 8ba84a5..0080f00 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -1347,6 +1347,7 @@ qa_scripts = \ > qa/qa_error.py \ > qa/qa_filters.py \ > qa/qa_group.py \ > + qa/qa_global_hooks.py \ > qa/qa_instance.py \ > qa/qa_instance_utils.py \ > qa/qa_iptables.py \ > diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py > index 9b475e2..2798a69 100755 > --- a/qa/ganeti-qa.py > +++ b/qa/ganeti-qa.py > @@ -48,6 +48,7 @@ import qa_env > import qa_error > import qa_filters > import qa_group > +import qa_global_hooks > import qa_instance > import qa_iptables > import qa_maintd > @@ -947,6 +948,16 @@ def RunPerformanceTests(): > qa_config.ReleaseManyNodes(inodes) > > > +def RunGlobalHooksTests(): > Please move this function to the qa_global_hooks file rather than keep it here - it's completely related to global hooks and devoted to a single test - the ganeti-qa file is cluttered enough as-is. Also, add a docstring please. > + if not qa_config.TestEnabled("global-hooks"): > + return > While this is more compact, please use the RunTestIf constructs as they will show helpful messages about tests being skipped. > + qa_global_hooks.TestHooksInitialize() > + qa_global_hooks.TestHookSucceeded() > + qa_global_hooks.TestHookFailed() > + qa_global_hooks.TestHookDisappeared() > + qa_global_hooks.TestHooksDeinitialize() > + > + > def RunQa(): > """Main QA body. > > @@ -965,6 +976,8 @@ def RunQa(): > RunTestBlock(RunNetworkTests) > RunTestBlock(RunFilterTests) > > + RunTestBlock(RunGlobalHooksTests) > + > # The master shouldn't be readded or put offline; "delay" needs a > non-master > # node to test > pnode = qa_config.AcquireNode(exclude=qa_config.GetMasterNode()) > diff --git a/qa/qa-sample.json b/qa/qa-sample.json > index ac10a05..9b0a1a1 100644 > --- a/qa/qa-sample.json > +++ b/qa/qa-sample.json > @@ -253,6 +253,8 @@ > > "job-list": true, > > + "global-hooks": true, > + > "jobqueue-performance": true, > "parallel-performance": true, > > diff --git a/qa/qa_global_hooks.py b/qa/qa_global_hooks.py > new file mode 100644 > index 0000000..4db151b > --- /dev/null > +++ b/qa/qa_global_hooks.py > @@ -0,0 +1,120 @@ > +# > +# > + > +# Copyright (C) 2012, 2014 Google Inc. > Correct this to 2015. > +# All rights reserved. > +# > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions are > +# met: > +# > +# 1. Redistributions of source code must retain the above copyright > notice, > +# this list of conditions and the following disclaimer. > +# > +# 2. Redistributions in binary form must reproduce the above copyright > +# notice, this list of conditions and the following disclaimer in the > +# documentation and/or other materials provided with the distribution. > +# > +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS > +# IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > +# TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A > PARTICULAR > +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR > +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > +# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > +# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > +# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > +# LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING > +# NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > + > +"""QA tests for the universal job hooks. > + > +""" > + > +import time > + > +from ganeti import constants > +from ganeti import pathutils > +from qa_config import GetMasterNode > +from qa_job_utils import ExecuteJobProducingCommand > +from qa_utils import AssertEqual, GetCommandOutput, IsFileExists > + > +PRE_PATH = "%s/global-pre.d" % pathutils.HOOKS_BASE_DIR > +POST_PATH = "%s/global-post.d" % pathutils.HOOKS_BASE_DIR > +H_DIR = "/var/log/ganeti/qa_global_hooks" > + > + > +def _GetHookFilePath(job_id, phase, status=None): > Also, docstring. > + h_fname = H_DIR + "/%d_OP_TEST_DELAY_" % job_id > + if phase == "pre": > + return h_fname + phase Can't the phase be added in the string formatting above? > + return h_fname + phase + "_" + status > + > + > +def TestHooksInitialize(): > + """Creates global hooks on the master node""" > + master = GetMasterNode().primary > + GetCommandOutput(master, "mkdir -p %s" % pathutils.HOOKS_BASE_DIR) > + GetCommandOutput(master, "mkdir -p %s" % PRE_PATH) > + GetCommandOutput(master, "mkdir -p %s" % POST_PATH) > + GetCommandOutput(master, "mkdir -p %s" % H_DIR) > + h_common = "CMD=$'#!/bin/sh\\ntouch %s/" % H_DIR + \ > + "$GANETI_JOB_ID\"_\"$GANETI_OP_CODE" > May I suggest using the """ format of Python strings so you do not have to encode newlines and quotes? > + h_pre = h_common + "\"_pre\"'" > + h_post = h_common + "\"_post_\"$GANETI_POST_STATUS'" > Also, for readability purposes define these two by formatting them with H_DIR and the suffix, making the format more clear? > + h_name = "/qa_test_hook" > + h_echo = ";echo \"$CMD\" > %s" + h_name > + GetCommandOutput(master, h_pre + h_echo % PRE_PATH) > + GetCommandOutput(master, h_post + h_echo % POST_PATH) > In conclusion, to make this a lot more readable: - have a triple-quoted sequence of commands - fill it in with arguments prior to execution - decide whether constants are local (h_name) or not > + h_chmod = "chmod +x " > + GetCommandOutput(master, h_chmod + PRE_PATH + h_name) > + GetCommandOutput(master, h_chmod + POST_PATH + h_name) > Also, try to be consistent regarding how you construct your commands - why string concatenation and not formatting here? > + > + > +def TestHookSucceeded(): > + master = GetMasterNode().primary > + job_id = ExecuteJobProducingCommand("gnt-debug delay --submit 1") > + time.sleep(3) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "pre")), True) > Since these error messages will look something like "False != True", consider using the optional msg parameter to ease debugging by saying what you are testing. > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_SUCCESS)), True) + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_ERROR)), False) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_DISAPPEAR)), False) > + > + > +def TestHookFailed(): > + master = GetMasterNode().primary > + job_id = ExecuteJobProducingCommand("gnt-debug delay --submit 0") > + time.sleep(1) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_SUCCESS)), False) + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_ERROR)), True) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_DISAPPEAR)), False) > + > + > +def TestHookDisappeared(): > + master = GetMasterNode().primary > + job_id = ExecuteJobProducingCommand("gnt-debug delay --submit 10") > + time.sleep(1) > + GetCommandOutput(master, "gnt-job cancel --kill --yes-do-it %d" % > job_id) > + time.sleep(10) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "pre")), True) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_SUCCESS)), False) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_ERROR)), False) > + AssertEqual(IsFileExists(master, _GetHookFilePath(job_id, "post", > + constants.POST_HOOKS_STATUS_DISAPPEAR)), True) > + > + > +def TestHooksDeinitialize(): > s/Deinitialize/Cleanup/ > + master = GetMasterNode().primary > + rm = "rm " > + GetCommandOutput(master, rm + PRE_PATH + "/*") > + GetCommandOutput(master, rm + POST_PATH + "/*") > + GetCommandOutput(master, rm + "-rf " + H_DIR) > diff --git a/qa/qa_utils.py b/qa/qa_utils.py > index f2b2c2e..b90e944 100644 > --- a/qa/qa_utils.py > +++ b/qa/qa_utils.py > @@ -523,6 +523,17 @@ def BackupFile(node, path): > return result > > > +def IsFileExists(node, path): > + """Checks if a file on the node exists. > + > + """ > + cmd = ("[[ -f \"%s\" ]] && echo yes || echo no" % path) > + > + # Return temporary filename > + result = GetCommandOutput(node, cmd).strip() > + return True if result == "yes" else False > + > + > @contextlib.contextmanager > def CheckFileUnmodified(node, filename): > """Checks that the content of a given file remains the same after > running a > -- > 2.6.0.rc2.230.g3dd15c0 > > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.
