Merge pull request #64 from tmakatos/master

revert commits that broke the storage BST and the nightly
revert 666f0b56 (CA-122253)

This commit reverts "CA-122253: Improve error reporting for LVM-based SR
snapshots" as it hasn't been tested by a storage BST and the imminent
backout merge from trunk-storage into trunk could introduce regressions.

Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>
Reviewed-by: Germano Percossi <germano.percossi@citrix.com>

GitHub: closes #65 on xapi-project/github
CP-6697: Add "coalesce" flag to SR other-config

When coalesce=false is set, SR GC will not do any coalesce (including
leaf coalesce) on the given SR. The rational is as follows:

Some times 3rd party user (e.g. CloudStack) need to introduce an
existing storage share (e.g. NFS) as a XenServer SR. These shares
might contain some VDI files. This is all normal. However, these VDI
files might be VHD chains (unlike the VHD "trees" in XenServer
VDI as no snapshot stub node exists). So when the SR is introduced,
XenServer will coalesce the chain automatically. The senario of
VHD chains are considered valid, but the consequence is not what
users normally want.

We already had a other-config:gc flag with which the user can
disable the GC completely. But unfortunately, the basic VDI.destroy
function also depends on GC. When we delete a file or LVM based VHD,
the VHD file is not immediately deleted but only marked as hidden.
It's up to the GC to actually remove the files and do all the
cleanup steps following that. With GC completely disabled, the VHD
file will never be deleted on disk, so the space will never be
freed up.

So we introduced this coalesce flag, if set to false (and gc remains
the default true), only coalescing (including leaf-coalescing) will
be disabled and the normal prune (i.e. hidden -> delete) can still
take place.

Unfortunately, we later on found out that this is not directly
useful for the current installation of CloudStack, because when
CloudStack copies VDI snapshots to the secondary storage, it
does a literal copy using "dd" without doing re-parenting. So
that regardless what this patch does

  * GC always fails scanning their existing secondary storage
  * And if the scan had passed (or if we now let it skip the
error), since the VHD chain/tree can not be established without
correct parenting information and these VHDs all have their "hidden"
flag set to true, these VHD files would have been considered as
orphanced VHD roots which are ready for deletion.

This feature will become useful when CloudStack starts to use
the VDI.copy's new rebase mirror feature and the old installation
being converted or phased out.

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CP-6697: Add "nosubdir" flag for NFSSR

The normal NFSSR creates a sub directory under the NFS directory
given by params "server:serverpath", and use that sub directory
as the SR's working directory.

This patch set add an extra flag "sm-config:nosubdir", which
defaults to false. But when "nosubdir=true", NFSSR will avoid
creating that sub directory. Instead the SR will mount and use
the "serverpath" directory as the working directly.

The purpose of this patch is to provide a behavior compatible
with CloudStack's current NFSSR fork (which always use the
"nosubdir" semantics), so that they can move to the official
NFSSR with the help of this flag.

The reasons for choosing sm-config instead of other key/value
maps are:

  * device-config is RO map, so one can not change it's value
without migrating all VMs away in order to unplug the PBD
and recreate one . This will add considerable burden to CS's
already quite complicated upgrade process from the NFSSR fork
to the new one.

  * other-config is RW map, but it's not a param for sr-create.
So setting nosubdir during sr-create is no-op. One must then
reset the flag and unplug/plug the PBD to make it effective.

This patch also takes care of some corner case when someone
creates both a standard NFSSR and a "nosubdir" NFSSR under the
same serverpath. Ideally, they shouldn't interfere with each
other. This could be useful if CloudStack wants to gradually
moave all their current nosubdir NFSSRs to standard NFSSRs for
their primary storage.

The patch is partially based on a previous version implemented by
Anthony Xu <Xuefei.Xu@citrix.com>.

PS. the fix for an issue (CA-125137) caused by a previous version
of this patch is now integrated in this version.

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CP-6697: Fix FileSR capability config and param docs

This patch fix the outdated and incorrect configuration of
FileSR capabilities config and param doc. They are registered
by SM with XAPI, which will ultimately reflects on things like
XE tab completion and XE helps etc.

As we can see from the existing FileSR.py implementation, FileSR's
mandatory device-config param is not "path" but "location".

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CP-6697: Refactor _checkmount to base class FileSR

It's common for all its sub classes, and now we intend to use it
in the base class (FileSR) as well, so we do the refactoring by
moving it to the base class instead.

Note that util.ismount doesn't seem to be very different from the
standard os.path.ismount. It might date back to days when the
os.path.ismount doesn't exist. Here we intentionally keep it
unchanged for backward compatability, in case the code needs to
be backport to earlier releases (though quite unlikely).

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CP-6697: Modernize FileSR as an independent SR type

Currently, almost all the file-based SR types have two paths in
concept and implementation:

  * remotepath: the source path of the storage, e.g. a NFS share's
    address like server:/serverpath

  * path: the local path in /var/run/sr-mount/$sr_uuid, where
    the remotepath is mounted to

However, FileSR, the base class of all file-based SR types, doesn't
follow this convention itself. It doesn't have the  "remotepath"
varible; instead it defines the source path of the storage to be
the "path" (which should, conceptually, be the "remotepath"); it
doesn't have an entry in /var/run/sr-mount like other SRs.

