CLOUDSTACK-8601. VMFS storage added as local storage can be re-added …

[likitha]

…as shared storage.

Fail addition of a VMFS shared storage pool in case it has already been added as local storage in CS.

[asfbot]

cloudstack-pull-requests #680 SUCCESS
This pull request looks good

[mike-tutkowski]

I recommend that all three of these member variables be made private.

[mike-tutkowski]

Aside from a couple minor comments, LGTM. :)

[remibergsma]

Who wants to step in and finish this work? It seems the original author is not able to finish it. If no one steps in, we'll have to close the PR without merging it so please help :-).

[mike-tutkowski]

The code LGTM. Are we under the assumption that it was properly tested? If so, I can just merge it at this point.

[remibergsma]

@mike-tutkowski Thanks! It seems the testing / verification is still to do and that is the work. Merging itself can be done with a one-liner. Are you able to verify the fix in your lab?

[mike-tutkowski]

Hey...I just noticed something.

I have a datastore that I manually added to my ESXi hosts (shared storage) that is based on an iSCSI target.

When I restart the CS MS, this datastore (which I already added to CloudStack as shared primary storage) gets re-added as local storage (so it's listed as both shared and local primary storages).

I think this is perhaps what led to this pull-request in the first place, but the problem still exists.

What happens now that this pull-request code is in is that the datastore conflict is detected and an error message is printed, but I still see my shared primary storage has been added as well as local storage.

I'm looking into this more, but wanted to point out this observation.

I had forgotten about this issue (I think it's pretty old). At the time, we put in a workaround where datastores that started with "-" or "" were assumed to be shared and not re-added as local. It just happens that my manually added, shared datastore does not start with "-" or "", so I see the problem.

[mike-tutkowski]

OK, I mainly remembered the issue correctly, but not entirely. The way we avoid this problem (usually) is by looking to see if the datastore starts with "-iqn." or "_iqn.".

If it does, we assume it is not local storage; otherwise, it is assumed to be local storage (an incorrect assumption in this situation that leads to shared storage (that has already been added to CS as shared primary storage) being added to CS as local primary storage).

[mike-tutkowski]

We need a more robust solution.

Does anyone know how to detect that a datastore does not reside on the local storage of the host in question?

[mike-tutkowski]

Here is the relevant code (in HostMO.java):

public List<Pair<ManagedObjectReference, String>> getLocalDatastoreOnHost() throws Exception {
    List<Pair<ManagedObjectReference, String>> dsList = new ArrayList<Pair<ManagedObjectReference, String>>();

    ObjectContent[] ocs = getDatastorePropertiesOnHyperHost(new String[] {"name", "summary"});
    if (ocs != null) {
        for (ObjectContent oc : ocs) {
            DatastoreSummary dsSummary = (DatastoreSummary)VmwareHelper.getPropValue(oc, "summary");
            if (dsSummary.isMultipleHostAccess() == false && dsSummary.isAccessible() && dsSummary.getType().equalsIgnoreCase("vmfs")) {
                ManagedObjectReference morDs = oc.getObj();
                String name = (String)VmwareHelper.getPropValue(oc, "name");

                if (!name.startsWith("-iqn.") && !name.startsWith("_iqn.")) {
                    dsList.add(new Pair<ManagedObjectReference, String>(morDs, name));
                }
            }
        }
    }
    return dsList;
}

[mike-tutkowski]

I see we already do this:

dsSummary.isMultipleHostAccess() == false

But that doesn't seem to work the way we thought.

[mike-tutkowski]

I found an old e-mail from me on this:

http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201312.mbox/%3CCACOnxCh2HGTWRWkvB-__VGgA9q-XJyA-mOuyd4iRnOkkZ7b37Q@mail.gmail.com%3E