GitLab

For the type system, we want a slightly relaxed rule for constant
naming, so we update the pylint rule. But the old _TPInt and _TNEString
were not clear enough, so we expand them.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, we have one structual validation for opcode attributes: the
_OP_REQP, which checks that a given attribute is not 'None', and the
rest of the checks are done at runtime. This means our type system has
two types: None versus Not-None.

We have been hit many times by small, trivial bugs in this area, and
only a huge amount of unittest and/or hand-written checks would ensure
that we cover all possibilities. This patch attempts to redress the
needs for manual checks by introducing a micro-type system for the
validation of the opcode attributes. What we lose, from the start, are
the custom error messages (e.g. "Invalid reboot mode, choose one of …",
or "The disk index must be a positive integer"). What we gain is the
ability to express easily things as:

- this parameter must be None or an int
- this parameter must be a non-empty list
- this parameter must be either none or a list of dictionaries with keys
  from the list of valid hypervisors and the values dictionaries with
  keys strings and values either None or strings; furthermore, the list
  must be non-empty

These examples show that we have a composable (as opposed to just a few
static types) system, and that we can nest it a few times (just for
sanity; we could nest it up to stack depth).

We also gain lots of ))))))), which is not that nice :)

The current patch moves the existing _OP_REQP to the new framework, but
if accepted, a lot more validations should move to it. In the end, we
definitely should declare a type for all the opcode parameters
(eventually moving _OP_REQP directly to opcodes.py and validating in the
load/init case, and build __slots__ from it).
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

For a few LUs, a few tests in, or even the whole CheckPrereq, can be
moved to CheckArguments, as they don't touch state and only do a 'type'
validation.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, the base class LogicalUnit's CheckPrereq will raise
NotImplementedError, which means that the child LUs have to implement
it. However, many LUs don't actually have a need for this function
(hence the many "pass" statements as the only body).

By changing the base class behaviour, we can simplify many LUs.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

All code has been switched to the new-style LU… time for cleanup.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The export mode is checked in two places with the exact same code…
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

When LogicalUnit.CheckArguments was introduced, not all code dealing
with static argument checking was moved to it; many of these checks were
left in ExpandNames. With time, most of them migrated, and this patch
does the final cleanups.

The patch is straightforward, with the exception of LURebootInstance,
where an old style ParameterError exception is converted to the new
OpPrereqError with ECODE_INVAL.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

LUExportInstance had two opcode fields set to default via both
_CheckBooleanOpField and getattr(…, False).
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

For this, we needed to extend the NodeImage class with a few extra
variables, and we do a trick in the node verification where we pick the
first node that returned valid OS data as the reference node, and then
we compare all other nodes against it.

The checks added are:

- consistency of DiagnoseOS responses
- multiple paths for an OS
- inconsistent OS between a reference node and the current node
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This patch exports all the way from backend a new field ‘api_version’
which holds the list of support API versions, and exposes the (already
computed) ‘parameters’ field.

The patch also reworks (again) the field calculation in its Exec()
method. All callers of LUDiagnoseOS pass in the 'valid' and 'variants'
parameters, thus having the special casing of whether to compute or not
the validity seems overkill. We move to a model where we always compute
these across-nodes arguments, in order to simplify the code, and we also
change the parameters set to be intersection of all node's values (which
means a change in description will drop the parameter from the list of
parameters).

Additionally, we update scripts/gnt-os, which was broken for multi-dir
OSes since the introduction of variants…
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Nothing special here, just copy/adjust the beparams code.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

We move the instance OS rename checks earlier, as we need to run the
validation against the new OS, if it has changed.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

We use _GetUpdatedParams in order to support removal too, and then
validate the OS parameters if the OS exists.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This patch adds controls for whether we recognize
constants.VALUE_DEFAULT or not as a default value, and also adds
dash-prefixes as another way for parameter removal.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This is not yet complete, as it lacks proper support for instance
import.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

