Re: [PATCH master] Open VDI Port when spice_use_vdagent is enabled

[Andrea Spadaccini]

Hi Guido,

>>> To use the features spice agent provides, a dedicated virtio-serial
>>> channel must be created in qemu-kvm. The communication between the agent
>>> and the other spice components takes place over this channel.
>>
>> LGTM.
>
> Pushed.

Thanks for taking care of this patch. :)

Andrea

Re: [PATCH master] Implement rbd disk template

[Andrea Spadaccini]

Hello Constantinos,

> Thanks for the many review rounds.
> I refactored the mapping/unmapping/parsing code
> as Michael suggested, and resending the patch.

Thanks for sending the revised patch.

LGTM

For _ParseRbdShowmappedOutput, I don't like the splitting approach,
and I'd prefer a regex. Also, map/filter are "considered somewhat
obsolete" [1] and I'd prefer list comprehension over them.

If you could rewrite it as a regex search with groups matching it
would be ideal for me. However, the function works, is tested and I
don't see any pitfalls so from my point of view it can be committed.
I'll wait for comments from Michael on that before committing.

Thanks again,
Andrea

[1] http://docs.python.org/howto/functional.html#built-in-functions

Re: [PATCH master] Open VDI Port when spice_use_vdagent is enabled

[Andrea Spadaccini]

Hi Nikos,

> To use the features spice agent provides, a dedicated virtio-serial
> channel must be created in qemu-kvm. The communication between the agent
> and the other spice components takes place over this channel.

LGTM.

Will push it tomorrow, as long as I check with someone else on the
team about your CLA.

Thanks,
Andrea

Re: [PATCH master] Open VDI Port when spice_use_vdagent is enabled

[Andrea Spadaccini]

Hi Nikos,

> To use the features spice agent provides, a dedicated virtio-serial
> channel must be created in qemu-kvm. The communication between the agent
> and the other spice components takes place over this channel.

Thanks for the patch. On a first look, it seems good; I'd like to try
it before committing (I don't remember the details of spice-vdagent)
but I am unable to do that before Thursday. I'll send you a reply
later this week.

Cheers,
Andrea

Re: [PATCH master] Implement rbd disk template

[Andrea Spadaccini]

Hi Iustin,

>> +      raise errors.ProgrammerError("Invalid configuration data %s" %
>> +                                   str(unique_id))
>>
>> You don't need to convert unique_id to string, %s will do that for you.
>
> This is wrong. You definitely want this, as unique_id here can be a
> tuple, and the "%" string operator treats tuples specially (remember,
> tuples are not _just_ read-only lists). So for safety, str(unique_id) is
> wanted here.

Right, the syntax for making %s convert a tuple would be just awkward.

Thanks for pointing it out!
Andrea

Re: [PATCH master] Implement rbd disk template

[Andrea Spadaccini]

Hi,

>    showmap_cmd = [RBD_CMD, "showmapped", "-p", "rbd"]

Sorry, this should be

  showmap_cmd = [RBD_CMD, "showmapped", "-p", pool]

Thanks,
Andrea

Re: [PATCH master] Implement rbd disk template

[Andrea Spadaccini]

Hello Constantinos,
Thanks for resending and fixing the "make lint" errors.

I tried your patch and it works fine on my test cluster.

The functionality implemented in the patch looks good. I think that
for having a more complete support we need:
- checks in cluster-verify (e.g., is the ceph config file present in
all the Ganeti nodes? Is rbd running correctly on them?)
- a bit of documentation (man pages, admin guide stating that it is
experimental)

Can you please send two separate patches for cluster-verify and docs?

Now to the review. :)

A couple of points (like the first) might seem nitpicky, but it is for
compliance to the Style Guide:
http://code.google.com/p/ganeti/wiki/StyleGuide.

+  @classmethod
+  def Create(cls, unique_id, children, size, params):
+    """Create a new rbd device

Please always end the first sentence of a comment with a dot.

+      raise errors.ProgrammerError("Invalid configuration data %s" %
+                                   str(unique_id))

You don't need to convert unique_id to string, %s will do that for you.

> +    # Provision a new rbd volume (Image) inside the RADOS cluster
> +    cmd = ["rbd", "create", "-p", "%s" % rbd_pool,
> +           "%s" % rbd_name, "--size", "%s" % size]

I would move "rbd" to a constant, so that if we later add
configure-time binary path resolution we can just change it in one
point. This for all the occurrences of "rbd".

Also, please don't use string formatting to add parameters to the
list: if they are strings (rbd_pool, rbd_name), just add them; if they
are other types (size), convert them. This should be applied to all
lists of command/parameters in this patch.

[…]

> +   Then returns it's device path.

s/it's/its/

[…]

> +    cmd1 = ["rbd", "showmapped"]

Please use more descriptive names than "cmd1" and "cmd2".

Also, you can turn this into
showmap_cmd = [RBD_CMD, "showmapped", "-p", "rbd"]
and avoid checking for the rbd pool later.

> +    result = utils.RunCmd(cmd1)
> +    if result.failed:
> +      _ThrowError("rbd showmapped failed (%s): %s",
> +                  result.fail_reason, result.output)
> +    else:
> +      cmd2 = "echo '%s' | grep %s | grep %s" % (result.output, pool, name)

If you checked for pool in cmd1 (showmap_cmd), you can avoid checking now.

Also, I would use a separate and testable method to parse the output
of rbd showmapped, as you do it many times. Please also add basic unit
tests for the method.

> +      result = utils.RunCmd(cmd2)
> +      if not result.failed:
> +        # The mapping already exists.
> +        # Parse the result and return the rbd device
> +        try:
> +          rbd_dev = re.search("(/dev/rbd\d+)", result.output).group(1)

I think parentheses here are unnecessary if you select group 0.

Also, instead of relying on exceptions for control flow, here you can
check the result of re.search:

rbd_dev = re.search("/dev/rbd\d+", result.output)
if not rbd_dev:
_ThrowError("…")
return rbd_dev.group(0)

> +        except AttributeError, err:
> +          _ThrowError("The rbd mapping exists but couldn't parse "
> +                      "the result of rbd showmapped to find the "
> +                      "corresponding rbd block device (/dev/rbd*): %s",
> +                      str(err))

This should all be in a method, as described before.

[…]

> +        cmd3 = "rbd showmapped | grep %s | grep %s" % (pool, name)

Ditto as before.

[…]

> +    # Check if the mapping already exists
> +    cmd1 = ["rbd", "showmapped"]

Idem.

[…]

> +def _CheckRADOSFreeSpace(name):
> +  """Compute disk size requirements inside the RADOS cluster
> +
> +  """
> +  # For the RADOS cluster we assume there is always enough space
> +  enough_space = True
> +  if not enough_space:
> +raise errors.OpPrereqError("Not enough disk space inside the RADOS"
> +   "cluster to allocate disks for instance: %s" %
> +   (name), errors.ECODE_NORES)

I'd prefer this function to just pass. Add a FIXME or a TODO comment.
Also, do you plan to implement it?

> diff --git a/tools/burnin b/tools/burnin
> index a7ac6f6..8994396 100755
> --- a/tools/burnin
> +++ b/tools/burnin
> @@ -446,7 +446,8 @@ class Burner(object):
>                                 constants.DT_FILE,
>                                 constants.DT_SHARED_FILE,
>                                 constants.DT_PLAIN,
> -                                constants.DT_DRBD8)
> +                                constants.DT_DRBD8,
> +                                constants.DT_RBD)

I know this was already like this, but since you're changing it please
end the multi-line list with a parens on its own line:

constants.DT_RBD,
)

Thanks,
Andrea

Re: [okeanos-dev] Re: [RFC master] Implement rbd disk template

[Andrea Spadaccini]

Hello Stratos,

>> After fighting with a couple of ceph/rbd bugs, today I managed to set
>> up a fully operational ceph cluster. Instance creation works, but
>> burnin does not: can you please tell me which commit ID did you base
>> your tests on? I want to make sure I debug an RBD issue and not
>> something unrelated.
>
> Concerning rbd/rados, we encountered a couple of issues too, while
> testing RBD disks with Ganeti, and so we cherry-picked some commits from
> the master branch, and applied them on v0.39. However, since v0.40 will
> be probably released soon, and will contain a lot of reworked/refactored
> code, I think it would make sense to build and test Ceph from the master
> branch, instead of debugging v0.39 code.
>
> The most critical issues we encountered and their fixes/commits are
> listed below:

Yes, I have already had #4 (that I "fixed" for testing by truncating
to 20 chars the image name in ganeti); I am running ceph 0.39 and will
switch to git master if I encounter some of these problems: thanks for
the suggestion.

> I should also note, that we use ext4 (and not btrfs) as the underlying
> filesystem for the OSDs.

Why are you using ext4? For what I understand, it seems like rados is
designed to take advantage of btrfs features.

Also, can you please tell me the Ganeti revision on which you based your patch?

>> I will post a detailed patch review after testing it, but before
>> please fix the issues that "make lint" will give you (there are a
>> couple of them).
>
> Constantinos will follow-up with an e-mail about the RBD disk template
> patchset.

Great!

Thanks,
Andrea

Re: [RFC master] Implement rbd disk template

[Andrea Spadaccini]

Hello,

> Introduce the rbd disk template, which handles provisioning and
> management of instance disks as block devices mapped to rbd volumes
> on a RADOS cluster.

Thanks for sending the patch.

After fighting with a couple of ceph/rbd bugs, today I managed to set
up a fully operational ceph cluster. Instance creation works, but
burnin does not: can you please tell me which commit ID did you base
your tests on? I want to make sure I debug an RBD issue and not
something unrelated.

I will post a detailed patch review after testing it, but before
please fix the issues that "make lint" will give you (there are a
couple of them).

Thanks,
Andrea

Re: [PATCH master] Fix wrong variable name

[Andrea Spadaccini]

Hi Iustin,

> -  for dt_params in hvparams.values():
> +  for dt_params in diskparams.values():

LGTM, thanks.

Re: [PATCH master 1/4] gnt-instance set-mem

[Andrea Spadaccini]

Hi Guido,
+ganeti-devel

>>> +**set-mem** [--force] [--submit] {*instance*} {*memory*}
>>> +
>>> +Set the run-time memory of an instance to the specified amount.
>>
>> You might want to specify that the memory amount is measured in MB.
>
> Sure. Changed to:
> Set the run-time memory of an instance to the specified amount, in MiB.

Ah it's MiB. So the error message in LUCheckMemory.SetPrereq needs to
be fixed too:

+  raise errors.OpPrereqError("Instance %s must have memory between %d and"
+ " %d MB of memory unless --force is given" %

Thanks,
Andrea

Re: [PATCH master 1/4] gnt-instance set-mem

[Andrea Spadaccini]

Hi Guido,

> +**set-mem** [--force] [--submit] {*instance*} {*memory*}
> +
> +Set the run-time memory of an instance to the specified amount.

You might want to specify that the memory amount is measured in MB.

Andrea

[PATCH master] Fix KVM migration finalization

[Andrea Spadaccini]

KVM execution must be resumed (_CONT_CMD) if we are done with non-live
migration, not with live migration.

Signed-off-by: Andrea Spadaccini 
---
 lib/hypervisor/hv_kvm.py |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py
index 65a2790..ee19b1e 100644
--- a/lib/hypervisor/hv_kvm.py
+++ b/lib/hypervisor/hv_kvm.py
@@ -1714,7 +1714,7 @@ class KVMHypervisor(hv_base.BaseHypervisor):
   pidfile, pid, _ = self._InstancePidAlive(instance.name)
   utils.KillProcess(pid)
   self._RemoveInstanceRuntimeFiles(pidfile, instance.name)
-elif live:
+elif not live:
   self._CallMonitorCommand(instance.name, self._CONT_CMD)
 
   def GetMigrationStatus(self, instance):
-- 
1.7.3.1

Re: [MERGE] stable-2.5 into devel-2.5

[Andrea Spadaccini]

Hi Guido,

>    Merge branch 'stable-2.5' into devel-2.5
>
>    * stable-2.5:
>      KVM: support version reported by 1.0

LGTM

Re: [PATCH stable-2.5] KVM: support version reported by 1.0

[Andrea Spadaccini]

Hi Guido,

[…]

> +  def _ParseKVMVersion(cls, text):
> +    """Parse the KVM version from the --help output.
> +
> +    @type text: string
> +    @param text: output of kvm --version

s/version/help/

Rest LGTM, but please wait for the original reviewers to ack the patch
before pushing it.

Andrea

Re: [PATCH master] qa: fix disk parameters test

[Andrea Spadaccini]

Hi Iustin,

> thanks, LGTM.

Pushed with minor changes to the commit message.

Thanks,
Andrea

Re: [PATCH master] qa: fix disk parameters test

[Andrea Spadaccini]

Hi Iustin,

> One general comment: this doubles the number of calls (3→6) and also
> adds them in cluster modify (so 3→12).

Cluster modify was already tested, so it's 6→12, but of course your
point is still valid.

> Given that each separate call
> does create a job, submit it, etc., this will increase the QA time
> somewhat needlessly (all these could be unittests instead). I.e. it's
> fine to run one or two parameters, but you don't need to cmd-line QA all
> of them.

Will remove the 3 additional tests. (And will update the commit
message in case of LGTM)

> Anyway, that aside:
>> +# data for testing failures due to bad keys/values for disk parameters
>> +_FAIL_PARAMS = ("nonexistent:resync-rate=1",
>> +                "drbd:nonexistent=1",
>> +                "drbd:resync-rate=invalid",
>> +                "drbd:data-stripes=invalid",
>> +                "drbd:meta-stripes=invalid",
>> +                "plain:stripes=invalid",
>> +                )
>
> You're using tuples as list, please don't do that. If you iterate over a
> tuple it means you're misusing it.

I'll change it to a list, even if I don't really see why this is a
misuse of tuples ­— except for breaking the Ganeti conventions.

Thanks,
Andrea

Interdiff:
diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py
index f746d5e..6624600 100644
--- a/qa/qa_cluster.py
+++ b/qa/qa_cluster.py
@@ -59,13 +59,10 @@ def _CheckFileOnAllNodes(filename, content):


 # data for testing failures due to bad keys/values for disk parameters
-_FAIL_PARAMS = ("nonexistent:resync-rate=1",
+_FAIL_PARAMS = ["nonexistent:resync-rate=1",
 "drbd:nonexistent=1",
 "drbd:resync-rate=invalid",
-"drbd:data-stripes=invalid",
-"drbd:meta-stripes=invalid",
-"plain:stripes=invalid",
-)
+]


 def TestClusterInitDisk():

[PATCH master] qa: fix disk parameters test

[Andrea Spadaccini]

Fix an error in the disk parameters tests (the arguments to gnt-cluster
were not passed correctly), move them to separate functions and add a
few more tests for the most recent parameters.

Signed-off-by: Andrea Spadaccini 
---
 qa/ganeti-qa.py  |2 ++
 qa/qa_cluster.py |   40 +++-
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py
index fc50a09..4d6c99e 100755
--- a/qa/ganeti-qa.py
+++ b/qa/ganeti-qa.py
@@ -155,11 +155,13 @@ def RunClusterTests():
 
   """
   for test, fn in [
+("create-cluster", qa_cluster.TestClusterInitDisk),
 ("cluster-renew-crypto", qa_cluster.TestClusterRenewCrypto),
 ("cluster-verify", qa_cluster.TestClusterVerify),
 ("cluster-reserved-lvs", qa_cluster.TestClusterReservedLvs),
 # TODO: add more cluster modify tests
 ("cluster-modify", qa_cluster.TestClusterModifyBe),
+("cluster-modify", qa_cluster.TestClusterModifyDisk),
 ("cluster-rename", qa_cluster.TestClusterRename),
 ("cluster-info", qa_cluster.TestClusterVersion),
 ("cluster-info", qa_cluster.TestClusterInfo),
diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py
index 28b8aef..f746d5e 100644
--- a/qa/qa_cluster.py
+++ b/qa/qa_cluster.py
@@ -58,13 +58,25 @@ def _CheckFileOnAllNodes(filename, content):
 AssertEqual(qa_utils.GetCommandOutput(node["primary"], cmd), content)
 
 
+# data for testing failures due to bad keys/values for disk parameters
+_FAIL_PARAMS = ("nonexistent:resync-rate=1",
+"drbd:nonexistent=1",
+"drbd:resync-rate=invalid",
+"drbd:data-stripes=invalid",
+"drbd:meta-stripes=invalid",
+"plain:stripes=invalid",
+)
+
+
+def TestClusterInitDisk():
+  """gnt-cluster init -D"""
+  name = qa_config.get("name")
+  for param in _FAIL_PARAMS:
+AssertCommand(["gnt-cluster", "init", "-D", param, name], fail=True)
+
+
 def TestClusterInit(rapi_user, rapi_secret):
   """gnt-cluster init"""
-  # data for testing failures due to bad keys/values for disk parameters
-  fail_params = ("-D nonexistent:resync-rate=1",
- "-D drbd:nonexistent=1",
- "-D drbd:resync-rate=invalid")
-
   master = qa_config.GetMasterNode()
 
   rapi_dir = os.path.dirname(constants.RAPI_USERS_FILE)
@@ -110,22 +122,10 @@ def TestClusterInit(rapi_user, rapi_secret):
   if htype:
 cmd.append("--enabled-hypervisors=%s" % htype)
 
-  # test gnt-cluster init failures due to bad keys/values in disk parameters
-  for param in fail_params:
-cmd.extend([param, qa_config.get("name")])
-AssertCommand(cmd, fail=True)
-cmd.pop()
-cmd.pop()
-
   cmd.append(qa_config.get("name"))
   AssertCommand(cmd)
 
   cmd = ["gnt-cluster", "modify"]
-  # test gnt-cluster modify failures due to bad keys/values in disk parameters
-  for param in fail_params:
-cmd.append(param)
-AssertCommand(cmd, fail=True)
-cmd.pop()
 
   # hypervisor parameter modifications
   hvp = qa_config.get("hypervisor-parameters", {})
@@ -279,6 +279,12 @@ def TestClusterReservedLvs():
 AssertCommand(cmd, fail=fail)
 
 
+def TestClusterModifyDisk():
+  """gnt-cluster modify -D"""
+  for param in _FAIL_PARAMS:
+AssertCommand(["gnt-cluster", "modify", "-D", param], fail=True)
+
+
 def TestClusterModifyBe():
   """gnt-cluster modify -B"""
   for fail, cmd in [
-- 
1.7.3.1

Re: [RFC] Support instance disks on RADOS cluster with rbd

[Andrea Spadaccini]

Hello Vangelis,

> We have been working on extending Ganeti to support instance disks on an
> external RADOS cluster, with rbd. RADOS is the distrubuted object
> storage pool that underlies the Ceph parallel filesystem. rbd is a block
> device driver for Linux which constructs disks from objects stored on
> RADOS.

[…]

> Our current implementation is based on devel-2.4. We have a patch for
> devel-2.5 underway, there don't seem to be many required changes
> compared to 2.4, yet. We'd be glad to submit it as an RFC for your
> consideration, if it is of interest.

Please post those patches as RFC. If the 2.5 version is complete, post
that one, otherwise just post the 2.4 version. I'll help you port them
to the master branch and, with the help of the rest of the team, review
them.

Thanks,
Andrea

[PATCH master] Fix parameters for QueryJob in JobExecutor

[Andrea Spadaccini]

When gnt-job submit is used with the --each option, it sends as
parameters for QueryJobs a list of lists, each containing a job ID.
While this works now, it is not the intended format for QueryJobs.

This patch fixes this behavior, by making it send a list of job IDs.

Signed-off-by: Andrea Spadaccini 
---
 lib/cli.py |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/cli.py b/lib/cli.py
index 27edc89..30f6771 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -3153,7 +3153,7 @@ class JobExecutor(object):
   for (_, _, ops) in self.queue:
 # SubmitJob will remove the success status, but raise an exception if
 # the submission fails, so we'll notice that anyway.
-results.append([True, self.cl.SubmitJob(ops)])
+results.append([True, self.cl.SubmitJob(ops)[0]])
 else:
   results = self.cl.SubmitManyJobs([ops for (_, _, ops) in self.queue])
 for ((status, data), (idx, name, _)) in zip(results, self.queue):
-- 
1.7.3.1

Re: [PATCH devel-2.5] locking: Add “__repr__” to SharedLock and PipeCondition

[Andrea Spadaccini]

Hi Michael,

>> Why are you building and destroying a list instead of using just
>> string formatting?
>
> Copy and paste. :-)

:)

> Interdiff:

LGTM

Andrea

Re: [PATCH devel-2.5] locking: Add “__repr__” to SharedLock and PipeCondition

[Andrea Spadaccini]

Hi Michael,

> +  def __repr__(self):
> +    status = ["%s.%s" % (self.__class__.__module__, self.__class__.__name__),
> +              "waiters=%s" % self._waiters]
> +
> +    return "<%s at %#x>" % (" ".join(status), id(self))

Why are you building and destroying a list instead of using just
string formatting?

  return "<%s.%s waiters=%s at %#x>" % (self.__class__.__module__,
self.__class__.__name__,
self._waiters, id(self))

Andrea

Re: [PATCH master] Fix cluster destroy failure

[Andrea Spadaccini]

>> +      logging.warning("Error disabling the master IP address: %s",
>> +                      result.fail_msg)
>
> Please use self.LogWarning—logging.warning doesn't go to the opcode log.

Pushed with this change, thanks.

Andrea

[PATCH master] Fix cluster destroy failure

[Andrea Spadaccini]

Cluster destroy would fail if there were errors while deactivating the
master IP address. This patch demotes such errors to warnings, allowing
the cluster to be destroyed even if the master IP address turndown
fails.

Signed-off-by: Andrea Spadaccini 
---
 lib/cmdlib.py |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index cc2d2f0..70979dd 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -1498,7 +1498,9 @@ class LUClusterDestroy(LogicalUnit):
 ems = self.cfg.GetUseExternalMipScript()
 result = self.rpc.call_node_deactivate_master_ip(master_params.name,
  master_params, ems)
-result.Raise("Could not disable the master role")
+if result.fail_msg:
+  logging.warning("Error disabling the master IP address: %s",
+  result.fail_msg)
 
 return master_params.name
 
-- 
1.7.3.1

[PATCH master] Add docs for missing option in gnt-debug man page

[Andrea Spadaccini]

Document the --each option of gnt-debug submit-job.

Signed-off-by: Andrea Spadaccini 
---
 man/gnt-debug.rst |5 -
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/man/gnt-debug.rst b/man/gnt-debug.rst
index 7989631..2122825 100644
--- a/man/gnt-debug.rst
+++ b/man/gnt-debug.rst
@@ -63,7 +63,7 @@ SUBMIT-JOB
 ~~
 
 **submit-job** [--verbose] [--timing-stats] [--job-repeat ``N``]
-[--op-repeat ``N``] {opcodes_file...}
+[--op-repeat ``N``] [--each] {opcodes_file...}
 
 This command builds a list of opcodes from files in JSON format and
 submits a job per file to the master daemon. It can be used to test
@@ -82,6 +82,9 @@ passing the arguments N times) while op-repeat will cause N 
copies
 of each of the opcodes in the file to be executed (equivalent to
 each file containing N copies of the opcodes).
 
+The ``each`` option allow to submit each job separately (using ``N``
+SubmitJob LUXI requests instead of one SubmitManyJobs request).
+
 TEST-JOBQUEUE
 ~
 
-- 
1.7.3.1

Re: [PATCH master 3/4] Add the remaining DRBD dynamic sync disk params

[Andrea Spadaccini]

Hi Iustin,

>> -    args.append("--create-device")
>> -    result = utils.RunCmd(args)
>> -    if result.failed:
>> -      logging.error("Can't change syncer rate: %s - %s",
>> -                    result.fail_reason, result.output)
>> -    return not result.failed
>> +    if not errs:
>
> (without having the full patch context) maybe it would a better idea to
> return early, instead of of if not errs…

Yes, it's more readable. Fixed.

>> -    return self._SetMinorSyncParams(self.minor, params) and children_result
>> +    return children_result.extend(self._SetMinorSyncParams(self.minor, 
>> params))
>
> This doesn't work, extend() is an in-place operation which doesn't
> return anything.

What a stupid mistake.. Fixed, thanks.

Interdiff:

--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1595,7 +1595,6 @@ class DRBD8(BaseDRBD):

 """

