On 16 May 2013 16:52, Bernardo Dal Seno <[email protected]> wrote: > On 16 May 2013 10:23, Thomas Thrainer <[email protected]> wrote: >> All LUCluster* classes are extracted to cluster.py. Shared functions are >> extracted to common.py, helper functions only used by LUCluster* are >> extracted to cluster.py. >> >> Signed-off-by: Thomas Thrainer <[email protected]> >> --- >> Makefile.am | 1 + >> lib/cmdlib/__init__.py | 3427 >> +------------------------------------ >> lib/cmdlib/cluster.py | 2875 +++++++++++++++++++++++++++++++ >> lib/cmdlib/common.py | 602 +++++++ >> test/py/ganeti.cmdlib_unittest.py | 107 +- >> 5 files changed, 3547 insertions(+), 3465 deletions(-) >> create mode 100644 lib/cmdlib/cluster.py > > Mostly LGTM, except... > > >> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py >> new file mode 100644 >> index 0000000..211a0a6 >> --- /dev/null >> +++ b/lib/cmdlib/cluster.py >> @@ -0,0 +1,2875 @@ >> +# >> +# >> + >> +# Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 Google Inc. >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, but >> +# WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> +# General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program; if not, write to the Free Software >> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> +# 02110-1301, USA. >> + >> + >> +"""Logical units dealing with the cluster.""" >> + >> +import OpenSSL >> + >> +import copy >> +import itertools >> +import logging >> +import operator >> +import os >> +import re >> +import time >> + >> +from ganeti import compat >> +from ganeti import constants >> +from ganeti import errors >> +from ganeti import hypervisor >> +from ganeti import locking >> +from ganeti import netutils >> +from ganeti import objects >> +from ganeti import opcodes >> +from ganeti import pathutils >> +from ganeti import query >> +from ganeti import rpc >> +from ganeti import runtime >> +from ganeti import ssh >> +from ganeti import uidpool >> +from ganeti import utils >> +from ganeti import vcluster >> +from ganeti import masterd > > All the other imports are alphabetically ordered, please put masterd > in order too. > > >> + >> +from ganeti.cmdlib.base import NoHooksLU, _QueryBase, LogicalUnit, \ >> + ResultWithJobs >> +from ganeti.cmdlib.common import _ShareAll, _RunPostHook, \ >> + _ComputeAncillaryFiles, _RedistributeAncillaryFiles, _UploadHelper, \ >> + _GetWantedInstances, _MergeAndVerifyHvState, _MergeAndVerifyDiskState, \ >> + _GetUpdatedIPolicy, _ComputeNewInstanceViolations, _GetUpdatedParams, \ >> + _CheckOSParams, _CheckHVParams, _AdjustCandidatePool, _CheckNodePVs, \ >> + _ComputeIPolicyInstanceViolation, _AnnotateDiskParams, \ >> + _SupportsOob >> + >> +import ganeti.masterd.instance # pylint: disable=W0611 > > Why this import? And why here? > > These three functions should be moved before their use: > >> +def _ValidateNetmask(cfg, netmask): >> + """Checks if a netmask is valid. > >> +def _VerifyCertificate(filename): >> + """Verifies a certificate for L{LUClusterVerifyConfig}. > >> +def _GetAllHypervisorParameters(cluster, instances): >> + """Compute the set of all hypervisor parameters.
I've seen this has been done in the next patch. Probably due to a glich while rebasing. It's okay to leave things as they are if you don't want to become mad in splitting patches, but in that case please update the comment in the following patch. Bernardo
