As we will introduce another set of containers using the __slots__ trick
we abstract away as much as possible to separate bases classes. The
child classes then adapt them for their needs. This leads to less code
duplication.

Signed-off-by: René Nussbaumer <rn@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This function will also be needed for the iallocator refactor. Also this
function does not belong to cmdlib but rather to rpc. Lets move it
there.

Signed-off-by: René Nussbaumer <rn@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This makes it possible to better untangle the IAllocator code, which
would otherwise lead to a recursive import structure as we need those
helpers there as well.

Signed-off-by: René Nussbaumer <rn@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

Commit f037e9d7 added command line flags to filter jobs, but the
documentation was not updated. Also mention the ability to use
more complex filters.

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

We can build the test groups directly in the `testSuite' helper,
instead of doing it (much later) in the test harness.

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

Since the recent commits improved the speed of the two "slow" test
groups to regular test speed, we can remove this kludge and simplify
significantly our test runner, yay!

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

The Cluster object, as it is defined right now, has many '[String]'
members, which means that in a standard arbitrary generator these will
become very big, which is the reason for the current slowness of the
test 'Config_serialisation'.

By resizing the generator to 8 (arbitrary chosen to limit the list
length and the string sizes), and by reducing a bit the node count, we
can make this test be as fast as the others (about 10x
improvement). This means we can test more cases, for the same cost.

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

This test has a few deficiencies, which this patch addresses:

- using arbitrary 1 or 2 node count for allocation is obsolete,
  nowadays we need to use a number appropriate for the instance's disk
  template (and we should remove that parameter…)
- generating a random node is sub-optimal, since we could generate an
  offline node, and no instance will fit on a cluster composed of only
  offline nodes
- generating arbitrary instances "such that" they can be allocated is
  an expensive test; let's rather generate instances smaller than our
  template node, and add a check that they indeed can be allocated
- using boolean return type, instead of nicely annotated properties

For the nice annotation and the extra check, we need to change a
helper function's signature, so that we can extract a bit more
information out of it.

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

Currently, this test is very slow. Upon investigation, this is due to
how `tieredAlloc' works:

- tries to allocate one instance
- if failed, shrink the instance by the "most failed" resource
- restart

In this algorithm, if the "most failed" resource is e.g. memory, and
the maximum available memory is much smaller than the current
template, it means we will have to shrink and try to allocate many
many times until we finally get with the to-be-allocated instance
memory size to a reasonable value. In the real world, this is not the
case, but when testing with arbitrary memory/node values, it can be
that we execute the shrink hundreds of thousands of times per test.

So we "improve" the test by directly generating an instance just
slightly bigger than the node, so that we don't have to shrink too
many times. This requires a new export from test/…/Instance.hs.

Additionally, we allow up to 5 instances to be tiered-allocated, and
we cleanup the test checks, making the conditions much, much more
readable (IMHO).

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

This test expands the "single-alloc-no-rebalance" by allocating a few
instances on a small cluster, and ensuring that after we allocate all
of them, either we can't rebalance or if we rebalance the score
improvement is very small.

The last condition is needed because sometime rounding errors (we're
using double-precision floating point) can accumulate and result in
what is a no real change in the cluster state, but with an
infinitesimal score decrease (e.g. 1e-14).

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

Currently, this just checks that a cluster cannot be rebalanced after
a single instance allocation. However, we can also test whether the
allocation decision computed a correct new cluster score, by checking
that against the one computed from the actual new node list.

Also, for nicer display, we convert the test from a Boolean to a
Property, with nice annotations.

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

* devel-2.6:
  Fix bug in non-mirrored instance allocation
  Fix gnt-debug iallocator

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

* stable-2.6:
  Fix bug in non-mirrored instance allocation
  Fix gnt-debug iallocator

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

The function `allocateOnSingle' has a bug in the calculation of the
cluster score used for deciding which of the many target nodes to use
in placing the instance: it uses the original node list for the score
calculation.

Due to this, since the original node list is the same for all target
nodes, it means that basically `allocateOnSingle' returns the same
score, no matter the target node, and hence the choosing of the node
is arbitrary, instead of being done on the basis of the algorithm.