-errs = []
 args = ["drbdsetup", cls._DevPath(minor), "syncer"]
 if params[constants.LDP_DYNAMIC_RESYNC]:
   version = cls._GetVersion(cls._GetProcData())
@@ -1608,14 +1607,14 @@ class DRBD8(BaseDRBD):
 msg = ("The current DRBD version (8.%d.%d) does not support the "
"dynamic resync speed controller" % (vmin, vrel))
 logging.error(msg)
-errs.append(msg)
+return [msg]

   if params[constants.LDP_PLAN_AHEAD] == 0:
 msg = ("A value of 0 for c-plan-ahead disables the dynamic sync speed"
" controller at DRBD level. If you want to disable it, please"
" set the dynamic-resync disk parameter to False.")
 logging.error(msg)
-errs.append(msg)
+return [msg]

   # add the c-* parameters to args
   args.extend(["--c-plan-ahead", params[constants.LDP_PLAN_AHEAD],
@@ -1628,16 +1627,15 @@ class DRBD8(BaseDRBD):
 else:
   args.extend(["-r", "%d" % params[constants.LDP_RESYNC_RATE]])

-if not errs:
-  args.append("--create-device")
-  result = utils.RunCmd(args)
-  if result.failed:
-msg = ("Can't change syncer rate: %s - %s" %
-   (result.fail_reason, result.output))
-logging.error(msg)
-errs.append(msg)
+args.append("--create-device")
+result = utils.RunCmd(args)
+if result.failed:
+  msg = ("Can't change syncer rate: %s - %s" %
+ (result.fail_reason, result.output))
+  logging.error(msg)
+  return msg

-return errs
+return []

   def SetSyncParams(self, params):
 """Set the synchronization parameters of the DRBD syncer.
@@ -1653,8 +1651,10 @@ class DRBD8(BaseDRBD):
   err = "Not attached during SetSyncParams"
   logging.info(err)
   return [err]
+
 children_result = super(DRBD8, self).SetSyncParams(params)
-return children_result.extend(self._SetMinorSyncParams(self.minor, params))
+children_result.extend(self._SetMinorSyncParams(self.minor, params))
+return children_result

Re: [PATCH master 3/4] Add the remaining DRBD dynamic sync disk params

[Andrea Spadaccini]

Hi Iustin,

>> I throw an error every time SetSyncParams or _SetMinorSyncParams is
>> called, and that is equivalent (right now) just to the DRBD sync speed
>> changes. These were not checked before.
>>
>> As for the composition of errors, we can make each function return a
>> list of errors and extend() the list of errors with the ones returned
>> by the children.
>>
>> Will send an interdiff if that sounds good.
>
> Yep, it does.

Interdiff:

diff --git a/lib/bdev.py b/lib/bdev.py
index 59d6cba..030ef40 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -228,12 +228,15 @@ class BlockDev(object):

 @param params: dictionary of LD level disk parameters related to the
 synchronization.
+@rtype: list
+@return: a list of error messages, emitted both by the current node and by
+children. An empty list means no errors.

 """
-result = True
+result = []
 if self._children:
   for child in self._children:
-result = result and child.SetSyncParams(params)
+result.extend(child.SetSyncParams(params))
 return result

   def PauseResumeSync(self, pause):
@@ -1475,8 +1478,10 @@ class DRBD8(BaseDRBD):
 # sync speed only after setting up both sides can race with DRBD
 # connecting, hence we set it here before telling DRBD anything
 # about its peer.
-if not self._SetMinorSyncParams(minor, self.params):
-  _ThrowError("drbd%d: can't set the synchronization parameters")
+sync_errors = self._SetMinorSyncParams(minor, self.params)
+if sync_errors:
+  _ThrowError("drbd%d: can't set the synchronization parameters: %s" %
+  (minor, utils.CommaJoin(sync_errors)))

 if netutils.IP6Address.IsValid(lhost):
   if not netutils.IP6Address.IsValid(rhost):
@@ -1585,11 +1590,12 @@ class DRBD8(BaseDRBD):
 @param minor: the drbd minor whose settings we change
 @type params: dict
 @param params: LD level disk parameters related to the synchronization
-@rtype: boolean
-@return: the success of the operation
+@rtype: list
+@return: a list of error messages

 """

+errs = []
 args = ["drbdsetup", cls._DevPath(minor), "syncer"]
 if params[constants.LDP_DYNAMIC_RESYNC]:
   version = cls._GetVersion(cls._GetProcData())
@@ -1599,16 +1605,17 @@ class DRBD8(BaseDRBD):
   # By definition we are using 8.x, so just check the rest of the version
   # number
   if vmin != 3 or vrel < 9:
-logging.error("The current DRBD version (8.%d.%d) does not support the"
-  " dynamic resync speed controller", vmin, vrel)
-return False
+msg = ("The current DRBD version (8.%d.%d) does not support the "
+   "dynamic resync speed controller" % (vmin, vrel))
+logging.error(msg)
+errs.append(msg)
   if params[constants.LDP_PLAN_AHEAD] == 0:
-logging.error("A value of 0 for c-plan-ahead disables the dynamic sync"
-  " speed controller at DRBD level. If you want to disable"
-  " it, please set the dynamic-resync disk parameter to"
-  " False.")
-return False
+msg = ("A value of 0 for c-plan-ahead disables the dynamic sync speed"
+   " controller at DRBD level. If you want to disable it, please"
+   " set the dynamic-resync disk parameter to False.")
+logging.error(msg)
+errs.append(msg)

   # add the c-* parameters to args
   args.extend(["--c-plan-ahead", params[constants.LDP_PLAN_AHEAD],
@@ -1621,27 +1628,33 @@ class DRBD8(BaseDRBD):
 else:
   args.extend(["-r", "%d" % params[constants.LDP_RESYNC_RATE]])

-args.append("--create-device")
-result = utils.RunCmd(args)
-if result.failed:
-  logging.error("Can't change syncer rate: %s - %s",
-result.fail_reason, result.output)
-return not result.failed
+if not errs:
+  args.append("--create-device")
+  result = utils.RunCmd(args)
+  if result.failed:
+msg = ("Can't change syncer rate: %s - %s" %
+   (result.fail_reason, result.output))
+logging.error(msg)
+errs.append(msg)
+
+return errs

   def SetSyncParams(self, params):
 """Set the synchronization parameters of the DRBD syncer.

 @type params: dict
 @param params: LD level disk parameters related to the synchronization
-@rtype: boolean
-@return: the success of the operation
+@rtype: list
+@return: a list of error messages, emitted both by the current node and by
+children. An empty list means no errors

 """
 if self.minor is None:
-  logging.info("Not attached during SetSyncParams")
-  return False
+  err = "Not attached during SetSyncParams"
+  logging.info(err)
+  return [err]
 children_result = super(DRBD8, self).SetSyncParams(params)
-return self._SetMinorSyncParams(self.minor, params) and children_result
+return children_resu

Re: [PATCH master 4/4] Document DRBD dynamic resync params in man pages

[Andrea Spadaccini]

Hi Iustin,

> Anyway, LGTM, just saying that not-mentioned and unrelated changes are
> not good :)

Right, will add that to the commit message. Thanks :)

Andrea

Re: [PATCH master 3/4] Add the remaining DRBD dynamic sync disk params

[Andrea Spadaccini]

Hi Iustin,

>> Actually, the failure you
>> quoted above can be raised both for c-plan-ahead==0 and for a failure
>> in the execution of drbdsetup itself.
>>
>> I can change the signature by making it return an error object instead
>> of a boolean (or a tuple (err, error_msg)), if you think that's a good
>> idea.
>
> That still doesn't answer the question: how to handle failures in
> recursive calls, or better said how do you compose 'False' return
> values. As I read the code, currently you're not altering at all the 
> SetSyncParams
> calls, just adding the two ThrowError in the cases where DRBD
> explicitly sets the speed? Maybe we want to change the SetSyncParams
> recursive calls too…

I throw an error every time SetSyncParams or _SetMinorSyncParams is
called, and that is equivalent (right now) just to the DRBD sync speed
changes. These were not checked before.

As for the composition of errors, we can make each function return a
list of errors and extend() the list of errors with the ones returned
by the children.

Will send an interdiff if that sounds good.

Thanks,
Andrea

Re: [PATCH master 3/4] Add the remaining DRBD dynamic sync disk params

[Andrea Spadaccini]

Hi Iustin,

>> -    self._SetMinorSyncParams(minor, self.params)
>> +    if not self._SetMinorSyncParams(minor, self.params):
>> +      _ThrowError("drbd%d: can't set the synchronization parameters")
>
> Please don't do this: it makes an opaque error, which is not good.
>>
>>      if netutils.IP6Address.IsValid(lhost):
>>        if not netutils.IP6Address.IsValid(rhost):
>> @@ -1602,9 +1603,20 @@ class DRBD8(BaseDRBD):
>>                        " dynamic resync speed controller", vmin, vrel)
>>          return False
>>
>> +      if params[constants.LDP_PLAN_AHEAD] == 0:
>> +        logging.error("A value of 0 for c-plan-ahead disables the dynamic 
>> sync"
>> +                      " speed controller at DRBD level. If you want to 
>> disable"
>> +                      " it, please set the dynamic-resync disk parameter to"
>> +                      " False.")
>> +        return False
>
> And instead to a ThrowError here, with that message.

I didn't throw an exception here because my understanding is that this
method could be called as part of a recursive function, and throwing
an exception would break the recursion loop. Actually, the failure you
quoted above can be raised both for c-plan-ahead==0 and for a failure
in the execution of drbdsetup itself.

I can change the signature by making it return an error object instead
of a boolean (or a tuple (err, error_msg)), if you think that's a good
idea.

Thanks,
Andrea

[PATCH master 0/4] DRBD dynamic sync speed parameter

[Andrea Spadaccini]

This patch series implements the remaining disk parameters listed in the new
resource model design doc, and also adds a new parameter that is used to trigger
the DRBD dynamic sync speed algorithm.

Andrea Spadaccini (4):
  Describe the dynamic-resync par. in the design doc
  Add the dynamic-resync DRBD disk parameter
  Add the remaining DRBD dynamic sync disk params
  Document DRBD dynamic resync params in man pages

 doc/design-resource-model.rst |   11 ++
 lib/bdev.py   |   77 +---
 lib/cmdlib.py |6 +++
 lib/constants.py  |   68 +++-
 man/gnt-cluster.rst   |   52 +--
 man/gnt-group.rst |4 +-
 6 files changed, 174 insertions(+), 44 deletions(-)

-- 
1.7.3.1

[PATCH master 2/4] Add the dynamic-resync DRBD disk parameter

[Andrea Spadaccini]

constants.py, cmdlib.py:
- add the dynamic-resync parameter, both at DT and LD levels;

lib/bdev.py:
- change SetSyncSpeed to SetSyncParams, and _SetMinorSyncSpeed to
  _SetMinorSyncParams;
- use the dynamic-resync parameter.

Later patches will implement the rest of the parameters.

Signed-off-by: Andrea Spadaccini 
---
 lib/bdev.py  |   64 -
 lib/cmdlib.py|1 +
 lib/constants.py |9 ++-
 3 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index 3f9441d..030d195 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -221,16 +221,19 @@ class BlockDev(object):
 """
 raise NotImplementedError
 
-  def SetSyncSpeed(self, speed):
-"""Adjust the sync speed of the mirror.
+  def SetSyncParams(self, params):
+"""Adjust the synchronization parameters of the mirror.
 
 In case this is not a mirroring device, this is no-op.
 
+@param params: dictionary of LD level disk parameters related to the
+synchronization.
+
 """
 result = True
 if self._children:
   for child in self._children:
-result = result and child.SetSyncSpeed(speed)
+result = result and child.SetSyncParams(params)
 return result
 
   def PauseResumeSync(self, pause):
@@ -238,7 +241,7 @@ class BlockDev(object):
 
 In case this is not a mirroring device, this is no-op.
 
-@param pause: Wheater to pause or resume
+@param pause: Whether to pause or resume
 
 """
 result = True
@@ -1472,8 +1475,7 @@ class DRBD8(BaseDRBD):
 # sync speed only after setting up both sides can race with DRBD
 # connecting, hence we set it here before telling DRBD anything
 # about its peer.
-sync_speed = self.params.get(constants.LDP_RESYNC_RATE)
-self._SetMinorSyncSpeed(minor, sync_speed)
+self._SetMinorSyncParams(minor, self.params)
 
 if netutils.IP6Address.IsValid(lhost):
   if not netutils.IP6Address.IsValid(rhost):
@@ -1573,40 +1575,61 @@ class DRBD8(BaseDRBD):
 self._children = []
 
   @classmethod
-  def _SetMinorSyncSpeed(cls, minor, kbytes):
-"""Set the speed of the DRBD syncer.
+  def _SetMinorSyncParams(cls, minor, params):
+"""Set the parameters of the DRBD syncer.
 
 This is the low-level implementation.
 
 @type minor: int
 @param minor: the drbd minor whose settings we change
-@type kbytes: int
-@param kbytes: the speed in kbytes/second
+@type params: dict
+@param params: LD level disk parameters related to the synchronization
 @rtype: boolean
 @return: the success of the operation
 
 """
-result = utils.RunCmd(["drbdsetup", cls._DevPath(minor), "syncer",
-   "-r", "%d" % kbytes, "--create-device"])
+
+args = ["drbdsetup", cls._DevPath(minor), "syncer"]
+if params[constants.LDP_DYNAMIC_RESYNC]:
+  version = cls._GetVersion(cls._GetProcData())
+  vmin = version["k_minor"]
+  vrel = version["k_point"]
+
+  # By definition we are using 8.x, so just check the rest of the version
+  # number
+  if vmin != 3 or vrel < 9:
+logging.error("The current DRBD version (8.%d.%d) does not support the"
+  " dynamic resync speed controller", vmin, vrel)
+return False
+
+  # add the c-* parameters to args
+  # TODO(spadaccio) use the actual parameters
+  args.extend(["--c-plan-ahead", "20"])
+
+else:
+  args.extend(["-r", "%d" % params[constants.LDP_RESYNC_RATE]])
+
+args.append("--create-device")
+result = utils.RunCmd(args)
 if result.failed:
   logging.error("Can't change syncer rate: %s - %s",
 result.fail_reason, result.output)
 return not result.failed
 
-  def SetSyncSpeed(self, kbytes):
-"""Set the speed of the DRBD syncer.
+  def SetSyncParams(self, params):
+"""Set the synchronization parameters of the DRBD syncer.
 
-@type kbytes: int
-@param kbytes: the speed in kbytes/second
+@type params: dict
+@param params: LD level disk parameters related to the synchronization
 @rtype: boolean
 @return: the success of the operation
 
 """
 if self.minor is None:
-  logging.info("Not attached during SetSyncSpeed")
+  logging.info("Not attached during SetSyncParams")
   return False
-children_result = super(DRBD8, self).SetSyncSpeed(kbytes)
-return self._SetMinorSyncSpeed(self.minor, kbytes) and children_result
+children_result = super(DRBD8, self).SetSyncParams(params)
+return self._SetMinorSyncParams(self.minor, params) and 

[PATCH master 4/4] Document DRBD dynamic resync params in man pages

[Andrea Spadaccini]

Also, remove some spurious spaces in the documentation of other options.

Signed-off-by: Andrea Spadaccini 
---
 man/gnt-cluster.rst |   52 +++---
 man/gnt-group.rst   |4 +-
 2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/man/gnt-cluster.rst b/man/gnt-cluster.rst
index 7293f3e..53fa5eb 100644
--- a/man/gnt-cluster.rst
+++ b/man/gnt-cluster.rst
@@ -175,9 +175,9 @@ INIT
 | [--file-storage-dir *dir*]
 | [--enabled-hypervisors *hypervisors*]
 | [{-H|--hypervisor-parameters} 
*hypervisor*:*hv-param*=*value*[,*hv-param*=*value*...]]
-| [{-B|--backend-parameters} *be-param*=*value* [,*be-param*=*value*...]]
-| [{-N|--nic-parameters} *nic-param*=*value* [,*nic-param*=*value*...]]
-| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param*=*value*...]]
+| [{-B|--backend-parameters} *be-param*=*value*[,*be-param*=*value*...]]
+| [{-N|--nic-parameters} *nic-param*=*value*[,*nic-param*=*value*...]]
+| [{-D|--disk-parameters} 
*disk-template*:*disk-param*=*value*[,*disk-param*=*value*...]]
 | [--maintain-node-health {yes \| no}]
 | [--uid-pool *user-id pool definition*]
 | [{-I|--default-iallocator} *default instance allocator*]
@@ -366,22 +366,24 @@ cluster and node group level; the cluster-level parameter 
are inherited
 by the node group at the moment of its creation, and can be further
 modified at node group level using the **gnt-group**(8) command. 
 
-List of disk parameters available for the **drbd** template:
+The following is the list of disk parameters available for the **drbd**
+template, with measurement units specified in square brackets at the end
+of the description (when applicable):
 
 resync-rate
-Re-synchronization rate, expressed in KiB/s
+Static re-synchronization rate. [KiB/s]
 
 data-stripes
-Number of stripes to use for data LVs
+Number of stripes to use for data LVs.
 
 meta-stripes
-Number of stripes to use for meta LVs
+Number of stripes to use for meta LVs.
 
 disk-barriers
 What kind of barriers to **disable** for disks. It can either assume
 the value "n", meaning no barrier disabled, or a non-empty string
 containing a subset of the characters "bfd". "b" means disable disk
-barriers, "f" means disable disk flushes, "d" disables disk drains
+barriers, "f" means disable disk flushes, "d" disables disk drains.
 
 meta-barriers
 Boolean value indicating whether the meta barriers should be
@@ -401,10 +403,36 @@ net-custom
 String containing additional parameters to be appended to the
 arguments list of ``drbdsetup net``.
 
