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.
> diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py
> index 91403ad..a922ad9 100644
> --- a/lib/cmdlib/common.py
> +++ b/lib/cmdlib/common.py
> @@ -20,10 +20,18 @@
>
>
> """Common functions used by multiple logical units."""
> +import copy
> +import os
>
> +from ganeti import constants
> from ganeti import errors
> from ganeti import locking
> from ganeti import utils
> +from ganeti import hypervisor
These two break the order.
> +from ganeti import objects
> +from ganeti import pathutils
> +from ganeti import rpc
> +from ganeti import ssconf
This the order of the functions in the file:
_RunPostHook
_RedistributeAncillaryFiles
_ComputeAncillaryFiles
_UploadHelper
_MergeAndVerifyHvState
_MergeAndVerifyDiskState
_GetUpdatedIPolicy
_ComputeNewInstanceViolations
_GetUpdatedParams
_CheckOSParams
_CheckHVParams
_AdjustCandidatePool
_CheckNodePVs
_ComputeMinMaxSpec
_ComputeIPolicySpecViolation
_ComputeIPolicyInstanceViolation
_ComputeViolatingInstances
_AnnotateDiskParams
_SupportsOob
_UpdateAndVerifySubDict
_FilterVmNodes
The order doesn't always reflect the definition-preceding-use rule,
but that's not a big deal, but what bothers me is
_ComputeNewInstanceViolations is very far from the ipolicy functions.
It's better to move _GetUpdatedParams, _GetUpdatedIPolicy, and
_ComputeNewInstanceViolations later in the file.
> +def _ComputeIPolicyInstanceViolation(ipolicy, instance, cfg,
> +
> compute_fn=_ComputeIPolicySpecViolation):
"compute_fn" was "_compute_fn" before, and I think it should remain that way.
> diff --git a/test/py/ganeti.cmdlib_unittest.py
> b/test/py/ganeti.cmdlib_unittest.py
> index bc4e7ee..8d0ecdd 100755
> --- a/test/py/ganeti.cmdlib_unittest.py
> +++ b/test/py/ganeti.cmdlib_unittest.py
> @@ -34,6 +34,8 @@ import copy
> from ganeti import constants
> from ganeti import mcpu
> from ganeti import cmdlib
> +from ganeti.cmdlib import cluster
> +from ganeti.cmdlib import common
Uhm, maybe we should split the tests too. But not in this patch, anyway.
Thanks,
Bernardo