This has gone uncaught until reported because the unittests only test
1 allocation at a time on an empty cluster, and do not check the
consistency of the score. I'll send separate patches on the master
branch for adding more checks to prevent this in the future.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

Commit 2cdaf225, “Re-enable standard hlint warnings”, got it almost
right. The only problem is that (confusingly) the default set of hints
is not in HLint.Default, but in HLint.HLint (it includes Default and
some built-ins).

After changing the lint file to correctly include the defaults, we had
another 128 suggestions:

  - Error: Eta reduce (2)
  - Error: Redundant bracket (4)
  - Error: Redundant do (17)
  - Error: Redundant lambda (7)
  - Error: Redundant return (1)
  - Warning: Avoid lambda (2)
  - Warning: Redundant $ (42)
  - Warning: Redundant bracket (35)
  - Warning: Use : (1)
  - Warning: Use String (4)
  - Warning: Use camelCase (10)
  - Warning: Use section (3)

which are fixed by the current patch. Note that the 10 "Use camelCase"
were all due to hlint not “knowing” the idiom of ‘case_’ (it does for
‘prop_’), for which I filled
http://code.google.com/p/ndmitchell/issues/detail?id=558

.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

While investigating how we could test the Daemon.hs module, I realised
that we have a very, erm, sub-optimal situation:

- HTools/CLI.hs has a nice IO/pure separation testing in cmdline
  parsing, which allows some basic functionality to be tested, but
  uses direct 'read' in many options, which fails at runtime when
  evaluating the argument, and not when parsing the options
- Daemon.hs lacks that, but has a much nicer 'reqWithConversion'
  helper that can be used for nicer option parsing, and uses that +
  tryRead instead of plain 'read'

Since this situation is very bad, let's clean it up. We introduce yet
another module, Common.hs, that holds functionality common to all
command line programs (daemons or not). We move the parsing to this
module, and introduce a type class to handle option types which
support --help/--version. This allows removal of duplicated code from
CLI.hs and Daemon.hs.

The other part of the patch is cleanup/rework of the tests for this
code: we introduce some helpers (checkOpt, passFailOpt,
checkEarlyExit) that can be used from the much-slimmer now tests for
CLI and Daemon. In the common module, we just test the yes/no helper
we have. Many new tests for boolean options and numeric options are
added.

A side change is the removal of the obsolete `--replay-count',
`--test-size' options (unused since commit 95f6c931, “Switch Haskell
test harness to test-framework”).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Found via the newly added unit-tests, which test most of the
serialisation code in Query/Language (except for QueryResult, for
which we already tests both sub-components separately).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

These tests are node specific only because we don't have other query
types implemented yet, but what they actually test is the various
filter types.

The tests are trying to cover most filter functionality; missing for
now is proper checking for ContainsFilter and TrueFilter, the rest
should be more or less covered.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

These new tests check that:

- no known fields return unknown
- any unknown field returns unknown
- the type of the fields is consistent between the getters and the
  field definition
- the length of each result row corresponds with the number of fields
  queried, and the length of the field definitions returned
- the length of the rows corresponds to the number of nodes
- querying fields on empty fields returns all fields

Finally this patch found a bug, in that the pinst_list/sinst_list
fields were declared as QFTNumber (copy-paste error from
pinst_cnt/sinst_cnt), yay!

I also changed genEmptyCluster to ensure that it generates unique node
names, so that the number of result rows is consistent with what we
requested, and switched ResultEntry from a normal constructor to
record syntax, so that we can extract the fields without having to use
pattern matching.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This is symmetric to failTest, and allows us to use it in cases where
we need to return a property.

While replacing 'property True' with it, I saw a case where we can
simplify the use and thus reworked that check.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Using the recently-added genArbitrary, we can now implement Arbitrary
instances for even "huge" objects like Cluster, so let's use that to
implement entire ConfigData serialisation tests.

Note that, as we don't have yet proper types for some of the Params
fields, we have to cheat via FlexibleInstances and
TypeSynonymInstances, using either empty items or real arbitrary
values.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