+dynamic-resync
+Boolean indicating whether to use the dynamic resync speed
+controller or not. If enabled, c-plan-ahead must be non-zero and all
+the c-* parameters will be used by DRBD. Otherwise, the value of
+resync-rate will be used as a static resync speed.
+
+c-plan-ahead
+Agility factor of the dynamic resync speed controller. (the higher,
+the slower the algorithm will adapt the resync speed). A value of 0
+(that is the default) disables the controller. [ds]
+
+c-fill-target
+Maximum amount of in-flight resync data for the dynamic resync speed
+controller. [sectors]
+
+c-delay-target
+Maximum estimated peer response latency for the dynamic resync speed
+controller. [ds]
+
+c-min-rate
+Minimum resync speed for the dynamic resync speed controller. [KiB/s]
+
+c-max-rate
+Upper bound on resync speed for the dynamic resync speed controller.
+[KiB/s]
+
 List of parameters available for the **plain** template:
 
 stripes
-Number of stripes to use for new LVs
+Number of stripes to use for new LVs.
 
 The option ``--maintain-node-health`` allows one to enable/disable
 automatic maintenance actions on nodes. Currently these include
@@ -493,9 +521,9 @@ MODIFY
 | [--no-lvm-storage]
 | [--enabled-hypervisors *hypervisors*]
 | [{-H|--hypervisor-parameters} 
*hypervisor*:*hv-param*=*value*[,*hv-param*=*value*...]]
-| [{-B|--backend-parameters} *be-param*=*value* [,*be-param*=*value*...]]
-| [{-N|--nic-parameters} *nic-param*=*value* [,*nic-param*=*value*...]]
-| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param*=*value*...]]
+| [{-B|--backend-parameters} *be-param*=*value*[,*be-param*=*value*...]]
+| [{-N|--nic-parameters} *nic-param*=*value*[,*nic-param*=*value*...]]
+| [{-D|--disk-parameters} 
*disk-template*:*disk-param*=*value*[,*disk-param*=*value*...]]
 | [--uid-pool *user-id pool definition*]
 | [--add-uids *user-id pool definition*]
 | [--remove-uids *user-id pool definition*]
diff --git a/man/gnt-group.rst b/man/gnt-group.rst
index 4980cf9..6451ced 100644
--- a/man/gnt-group.rst
+++ b/man/gnt-group.rst
@@ -26,7 +26,7 @@ ADD
 | **add**
 | [--node-parameters=*NDPARAMS*]
 | [--alloc-policy=*POLICY*]
-| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param

[PATCH master 1/4] Describe the dynamic-resync par. in the design doc

[Andrea Spadaccini]

Signed-off-by: Andrea Spadaccini 
---
 doc/design-resource-model.rst |   11 +++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/doc/design-resource-model.rst b/doc/design-resource-model.rst
index 543da3c..ceec8f1 100644
--- a/doc/design-resource-model.rst
+++ b/doc/design-resource-model.rst
@@ -741,6 +741,17 @@ brackets.
 ||  |for drbd, when using the |constants.py, not|  
|
 ||  |static syncer, in KiB/s  |changeable via Ganeti|  
|
 
++--+-+-+--+
+|drbd|dynamic-resync|Whether to use the   |Not supported.   |bool  
|
+||  |dynamic resync speed | |  
|
+||  |controller or not. If| |  
|
+||  |enabled, c-plan-ahead| |  
|
+||  |must be non-zero and all | |  
|
+||  |the c-* parameters will  | |  
|
+||  |be used by DRBD. | |  
|
+||  |Otherwise, the value of  | |  
|
+||  |resync-rate will be used | |  
|
+||  |as a static resync speed.| |  
|
+++--+-+-+--+
 |drbd|c-plan-ahead  |Agility factor of the|Not supported.   |int   
|
 ||  |dynamic resync speed | |  
|
 ||  |controller. (the higher, | |  
|
-- 
1.7.3.1

[PATCH master 3/4] Add the remaining DRBD dynamic sync disk params

[Andrea Spadaccini]

Add the c-plan-ahead, c-fill-target, c-delay-target, c-max-rate,
c-min-rate parameters; don't ignore errors while setting the
synchronization speed.

Signed-off-by: Andrea Spadaccini 
---
 lib/bdev.py  |   21 +++---
 lib/cmdlib.py|5 
 lib/constants.py |   61 ++
 3 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index 030d195..59d6cba 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1475,7 +1475,8 @@ class DRBD8(BaseDRBD):
 # sync speed only after setting up both sides can race with DRBD
 # connecting, hence we set it here before telling DRBD anything
 # about its peer.
-self._SetMinorSyncParams(minor, self.params)
+if not self._SetMinorSyncParams(minor, self.params):
+  _ThrowError("drbd%d: can't set the synchronization parameters")
 
 if netutils.IP6Address.IsValid(lhost):
   if not netutils.IP6Address.IsValid(rhost):
@@ -1602,9 +1603,20 @@ class DRBD8(BaseDRBD):
   " dynamic resync speed controller", vmin, vrel)
 return False
 
+  if params[constants.LDP_PLAN_AHEAD] == 0:
+logging.error("A value of 0 for c-plan-ahead disables the dynamic sync"
+  " speed controller at DRBD level. If you want to disable"
+  " it, please set the dynamic-resync disk parameter to"
+  " False.")
+return False
+
   # add the c-* parameters to args
-  # TODO(spadaccio) use the actual parameters
-  args.extend(["--c-plan-ahead", "20"])
+  args.extend(["--c-plan-ahead", params[constants.LDP_PLAN_AHEAD],
+   "--c-fill-target", params[constants.LDP_FILL_TARGET],
+   "--c-delay-target", params[constants.LDP_DELAY_TARGET],
+   "--c-max-rate", params[constants.LDP_MAX_RATE],
+   "--c-min-rate", params[constants.LDP_MIN_RATE],
+  ])
 
 else:
   args.extend(["-r", "%d" % params[constants.LDP_RESYNC_RATE]])
@@ -1866,7 +1878,8 @@ class DRBD8(BaseDRBD):
   # the device
   self._SlowAssemble()
 
-self.SetSyncParams(self.params)
+if not self.SetSyncParams(self.params):
+  _ThrowError("drbd%d: can't set the synchronization parameters")
 
   def _SlowAssemble(self):
 """Assembles the DRBD device from a (partially) configured device.
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 95a61bf..49873f6 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8176,6 +8176,11 @@ def _ComputeLDParams(disk_template, disk_params):
   constants.LDP_DISK_CUSTOM: dt_params[constants.DRBD_DISK_CUSTOM],
   constants.LDP_NET_CUSTOM: dt_params[constants.DRBD_NET_CUSTOM],
   constants.LDP_DYNAMIC_RESYNC: dt_params[constants.DRBD_DYNAMIC_RESYNC],
+  constants.LDP_PLAN_AHEAD: dt_params[constants.DRBD_PLAN_AHEAD],
+  constants.LDP_FILL_TARGET: dt_params[constants.DRBD_FILL_TARGET],
+  constants.LDP_DELAY_TARGET: dt_params[constants.DRBD_DELAY_TARGET],
+  constants.LDP_MAX_RATE: dt_params[constants.DRBD_MAX_RATE],
+  constants.LDP_MIN_RATE: dt_params[constants.DRBD_MIN_RATE],
   }
 
 drbd_params = \
diff --git a/lib/constants.py b/lib/constants.py
index 9166976..83a7030 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -939,6 +939,11 @@ LDP_DEFAULT_METAVG = "default-metavg"
 LDP_DISK_CUSTOM = "disk-custom"
 LDP_NET_CUSTOM = "net-custom"
 LDP_DYNAMIC_RESYNC = "dynamic-resync"
+LDP_PLAN_AHEAD = "c-plan-ahead"
+LDP_FILL_TARGET = "c-fill-target"
+LDP_DELAY_TARGET = "c-delay-target"
+LDP_MAX_RATE = "c-max-rate"
+LDP_MIN_RATE = "c-min-rate"
 DISK_LD_TYPES = {
   LDP_RESYNC_RATE: VTYPE_INT,
   LDP_STRIPES: VTYPE_INT,
@@ -948,6 +953,11 @@ DISK_LD_TYPES = {
   LDP_DISK_CUSTOM: VTYPE_STRING,
   LDP_NET_CUSTOM: VTYPE_STRING,
   LDP_DYNAMIC_RESYNC: VTYPE_BOOL,
+  LDP_PLAN_AHEAD: VTYPE_INT,
+  LDP_FILL_TARGET: VTYPE_INT,
+  LDP_DELAY_TARGET: VTYPE_INT,
+  LDP_MAX_RATE: VTYPE_INT,
+  LDP_MIN_RATE: VTYPE_INT,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())
 
@@ -962,6 +972,11 @@ DRBD_DEFAULT_METAVG = "metavg"
 DRBD_DISK_CUSTOM = "disk-custom"
 DRBD_NET_CUSTOM = "net-custom"
 DRBD_DYNAMIC_RESYNC = "dynamic-resync"
+DRBD_PLAN_AHEAD = "c-plan-ahead"
+DRBD_FILL_TARGET = "c-fill-target"
+DRBD_DELAY_TARGET = "c-delay-target"
+DRBD_MAX_RATE = "c-max-rate"
+DRBD_MIN_RATE = "c-min-rate"
 LV_STRIPES = "stripes"
 DISK_DT_TYPES = {
   DRBD_RESYNC_RATE: VTYPE_INT,
@@ -973,6 +988,11 @@ DISK_DT_TYPES = {
   DRBD_DISK_CUSTOM: VTYPE_STRING,
   DRBD_NET_CUSTOM: VTYPE_STRING,
   DRBD_DYNAMIC_RE

Re: [PATCH master 2/3] Man page for gnt-cluster

[Andrea Spadaccini]

Hi Iustin,

>> >> > +| [--specs-disk-count *spec-param*=*value* [,*spec-param*=*value*...]]
>> >>
>> >> Will it work if you put a space between spec-params?
>> >
>> > Like in all our commands, not :) But the markup format requires it :(
>>
>> I was *so* sure that I did not put those spaces while documenting the
>> disk parameters options.. :/
>
> Does it work (the markup) correctly? If so, then yes, we should update
> the man pages to be more correct.

Ok, so: I expressed myself incorrectly, as I was sure I did it but in
fact I didn't (and that what I was trying to communicate with the
phrase you quoted).

But in the end I tried it and indeed removing the space produces the
expected output.

I will fix gnt-group and gnt-cluster in the patches on which I'm working now.

Andrea

Re: [PATCH master 2/3] Man page for gnt-cluster

[Andrea Spadaccini]

Hi Iustin,

>> […]
>> > +| [--specs-disk-count *spec-param*=*value* [,*spec-param*=*value*...]]
>>
>> Will it work if you put a space between spec-params?
>
> Like in all our commands, not :) But the markup format requires it :(

I was *so* sure that I did not put those spaces while documenting the
disk parameters options.. :/

Re: [PATCH master 2/3] Man page for gnt-cluster

[Andrea Spadaccini]

Hi Agata,

[…]
> +| [--specs-disk-count *spec-param*=*value* [,*spec-param*=*value*...]]

Will it work if you put a space between spec-params?

Andrea

Re: [PATCH master] Restore diskparams in the gnt-group options check

[Andrea Spadaccini]

Hi René,

>> I would just put it into the parentheses and OR it. But either way is
>> fine with me.
>
> This is a LGTM btw. :-P

Thanks ;)

Re: [PATCH master 3/4] Add net-custom and disk-custom DRBD parameters

[Andrea Spadaccini]

Hi Michael,

On Thu, Dec 8, 2011 at 10:50 AM, Michael Hanselmann  wrote:
> Am 7. Dezember 2011 22:13 schrieb Andrea Spadaccini :
>> --- a/lib/bdev.py
>> +++ b/lib/bdev.py
>> @@ -1373,7 +1373,7 @@ class DRBD8(BaseDRBD):
>>     args.extend(barrier_args)
>>
>>     if self.params[constants.LDP_DISK_CUSTOM]:
>> -      args.append(self.params[constants.LDP_DISK_CUSTOM])
>> +      args.extend(self.params[constants.LDP_DISK_CUSTOM].split())
>
> Ugly and potentially dangerous. There are no checks ensuring DRBD
> won't have dangerous parameters or some requiring escaping in the
> future. Why don't you use the shlex.split function? Is there really no
> other way to pass these parameters (e.g.
> custom/0=-a,custom/1=123,custom/2=-b… using the FlatToDict function
> René wrote)?

I think the latter is an unnecessary complication, given that now we
already give much more room for the customization of DRBD behaviour.

shlex.split is a good suggestion, thanks. Here is the interdiff:
diff --git a/lib/bdev.py b/lib/bdev.py
index 0ac5b0c..3f9441d 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -24,6 +24,7 @@
 import re
 import time
 import errno
+import shlex
 import stat
 import pyparsing as pyp
 import os
@@ -1373,7 +1374,7 @@ class DRBD8(BaseDRBD):
 args.extend(barrier_args)

 if self.params[constants.LDP_DISK_CUSTOM]:
-  args.extend(self.params[constants.LDP_DISK_CUSTOM].split())
+  args.extend(shlex.split(self.params[constants.LDP_DISK_CUSTOM]))

 result = utils.RunCmd(args)
 if result.failed:
@@ -1500,7 +1501,7 @@ class DRBD8(BaseDRBD):
   args.extend(["-a", hmac, "-x", secret])

 if self.params[constants.LDP_NET_CUSTOM]:
-  args.extend(self.params[constants.LDP_NET_CUSTOM].split())
+  args.extend(shlex.split(self.params[constants.LDP_NET_CUSTOM]))

 result = utils.RunCmd(args)
 if result.failed:

[PATCH master] Restore diskparams in the gnt-group options check

[Andrea Spadaccini]

Commit a82823 accidentally removed opts.diskparameters from the list of
parameters that are checked for presence in gnt-group, thus causing
invocations of gnt-group modify with only disk paramaters to fail.

This commit restores the check and also simplifies the whole chain of
checks by using only conjunctions of negations (as opposed to using both
congjunctions of negations and negations of disjunctions).

Signed-off-by: Andrea Spadaccini 
---
 lib/client/gnt_group.py |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/client/gnt_group.py b/lib/client/gnt_group.py
index 055c28e..c38192f 100644
--- a/lib/client/gnt_group.py
+++ b/lib/client/gnt_group.py
@@ -135,8 +135,8 @@ def SetGroupParams(opts, args):
   @return: the desired exit code
 
   """
-  if (opts.ndparams is None and opts.alloc_policy is None and
-  not (opts.hv_state or opts.disk_state)):
+  if (opts.ndparams is None and opts.alloc_policy is None
+  and not opts.diskparams and not opts.hv_state and not opts.disk_state):
 ToStderr("Please give at least one of the parameters.")
 return 1
 
-- 
1.7.3.1

Re: [PATCH master 3/4] Add net-custom and disk-custom DRBD parameters

[Andrea Spadaccini]

Hi Iustin,

> This:
>
>> +    if self.params[constants.LDP_DISK_CUSTOM]:
>> +      args.append(self.params[constants.LDP_DISK_CUSTOM])
>
> and this:
>
>> +    if self.params[constants.LDP_NET_CUSTOM]:
>> +      args.append(self.params[constants.LDP_NET_CUSTOM])
>> +
>>      result = utils.RunCmd(args)
>
> won't work if the values contain more than a single item, because we
> don't execute the command via the shell (we just pass args).
>
> I'm not sure what we should do, because sometimes you do want to pass
> multiple parameters.
>
> Maybe simply args.extend(self.params[…].split()), and thus disallowing
> any quoting (" "/' ') and escaping in the parameters, but still allowing
> "-a 5 -b 18" constructs?

I think this is the way to go. I looked at the drbdsetup man page and
did not see any parameters for disk and net that might require
escaping.

Thanks,
Andrea.

Interdiff:
diff --git a/lib/bdev.py b/lib/bdev.py
index 7ec1204..0ac5b0c 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1373,7 +1373,7 @@ class DRBD8(BaseDRBD):
 args.extend(barrier_args)

 if self.params[constants.LDP_DISK_CUSTOM]:
-  args.append(self.params[constants.LDP_DISK_CUSTOM])
+  args.extend(self.params[constants.LDP_DISK_CUSTOM].split())

 result = utils.RunCmd(args)
 if result.failed:
@@ -1500,7 +1500,7 @@ class DRBD8(BaseDRBD):
   args.extend(["-a", hmac, "-x", secret])

 if self.params[constants.LDP_NET_CUSTOM]:
-  args.append(self.params[constants.LDP_NET_CUSTOM])
+  args.extend(self.params[constants.LDP_NET_CUSTOM].split())

 result = utils.RunCmd(args)
 if result.failed:

Re: [PATCH master] Add DRBD dynamic resync speed params to design doc

[Andrea Spadaccini]

Hi Iustin,

>> Actually, it doesn't really matter for disk at least - we should just
>> read using normal M/G/T suffix processing (as we do in the rest of
>> ganeti), whereas for seconds we don't have a 'd/m' suffix, so let's keep
>> deciseconds.
>
> Argh, or not. We can use just plain M/G/T, not MB/s, GB/s, etc. Let's
> just go with the drbd-native units, and call it done.

Ack. Thanks :)

Andrea

Re: [PATCH master] Add DRBD dynamic resync speed params to design doc

[Andrea Spadaccini]

Hi Iustin,

>> * Add the c-* DRBDparameters to the table containing the disk parameters
> LGTM,

Small question, unrelated to the units.

Should we add another parameter that explicitly enables the DRBD
dynamic sync speed algorithm?

In DRBD, you enable it by setting c-plan-ahead to a non-zero value. We
might keep it in this way or add another parameter
(use-dynamic-resync-controller) that enables it. In this way it is a
bit more complex but also more explicit: no confusion about
resync-speed being 100 MiB/s and actual speed being lower.

What do you think?

Thanks,
Andrea

Re: [PATCH master 1/3] manpages: Fix small errors in documentation

[Andrea Spadaccini]

Hi Bernardo,

> -it already can determine that a migration wont work (i.e. if the
> -instance is shutdown). Please note that the fallback will not happen
> +it already can determine that a migration won't work (i.e. if the
> +instance is shut down). Please note that the fallback will not happen

You might want to add the comma after i.e. here, too.

Andrea

Re: [PATCH master] Add DRBD dynamic resync speed params to design doc

[Andrea Spadaccini]

Hi Iustin,

>> > LGTM, except for the units; all the disk-related units in Ganeti are in
>> > MiB, so the new ones should also be in MiB or MiB/s.
>>
>> In this way we have parameters with the same name of the equivalent
>> DRBD ones, but with different measurement units: this will lead to
>> confusion and subtle errors during configuration IMO.
>>
>> Do you still want to change it to MiB and MiB/s? If yes, I think it's
>> also worth changing the ds to s.
>
> Depends. What is the smallest meaningful unit? For disk sizes/speeds,
> it's MiB - you don't get much more precise by using KiB.
>
> However, for time units, it might be that precision on the hundred
> millisecond scale (1ds) makes sense. But as you say, why not simply use
> milliseconds which are widely used?

You are right, the most meaningful units are MiB (/s) and ms. But I
still think that we should keep the original ones to avoid
configuration errors for the reasons I expressed before: avoid
configuration errors due to parameters with the same name and
mismatching units.

Thanks,
Andrea

Re: [PATCH master] Add DRBD dynamic resync speed params to design doc

[Andrea Spadaccini]

Hi Iustin,

> LGTM, except for the units; all the disk-related units in Ganeti are in
> MiB, so the new ones should also be in MiB or MiB/s.

In this way we have parameters with the same name of the equivalent
DRBD ones, but with different measurement units: this will lead to
confusion and subtle errors during configuration IMO.

Do you still want to change it to MiB and MiB/s? If yes, I think it's
also worth changing the ds to s.

Thanks,
Andrea

[PATCH master] Add DRBD dynamic resync speed params to design doc

[Andrea Spadaccini]

* Expand the Name column of the table (for c-delay-target)
* Add the c-* DRBDparameters to the table containing the disk parameters
* Add the unit of measurement in square brackets, when needed
* Document the supported DRBD version, warn about the DRBD version
  needed for barriers and for the dynamic resync speed parameters.
* Add links to some documentation about the dynamic resync speed
  parameters

Signed-off-by: Andrea Spadaccini 
---
 doc/design-resource-model.rst |  151 +++--
 1 files changed, 100 insertions(+), 51 deletions(-)

diff --git a/doc/design-resource-model.rst b/doc/design-resource-model.rst
index 212fdc7..543da3c 100644
--- a/doc/design-resource-model.rst
+++ b/doc/design-resource-model.rst
@@ -702,57 +702,106 @@ DRBD setups is added to Ganeti.
 At JSON level, since the object key has to be a string, the keys can be
 encoded via a separator (e.g. slash), or by having two dict levels.
 