While we only support the 'parameters' check today, the RPC call is
generic enough that will be able to support other checks in the future.
The backend function will both validate the parameters list (so as to
make sure we don't pass in extra parameters that the OS validation
doesn't care about) and the parameter values, via the OS verify script.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The patch also modifies the internal methods in LUDiagnoseOS and gnt-os
to deal with the format change of call_os_diagnose.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The OS parameters code will bump the number of lines over 10K, and thus
we need to silence this (no, we don't want any other module to become
this big…, so we use a targeted silence only).
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

If the repetition count is not passed or is passed as 0 we sleep exactly
one time, otherwise we sleep "repeat" times and log in between.
Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Commit cf26a87a

 added a tiny typo, which would break non-FQDN arguments
to modify node storage.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The update of the valid status in LUDiagnoseOS says:

  valid = valid and osl and osl[0][1]

However, in Python, “True and []” (which '[]' we get for an invalid OS)
will result in “[]”, and thus the valid field for an OS will be either
True or an empty list. Which is not what we want…
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, this function does three things:
- special handling of constants.VALUE_DEFAULT
- type enforcing of the resulting dict
- filling the dictionary with defaults

However, except for the first one, the second two do not belong in this
function:
- in the future, not all parameter dictionaries will be able to be
  enforced
- filling the dictionary with defaults cannot be done via a defaults
  dict in all cases, and should be done by the specialized functions
  (ideally we'd pass a partial function instance here, but we don't have
  that yet…)

As such, we remove the last items, and move them to the callers; this is
overall the same complexity, as we were calling this function in just
three places and constructing the many arguments was also complicated.

Furthermore, we move the function out of LUSetInstanceParams, as in the
future it will be used by LUSetClusterParams too.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, the existing cluster.Fill* functions take as argument an
instance. This means that in any case where we don't have an actual
instance object, we have to resort to calling the low-level
objects.FillDict function.

This is bad for two reasons:
- we have to know of, and we hardcode, the cluster object internals
  (e.g. that the nicparams are stored in a dict indexed by group)
- which can result in subtle bugs, if the underlying storage mechanisms
  change

This patch adds a lower-level implementation SimpleFillHV for FillHV and
SimpleFillBE for FillBE, and adds a completely new SimpleFillNIC (all
use cases until now hardcoded cluster.nicparams[constant.PP_DEFAULT]
directly); it then uses these new functions in cmdlib.py.

A side effect is that _CheckNicsBridgesExist loses the 'profile'
parameter, which was unused. If it's needed, we should add it later via
a proper profile parameter to SimpleFillNIC.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Since the introduction of OS-specific hvparams, we shouldn't ever use
objects.FillDict directly for instances, but instead go via the cluster
object. Otherwise the os_hvp will be ignored.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

In case an OS has inconsistent declarations, we might get into a case
where one node reports a valid variants list (with OS API >=15), and
another node has OS API < 15, in which case its supported_variants gets
the default value of None. This leads to the same variable having
inconsistent data types, which leads to subtle bugs later: instead of
reporting something like "Inconsistent OS API versions", the LU exits
with a run-time exception. Furthermore, in another datapath, variants is
initialized to '[]' in case of OS diagnose failures.

The patch changes _TryOSFromDisk to initialize variants to '[]' for
OS api level below 15, and changes the variants calculation in
DiagnoseOS to be more readable.
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This restores functionality lost in commit 387794f8

. Found during
tests using QA scripts. An instance should be started after it
has been temporarily shutdown for an export.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This should prevent bugs in our code from accidentally overwriting
disks.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The hostname and port received from the remote cluster should
be validated, just in case.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

With this change unknown disk and nic parameters will be refused, rather
than silently ignored, so that one can't pass them in by mistake and not
realize what went wrong.
Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

The various _Check* helper functions expect an lu to be passed in, but
the TL is passed instead. This works... sometimes! :)
Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently it's impossible to grow a disk if an instance is shutdown,
because the disk could not be assembled. Now we take care of assembling
it, and shutting it down after.
Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

If the disks= parameter is passed, we can assemble/wait for
sync/shutdown only some disks belonging to an instance, rather than all.

This is useful to only activate/sync/shutdown the affected disk when
growing it.
Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

The cluster domain secret file was not distributed to other nodes.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

The X509 key name and CA are passed from cmdlib all the way to
the backend import/export daemon. With the addition of an option
to choose the compression method, another parameter would have
to be passed all the way. By moving these options to a separate
object, adding new ones will become much easier.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

To prepare a remote export, the X509 key and certificate need to be generated.
A handshake value is also returned for an easier check whether both clusters
share the same cluster domain secret.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>