There are a few more that could be replaces, once we start using
appropriate (new)types.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

After getting really annoyed at yet another "<*> arbitrary", I thought
that we should be able to automate this, at least for types which are
simple enough and have already all the "prerequisites".

Hence the new genArbitrary function and its helpers, which can:

- build an arbitrary for Bounded types, using the regular "elements
  [minBound..maxBound]" (20 manually defined cases right now)
- build an arbitrary instance for single-constructor data types,
  e.g. "data A = A x y z", using a simple "arbitrary = pure A <*>
  arbitrary <*> arbitrary <*> arbitrary"
- build an arbitrary instance for multi-constructor data types, using
  "arbitrary = oneof [<arbitrary for each individual construct, per
  the previous>]"

Both normal and record-based constructors are supported.

It can also build arbitrary instances for new types and type synonyms,
although for these last two I'm not so confident on the soundness of
the instances.

Note that this helper won't work for types which are not well behaved;
for example, Node has the name as String not as FQDN, so our manually
written arbitrary instance has just a few overrides as getFQDN instead
of arbitrary, so we can't automate this particular type yet; this only
means we get another push to use proper types, instead of primitive
ones, for fields which have any kinds of restrictions ("what's good
for arbitrary is good for regular code" too).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This is not perfect, as for many of the parameters we don't have good
Arbitrary instances, but is better than nothing.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

While adding yet another JSON serialisation test, I realised that this
can be trivially abstracted; hence this patch, replacing both simple
versions (readJSON . showJSON == id) and the standard version (with
different error messages) across the tests with a single function
call.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Since the DiskLogicalId type is manually serialised/deserialised (see
Objects.hs, `encodeDLid' and `decodeDLId'), let's add a test that
checks that these are idempotent when combined.

Since we're at it, let's add the same test for Node serialisation,
which already has an Arbitrary instance.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This adds test properties for the various laws that the instances of
Result should follow; I could not find (offline) laws about `mappend',
but otherwise I implemented all laws that I could find.

Note that we have to silence hlint warnings for the things we want to
test, as otherwise hlint is all "this is already true based on the
functor law, why 'fmap id' and not just 'id'?".

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

The names were not in a proper hierarchy, leading to inconsistencies
about what they were actually tested.

We change this by reproducing in the test names the relative hierarchy
within the Ganeti directory, leading to nicer test suite names (in
test-framework output).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Since we now have separate namespaces due to the multi-file split, we
don't need to keep the name of the module in the property names, as we
don't have so many potential conflicts anymore.

We remove the group prefix handling from TestHelper and simply do a
sed over all the test files, removing it from the function names.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This is another module that is generic, and not htools-specific.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

During the recent moves and renames, two things have slipped through,
since I didn't run make check-local…

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

In order to have correct code coverage results, we must somehow import
all production modules into the test runner. Until now, this was done
manually (when we didn't forget) in QC.hs.

To improve the situation, we remove QC.hs and replace it with an
auto-generated file which imports all modules. This reduces the
maintenance burden and ensures we'll always have correct coverage.

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

This currently holds both production files and test files; since we
want to treat these separately, let's split the variable in two, and
introduce another one for the old, inclusive one.

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

This makes QC.hs obsolete, but we will remove it in a later patch,
once we automatically build a file with all the imports (to ensure
correct coverage results).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

These two files are not htools-specific, so let's move them out of the
HTools subdirectory/module hierarchy and directly under Ganeti.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Except for Ganeti.HTools.JSON, which needs rename, we split all the
other test suites into separate files.

We have to add another common test helper, due to import dependencies
(sigh), but otherwise this split is pretty straightforward.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

… from QC.hs into their own files, again mirroring the production code
source tree.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This required lots of other code moves, so I created it as a
standalone patch.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This splits the confd/utils tests, and adds the TestCommon module for
shared test code.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This is the first file split out from QC.hs - an easy one, since it
has just one test.

The patch changes the way we build hpc excludes, since now we'll have
many modules that need to be excluded, and hpc doesn't seem to be able
to do wildcards (it can do packages, but all our code is
not-package-based).

Further splitting will be done in bigger batches.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>