-++-+-+-+--+
-|Disk|Name |Description  |Current status   |Type  |
-|template| | | |  |
-++=+=+=+==+
-|plain   |stripes  |How many stripes to use  |Configured at|int   |
-|| |for newly created (plain)|./configure time, not|  |
-|| |logical voumes   |overridable at   |  |
-|| | |runtime  |  |
-++-+-+-+--+
-|drbd|data-stripes |How many stripes to use  |Same as for  |int   |
-|| |for data volumes |plain/stripes|  |
-++-+-+-+--+
-|drbd|metavg   |Default volume group for |Same as the main |string|
-|| |the metadata LVs |volume group,|  |
-|| | |overridable via  |  |
-|| | |'metavg' key |  |
-++-+-+-+--+
-|drbd|meta-stripes |How many stripes to use  |Same as for lvm  |int   |
-|| |for meta volumes |'stripes', suboptimal|  |
-|| | |as the meta LVs are  |  |
-|| | |small|  |
-++-+-+-+--+
-|drbd|disk-barriers|What kind of barriers to |Either all enabled or|string|
-|| |*disable* for disks; |all disabled, per|  |
-|| |either "n" or a string   |./configure time |  |
-|| |containing a subset of   |option   |  |
-|| |"bfd"| |  |
-++-+-+-+--+
-|drbd|meta-barriers|Whether to disable or not|Handled together with|bool  |
-|| |the barriers for the meta|disk-barriers|  |
-|| |volume   | |  |
-++-+-+-+--+
-|drbd|resync-rate  |The (static) resync rate |Hardcoded in |int   |
-|| |for drbd, when using the |constants.py, not|  |
-|| |static syncer, in MiB/s  |changeable via Ganeti|  |
-++-+-+-+--+
-|drbd|disk-custom  |Free-form string that|Not supported|string|
-|| |will be appended to the  | |  |
-|| |drbdsetup disk command   | |  |
-|| |line, for custom options | |  |
-|| |not supported by Ganeti  | |  |
-|| |itself   | |  |
-++-+-+-+--+
-|drbd|net-custom   |Free-form string for |Not supported|string|
-|| |custom net setup options | |  |
-++-+-+-+--+
-
-Note that the DRBD parameters might change once Ganeti supports DRBD 8.4, in
-which the :command:`drbdsetup` syntax has changed significantly.
-Moreover, new parameters for the dynamic synchronization algorithm will
-be added for DRBD versions >= 8.3.9.
+When needed, 

[PATCH master 4/4] Add documentation for disk parameters to man pages

[Andrea Spadaccini]

Document the stripes parameter for the plain template and the
resync-rate, datastripes, metastripes, disk-barriers, meta-barriers,
metavg, disk-custom and meta-custom parameters for the drbd template.

Signed-off-by: Andrea Spadaccini 
---
 man/gnt-cluster.rst |   62 ++
 man/gnt-group.rst   |   12 -
 2 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/man/gnt-cluster.rst b/man/gnt-cluster.rst
index 68a40ba..2cb57cc 100644
--- a/man/gnt-cluster.rst
+++ b/man/gnt-cluster.rst
@@ -177,6 +177,7 @@ INIT
 | [{-H|--hypervisor-parameters} 
*hypervisor*:*hv-param*=*value*[,*hv-param*=*value*...]]
 | [{-B|--backend-parameters} *be-param*=*value* [,*be-param*=*value*...]]
 | [{-N|--nic-parameters} *nic-param*=*value* [,*nic-param*=*value*...]]
+| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param*=*value*...]]
 | [--maintain-node-health {yes \| no}]
 | [--uid-pool *user-id pool definition*]
 | [{-I|--default-iallocator} *default instance allocator*]
@@ -352,6 +353,55 @@ link
 network script it is interpreted as a routing table number or
 name.
 
+The ``-D (--disk-parameters)`` option allows you to set the default disk
+template parameters at cluster level. The format used for this option is
+similar to the one use by the  ``-H`` option: the disk template name
+must be specified first, followed by a colon and by a comma-separated
+list of key-value pairs. These parameters can only be specified at
+cluster and node group level; the cluster-level parameter are inherited
+by the node group at the moment of its creation, and can be further
+modified at node group level using the **gnt-group**(8) command. 
+
+List of disk parameters available for the **drbd** template:
+
+resync-rate
+Re-synchronization rate, expressed in KiB/s
+
+data-stripes
+Number of stripes to use for data LVs
+
+meta-stripes
+Number of stripes to use for meta LVs
+
+disk-barriers
+What kind of barriers to **disable** for disks. It can either assume
+the value "n", meaning no barrier disabled, or a non-empty string
+containing a subset of the characters "bfd". "b" means disable disk
+barriers, "f" means disable disk flushes, "d" disables disk drains
+
+meta-barriers
+Boolean value indicating whether the meta barriers should be
+disabled (True) or not (False).
+
+metavg
+String containing the name of the default LVM volume group for DRBD
+metadata. By default, it is set to ``xenvg``. It can be overridden
+during the instance creation process by using the ``metavg`` key of
+the ``--disk`` parameter.
+
+disk-custom
+String containing additional parameters to be appended to the
+arguments list of ``drbdsetup disk``.
+
+net-custom
+String containing additional parameters to be appended to the
+arguments list of ``drbdsetup net``.
+
+List of parameters available for the **plain** template:
+
+stripes
+Number of stripes to use for new LVs
+
 The option ``--maintain-node-health`` allows one to enable/disable
 automatic maintenance actions on nodes. Currently these include
 automatic shutdown of instances and deactivation of DRBD devices on
@@ -441,6 +491,7 @@ MODIFY
 | [{-H|--hypervisor-parameters} 
*hypervisor*:*hv-param*=*value*[,*hv-param*=*value*...]]
 | [{-B|--backend-parameters} *be-param*=*value* [,*be-param*=*value*...]]
 | [{-N|--nic-parameters} *nic-param*=*value* [,*nic-param*=*value*...]]
+| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param*=*value*...]]
 | [--uid-pool *user-id pool definition*]
 | [--add-uids *user-id pool definition*]
 | [--remove-uids *user-id pool definition*]
@@ -458,11 +509,12 @@ Modify the options for the cluster.
 
 The ``--vg-name``, ``--no-lvm-storarge``, ``--enabled-hypervisors``,
 ``-H (--hypervisor-parameters)``, ``-B (--backend-parameters)``,
-``--nic-parameters``, ``-C (--candidate-pool-size)``,
-``--maintain-node-health``, ``--prealloc-wipe-disks``, ``--uid-pool``,
-``--node-parameters``, ``--master-netdev``, ``--master-netmask`` and
-``--use-external-mip-script`` options are described in the
-**init** command.
+``-D (--disk-parameters)``, ``--nic-parameters``, ``-C
+(--candidate-pool-size)``, ``--maintain-node-health``,
+``--prealloc-wipe-disks``, ``--uid-pool``, ``--node-parameters``,
+``--master-netdev``, ``--master-netmask`` and
+``--use-external-mip-script`` options are described in the **init**
+command.
 
 The ``--add-uids`` and ``--remove-uids`` options can be used to
 modify the user-id pool by adding/removing a list of user-ids or
diff --git a/man/gnt-group.rst b/man/gnt-group.rst
index 51a2fe5..523182d 100644
--- a/man/gnt-group.rst
+++ b/man/gnt-group.rst
@@ -26,6 +26,7 @@ ADD
 | **add**
 | [--node-parameters=*NDPARAMS*]
 | [--alloc-policy=*POLICY*]
+| [{-D|--disk-parameters} *disk-template*:*disk-param*=*value* 
[,*disk-param*=*value*...]]
 | {*group*}
 

[PATCH master 2/4] Add the metavg DRBD disk parameter

[Andrea Spadaccini]

This parameter represents the default metadata volume group for DRBD
disks. It can be overridden at instance creation time using the metavg
instance disk parameter.

Signed-off-by: Andrea Spadaccini 
---
 lib/cmdlib.py|7 +--
 lib/constants.py |6 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 9b711cd..af494ce 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8061,6 +8061,7 @@ def _ComputeLDParams(disk_template, disk_params):
   constants.LDP_RESYNC_RATE: dt_params[constants.DRBD_RESYNC_RATE],
   constants.LDP_BARRIERS: dt_params[constants.DRBD_DISK_BARRIERS],
   constants.LDP_NO_META_FLUSH: dt_params[constants.DRBD_META_BARRIERS],
+  constants.LDP_DEFAULT_METAVG: dt_params[constants.DRBD_DEFAULT_METAVG],
   }
 
 drbd_params = \