This leads to two disadvantages:

  * When FileSR is used as an independent SR types, many of the
    the library code and utilities doesn't work with it, because
    they assume the existance of both variables or the entry in
    /var/run/sr-mount. E.g., SM GC assumes that all the SRs it
    works with must have a /var/run/sr-mount entry. Issues like
    this haven't been spot before, because FileSR hasn't been
    used as an indepedent SR type for a long time, also some of
    the methods that could potentially tigger these issues were
    shadowed by method overloading in the children classes.

  * This inconsistency also reduces the chance of code sharing.
    Some functions that could have been implemented in the base
    class can't be defined in the base class, because e.g. there
    is no "remotepath" variable defined or the "path" variable
    has a meaning conceptually different from the one in the
    child class. These code would then have to be duplicated into
    each children class and involve on their own, which is
    quite error prone.

A big part of this patch is to address this issue by creating
the same conceptual model for FileSR.

  * The source path of the storage which passed in as
    device-config:location (like before) is defined as the
    "remotepath"

  * There is now a /var/run/sr-mount entry, where the
    "remotepath" is bind mount onto, which is the "path". This
    is conceptually consistent with all other file-based SR types,
    and implementation-wise too.

With these changes, most of the library code will immediately
work with FileSR with no changes required on them (which is our
point). And FileSR then becomes an independent SR type out of box.

A little bit of change is done to attach (and similarly detach):
we now avoid calling super().attach from children classes, because
the attach method in the base class now does more work specific
to FileSR itself.

We haven't done much refactoring as part of this patch itself,
though it should introduce some potentials for code refactoring
and reusing in the future.

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CP-6697: normal ismount can not deal with bind mount

This is a known design issue of bind mount on Linux. As we
now use bind mount for FileSR, we need to fix this. Here we
implement a customized version specific to our environment.

PS. the fix for an issue (CA-125122) caused by a previous
version of this patch is now integrated in this version.

Signed-off-by: Zheng Li <zheng.li@citrix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@citrix.com>
CA-116892: Fix "Unrecognised bus type" VDIs.

Issue: After the upgrade to CentOS 6.4, the "device/bus" path used to
determine the host's CDROM details is no longer present which causes
their name to appear as "Unrecognised bus type"
Fix: Use "device/subsystem" to obtain SCSI details of the host CDROM drive.

Signed-off-by: Siddharth Vinothkumar <siddharth.vinothkumar@citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>

GitHub: closes #66 on xapi-project/github
CA-122253: Improve error reporting for LVM-based SR snapshots

Return the actual error instead of returning errors that might have
occurred in the error-handling path.

This commit was previously reverted (see commit
0d2d903d7541070ec97028a1b201109f4070bcb3) as we had to merge into trunk
and we didn't have enough time to test it.

Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>
Reviewed-by: Vineeth Thampi Raveendran <vineeth.thampi@citrix.com>

GitHub: closes #60 on xapi-project/sm
Merge CP-6697-3

This is the same as #58 and #63 combined (which were reverted) with
bug fixing commits squashed into the relevant original commits.

Github: close #69 on xapi-project/sm
CP-7292: add pylintrc 0.21.1 standard template

As pylintrc-0.13.1, this backup can helps us to identify our setup
customization when we upgrade to newer release of pylint in the future.

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: port our customization onto the new pylintrc template

Get the customization we previously made by diffing the current
pylintrc and pylintrc 0.13.1 template we backed up, then ported
it onto the new pylintrc (based on pylintrc 0.21.1 template).

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: remove the pylintrc 0.13.1 template backup

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: E1201 is now deprecated

Putting it in the config would stop pylint from working.

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: Inherit XenError from the Exception base class (E0710)

Let XenError inherit from the Exception base class, so that we can
get rid of the E0710 exemption. Otherwise pylint complains with

E0710: Raising a new style class which doesn't inherit from BaseException

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: Remove E1101 from the exemption list

E1101: Used when a variable is accessed for a nonexistent member.

Some type magic in a small portion of blktap2.py is too advanced
for pylint to comprehend. So we explicitly tell pylint to ignore
that particular warning on that particular section.

By doing so, we can now enable E1101 for all the rest part of our
code to achieve better safety.

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: remove the dependency on simplejson

The extra dependency on simplejson makes pylint to complain, because
the 3rd party module is not available in our building environment.

Replacing this single instance with python's standard json lib, we
can achieve equally good result for our simple object.

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: remove dependency on iofi

iofi is a customize module which doesn't exist in either xenbuilder
chroot or a standard XenServer host. Hence pylint complains about
the reference to a non-existed module.

My understanding is that the module is for debugging purpose and can
be purposely copied onto the system when necessary. But in general
cases, it won't be on the system.

The change workaround the pylint complain by using dynamic loading
via module name. This also fits the conditional loading concept
in context, where loading which module could be calculated and
dynamically decided (though not necessary in this instance).

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>
CP-7292: Make use of the exit code of pylint

In the previous version of pylint we use, the exit code of a pylint
process is always zero (regardless there are issues found or not).
So we have to capture the output of pylint to make a judgement call
ourselves.

The newer vesion of pylint exit with non-zero code if any issue
has been found. So we can now interpreter the exit code rathe than
the output text, which makes things simpler.

Unfortunately, a bug in the exact version 0.21.1 [1] stops us from
passing all the python files to check to pylint all at once, so we
still have to iterate over all the files one by one and record the
existence of any errors. The code can become considerablly simpler
when we move to a even newer version of pylint with this bug fixed.

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=591670

Signed-off-by: Zheng Li <zheng.li@eu.citrix.com>
Reviewed-by: Chandrika Srinivasan <chandrika.srinivasan@citrix.com>