@@ -8179,8 +8180,9 @@ def _GenerateDiskTemplate(lu, template_name,
   names.append(lv_prefix + "_meta")
 for idx, disk in enumerate(disk_info):
   disk_index = idx + base_index
+  drbd_default_metavg = drbd_params[constants.LDP_DEFAULT_METAVG]
   data_vg = disk.get(constants.IDISK_VG, vgname)
-  meta_vg = disk.get(constants.IDISK_METAVG, data_vg)
+  meta_vg = disk.get(constants.IDISK_METAVG, drbd_default_metavg)
   disk_dev = _GenerateDRBD8Branch(lu, primary_node, remote_node,
   disk[constants.IDISK_SIZE],
   [data_vg, meta_vg],
@@ -9190,8 +9192,9 @@ class LUInstanceCreate(LogicalUnit):
 constants.IDISK_SIZE: size,
 constants.IDISK_MODE: mode,
 constants.IDISK_VG: data_vg,
-constants.IDISK_METAVG: disk.get(constants.IDISK_METAVG, data_vg),
 }
+  if constants.IDISK_METAVG in disk:
+new_disk[constants.IDISK_METAVG] = disk[constants.IDISK_METAVG]
   if constants.IDISK_ADOPT in disk:
 new_disk[constants.IDISK_ADOPT] = disk[constants.IDISK_ADOPT]
   self.disks.append(new_disk)
diff --git a/lib/constants.py b/lib/constants.py
index 14d675e..76f8200 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -918,11 +918,13 @@ LDP_RESYNC_RATE = "resync-rate"
 LDP_STRIPES = "stripes"
 LDP_BARRIERS = "disabled-barriers"
 LDP_NO_META_FLUSH = "disable-meta-flush"
+LDP_DEFAULT_METAVG = "default-metavg"
 DISK_LD_TYPES = {
   LDP_RESYNC_RATE: VTYPE_INT,
   LDP_STRIPES: VTYPE_INT,
   LDP_BARRIERS: VTYPE_STRING,
   LDP_NO_META_FLUSH: VTYPE_BOOL,
+  LDP_DEFAULT_METAVG: VTYPE_STRING,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())
 
@@ -932,6 +934,7 @@ DRBD_DATA_STRIPES = "data-stripes"
 DRBD_META_STRIPES = "meta-stripes"
 DRBD_DISK_BARRIERS = "disk-barriers"
 DRBD_META_BARRIERS = "meta-barriers"
+DRBD_DEFAULT_METAVG = "metavg"
 LV_STRIPES = "stripes"
 DISK_DT_TYPES = {
   DRBD_RESYNC_RATE: VTYPE_INT,
@@ -939,6 +942,7 @@ DISK_DT_TYPES = {
   DRBD_META_STRIPES: VTYPE_INT,
   DRBD_DISK_BARRIERS: VTYPE_STRING,
   DRBD_META_BARRIERS: VTYPE_BOOL,
+  DRBD_DEFAULT_METAVG: VTYPE_STRING,
   LV_STRIPES: VTYPE_INT,
   }
 
@@ -1711,6 +1715,7 @@ DISK_LD_DEFAULTS = {
 LDP_RESYNC_RATE: CLASSIC_DRBD_SYNC_SPEED,
 LDP_BARRIERS: _autoconf.DRBD_BARRIERS,
 LDP_NO_META_FLUSH: _autoconf.DRBD_NO_META_FLUSH,
+LDP_DEFAULT_METAVG: DEFAULT_VG,
 },
   LD_LV: {
 LDP_STRIPES: _autoconf.LVM_STRIPECOUNT
@@ -1731,6 +1736,7 @@ DISK_DT_DEFAULTS = {
 DRBD_META_STRIPES: DISK_LD_DEFAULTS[LD_LV][LDP_STRIPES],
 DRBD_DISK_BARRIERS: DISK_LD_DEFAULTS[LD_DRBD8][LDP_BARRIERS],
 DRBD_META_BARRIERS: DISK_LD_DEFAULTS[LD_DRBD8][LDP_NO_META_FLUSH],
+DRBD_DEFAULT_METAVG: DISK_LD_DEFAULTS[LD_DRBD8][LDP_DEFAULT_METAVG],
 },
   DT_DISKLESS: {
 },
-- 
1.7.3.1

[PATCH master 3/4] Add net-custom and disk-custom DRBD parameters

[Andrea Spadaccini]

Those parameters can be used to pass options directly to drbdsetup disk
and drbdsetup net.

Signed-off-by: Andrea Spadaccini 
---
 lib/bdev.py  |7 +++
 lib/cmdlib.py|2 ++
 lib/constants.py |   12 
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index f249c16..7ec1204 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1372,6 +1372,9 @@ class DRBD8(BaseDRBD):
self.params[constants.LDP_NO_META_FLUSH])
 args.extend(barrier_args)
 
+if self.params[constants.LDP_DISK_CUSTOM]:
+  args.append(self.params[constants.LDP_DISK_CUSTOM])
+
 result = utils.RunCmd(args)
 if result.failed:
   _ThrowError("drbd%d: can't attach local disk: %s", minor, result.output)
@@ -1495,6 +1498,10 @@ class DRBD8(BaseDRBD):
   args.append("-m")
 if hmac and secret:
   args.extend(["-a", hmac, "-x", secret])
+
+if self.params[constants.LDP_NET_CUSTOM]:
+  args.append(self.params[constants.LDP_NET_CUSTOM])
+
 result = utils.RunCmd(args)
 if result.failed:
   _ThrowError("drbd%d: can't setup network: %s - %s",
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index af494ce..63417fd 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8062,6 +8062,8 @@ def _ComputeLDParams(disk_template, disk_params):
   constants.LDP_BARRIERS: dt_params[constants.DRBD_DISK_BARRIERS],
   constants.LDP_NO_META_FLUSH: dt_params[constants.DRBD_META_BARRIERS],
   constants.LDP_DEFAULT_METAVG: dt_params[constants.DRBD_DEFAULT_METAVG],
+  constants.LDP_DISK_CUSTOM: dt_params[constants.DRBD_DISK_CUSTOM],
+  constants.LDP_NET_CUSTOM: dt_params[constants.DRBD_NET_CUSTOM],
   }
 
 drbd_params = \
diff --git a/lib/constants.py b/lib/constants.py
index 76f8200..d8e3e07 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -919,12 +919,16 @@ LDP_STRIPES = "stripes"
 LDP_BARRIERS = "disabled-barriers"
 LDP_NO_META_FLUSH = "disable-meta-flush"
 LDP_DEFAULT_METAVG = "default-metavg"
+LDP_DISK_CUSTOM = "disk-custom"
+LDP_NET_CUSTOM = "net-custom"
 DISK_LD_TYPES = {
   LDP_RESYNC_RATE: VTYPE_INT,
   LDP_STRIPES: VTYPE_INT,
   LDP_BARRIERS: VTYPE_STRING,
   LDP_NO_META_FLUSH: VTYPE_BOOL,
   LDP_DEFAULT_METAVG: VTYPE_STRING,
+  LDP_DISK_CUSTOM: VTYPE_STRING,
+  LDP_NET_CUSTOM: VTYPE_STRING,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())
 
@@ -935,6 +939,8 @@ DRBD_META_STRIPES = "meta-stripes"
 DRBD_DISK_BARRIERS = "disk-barriers"
 DRBD_META_BARRIERS = "meta-barriers"
 DRBD_DEFAULT_METAVG = "metavg"
+DRBD_DISK_CUSTOM = "disk-custom"
+DRBD_NET_CUSTOM = "net-custom"
 LV_STRIPES = "stripes"
 DISK_DT_TYPES = {
   DRBD_RESYNC_RATE: VTYPE_INT,
@@ -943,6 +949,8 @@ DISK_DT_TYPES = {
   DRBD_DISK_BARRIERS: VTYPE_STRING,
   DRBD_META_BARRIERS: VTYPE_BOOL,
   DRBD_DEFAULT_METAVG: VTYPE_STRING,
+  DRBD_DISK_CUSTOM: VTYPE_STRING,
+  DRBD_NET_CUSTOM: VTYPE_STRING,
   LV_STRIPES: VTYPE_INT,
   }
 
@@ -1716,6 +1724,8 @@ DISK_LD_DEFAULTS = {
 LDP_BARRIERS: _autoconf.DRBD_BARRIERS,
 LDP_NO_META_FLUSH: _autoconf.DRBD_NO_META_FLUSH,
 LDP_DEFAULT_METAVG: DEFAULT_VG,
+LDP_DISK_CUSTOM: "",
+LDP_NET_CUSTOM: "",
 },
   LD_LV: {
 LDP_STRIPES: _autoconf.LVM_STRIPECOUNT
@@ -1737,6 +1747,8 @@ DISK_DT_DEFAULTS = {
 DRBD_DISK_BARRIERS: DISK_LD_DEFAULTS[LD_DRBD8][LDP_BARRIERS],
 DRBD_META_BARRIERS: DISK_LD_DEFAULTS[LD_DRBD8][LDP_NO_META_FLUSH],
 DRBD_DEFAULT_METAVG: DISK_LD_DEFAULTS[LD_DRBD8][LDP_DEFAULT_METAVG],
+DRBD_DISK_CUSTOM: DISK_LD_DEFAULTS[LD_DRBD8][LDP_DISK_CUSTOM],
+DRBD_NET_CUSTOM: DISK_LD_DEFAULTS[LD_DRBD8][LDP_NET_CUSTOM],
 },
   DT_DISKLESS: {
 },
-- 
1.7.3.1

[PATCH master 1/4] Move LD parameters constants to the LDP_ namespace

[Andrea Spadaccini]

Add the LDP_ prefix to the LD parameters-related constants, in order to
avoid pollution in the global constants namespace.

Signed-off-by: Andrea Spadaccini 
---
 lib/bdev.py  |   10 +-
 lib/cmdlib.py|   12 ++--
 lib/constants.py |   36 ++--
 3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index fabba9f..f249c16 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -415,7 +415,7 @@ class LogicalVolume(BlockDev):
   " in lvm.conf using either 'filter' or 'preferred_names'")
 free_size = sum([pv[0] for pv in pvs_info])
 current_pvs = len(pvlist)
-desired_stripes = params[constants.STRIPES]
+desired_stripes = params[constants.LDP_STRIPES]
 stripes = min(current_pvs, desired_stripes)
 if stripes < desired_stripes:
   logging.warning("Could not use %d stripes for VG %s, as only %d PVs are"
@@ -1368,8 +1368,8 @@ class DRBD8(BaseDRBD):
 
 barrier_args = \
   self._ComputeDiskBarrierArgs(vmaj, vmin, vrel,
-   self.params[constants.BARRIERS],
-   self.params[constants.NO_META_FLUSH])
+   self.params[constants.LDP_BARRIERS],
+   self.params[constants.LDP_NO_META_FLUSH])
 args.extend(barrier_args)
 
 result = utils.RunCmd(args)
@@ -1468,7 +1468,7 @@ class DRBD8(BaseDRBD):
 # sync speed only after setting up both sides can race with DRBD
 # connecting, hence we set it here before telling DRBD anything
 # about its peer.
-sync_speed = self.params.get(constants.RESYNC_RATE)
+sync_speed = self.params.get(constants.LDP_RESYNC_RATE)
 self._SetMinorSyncSpeed(minor, sync_speed)
 
 if netutils.IP6Address.IsValid(lhost):
@@ -1835,7 +1835,7 @@ class DRBD8(BaseDRBD):
   # the device
   self._SlowAssemble()
 
-sync_speed = self.params.get(constants.RESYNC_RATE)
+sync_speed = self.params.get(constants.LDP_RESYNC_RATE)
 self.SetSyncSpeed(sync_speed)
 
   def _SlowAssemble(self):
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 561a9bf..9b711cd 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8058,9 +8058,9 @@ def _ComputeLDParams(disk_template, disk_params):
   dt_params = disk_params[disk_template]
   if disk_template == constants.DT_DRBD8:
 drbd_params = {
-  constants.RESYNC_RATE: dt_params[constants.DRBD_RESYNC_RATE],
-  constants.BARRIERS: dt_params[constants.DRBD_DISK_BARRIERS],
-  constants.NO_META_FLUSH: dt_params[constants.DRBD_META_BARRIERS],
+  constants.LDP_RESYNC_RATE: dt_params[constants.DRBD_RESYNC_RATE],
+  constants.LDP_BARRIERS: dt_params[constants.DRBD_DISK_BARRIERS],
+  constants.LDP_NO_META_FLUSH: dt_params[constants.DRBD_META_BARRIERS],
   }
 
 drbd_params = \
@@ -8071,7 +8071,7 @@ def _ComputeLDParams(disk_template, disk_params):
 
 # data LV
 data_params = {
-  constants.STRIPES: dt_params[constants.DRBD_DATA_STRIPES],
+  constants.LDP_STRIPES: dt_params[constants.DRBD_DATA_STRIPES],
   }
 data_params = \
   objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
@@ -8080,7 +8080,7 @@ def _ComputeLDParams(disk_template, disk_params):
 
 # metadata LV
 meta_params = {
-  constants.STRIPES: dt_params[constants.DRBD_META_STRIPES],
+  constants.LDP_STRIPES: dt_params[constants.DRBD_META_STRIPES],
   }
 meta_params = \
   objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
@@ -8093,7 +8093,7 @@ def _ComputeLDParams(disk_template, disk_params):
 
   elif disk_template == constants.DT_PLAIN:
 params = {
-  constants.STRIPES: dt_params[constants.LV_STRIPES],
+  constants.LDP_STRIPES: dt_params[constants.LV_STRIPES],
   }
 params = \
   objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
diff --git a/lib/constants.py b/lib/constants.py
index c91f5f5..14d675e 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -914,15 +914,15 @@ NDS_PARAMETER_TYPES = {
 NDS_PARAMETERS = frozenset(NDS_PARAMETER_TYPES.keys())
 
 # Logical Disks parameters
-RESYNC_RATE = "resync-rate"
-STRIPES = "stripes"
-BARRIERS = "disabled-barriers"
-NO_META_FLUSH = "disable-meta-flush"
+LDP_RESYNC_RATE = "resync-rate"
+LDP_STRIPES = "stripes"
+LDP_BARRIERS = "disabled-barriers"
+LDP_NO_META_FLUSH = "disable-meta-flush"
 DISK_LD_TYPES = {
-  RESYNC_RATE: VTYPE_INT,
-  STRIPES: VTYPE_INT,
-  BARRIERS: VTYPE_STRING,
-  NO_META_FLUSH: VTYPE_BOOL,
+  LDP_RESYNC_RATE: VTYPE_INT,
+  LDP_STRIPES: VTYPE_INT,
+  LDP_BARRIERS: VTYPE_STRING,
+  LDP_NO_META_FLUSH: VTYPE_BOOL,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())
 
@@ -1708,12 +1708,12 @@ NDC_DEFAULTS = {
 
 DISK_LD_DEFAULTS = {
   LD_DRBD8: {
-RESYNC_RATE: CLASSIC_DRBD_SYNC_SPEED,
-BARRIER

[PATCH master 0/4] Last disk parameters - implementation and docs

[Andrea Spadaccini]

This is the final patch series for the disk parameters that are listed in the
design document about the new resource model. A patch that adds to it the
parameters needed for the DRBD sync rate adaptation algorithm will follow (and
next, the actual implementation will).

Andrea Spadaccini (4):
  Move LD parameters constants to the LDP_ namespace
  Add the metavg DRBD disk parameter
  Add net-custom and disk-custom DRBD parameters
  Add documentation for disk parameters to man pages

 lib/bdev.py |   17 ++
 lib/cmdlib.py   |   21 ++--
 lib/constants.py|   54 +--
 man/gnt-cluster.rst |   62 ++
 man/gnt-group.rst   |   12 -
 5 files changed, 128 insertions(+), 38 deletions(-)

-- 
1.7.3.1

Re: [PATCH master] Add DRBD barriers disk parameters

[Andrea Spadaccini]

Hi Iustin,

> The logical disk parameters should have a namespace, and not pollute the
> top level of constants.py. Something like LDP_BARRIERS, LDP_STRIPES,
> etc. LGTM on this patch, but this should be adjusted in a separate one.

Thanks, will send a patch to fix it.

Andrea

Re: [PATCH master] Add DRBD barriers disk parameters

[Andrea Spadaccini]

Hi Iustin,
here's the interdiff with the required changes.

Thanks,
Andrea

Interdiff:

diff --git a/Makefile.am b/Makefile.am
index e87ae62..839bb60 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -984,8 +984,8 @@ lib/_autoconf.py: Makefile | lib/.dir
echo "TOOLSDIR = '$(toolsdir)'"; \
echo "GNT_SCRIPTS = [$(foreach i,$(notdir $(gnt_scripts)),'$(i)',)]"; \
echo "PKGLIBDIR = '$(pkglibdir)'"; \
-   echo "DISABLED_DRBD_BARRIERS = '$(DISABLED_DRBD_BARRIERS)'"; \
-   echo "DISABLE_DRBD_META_FLUSH = $(DISABLE_DRBD_META_FLUSH)"; \
+   echo "DRBD_BARRIERS = '$(DRBD_BARRIERS)'"; \
+   echo "DRBD_NO_META_FLUSH = $(DRBD_NO_META_FLUSH)"; \
echo "SYSLOG_USAGE = '$(SYSLOG_USAGE)'"; \
echo "DAEMONS_GROUP = '$(DAEMONS_GROUP)'"; \
echo "ADMIN_GROUP = '$(ADMIN_GROUP)'"; \
diff --git a/configure.ac b/configure.ac
index f5fd21a..b672d95 100644
--- a/configure.ac
+++ b/configure.ac
@@ -210,18 +210,18 @@ AC_ARG_ENABLE([drbd-barriers],
   [AS_HELP_STRING([--enable-drbd-barriers],
 [enable by default the DRBD barriers functionality (>= 8.0.12)
(default: enabled)])],
   [[if test "$enableval" != no; then
-  DISABLED_DRBD_BARRIERS=n
-  DISABLE_DRBD_META_FLUSH=False
+  DRBD_BARRIERS=n
+  DRBD_NO_META_FLUSH=False
 else
-  DISABLED_DRBD_BARRIERS=bfd
-  DISABLE_DRBD_META_FLUSH=True
+  DRBD_BARRIERS=bfd
+  DRBD_NO_META_FLUSH=True
 fi
   ]],
-  [DISABLED_DRBD_BARRIERS=n
-   DISABLE_DRBD_META_FLUSH=False
+  [DRBD_BARRIERS=n
+   DRBD_NO_META_FLUSH=False
   ])
-AC_SUBST(DISABLED_DRBD_BARRIERS, $DISABLED_DRBD_BARRIERS)
-AC_SUBST(DISABLE_DRBD_META_FLUSH, $DISABLE_DRBD_META_FLUSH)
+AC_SUBST(DRBD_BARRIERS, $DRBD_BARRIERS)
+AC_SUBST(DRBD_NO_META_FLUSH, $DRBD_NO_META_FLUSH)

 # --enable-syslog[=no/yes/only]
 AC_ARG_ENABLE([syslog],
diff --git a/lib/bdev.py b/lib/bdev.py
index 26d581a..fabba9f 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1368,8 +1368,8 @@ class DRBD8(BaseDRBD):

 barrier_args = \
   self._ComputeDiskBarrierArgs(vmaj, vmin, vrel,
-   self.params[constants.DISABLED_BARRIERS],
-   self.params[constants.DISABLE_META_FLUSH])
+   self.params[constants.BARRIERS],
+   self.params[constants.NO_META_FLUSH])
 args.extend(barrier_args)

 result = utils.RunCmd(args)
@@ -1385,7 +1385,7 @@ class DRBD8(BaseDRBD):
 disabled_barriers and disable_meta_flush arguments, and according to the
 supported ones in the DRBD version vmaj.vmin.vrel

-If the desired option is unsupported, logs a WARNING message.
+If the desired option is unsupported, raises errors.BlockDeviceError.



 """
 disabled_barriers_set = frozenset(disabled_barriers)
@@ -1393,20 +1393,22 @@ class DRBD8(BaseDRBD):
   raise errors.BlockDeviceError("%s is not a valid option set for DRBD"
 " barriers" % disabled_barriers)

-args = list()
+args = []

 # The following code assumes DRBD 8.x, with x < 4 and x != 1 (DRBD 8.1.x
 # does not exist)
-assert vmaj == 8 and vmin in (0, 2, 3)
+if not vmaj == 8 and vmin in (0, 2, 3):
+  raise errors.BlockDeviceError("Unsupported DRBD version: %d.%d.%d" %
+(vmaj, vmin, vrel))

-def _AppendOrWarn(option, min_version):
+def _AppendOrRaise(option, min_version):
   """Helper for DRBD options"""
   if min_version is not None and vrel >= min_version:
 args.append(option)
   else:
-logging.warning("Could not append the option %s as the DRBD version"
-" %d.%d.%d does not support it.", option, vmaj, vmin,
-vrel)
+raise errors.BlockDeviceError("Could not use the option %s as the"
+  " DRBD version %d.%d.%d does not support"
+  " it." % (option, vmaj, vmin, vrel))

 # the minimum version for each feature is encoded via pairs of (minor
 # version -> x) where x is version in which support for the option was
@@ -1428,23 +1430,23 @@ class DRBD8(BaseDRBD):

 # meta flushes
 if disable_meta_flush:
-  _AppendOrWarn(cls._DISABLE_META_FLUSH_OPTION,
-meta_flush_supported.get(vmin, None))
+  _AppendOrRaise(cls._DISABLE_META_FLUSH_OPTION,
+ meta_flush_supported.get(vmin, None))

 # disk flushes
 if constants.DRBD_B_DISK_FLUSH in disabled_barriers_set:
-  _AppendOrWarn(cls._DISABLE_FLUSH_OPTION,
-disk_flush_supported.get(vmin, None))
+  _AppendOrRaise(cls._DISABLE_FLUSH_OPTION,
+ disk_flush_supported.get(vmin, None))

 # disk drain
 if constants.DRBD_B_DISK_DRAIN in disabled_barriers_set:
-  _AppendOrWarn(cls._DISABLE_DRAIN_OPTION,
-disk_drain_supported.get(vmin, None))
+  _AppendOrRaise(cls._DISABLE_DRAIN_OPTION,
+ 

Re: [PATCH master] Unify some file lists in Makefile.am

[Andrea Spadaccini]

Hi Iustin,

> These were repeated needlessly; I hope I grouped them correctly.

LGTM

Andrea

Re: [PATCH master] Add DRBD barriers disk parameters

[Andrea Spadaccini]

Hi Iustin,

>> > DRBD_BARRIERS, DRBD_META_FLUSH?
>>
>> These are simpler, but I think that they do not convey the meaning of
>> the parameters. DRBD_META_FLUSH=True would imply that the meta
>> barriers are disabled, and IMHO it is not immediately clear to someone
>> who's reading the code.
>
> True. On the other hand, I saw these two together on two lines and got
> very confused why they have a different verb time. Maybe
> DRBD_NO_META_FLUSH (clear enough) and DRBD_BARRIERS (this is not
> boolean, so the chance of confusion is less anyway - you have to
> understand the type?)

Thanks for the good suggestion, I'll use these variable names.

Andrea

Re: [PATCH master] Add DRBD barriers disk parameters

[Andrea Spadaccini]

Hi Iustin,

> A somewhat big patch, by itself…

Yeah.. I should have split it into a few patches, sorry.

>> +       echo "DISABLED_DRBD_BARRIERS = '$(DISABLED_DRBD_BARRIERS)'"; \
>> +       echo "DISABLE_DRBD_META_FLUSH = $(DISABLE_DRBD_META_FLUSH)"; \
>
> While I understand the rationale for DISABLED vs. DISABLE, the use of
> two different forms (affirmative vs. passive) is not that nice…

[ quote from the bottom of the mail ]
> So the DISABLED vs DISABLE is even present in the code? Why not simply
> DRBD_BARRIERS, DRBD_META_FLUSH?

These are simpler, but I think that they do not convey the meaning of
the parameters. DRBD_META_FLUSH=True would imply that the meta
barriers are disabled, and IMHO it is not immediately clear to someone
who's reading the code.

>> -  @classmethod
>> -  def _AssembleLocal(cls, minor, backend, meta, size):
>> +  def _AssembleLocal(self, minor, backend, meta, size):
>
> Just a note: since all you need self for is the params, you could have
> also passed the params to this function via a separate arg.

Yes, but then why have a class method that depends on the state of the
instance itself? Isn't that an instance method? :)

>> +    args = list()
>
> args = []

Ack.

>> +    # The following code assumes DRBD 8.x, with x < 4 and x != 1 (DRBD 8.1.x
>> +    # does not exist)
>> +    assert vmaj == 8 and vmin in (0, 2, 3)
>
> No, you can't do that. assert should only be used on values computed
> *by* the programs; and environment value should never be asserted on,
> but verified and raise errors.BlockDeviceError if it doesn't match what
> you expect.

Ack, will raise BlockDeviceError.

>> +    def _AppendOrWarn(option, min_version):
>> +      """Helper for DRBD options"""
>> +      if min_version is not None and vrel >= min_version:
>> +        args.append(option)
>> +      else:
>> +        logging.warning("Could not append the option %s as the DRBD version"
>> +                        " %d.%d.%d does not support it.", option, vmaj, 
>> vmin,
>> +                        vrel)
>
> Not sure if this is the right place; first, it might fill the disk with
> lots of warnings (one for each DRBD device), and second this should be
> somehow reported back to the master; ideally you shouldn't be able to
> enable an option if it's not supported.
>
> I rather think this should be a proper exception.

Ack, will also raise BlockDeviceError here.

Thanks,
Andrea

[PATCH master] Add DRBD barriers disk parameters

[Andrea Spadaccini]

Add the disk-barriers and meta-barriers parameters described in the
design doc.

constants.py:
* add the needed LD and DT-level parameters, use the defaults provided
  at ./configure time;
* add constants representing which barriers should be disabled and the
  set of valid options.

lib/bdev.py:
* factor the barriers handling code to a class method, for testing
  purposes;
* implement the more granular version checking logic;
* use the LD level parameters;
* add stricted check on DRBD version (8.0, 8.2 or 8.3), as we do not
  support 8.4 yet.

lib/cmdlib.py:
* translate DT level parameters to LD level ones.

configure.ac, Makefile.am:
* set both disk and meta barriers parameters depending on the value of
  --enable-drbd-barriers.

test/ganeti.bdev_unittest.py:
* unit tests for the code that sets DRBD barrier parameters depending on
  the version.

doc/design-resource-model.rst:
* reword the description of meta-barriers;
* change all disk parameters names to use dashes instead of underscores.

Signed-off-by: Andrea Spadaccini 
---
 Makefile.am   |3 +-
 configure.ac  |   15 --
 doc/design-resource-model.rst |   14 +++---
 lib/bdev.py   |  113 +
 lib/cmdlib.py |4 +-
 lib/constants.py  |   32 +++-
 test/ganeti.bdev_unittest.py  |   60 ++
 7 files changed, 205 insertions(+), 36 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 171e9dd..e87ae62 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -984,7 +984,8 @@ lib/_autoconf.py: Makefile | lib/.dir
  echo "TOOLSDIR = '$(toolsdir)'"; \
  echo "GNT_SCRIPTS = [$(foreach i,$(notdir $(gnt_scripts)),'$(i)',)]"; 
\
  echo "PKGLIBDIR = '$(pkglibdir)'"; \
- echo "DRBD_BARRIERS = $(DRBD_BARRIERS)"; \
+ echo "DISABLED_DRBD_BARRIERS = '$(DISABLED_DRBD_BARRIERS)'"; \
+ echo "DISABLE_DRBD_META_FLUSH = $(DISABLE_DRBD_META_FLUSH)"; \
  echo "SYSLOG_USAGE = '$(SYSLOG_USAGE)'"; \
  echo "DAEMONS_GROUP = '$(DAEMONS_GROUP)'"; \
  echo "ADMIN_GROUP = '$(ADMIN_GROUP)'"; \
diff --git a/configure.ac b/configure.ac
index 7c4b148..f5fd21a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -208,15 +208,20 @@ AC_MSG_NOTICE([Group for clients is $group_admin])
 # --enable-drbd-barriers
 AC_ARG_ENABLE([drbd-barriers],
   [AS_HELP_STRING([--enable-drbd-barriers],
-[enable the DRBD barrier functionality (>= 8.0.12) (default: enabled)])],
+[enable by default the DRBD barriers functionality (>= 8.0.12) (default: 
enabled)])],
   [[if test "$enableval" != no; then
-  DRBD_BARRIERS=True
+  DISABLED_DRBD_BARRIERS=n
+  DISABLE_DRBD_META_FLUSH=False
 else
-  DRBD_BARRIERS=False
+  DISABLED_DRBD_BARRIERS=bfd
+  DISABLE_DRBD_META_FLUSH=True
 fi
   ]],
-  [DRBD_BARRIERS=True])
-AC_SUBST(DRBD_BARRIERS, $DRBD_BARRIERS)
+  [DISABLED_DRBD_BARRIERS=n
+   DISABLE_DRBD_META_FLUSH=False
+  ])
+AC_SUBST(DISABLED_DRBD_BARRIERS, $DISABLED_DRBD_BARRIERS)
+AC_SUBST(DISABLE_DRBD_META_FLUSH, $DISABLE_DRBD_META_FLUSH)
 
 # --enable-syslog[=no/yes/only]
 AC_ARG_ENABLE([syslog],
diff --git a/doc/design-resource-model.rst b/doc/design-resource-model.rst
index e9e179f..212fdc7 100644
--- a/doc/design-resource-model.rst
+++ b/doc/design-resource-model.rst
@@ -724,28 +724,28 @@ encoded via a separator (e.g. slash), or by having two 
dict levels.
 || | |as the meta LVs are  |  |
 || | |small|  |
 ++-+-+-+--+
-|drbd|disk_barriers|What kind of barriers to |Either all enabled or|string|
+|drbd|disk-barriers|What kind of barriers to |Either all enabled or|string|
 || |*disable* for disks; |all disabled, per|  |
 || |either "n" or a string   |./configure time |  |
 || |containing a subset of   |option   |  |
 || |"bfd"| |  |
 ++-+-+-+--+
-|drbd|meta_barriers|Whether barriers are |Handled together with|bool  |
-|| |enabled or not for the   |disk_barriers|  |
-|| |meta volume  | |  |
+|drbd|meta-barriers|Whether to disable or not|Handled together with|bool  |
+|| |the barriers for the meta|disk-barriers|  |
+|| |volume   | |  |
 ++-+-

Re: [PATCH master] LV stripes parameters for plain and drbd

[Andrea Spadaccini]

Hi Iustin,

> LGTM, but there's some inconsistency in the naming. resync-rate, but
> datastripes (no dash). Should we consistently name these?

Yes, I'll split words with dashes before pushing.

Thanks,
Andrea

[PATCH master] LV stripes parameters for plain and drbd

[Andrea Spadaccini]

configure.ac:
* change the documentation of --with-lvm-stripecount parameter to
  reflect the change

doc/design-resource-model.rst:
* change drbd/stripes to drbd/datastripes

rest of files:
* add the plain/stripes, drbd/datastripes and drbd/metastripes disk
  parameters

Signed-off-by: Andrea Spadaccini 
---
 configure.ac  |2 +-
 doc/design-resource-model.rst |4 ++--
 lib/bdev.py   |6 +-
 lib/cmdlib.py |   35 ++-
 lib/constants.py  |   15 +--
 5 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/configure.ac b/configure.ac
index d08bd6b..7c4b148 100644
--- a/configure.ac
+++ b/configure.ac
@@ -147,7 +147,7 @@ AC_SUBST(KVM_PATH, $kvm_path)
 # --with-lvm-stripecount=...
 AC_ARG_WITH([lvm-stripecount],
   [AS_HELP_STRING([--with-lvm-stripecount=NUM],
-[the number of stripes to use for LVM volumes]
+[the default number of stripes to use for LVM volumes]
 [ (default is 1)]
   )],
   [lvm_stripecount="$withval"],
diff --git a/doc/design-resource-model.rst b/doc/design-resource-model.rst
index 8201ed6..1032e8a 100644
--- a/doc/design-resource-model.rst
+++ b/doc/design-resource-model.rst
@@ -711,8 +711,8 @@ encoded via a separator (e.g. slash), or by having two dict 
levels.
 || |logical voumes   |overridable at   |  |
 || | |runtime  |  |
 ++-+-+-+--+
-|drbd|stripes  |How many stripes to use  |Same as for plain|int   |
-|| |for data volumes | |  |
+|drbd|datastripes  |How many stripes to use  |Same as for  |int   |
+|| |for data volumes |plain/stripes|  |
 ++-+-+-+--+
 |drbd|metavg   |Default volume group for |Same as the main |string|
 || |the metadata LVs |volume group,|  |
diff --git a/lib/bdev.py b/lib/bdev.py
index 2182beb..230a4a4 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -415,7 +415,11 @@ class LogicalVolume(BlockDev):
   " in lvm.conf using either 'filter' or 'preferred_names'")
 free_size = sum([pv[0] for pv in pvs_info])
 current_pvs = len(pvlist)
-stripes = min(current_pvs, constants.LVM_STRIPECOUNT)
+desired_stripes = params[constants.STRIPES]
+stripes = min(current_pvs, desired_stripes)
+if stripes < desired_stripes:
+  logging.warning("Could not use %d stripes for VG %s, as only %d PVs are"
+  " available.", desired_stripes, vg_name, current_pvs)
 
 # The size constraint should have been checked from the master before
 # calling the create function.
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 71fac54..1fbdc28 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8057,22 +8057,47 @@ def _ComputeLDParams(disk_template, disk_params):
   result = list()
   dt_params = disk_params[disk_template]
   if disk_template == constants.DT_DRBD8:
-params = {
+drbd_params = {
   constants.RESYNC_RATE: dt_params[constants.DRBD_RESYNC_RATE]
   }
 
 drbd_params = \
-  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_DRBD8], params)
+  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_DRBD8],
+   drbd_params)
 
 result.append(drbd_params)
-result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
-result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+
+# data LV
+data_params = {
+  constants.STRIPES: dt_params[constants.DRBD_DATA_STRIPES],
+  }
+data_params = \
+  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
+   data_params)
+result.append(data_params)
+
+# metadata LV
+meta_params = {
+  constants.STRIPES: dt_params[constants.DRBD_META_STRIPES],
+  }
+meta_params = \
+  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
+   meta_params)
+result.append(meta_params)
 
   elif (disk_template == constants.DT_FILE or
 disk_template == constants.DT_SHARED_FILE):
 result.append(constants.DISK_LD_DEFAULTS[constants.LD_FILE])
+
   elif disk_template == constants.DT_PLAIN:
-result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+params = {
+  constants.STRIPES: dt_params[constants.LV_STRIPES],
+  }
+params = \
+  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_LV],
+   params)
+result.append(params)
+
   elif disk_template == constants.DT_BLOCK:
 result.append(constants.DISK_LD_DEFAULTS[constants.LD_BLOCKDEV])
 
diff --git a/lib/constants.py b/lib/constants.py
index f5ad7f

Re: [PATCH master] More fixes after commit 78519c106

[Andrea Spadaccini]

Hi Michael,

> A quick QA run successfully finished with these changes.

LGTM

Re: [PATCH master 4/5] Use disk parameters in Logical Units

[Andrea Spadaccini]

Hi Iustin,

>> I see. Still, the unpacking should be done consistently - either in all
>> cases in _GenerateDiskTemplate, or in all cases in the callers; so I'd
>> pass ld_params always.
>
> Right, it should be consistent. I prefer to do all the unpacking in
> the same function that calls _ComputeLDParams, so in this case in
> _GenerateDiskTemplate; I'll send an interdiff.

Interdiff:
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 9a0da0d..a1d322b 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8050,14 +8050,14 @@ def _ComputeLDParams(disk_template, disk_params):


 def _GenerateDRBD8Branch(lu, primary, secondary, size, vgnames, names,
- iv_name, p_minor, s_minor, ld_params):
+ iv_name, p_minor, s_minor, drbd_params, data_params,
+ meta_params):
   """Generate a drbd8 device complete with its children.

   """
   assert len(vgnames) == len(names) == 2
   port = lu.cfg.AllocatePort()
   shared_secret = lu.cfg.GenerateDRBDSecret(lu.proc.GetECId())
-  drbd_params, data_params, meta_params = ld_params

   dev_data = objects.Disk(dev_type=constants.LD_LV, size=size,
   logical_id=(vgnames[0], names[0]),
@@ -8108,6 +8108,7 @@ def _GenerateDiskTemplate(lu, template_name,
   params=ld_params[0])
   disks.append(disk_dev)
   elif template_name == constants.DT_DRBD8:
+drbd_params, data_params, meta_params = ld_params
 if len(secondary_nodes) != 1:
   raise errors.ProgrammerError("Wrong template configuration")
 remote_node = secondary_nodes[0]
@@ -8129,7 +8130,7 @@ def _GenerateDiskTemplate(lu, template_name,
   names[idx * 2:idx * 2 + 2],
   "disk/%d" % disk_index,
   minors[idx * 2], minors[idx * 2 + 1],
-  ld_params)
+  drbd_params, data_params, meta_params)
   disk_dev.mode = disk[constants.IDISK_MODE]
   disks.append(disk_dev)
   elif template_name == constants.DT_FILE:

Re: [PATCH master 5/5] Add DRBD8 static resync speed disk parameter

[Andrea Spadaccini]

Hi Iustin,

> LGTM. I'd keep the actual 60 * 1024 in a separate constant though (e.g.
> CLASSIC_DRBD_SYNC_SPEED or such).

Thanks.

Interdiff:
diff --git a/lib/constants.py b/lib/constants.py
index 66e8766..f5ad7f1 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -597,6 +597,7 @@ MAX_TAGS_PER_OBJ = 4096

 # others
 DEFAULT_BRIDGE = "xen-br0"
+CLASSIC_DRBD_SYNC_SPEED = 60 * 1024  # 60 MiB, expressed in KiB
 IP4_ADDRESS_LOCALHOST = "127.0.0.1"
 IP4_ADDRESS_ANY = "0.0.0.0"
 IP6_ADDRESS_LOCALHOST = "::1"
@@ -1674,7 +1675,7 @@ NDC_DEFAULTS = {

 DISK_LD_DEFAULTS = {
   LD_DRBD8: {
-RESYNC_RATE: 60 * 1024  # 60 MiB, expressed in KiB
+RESYNC_RATE: CLASSIC_DRBD_SYNC_SPEED,
 },
   LD_LV: {
 },

Re: [PATCH master 4/5] Use disk parameters in Logical Units

[Andrea Spadaccini]

Hi Iustin,

>> The _ComputeLDParams function returns always a list of those dicts. In
>> most cases, there is only one element in the list and so Disk usually
>> gets just the first element of the list (ld_params[0] in quoted code).
>> For LD_DRBD8, I pass the list to _GenerateDRBD8Branch, and hence the
>> use of ld_params without the [0] subscription in this code snippet; in
>> this case, _GenerateDRBD8Branch will unpack the list and pass the
>> individual elements (that are dicts) to each Disk object.
>
> I see. Still, the unpacking should be done consistently - either in all
> cases in _GenerateDiskTemplate, or in all cases in the callers; so I'd
> pass ld_params always.

Right, it should be consistent. I prefer to do all the unpacking in
the same function that calls _ComputeLDParams, so in this case in
_GenerateDiskTemplate; I'll send an interdiff.

Thanks,
Andrea

Re: [PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

Hi Iustin,

> I added the tests and addressed the comments made by Michael in his
> first reply. I also changed a bit the logic of the upgrade of disk
> parameters in objects.py, and removed the code duplication by creating
> a new function.
>
> Interdiff:

Changing 3/5, I introduced a bug in export (caught by QA); this
additional interdiff fixes it and also adds a missing .copy():

diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
index 8211f31..32f6497 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -1176,9 +1176,11 @@ class ExportInstanceHelper:
 " result '%s'", idx, src_node, result.payload)
   else:
 disk_id = tuple(result.payload)
+disk_params = constants.DISK_LD_DEFAULTS[constants.LD_LV].copy()
 new_dev = objects.Disk(dev_type=constants.LD_LV, size=disk.size,
logical_id=disk_id, physical_id=disk_id,
-   iv_name=disk.iv_name)
+   iv_name=disk.iv_name,
+   params=disk_params)

   self._snap_disks.append(new_dev)

diff --git a/lib/objects.py b/lib/objects.py
index 87218f6..f568149 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -773,7 +773,7 @@ class Disk(ConfigObject):
 child.UpgradeConfig()

 if not self.params:
-  self.params = constants.DISK_LD_DEFAULTS[self.dev_type]
+  self.params = constants.DISK_LD_DEFAULTS[self.dev_type].copy()
 else:
   self.params = FillDict(constants.DISK_LD_DEFAULTS[self.dev_type],
  self.params)

Re: [PATCH master 5/5] Add DRBD8 static resync speed disk parameter

[Andrea Spadaccini]

Hi Iustin,

>>> +  RESYNC_RATE: VTYPE_STRING,
>>
>> Shouldn't this be VTYPE_INT?
>
> Yes, thanks!

Interdiff:

diff --git a/lib/constants.py b/lib/constants.py
index 6a46592..66e8766 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -898,7 +898,7 @@ NDS_PARAMETERS = frozenset(NDS_PARAMETER_TYPES.keys())
 # Logical Disks parameters
 RESYNC_RATE = "resync-rate"
 DISK_LD_TYPES = {
-  RESYNC_RATE: VTYPE_STRING,
+  RESYNC_RATE: VTYPE_INT,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())

Re: [PATCH master 3/5] Use disk parameters in noded

[Andrea Spadaccini]

Hi Iustin,

> Can't you use here FillDict instead of reimplementing it?

Done.

Interdiff:

diff --git a/lib/bdev.py b/lib/bdev.py
index 9f8f7c6..b9fc651 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -2220,14 +2220,6 @@ if constants.ENABLE_FILE_STORAGE or
constants.ENABLE_SHARED_FILE_STORAGE:
   DEV_MAP[constants.LD_FILE] = FileStorage


-def _GetUpdatedDiskParams(dev_type, params):
-  dev_params = constants.DISK_LD_DEFAULTS[dev_type]
-  if params:
-dev_params.update(params)
-
-  return dev_params
-
-
 def _VerifyDiskType(dev_type):
   if dev_type not in DEV_MAP:
 raise errors.ProgrammerError("Invalid block device type '%s'" % dev_type)
@@ -2247,7 +2239,8 @@ def FindDevice(disk, children):

   """
   _VerifyDiskType(disk.dev_type)
-  dev_params = _GetUpdatedDiskParams(disk.dev_type, disk.params)
+  dev_params = objects.FillDict(constants.DISK_LD_DEFAULTS[disk.dev_type],
+disk.params)
   device = DEV_MAP[disk.dev_type](disk.physical_id, children, disk.size,
   dev_params)
   if not device.attached:
@@ -2269,7 +2262,8 @@ def Assemble(disk, children):

   """
   _VerifyDiskType(disk.dev_type)
-  dev_params = _GetUpdatedDiskParams(disk.dev_type, disk.params)
+  dev_params = objects.FillDict(constants.DISK_LD_DEFAULTS[disk.dev_type],
+disk.params)
   device = DEV_MAP[disk.dev_type](disk.physical_id, children, disk.size,
   dev_params)
   device.Assemble()
@@ -2287,7 +2281,8 @@ def Create(disk, children):

   """
   _VerifyDiskType(disk.dev_type)
-  dev_params = _GetUpdatedDiskParams(disk.dev_type, disk.params)
+  dev_params = objects.FillDict(constants.DISK_LD_DEFAULTS[disk.dev_type],
+disk.params)
   device = DEV_MAP[disk.dev_type].Create(disk.physical_id, children, disk.size,
  dev_params)
   return device

Re: [PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

Hi Iustin,

> Indeed. The problem then remains about the embedded dicts which are
> empty. I'm fine to pre-declare them.

I added the tests and addressed the comments made by Michael in his
first reply. I also changed a bit the logic of the upgrade of disk
parameters in objects.py, and removed the code duplication by creating
a new function.

Interdiff:

diff --git a/lib/constants.py b/lib/constants.py
index 0c6b252..827db0a 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -460,6 +460,13 @@ LD_LV = "lvm"
 LD_DRBD8 = "drbd8"
 LD_FILE = "file"
 LD_BLOCKDEV = "blockdev"
+LOGICAL_DISK_TYPES = frozenset([
+  LD_LV,
+  LD_DRBD8,
+  LD_FILE,
+  LD_BLOCKDEV,
+  ])
+
 LDS_BLOCK = frozenset([LD_LV, LD_DRBD8, LD_BLOCKDEV])

 # drbd constants
diff --git a/lib/objects.py b/lib/objects.py
index 11d11cf..26e7cf4 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -110,6 +110,32 @@ def UpgradeBeParams(target):
 del target[constants.BE_MEMORY]


+def UpgradeDiskParams(diskparams):
+  """Upgrade the disk parameters.
+
+  @type diskparams: dict
+  @param diskparams: disk parameters to upgrade
+  @rtype: dict
+  @return: the upgraded disk parameters dit
+
+  """
+  result = dict()
+  if diskparams is None:
+result = constants.DISK_DT_DEFAULTS.copy()
+  else:
+# Update the disk parameter values for each disk template.
+# The code iterates over constants.DISK_TEMPLATES because new templates
+# might have been added.
+for template in constants.DISK_TEMPLATES:
+  if template not in diskparams:
+result[template] = constants.DISK_DT_DEFAULTS[template].copy()
+  else:
+result[template] = FillDict(constants.DISK_DT_DEFAULTS[template],
+diskparams[template])
+
+  return result
+
+
 class ConfigObject(object):
   """A generic config object.

@@ -1084,13 +1110,7 @@ class NodeGroup(TaggableObject):
 if self.mtime is None:
   self.mtime = time.time()

-if self.diskparams is None:
-  self.diskparams = constants.DISK_DT_DEFAULTS
-else:
-  for template in self.diskparams:
-self.diskparams[template] = \
-  FillDict(constants.DISK_DT_DEFAULTS[template],
-   self.diskparams[template])
+self.diskparams = UpgradeDiskParams(self.diskparams)

   def FillND(self, node):
 """Return filled out ndparams for L{objects.Node}
@@ -1250,13 +1270,7 @@ class Cluster(TaggableObject):
 if self.use_external_mip_script is None:
   self.use_external_mip_script = False

-if self.diskparams is None:
-  self.diskparams = constants.DISK_DT_DEFAULTS
-else:
-  for template in self.diskparams:
-self.diskparams[template] = \
-  FillDict(constants.DISK_DT_DEFAULTS[template],
-   self.diskparams[template])
+self.diskparams = UpgradeDiskParams(self.diskparams)

   def ToDict(self):
 """Custom function for cluster.
diff --git a/test/ganeti.constants_unittest.py
b/test/ganeti.constants_unittest.py
index acde75b..0b7736c 100755
--- a/test/ganeti.constants_unittest.py
+++ b/test/ganeti.constants_unittest.py
@@ -79,6 +79,12 @@ class TestConstants(unittest.TestCase):
 self.failUnless(constants.OP_PRIO_NORMAL > constants.OP_PRIO_HIGH)
 self.failUnless(constants.OP_PRIO_HIGH > constants.OP_PRIO_HIGHEST)

+  def testDiskDefaults(self):
+self.failUnless(set(constants.DISK_LD_DEFAULTS.keys()) ==
+constants.LOGICAL_DISK_TYPES)
+self.failUnless(set(constants.DISK_DT_DEFAULTS.keys()) ==
+constants.DISK_TEMPLATES)
+

 class TestExportedNames(unittest.TestCase):
   _VALID_NAME_RE = re.compile(r"^[A-Z][A-Z0-9_]+$")

Re: [PATCH master 4/5] Use disk parameters in Logical Units

[Andrea Spadaccini]

Hi Iustin,

>> @@ -8059,7 +8095,8 @@ def _GenerateDiskTemplate(lu, template_name,
>> +                              params=ld_params[0])
>> @@ -8082,7 +8119,8 @@ def _GenerateDiskTemplate(lu, template_name,
>> +                                      ld_params)
>> @@ -8099,7 +8137,8 @@ def _GenerateDiskTemplate(lu, template_name,
>> +                              params=ld_params[0])
>
>> @@ -8115,7 +8154,8 @@ def _GenerateDiskTemplate(lu, template_name,
>> +                              params=ld_params[0])
>
>> @@ -8128,7 +8168,8 @@ def _GenerateDiskTemplate(lu, template_name,
>> +                              params=ld_params[0])
>
> I'm not sure I follow correctly, but this means the type of the 'params'
> attribute on the disk object varies depending on its disk_type (either a
> dict or a list of dicts), which is bad;

No, the params attribute of Disk is always a dict.

The _ComputeLDParams function returns always a list of those dicts. In
most cases, there is only one element in the list and so Disk usually
gets just the first element of the list (ld_params[0] in quoted code).
For LD_DRBD8, I pass the list to _GenerateDRBD8Branch, and hence the
use of ld_params without the [0] subscription in this code snippet; in
this case, _GenerateDRBD8Branch will unpack the list and pass the
individual elements (that are dicts) to each Disk object.

Thanks,
Andrea

Re: [PATCH master 5/5] Add DRBD8 static resync speed disk parameter

[Andrea Spadaccini]

Hi Iustin,

>>  # Logical Disks parameters
>> +RESYNC_RATE = "resync-rate"
>>  DISK_LD_TYPES = {
>> +  RESYNC_RATE: VTYPE_STRING,
>
> Shouldn't this be VTYPE_INT?

Yes, thanks!

Andrea

Re: [PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

Hi Iustin,

>> > > Michael: this would mean that the dict are no longer empty.
>> >
>> > Yes, but even when all the parameters in the design doc are implemented,
>> > some of the 2nd level dicts, both on DT and LD level, will be empty (with
>> > the current design).
>>
>> Nope, they can't be - there must be a value in there. Will check the
>> design again and reply.
>
> Indeed, I see no parameters that should behave differently than any
> other parameter we support (i.e. with a definite value at cluster level
> and inheritable across nodegroups or other lower objects).

Yes, I agree. What I wrote is that "some of the 2nd level dicts will
be empty". What I mean is that, for example, there are no parameters
for LD_FILE and for DT_FILE, so both dictionaries will end up being
empty even after all the parameters are implemented.

Every parameter will *always* have a default value, both at cluster
and at node group level; patch 5/5 contains an example implementation
of a disk parameter, with a default both at LD and DT level. Note that
the user cannot change the LD parameter value, in fact he does not
even know that it exists, but can manipulate only the DT level.

I hope that I have clarified my point of view.

Thanks,
Andrea

Re: [PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

Hi Iustin,

> The bigger problem is that these parameters have no defaults, which is
> against our (ganeti) philosophy that all parameter have clear default
> values at cluster level.
>
> Andrea: this needs to be changed so that there are always defaults.

In this patch, no parameter is implemented, thus no defaults. In 5/5 you
can see the drbd sync speed as an example implementation, with default
values.

> Michael: this would mean that the dict are no longer empty.

Yes, but even when all the parameters in the design doc are implemented,
some of the 2nd level dicts, both on DT and LD level, will be empty (with
the current design).

Thanks,
Andrea

Re: [MERGE] stable-2.5 into devel-2.5

[Andrea Spadaccini]

Hi Michael,

>    Merge branch 'stable-2.5' into devel-2.5
>
>    * stable-2.5:
>      ConfigWriter: Fix epydoc error

LGTM

Re: [PATCH stable-2.5] ConfigWriter: Fix epydoc error

[Andrea Spadaccini]

Hi Michael,

> The parameter is called “mods”, not “modes”.
>
> Signed-off-by: Michael Hanselmann 
> Reviewed-by: Andrea Spadaccini 
> (cherry picked from commit 1730d4a1ab56ef36d082b614d3d0ab13f3e14a85)

LGTM

Re: [PATCH devel-2.4] ConfigWriter: Fix epydoc error

[Andrea Spadaccini]

Hi Michael,

> The parameter is called “mods”, not “modes”.

LGTM.

Re: [PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

Hi Michael,
[…]
>> +  DT_SHARED_FILE: {
>> +    },
>> +  DT_BLOCK: {
>> +    },
>> +  }
>
> If there are no defaults, why does it still need to be listed? It
> would be safer for extensions if the rest of the code treats
> non-existing entries as empty.

You are right on safety, but I think that it is cleaner in this way,
or I'd have to check for emptiness or use get() every time I access
the dicts, and that's also easy to forget. For safety, I can add unit
tests ensuring that all keys in DISK_TEMPLATES are defined in the
parameters dicts, and do the same for the set of logical disks.

>> +    if self.diskparams is None:
>> +      self.diskparams = constants.DISK_DT_DEFAULTS
>
> Are you sure you don't want to make a copy? Acutally I'm pretty sure
> you must copy (dict.copy).

Hmm.. yes, those dicts have to be copied.

> This code block is duplicated from node groups. Why do you actually
> depend on diskparams being non-empty, and why do you depend on both
> levels having entries?

See above.

>> +    else:
>> +      for template in self.diskparams:
>> +        self.diskparams[template] = \
>> +          FillDict(constants.DISK_DT_DEFAULTS[template],
>> +                   self.diskparams[template])
>
> Please add a comment describing what this code does.

Will do.

Thanks,
Andrea

[PATCH master 1/5] Add basic support for disk parameters

[Andrea Spadaccini]

objects.py:
  * add disk parameters to Disk, Cluster, NodeGroup

constants.py:
* add dictionaries that will hold types and default values for disk
parameters (for now, empty).

rest of files:
  * add to gnt-cluster and gnt-group the options to manipulate disk
parameters

Signed-off-by: Andrea Spadaccini 
---
 lib/bootstrap.py  |   23 ++-
 lib/cli.py|6 ++
 lib/client/gnt_cluster.py |   33 ++---
 lib/client/gnt_group.py   |   17 -
 lib/cmdlib.py |   41 -
 lib/constants.py  |   37 +
 lib/objects.py|   26 +-
 lib/opcodes.py|   10 ++
 8 files changed, 178 insertions(+), 15 deletions(-)

diff --git a/lib/bootstrap.py b/lib/bootstrap.py
index 2f6f985..ae5f31a 100644
--- a/lib/bootstrap.py
+++ b/lib/bootstrap.py
@@ -286,11 +286,11 @@ def InitCluster(cluster_name, mac_prefix, # pylint: 
disable=R0913
 master_netmask, master_netdev, file_storage_dir,
 shared_file_storage_dir, candidate_pool_size, 
secondary_ip=None,
 vg_name=None, beparams=None, nicparams=None, ndparams=None,
-hvparams=None, enabled_hypervisors=None, modify_etc_hosts=True,
-modify_ssh_setup=True, maintain_node_health=False,
-drbd_helper=None, uid_pool=None, default_iallocator=None,
-primary_ip_version=None, prealloc_wipe_disks=False,
-use_external_mip_script=False):
+hvparams=None, diskparams=None, enabled_hypervisors=None,
+modify_etc_hosts=True, modify_ssh_setup=True,
+maintain_node_health=False, drbd_helper=None, uid_pool=None,
+default_iallocator=None, primary_ip_version=None,
+prealloc_wipe_disks=False, use_external_mip_script=False):
   """Initialise the cluster.
 
   @type candidate_pool_size: int
@@ -425,6 +425,17 @@ def InitCluster(cluster_name, mac_prefix, # pylint: 
disable=R0913
 hv_class = hypervisor.GetHypervisor(hv_name)
 hv_class.CheckParameterSyntax(hv_params)
 
+  # diskparams is a mapping of disk-template->diskparams dict
+  for template, dt_params in diskparams.items():
+param_keys = set(dt_params.keys())
+default_param_keys = set(constants.DISK_DT_DEFAULTS[template].keys())
+if not (param_keys <= default_param_keys):
+  unknown_params = param_keys - default_param_keys
+  raise errors.OpPrereqError("Invalid parameters for disk template %s:"
+ " %s" % (template,
+  utils.CommaJoin(unknown_params)))
+utils.ForceDictType(dt_params, constants.DISK_DT_TYPES)
+
   # set up ssh config and /etc/hosts
   sshline = utils.ReadFile(constants.SSH_HOST_RSA_PUB)
   sshkey = sshline.split(" ")[1]
@@ -472,6 +483,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint: 
disable=R0913
 nicparams={constants.PP_DEFAULT: nicparams},
 ndparams=ndparams,
 hvparams=hvparams,
+diskparams=diskparams,
 candidate_pool_size=candidate_pool_size,
 modify_etc_hosts=modify_etc_hosts,
 modify_ssh_setup=modify_ssh_setup,
@@ -541,6 +553,7 @@ def InitConfig(version, cluster_config, master_node_config,
 uuid=uuid_generator.Generate([], utils.NewUUID, _INITCONF_ECID),
 name=constants.INITIAL_NODE_GROUP_NAME,
 members=[master_node_config.name],
+diskparams=cluster_config.diskparams,
 )
   nodegroups = {
 default_nodegroup.uuid: default_nodegroup,
diff --git a/lib/cli.py b/lib/cli.py
index 5d66567..95eff1e 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -69,6 +69,7 @@ __all__ = [
   "DEBUG_SIMERR_OPT",
   "DISKIDX_OPT",
   "DISK_OPT",
+  "DISK_PARAMS_OPT",
   "DISK_TEMPLATE_OPT",
   "DRAINED_OPT",
   "DRY_RUN_OPT",
@@ -752,6 +753,11 @@ HVOPTS_OPT = cli_option("-H", "--hypervisor-parameters", 
type="keyval",
 default={}, dest="hvparams",
 help="Hypervisor parameters")
 
+DISK_PARAMS_OPT = cli_option("-D", "--disk-parameters", dest="diskparams",
+ help="Disk template parameters, in the format"
+ " template:option=value,option=value,...",
+ type="identkeyval", action="append", default=[])
+
 HYPERVISOR_OPT = cli_option("-H", "--hypervisor-parameters", dest="hypervisor",
 help="Hypervisor and hypervisor options, in the"
 " format hypervisor:option=value,option=value,...",
diff --git a/l

[PATCH master 4/5] Use disk parameters in Logical Units

[Andrea Spadaccini]

Signed-off-by: Andrea Spadaccini 
---
 lib/cmdlib.py |  111 
 1 files changed, 95 insertions(+), 16 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 8367801..ef59a34 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8009,24 +8009,59 @@ def _GenerateUniqueNames(lu, exts):
   return results
 
 
+def _ComputeLDParams(disk_template, disk_params):
+  """Computes Logical Disk parameters from Disk Template parameters.
+
+  @type disk_template: string
+  @param disk_template: disk template, one of L{constants.DISK_TEMPLATES}
+  @type disk_params: dict
+  @param disk_params: disk template parameters; dict(template_name -> 
parameters
+  @rtype: list(dict)
+  @return: a list of dicts, one for each node of the disk hierarchy. Each dict
+contains the LD parameters of the node. The tree is flattened in-order.
+
+  """
+  if disk_template not in constants.DISK_TEMPLATES:
+raise errors.ProgrammerError("Unknown disk template %s" % disk_template)
+
+  result = list()
+  if disk_template == constants.DT_DRBD8:
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_DRBD8])
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+  elif (disk_template == constants.DT_FILE or
+disk_template == constants.DT_SHARED_FILE):
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_FILE])
+  elif disk_template == constants.DT_PLAIN:
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+  elif disk_template == constants.DT_BLOCK:
+result.append(constants.DISK_LD_DEFAULTS[constants.LD_BLOCKDEV])
+
+  return result
+
+
 def _GenerateDRBD8Branch(lu, primary, secondary, size, vgnames, names,
- iv_name, p_minor, s_minor):
+ iv_name, p_minor, s_minor, ld_params):
   """Generate a drbd8 device complete with its children.
 
   """
   assert len(vgnames) == len(names) == 2
   port = lu.cfg.AllocatePort()
   shared_secret = lu.cfg.GenerateDRBDSecret(lu.proc.GetECId())
+  drbd_params, data_params, meta_params = ld_params
+
   dev_data = objects.Disk(dev_type=constants.LD_LV, size=size,
-  logical_id=(vgnames[0], names[0]))
+  logical_id=(vgnames[0], names[0]),
+  params=data_params)
   dev_meta = objects.Disk(dev_type=constants.LD_LV, size=DRBD_META_SIZE,
-  logical_id=(vgnames[1], names[1]))
+  logical_id=(vgnames[1], names[1]),
+  params=meta_params)
   drbd_dev = objects.Disk(dev_type=constants.LD_DRBD8, size=size,
   logical_id=(primary, secondary, port,
   p_minor, s_minor,
   shared_secret),
   children=[dev_data, dev_meta],
-  iv_name=iv_name)
+  iv_name=iv_name, params=drbd_params)
   return drbd_dev
 
 
@@ -8034,7 +8069,7 @@ def _GenerateDiskTemplate(lu, template_name,
   instance_name, primary_node,
   secondary_nodes, disk_info,
   file_storage_dir, file_driver,
-  base_index, feedback_fn):
+  base_index, feedback_fn, disk_params):
   """Generate the entire disk layout for a given template type.
 
   """
@@ -8043,6 +8078,7 @@ def _GenerateDiskTemplate(lu, template_name,
   vgname = lu.cfg.GetVGName()
   disk_count = len(disk_info)
   disks = []
+  ld_params = _ComputeLDParams(template_name, disk_params)
   if template_name == constants.DT_DISKLESS:
 pass
   elif template_name == constants.DT_PLAIN:
@@ -8059,7 +8095,8 @@ def _GenerateDiskTemplate(lu, template_name,
   size=disk[constants.IDISK_SIZE],
   logical_id=(vg, names[idx]),
   iv_name="disk/%d" % disk_index,
-  mode=disk[constants.IDISK_MODE])
+  mode=disk[constants.IDISK_MODE],
+  params=ld_params[0])
   disks.append(disk_dev)
   elif template_name == constants.DT_DRBD8:
 if len(secondary_nodes) != 1:
@@ -8082,7 +8119,8 @@ def _GenerateDiskTemplate(lu, template_name,
   [data_vg, meta_vg],
   names[idx * 2:idx * 2 + 2],
   "disk/%d" % disk_index,
-  minors[idx * 2], minors[idx * 2 + 1])
+  minors[idx * 2], minors[idx * 2 + 1],
+  ld_params)
   disk_dev.mode = disk[constants.IDISK_MODE]
   disks.app

[PATCH master 3/5] Use disk parameters in noded

[Andrea Spadaccini]

* add the params attribute to BlockDev, and add the corresponding
  parameter to all the BlockDev classes;
* change the Create, Assemble and FindDevice factory functions interface
  to accept as parameters an objects.Disk instance and a list of
children block devices; update their callers;
* make the factory functions provide default values for params if
  needed;
* factor out a check in the block device factory functions.

Signed-off-by: Andrea Spadaccini 
---
 lib/backend.py |6 ++--
 lib/bdev.py|   98 ++--
 2 files changed, 70 insertions(+), 34 deletions(-)

diff --git a/lib/backend.py b/lib/backend.py
index 9fb9e21..abfd199 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -1513,7 +1513,7 @@ def BlockdevCreate(disk, size, owner, on_primary, info):
   clist.append(crdev)
 
   try:
-device = bdev.Create(disk.dev_type, disk.physical_id, clist, disk.size)
+device = bdev.Create(disk, clist)
   except errors.BlockDeviceError, err:
 _Fail("Can't create block device: %s", err)
 
@@ -1695,7 +1695,7 @@ def _RecursiveAssembleBD(disk, owner, as_primary):
   children.append(cdev)
 
   if as_primary or disk.AssembleOnSecondary():
-r_dev = bdev.Assemble(disk.dev_type, disk.physical_id, children, disk.size)
+r_dev = bdev.Assemble(disk, children)
 result = r_dev
 if as_primary or disk.OpenOnSecondary():
   r_dev.Open()
@@ -1888,7 +1888,7 @@ def _RecursiveFindBD(disk):
 for chdisk in disk.children:
   children.append(_RecursiveFindBD(chdisk))
 
-  return bdev.FindDevice(disk.dev_type, disk.physical_id, children, disk.size)
+  return bdev.FindDevice(disk, children)
 
 
 def _OpenRealBD(disk):
diff --git a/lib/bdev.py b/lib/bdev.py
index 033a4b1..9f8f7c6 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -130,7 +130,7 @@ class BlockDev(object):
   after assembly we'll have our correct major/minor.
 
   """
-  def __init__(self, unique_id, children, size):
+  def __init__(self, unique_id, children, size, params):
 self._children = children
 self.dev_path = None
 self.unique_id = unique_id
@@ -138,6 +138,7 @@ class BlockDev(object):
 self.minor = None
 self.attached = False
 self.size = size
+self.params = params
 
   def Assemble(self):
 """Assemble the device from its components.
@@ -166,7 +167,7 @@ class BlockDev(object):
 raise NotImplementedError
 
   @classmethod
-  def Create(cls, unique_id, children, size):
+  def Create(cls, unique_id, children, size, params):
 """Create the device.
 
 If the device cannot be created, it will return None
@@ -373,13 +374,13 @@ class LogicalVolume(BlockDev):
   _INVALID_NAMES = frozenset([".", "..", "snapshot", "pvmove"])
   _INVALID_SUBSTRINGS = frozenset(["_mlog", "_mimage"])
 
-  def __init__(self, unique_id, children, size):
+  def __init__(self, unique_id, children, size, params):
 """Attaches to a LV device.
 
 The unique_id is a tuple (vg_name, lv_name)
 
 """
-super(LogicalVolume, self).__init__(unique_id, children, size)
+super(LogicalVolume, self).__init__(unique_id, children, size, params)
 if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
   raise ValueError("Invalid configuration data %s" % str(unique_id))
 self._vg_name, self._lv_name = unique_id
@@ -391,7 +392,7 @@ class LogicalVolume(BlockDev):
 self.Attach()
 
   @classmethod
-  def Create(cls, unique_id, children, size):
+  def Create(cls, unique_id, children, size, params):
 """Create a new logical volume.
 
 """
@@ -433,7 +434,7 @@ class LogicalVolume(BlockDev):
 if result.failed:
   _ThrowError("LV create failed (%s): %s",
   result.fail_reason, result.output)
-return LogicalVolume(unique_id, children, size)
+return LogicalVolume(unique_id, children, size, params)
 
   @staticmethod
   def _GetVolumeInfo(lvm_cmd, fields):
@@ -718,7 +719,7 @@ class LogicalVolume(BlockDev):
 snap_name = self._lv_name + ".snap"
 
 # remove existing snapshot if found
-snap = LogicalVolume((self._vg_name, snap_name), None, size)
+snap = LogicalVolume((self._vg_name, snap_name), None, size, self.params)
 _IgnoreError(snap.Remove)
 
 vg_info = self.GetVGInfo([self._vg_name])
@@ -1098,7 +1099,7 @@ class DRBD8(BaseDRBD):
   # timeout constants
   _NET_RECONFIG_TIMEOUT = 60
 
-  def __init__(self, unique_id, children, size):
+  def __init__(self, unique_id, children, size, params):
 if children and children.count(None) > 0:
   children = []
 if len(children) not in (0, 2):
@@ -1112,7 +1113,7 @@ class DRBD8(BaseDRBD):
   if not _CanReadDevice(children[1].dev_path):
 logging.info("drbd%s: Ignoring unreadable meta devic

[PATCH master 5/5] Add DRBD8 static resync speed disk parameter

[Andrea Spadaccini]

Signed-off-by: Andrea Spadaccini 
---
 lib/bdev.py  |   15 ---
 lib/cmdlib.py|   11 ++-
 lib/constants.py |7 ++-
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/lib/bdev.py b/lib/bdev.py
index 9f8f7c6..7856d1c 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1373,8 +1373,7 @@ class DRBD8(BaseDRBD):
 if result.failed:
   _ThrowError("drbd%d: can't attach local disk: %s", minor, result.output)
 
-  @classmethod
-  def _AssembleNet(cls, minor, net_info, protocol,
+  def _AssembleNet(self, minor, net_info, protocol,
dual_pri=False, hmac=None, secret=None):
 """Configure the network part of the device.
 
@@ -1383,7 +1382,7 @@ class DRBD8(BaseDRBD):
 if None in net_info:
   # we don't want network connection and actually want to make
   # sure its shutdown
-  cls._ShutdownNet(minor)
+  self._ShutdownNet(minor)
   return
 
 # Workaround for a race condition. When DRBD is doing its dance to
@@ -1392,7 +1391,8 @@ class DRBD8(BaseDRBD):
 # sync speed only after setting up both sides can race with DRBD
 # connecting, hence we set it here before telling DRBD anything
 # about its peer.
-cls._SetMinorSyncSpeed(minor, constants.SYNC_SPEED)
+sync_speed = self.params.get(constants.RESYNC_RATE)
+self._SetMinorSyncSpeed(minor, sync_speed)
 
 if netutils.IP6Address.IsValid(lhost):
   if not netutils.IP6Address.IsValid(rhost):
@@ -1407,7 +1407,7 @@ class DRBD8(BaseDRBD):
 else:
   _ThrowError("drbd%d: Invalid ip %s" % (minor, lhost))
 
-args = ["drbdsetup", cls._DevPath(minor), "net",
+args = ["drbdsetup", self._DevPath(minor), "net",
 "%s:%s:%s" % (family, lhost, lport),
 "%s:%s:%s" % (family, rhost, rport), protocol,
 "-A", "discard-zero-changes",
@@ -1424,7 +1424,7 @@ class DRBD8(BaseDRBD):
   minor, result.fail_reason, result.output)
 
 def _CheckNetworkConfig():
-  info = cls._GetDevInfo(cls._GetShowData(minor))
+  info = self._GetDevInfo(self._GetShowData(minor))
   if not "local_addr" in info or not "remote_addr" in info:
 raise utils.RetryAgain()
 
@@ -1758,7 +1758,8 @@ class DRBD8(BaseDRBD):
   # the device
   self._SlowAssemble()
 
-self.SetSyncSpeed(constants.SYNC_SPEED)
+sync_speed = self.params.get(constants.RESYNC_RATE)
+self.SetSyncSpeed(sync_speed)
 
   def _SlowAssemble(self):
 """Assembles the DRBD device from a (partially) configured device.
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ef59a34..b000b2e 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8025,10 +8025,19 @@ def _ComputeLDParams(disk_template, disk_params):
 raise errors.ProgrammerError("Unknown disk template %s" % disk_template)
 
   result = list()
+  dt_params = disk_params[disk_template]
   if disk_template == constants.DT_DRBD8:
-result.append(constants.DISK_LD_DEFAULTS[constants.LD_DRBD8])
+params = {
+  constants.DRBD_RESYNC_RATE: dt_params[constants.RESYNC_RATE]
+  }
+
+drbd_params = \
+  objects.FillDict(constants.DISK_LD_DEFAULTS[constants.LD_DRBD8], params)
+
+result.append(drbd_params)
 result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
 result.append(constants.DISK_LD_DEFAULTS[constants.LD_LV])
+
   elif (disk_template == constants.DT_FILE or
 disk_template == constants.DT_SHARED_FILE):
 result.append(constants.DISK_LD_DEFAULTS[constants.LD_FILE])
diff --git a/lib/constants.py b/lib/constants.py
index c1adee8..01ddce5 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -590,7 +590,6 @@ MAX_TAGS_PER_OBJ = 4096
 
 # others
 DEFAULT_BRIDGE = "xen-br0"
-SYNC_SPEED = 60 * 1024
 IP4_ADDRESS_LOCALHOST = "127.0.0.1"
 IP4_ADDRESS_ANY = "0.0.0.0"
 IP6_ADDRESS_LOCALHOST = "::1"
@@ -882,12 +881,16 @@ NDS_PARAMETER_TYPES = {
 NDS_PARAMETERS = frozenset(NDS_PARAMETER_TYPES.keys())
 
 # Logical Disks parameters
+RESYNC_RATE = "resync-rate"
 DISK_LD_TYPES = {
+  RESYNC_RATE: VTYPE_STRING,
   }
 DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys())
 
 # Disk template parameters
+DRBD_RESYNC_RATE = "resync-rate"
 DISK_DT_TYPES = {
+  DRBD_RESYNC_RATE: VTYPE_INT,
   }
 
 DISK_DT_PARAMETERS = frozenset(DISK_DT_TYPES.keys())
@@ -1645,6 +1648,7 @@ NDC_DEFAULTS = {
 
 DISK_LD_DEFAULTS = {
   LD_DRBD8: {
+RESYNC_RATE: 60 * 1024  # 60 MiB, expressed in KiB
 },
   LD_LV: {
 },
@@ -1658,6 +1662,7 @@ DISK_DT_DEFAULTS = {
   DT_PLAIN: {
 },
   DT_DRBD8: {
+DRBD_RESYNC_RATE: DISK_LD_DEFAULTS[LD_DRBD8][RESYNC_RATE]
 },
   DT_DISKLESS: {
 },
-- 
1.7.3.1

[PATCH master 2/5] qa: add gnt-cluster tests related to disk params

[Andrea Spadaccini]

Signed-off-by: Andrea Spadaccini 
---
 qa/qa_cluster.py |   19 ++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py
index a5e4b3f..d0d1139 100644
--- a/qa/qa_cluster.py
+++ b/qa/qa_cluster.py
@@ -60,6 +60,11 @@ def _CheckFileOnAllNodes(filename, content):
 
 def TestClusterInit(rapi_user, rapi_secret):
   """gnt-cluster init"""
+  # data for testing failures due to bad keys/values for disk parameters
+  fail_params = ("-D nonexistent:resync-rate=1",
+ "-D drbd:nonexistent=1",
+ "-D drbd:resync-rate=invalid")
+
   master = qa_config.GetMasterNode()
 
   rapi_dir = os.path.dirname(constants.RAPI_USERS_FILE)
@@ -97,11 +102,23 @@ def TestClusterInit(rapi_user, rapi_secret):
   if htype:
 cmd.append("--enabled-hypervisors=%s" % htype)
 
-  cmd.append(qa_config.get("name"))
+  # test gnt-cluster init failures due to bad keys/values in disk parameters
+  for param in fail_params:
+cmd.extend([param, qa_config.get("name")])
+AssertCommand(cmd, fail=True)
+cmd.pop()
+cmd.pop()
 
+  cmd.append(qa_config.get("name"))
   AssertCommand(cmd)
 
   cmd = ["gnt-cluster", "modify"]
+  # test gnt-cluster modify failures due to bad keys/values in disk parameters
+  for param in fail_params:
+cmd.append(param)
+AssertCommand(cmd, fail=True)
+cmd.pop()
+
   # hypervisor parameter modifications
   hvp = qa_config.get("hypervisor-parameters", {})
   for k, v in hvp.items():
-- 
1.7.3.1

[PATCH master 0/5] Support for disk parameters

[Andrea Spadaccini]

This patch series introduces the disk parameters support as per the new resource
model design doc. Only one parameter is implemented, because I first want the
structure itself to be validated by reviews. Documentation will follow.

Andrea Spadaccini (5):
  Add basic support for disk parameters
  qa: add gnt-cluster tests related to disk params
  Use disk parameters in noded
  Use disk parameters in Logical Units
  Add DRBD8 static resync speed disk parameter

 lib/backend.py|6 +-
 lib/bdev.py   |  113 +---
 lib/bootstrap.py  |   23 +--
 lib/cli.py|6 ++
 lib/client/gnt_cluster.py |   33 -
 lib/client/gnt_group.py   |   17 --
 lib/cmdlib.py |  161 -
 lib/constants.py  |   44 -
 lib/objects.py|   26 +++-
 lib/opcodes.py|   10 +++
 qa/qa_cluster.py  |   19 +-
 11 files changed, 384 insertions(+), 74 deletions(-)

-- 
1.7.3.1

Re: [PATCH master] Set DRBD sync speed in DRBD8.Assemble

[Andrea Spadaccini]

Hi Iustin,

>> > There was a reason why we set the sync speed twice (look at the
>> > history). Is the behaviour preserved?
>>
>> Yes, it is.
>
> Actually, just found another use of set sync speed, but with a different
> name :((
>
> Beside the explicit calls in backend.py, we have one in bdev.py, look
> for 'Workaround for a race condition' and cls._SetMinorSyncSpeed.
>
> Please take a look at that and see if we need to integrate it - I don't
> think it's broken as it is now, but it will certainly ruin your future
> patches, as it uses a static constant.

Yes, I already saw that. In my code, the DRBD8._AssembleNet method has
already been converted to an instance method and uses the disk
parameter for _SetMinorSyncSpeed

Thanks,
Andrea

[PATCH master] design-resource-model: update disk params section

[Andrea Spadaccini]

Simplify design by moving all the parameters to disk template level,
explaining why this is sub-optimal. Add notes about DRBD versions,
corner cases and parameters application time.

Signed-off-by: Andrea Spadaccini 
---
 doc/design-resource-model.rst |   60 ++--
 1 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/doc/design-resource-model.rst b/doc/design-resource-model.rst
index 73fbd28..8201ed6 100644
--- a/doc/design-resource-model.rst
+++ b/doc/design-resource-model.rst
@@ -688,75 +688,81 @@ I/O performance for internal storage.
 Disk parameters
 ~~~
 
-The propose model for new disk parameters is a simple free-form one
-based on dictionaries, indexed per disk level (template or logical disk)
-and type (which depends on the level). At JSON level, since the object
-key has to be a string, we can encode the keys via a separator
-(e.g. slash), or by having two dict levels.
+The proposed model for the new disk parameters is a simple free-form one
+based on dictionaries, indexed per disk template and parameter name.
+Only the disk template parameters are visible to the user, and those are
+internally translated to logical disk level parameters.
+
+This is a simplification, because each parameter is applied to a whole
+nested structure and there is no way of fine-tuning each level's
+parameters, but it is good enough for the current parameter set. This
+model could need to be expanded, e.g., if support for three-nodes stacked
+DRBD setups is added to Ganeti.
+
+At JSON level, since the object key has to be a string, the keys can be
+encoded via a separator (e.g. slash), or by having two dict levels.
 
 ++-+-+-+--+
 |Disk|Name |Description  |Current status   |Type  |
 |template| | | |  |
 ++=+=+=+==+
-|dt/plain|stripes  |How many stripes to use  |Configured at|int   |
+|plain   |stripes  |How many stripes to use  |Configured at|int   |
 || |for newly created (plain)|./configure time, not|  |
 || |logical voumes   |overridable at   |  |
 || | |runtime  |  |
 ++-+-+-+--+
-|dt/drdb |stripes  |How many stripes to use  |Same as for lvm  |int   |
+|drbd|stripes  |How many stripes to use  |Same as for plain|int   |
 || |for data volumes | |  |
 ++-+-+-+--+
-|dt/drbd |metavg   |Default volume group for |Same as the main |string|
+|drbd|metavg   |Default volume group for |Same as the main |string|
 || |the metadata LVs |volume group,|  |
 || | |overridable via  |  |
 || | |'metavg' key |  |
-|| | | |  |
 ++-+-+-+--+
-|dt/drbd |metastripes  |How many stripes to use  |Same as for lvm  |int   |
+|drbd|metastripes  |How many stripes to use  |Same as for lvm  |int   |
 || |for meta volumes |'stripes', suboptimal|  |
 || | |as the meta LVs are  |  |
 || | |small|  |
 ++-+-+-+--+
-|ld/drbd8|disk_barriers|What kind of barriers to |Either all enabled or|string|
+|drbd|disk_barriers|What kind of barriers to |Either all enabled or|string|
 || |*disable* for disks; |all disabled, per|  |
 || |either "n" or a string   |./configure time |  |
 || |containing a subset of   |option   |  |
 || |"bfd"| |  |
 ++-+-+-+--+
-|ld/drbd8|meta_barriers|Whether barriers are |Handled together with|bool  |
+|drbd|meta_barriers|Whether barriers are |Handled together with|bool  |
 || |enabled or not for the   |disk_barriers|  |
 || |meta volume  | |  |
-|| | | |  |
 ++-+-+-+--+
-|ld/drbd8|resync_

Re: [PATCH master] Set DRBD sync speed in DRBD8.Assemble

[Andrea Spadaccini]

Hi Michael,

>> Instead of relying on clients of the class for setting the device speed
>> (and, in general, the DRBD parameters), move this responsibility inside
>> the Assemble method.
>
> There was a reason why we set the sync speed twice (look at the
> history). Is the behaviour preserved?

Yes, it is.

Thanks,
Andrea

[PATCH master] Set DRBD sync speed in DRBD8.Assemble

[Andrea Spadaccini]

Instead of relying on clients of the class for setting the device speed
(and, in general, the DRBD parameters), move this responsibility inside
the Assemble method.

Signed-off-by: Andrea Spadaccini 
---
 lib/backend.py |2 --
 lib/bdev.py|3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/backend.py b/lib/backend.py
index e9539a4..9fb9e21 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -1522,7 +1522,6 @@ def BlockdevCreate(disk, size, owner, on_primary, info):
   device.Assemble()
 except errors.BlockDeviceError, err:
   _Fail("Can't assemble device after creation, unusual event: %s", err)
-device.SetSyncSpeed(constants.SYNC_SPEED)
 if on_primary or disk.OpenOnSecondary():
   try:
 device.Open(force=True)
@@ -1697,7 +1696,6 @@ def _RecursiveAssembleBD(disk, owner, as_primary):
 
   if as_primary or disk.AssembleOnSecondary():
 r_dev = bdev.Assemble(disk.dev_type, disk.physical_id, children, disk.size)
-r_dev.SetSyncSpeed(constants.SYNC_SPEED)
 result = r_dev
 if as_primary or disk.OpenOnSecondary():
   r_dev.Open()
diff --git a/lib/bdev.py b/lib/bdev.py
index d8603df..033a4b1 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -1743,6 +1743,7 @@ class DRBD8(BaseDRBD):
   - if we have a configured device, we try to ensure that it matches
 our config
   - if not, we create it from zero
+  - anyway, set the device parameters
 
 """
 super(DRBD8, self).Assemble()
@@ -1756,6 +1757,8 @@ class DRBD8(BaseDRBD):
   # the device
   self._SlowAssemble()
 
+self.SetSyncSpeed(constants.SYNC_SPEED)
+
   def _SlowAssemble(self):
 """Assembles the DRBD device from a (partially) configured device.
 
-- 
1.7.3.1

Re: [PATCH master] Fix QA breakage caused by merge 0e82dcf9

[Andrea Spadaccini]

> Patch tested and confirmed to work by Andrea Spadaccini
> .

LGTM

[PATCH master] Reapply commit 2a6de57 after merge

[Andrea Spadaccini]

In the last merge I erroneously discarded the changes introduced by
commit 2a6de57 "Check the results of master IP RPCs". This commit
reintroduces them.

Signed-off-by: Andrea Spadaccini 
---
 lib/cmdlib.py |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index fa1b3d1..c2d8fc9 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -3983,8 +3983,9 @@ class LUClusterActivateMasterIp(NoHooksLU):
 """
 master_params = self.cfg.GetMasterNetworkParameters()
 ems = self.cfg.GetUseExternalMipScript()
-self.rpc.call_node_activate_master_ip(master_params.name,
-  master_params, ems)
+result = self.rpc.call_node_activate_master_ip(master_params.name,
+   master_params, ems)
+result.Raise("Could not activate the master IP")
 
 
 class LUClusterDeactivateMasterIp(NoHooksLU):
@@ -3997,8 +3998,9 @@ class LUClusterDeactivateMasterIp(NoHooksLU):
 """
 master_params = self.cfg.GetMasterNetworkParameters()
 ems = self.cfg.GetUseExternalMipScript()
-self.rpc.call_node_deactivate_master_ip(master_params.name, master_params,
-ems)
+result = self.rpc.call_node_deactivate_master_ip(master_params.name,
+ master_params, ems)
+result.Raise("Could not deactivate the master IP")
 
 
 def _WaitForSync(lu, instance, disks=None, oneshot=False):
-- 
1.7.3.1

[MERGE] devel-2.5 into master

[Andrea Spadaccini]

commit bbaf40480cdffa200cc7e1de578655ad88fa4d06
Merge: edc282a 0c1441e
Author: Andrea Spadaccini 
Date:   Fri Nov 18 11:27:16 2011 +

Merge branch 'devel-2.5'

* devel-2.5: (24 commits)
  LUInstanceCreate: Release unused node locks
  htools: rework message display construction
  hbal: handle empty node groups
  Document OpNodeMigrate's result for RAPI
  Ensure unused ports return to the free port pool
  Re-wrap a paragraph to eliminate a sphinx warning
  Fix newer pylint's E0611 error in compat.py
  Fail if node/group evacuation can't evacuate instances
  Update init script description
  LUInstanceRename: Compare name with name
  LUClusterRepairDiskSizes: Acquire instance locks in exclusive mode
  Update synopsis for “gnt-cluster repair-disk-sizes”
  Move hooks PATH environment variable to constants
  Check the results of master IP RPCs
  Add documentation for the master IP hooks
  Add master IP turnup and turndown hooks
  Add RunLocalHooks decorator
  Generalize HooksMaster
  Update NEWS for 2.5.0~rc4
  Bump version to 2.5.0~rc4
  ...

Conflicts:
NEWS
doc/hooks.rst
lib/backend.py
lib/cmdlib.py
lib/constants.py

diff --cc NEWS
index 98627bc,1e1cd6c..13d1cb8
--- a/NEWS
+++ b/NEWS
@@@ -1,18 -1,10 +1,18 @@@
  News
  
  
 +Version 2.6.0 beta 1
 +
 +
 +*(unreleased)*
 +
 +- Deprecated ``admin_up`` field. Instead, ``admin_state`` is introduced,
 +  with 3 possible values -- ``up``,``down`` and ``offline``.
 +
- Version 2.5.0 rc2
+ Version 2.5.0 rc4
  -
  
- *(Released Tue, 18 Oct 2011)*
+ *(Released Thu, 27 Oct 2011)*
  
  Incompatible/important changes and bugfixes
  ~~~
diff --cc htools/Ganeti/HTools/Program/Hbal.hs
index b881033,77d3834..9dc5ad1
--- a/htools/Ganeti/HTools/Program/Hbal.hs
+++ b/htools/Ganeti/HTools/Program/Hbal.hs
@@@ -280,47 -308,22 +280,46 @@@ selectGroup opts gl nlf ilf = d
Just grp ->
case lookup (Group.idx grp) ngroups of
  Nothing -> do
-   -- TODO: while this is unlikely to happen, log here the
-   -- actual group data to help debugging
-   hPutStrLn stderr "Internal failure, missing group idx"
-   exitWith $ ExitFailure 1
+   -- This will only happen if there are no nodes assigned
+   -- to this group
+   return (Group.name grp, (Container.empty, Container.empty))
  Just cdata -> return (Group.name grp, cdata)
  
 -  unless oneline $ printf "Group size %d nodes, %d instances\n"
 - (Container.size nl)
 - (Container.size il)
 +-- | Do a few checks on the cluster data.
 +checkCluster :: Int -> Node.List -> Instance.List -> IO ()
 +checkCluster verbose nl il = do
 +  -- nothing to do on an empty cluster
 +  when (Container.null il) $ do
 + printf "Cluster is empty, exiting.\n"::IO ()
 + exitWith ExitSuccess
  
 -  putStrLn $ "Selected node group: " ++ gname
 +  -- hbal doesn't currently handle split clusters
 +  let split_insts = Cluster.findSplitInstances nl il
 +  unless (null split_insts) $ do
 +hPutStrLn stderr "Found instances belonging to multiple node groups:"
 +mapM_ (\i -> hPutStrLn stderr $ "  " ++ Instance.name i) split_insts
 +hPutStrLn stderr "Aborting."
 +exitWith $ ExitFailure 1
 +
 +  printf "Loaded %d nodes, %d instances\n"
 + (Container.size nl)
 + (Container.size il)::IO ()
  
 -  when (length csf > 0 && not oneline && verbose > 1) $
 +  let csf = commonSuffix nl il
 +  when (not (null csf) && verbose > 1) $
 printf "Note: Stripping common suffix of '%s' from names\n" csf
  
 +-- | Do a few checks on the selected group data.
 +checkGroup :: Int -> String -> Node.List -> Instance.List -> IO ()
 +checkGroup verbose gname nl il = do
 +  printf "Group size %d nodes, %d instances\n"
 + (Container.size nl)
 + (Container.size il)::IO ()
 +
 +  putStrLn $ "Selected node group: " ++ gname
 +
let (bad_nodes, bad_instances) = Cluster.computeBadItems nl il
 -  unless (oneline || verbose == 0) $ printf
 +  unless (verbose == 0) $ printf
   "Initial check done: %d bad nodes, %d bad instances.\n"
   (length bad_nodes) (length bad_instances)
  
diff --cc lib/cmdlib.py
index dd62176,4297b8a..fa1b3d1
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@@ -3240,14 -3184,17 +3240,17 @@@ class LUClusterRepairDiskSizes(NoHooksL
  else:
self.wanted_names = None
self.needed_locks = {
 -locking.LEVEL_NODE: locking.ALL_SET,
 +locking.LEVEL_NODE_RES: lock

Re: [PATCH master 4/6] Update cluster verify to check IP address scripts

[Andrea Spadaccini]

Hi Iustin,

>> --- a/lib/cmdlib.py
>> +++ b/lib/cmdlib.py
>> @@ -3825,16 +3824,16 @@ def _ComputeAncillaryFiles(cluster, redist):
>>
>>    # Files which should only be on master candidates
>>    files_mc = set()
>> +
>>    if not redist:
>>      files_mc.add(constants.CLUSTER_CONF_FILE)
>> +    files_mc.add(constants.DEFAULT_MASTER_SETUP_SCRIPT)
>
> Hmm, hmm. Bad Ganeti.
[…]
> LGTM, but please add a "FIXME: this should also be replicated but Ganeti
> doesn't support files_mc replication".

Ok, thanks.
Andrea

Re: [PATCH master 4/6] Update cluster verify to check IP address scripts

[Andrea Spadaccini]

Hi Iustin,

>> @@ -3807,6 +3833,8 @@ def _ComputeAncillaryFiles(cluster, redist):
>>      for hv_name in cluster.enabled_hypervisors
>>      for filename in 
>> hypervisor.GetHypervisor(hv_name).GetAncillaryFiles()[0])
>>
>> +  files_vm.add(constants.DEFAULT_MASTER_SETUP_SCRIPT)
>
> I think this is wrong. The master setup script will only execute on
> master nodes, so this should be file_mc, not files_vm.

Here is the interdiff for this change:

--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -3825,16 +3824,16 @@ def _ComputeAncillaryFiles(cluster, redist):

   # Files which should only be on master candidates
   files_mc = set()
+
   if not redist:
 files_mc.add(constants.CLUSTER_CONF_FILE)
+files_mc.add(constants.DEFAULT_MASTER_SETUP_SCRIPT)

   # Files which should only be on VM-capable nodes
   files_vm = set(filename
 for hv_name in cluster.enabled_hypervisors
 for filename in hypervisor.GetHypervisor(hv_name).GetAncillaryFiles()[0])

-  files_vm.add(constants.DEFAULT_MASTER_SETUP_SCRIPT)

Re: [PATCH master 6/6] Use master IP address setup script in backend

[Andrea Spadaccini]

Hi Iustin,

> LGTM, except:
>
>> +  setup_script = constants.DEFAULT_MASTER_SETUP_SCRIPT
>> +  if use_external_mip_setup_script:
>> +    setup_script = constants.EXTERNAL_MASTER_SETUP_SCRIPT
>
> Please use if… else, since that's more appropriate here.

Will do.

Thanks,
Andrea

Re: [PATCH master 4/6] Update cluster verify to check IP address scripts

[Andrea Spadaccini]

Hi Iustin,

>> @@ -3807,6 +3833,8 @@ def _ComputeAncillaryFiles(cluster, redist):
>>      for hv_name in cluster.enabled_hypervisors
>>      for filename in 
>> hypervisor.GetHypervisor(hv_name).GetAncillaryFiles()[0])
>>
>> +  files_vm.add(constants.DEFAULT_MASTER_SETUP_SCRIPT)
>
> I think this is wrong. The master setup script will only execute on
> master nodes, so this should be file_mc, not files_vm.

Totally right, will change it with files_mc.

Thanks,
Andrea

Re: [PATCH master 3/6] Add --use-external-mip-setup-script flag

[Andrea Spadaccini]

Hi Iustin,

> LGTM with one remark:
>
>> +The ``--use-external-mip-setup-script`` options allows to specify
>> +whether to use an user-supplied master IP address setup script, whose
>> +default location is ``/etc/ganeti/scripts/master-ip-setup``. If
>> +the option value is set to False, the default script, whose default
>> +location is ``/usr/local/lib/ganeti/tools/master-ip-setup``, will be
>> +executed.
>
> I'm not sure whether "default" (in both sentences) is correct - there's
> no way to override the path, so maybe just "whose location is" ?

Yes, I will to drop "default". I had probably in mind the old patches
while I wrote it.

Thanks,
Andrea

Re: [PATCH master] check-python-code: Detect old-style pylint disable-msg lines

[Andrea Spadaccini]

Hi Michael,

> I have added LC_ALL=C, see interdiff below.
[…]
> Changed to “Commit b459a848d was supposed to replace all, but one was
> missed. Add a check to autotools/check-python-code.“.

LGTM

Re: [PATCH master] check-python-code: Detect old-style pylint disable-msg lines

[Andrea Spadaccini]

Hi Michael,

> +  if grep -n -H -E -i \

Do you really need -i for this check?

Rest LGTM, though IMO the commit message should detail both actions
(change the missed directive AND add the check).

Andrea

Re: [PATCH master 1/6] Add the default master-ip-setup script

[Andrea Spadaccini]

Interdiff that uses fping and removes the last explicit check for the
ip command's return value, as requested by Michael in the other
thread:

diff --git a/INSTALL b/INSTALL
index 1408ea1..960abca 100644
--- a/INSTALL
+++ b/INSTALL
@@ -44,6 +44,7 @@ Before installing, please verify that you have the
following programs:
 - `ElementTree Python module `_,
   if running python 2.4 (optional, used by ovfconverter tool)
 - `qemu-img `_, if you want to use ovfconverter
+- `fping `_

 These programs are supplied as part of most Linux distributions, so
 usually they can be installed via the standard package manager. Also
diff --git a/tools/master-ip-setup b/tools/master-ip-setup
index 98b1b06..bc3d9b0 100755
--- a/tools/master-ip-setup
+++ b/tools/master-ip-setup
@@ -39,19 +39,19 @@ start() {
   esac

   # Check if the master IP address is already configured on this machine
-  if ip addr show | grep -qw $MASTER_IP/$MASTER_NETMASK; then
+  if fping -S 127.0.0.1 $MASTER_IP >/dev/null 2>&1; then
 echo "Master IP address already configured on this machine. Doing nothing."
 exit 0
   fi

-  # Check if the master IP is already configured on another machine
-  if ping $MASTER_IP -w 2 >/dev/null; then
+  # Check if the master IP address is already configured on another machine
+  if fping $MASTER_IP >/dev/null 2>&1; then
 echo "Error: master IP address configured on another machine." >&2
 exit 1
   fi

-  ip addr add $MASTER_IP/$MASTER_NETMASK dev $MASTER_NETDEV label
$MASTER_NETDEV:0
-  if [[ $? != 0 ]]; then
+  if ! ip addr add $MASTER_IP/$MASTER_NETMASK \
+ dev $MASTER_NETDEV label $MASTER_NETDEV:0; then
 echo "Error during the activation of the master IP address" >&2
 exit 1
   fi

Re: [PATCH master 1/4] Add the default master-ip-setup script

[Andrea Spadaccini]

Hi Michael,

>> Hmm.. Ganeti does not seem to depend on fping on the nodes.
>
> Not very obvious, but:
>
> $ git grep -w fping
> doc/examples/basic-oob:  if fping -q "$host" > /dev/null 2>&1; then

Yes, I found that but I wouldn't call it an explicit dependency :) It
is in doc/examples and not mentioned in the docs.

I don't think it's good to introduce this dependency in the critical
process of master IP address setup, especially because in this case
the advantage is quite small (we discard ping's output, just relying
on the return value).

 +  ip addr add $MASTER_IP/$MASTER_NETMASK dev $MASTER_NETDEV label 
 $MASTER_NETDEV:0
 +  if [[ $? != 0 ]]; then
>>>
>>> if ! ip addr …; then
>>
>> I did not change this because the line gets too long. The command
>> itself is 80 characters, and embedding it in the if makes the line
>> really hard to read. I'd prefer to keep this as is, if possible.
>
> You can wrap lines using a backslash (it's fine here), e.g.:
>
> if ! ip addr add $MASTER_IP/$MASTER_NETMASK \
>        dev $MASTER_NETDEV label $MASTER_NETDEV:0

Ok, will do that.

Does the rest LGTY?

Also, please note that I reposted this script with some of the changes
you suggested as first patch in the newer patch series.

Thanks for the review,
Andrea

Re: [PATCH master 1/6] Add the default master-ip-setup script

[Andrea Spadaccini]

Hi Guido,

>> +  # Check if the master IP address is already configured on this machine
>> +  if ip addr show | grep -qw $MASTER_IP/$MASTER_NETMASK; then
>> +    echo "Master IP address already configured on this machine. Doing 
>> nothing."
>> +    exit 0
>> +  fi
>> +
>> +  # Check if the master IP is already configured on another machine
>> +  if ping $MASTER_IP -w 2 >/dev/null; then
>> +    echo "Error: master IP address configured on another machine." >&2
>> +    exit 1
>> +  fi
>
> You could use "fping" here and above to check. (same options, only
> difference is that above you'd bind to 127.0.0.1)

I see the improvement, but I think we should not sneak in a dependence
on fping for the master IP address setup process. (unless we already
depend on it and I didn't find the dependence)

Thanks,
Andrea