Fix LibvirtStorageAdaptor.java

[cl-k-takahashi]

Description

I fixed createNetfsStoragePool() function in LibvirtStorageAdaptor.java.
The current implementation of createNetfsStoragePool() does not unmount already mounted pool in non-English environment so I edited it to check the pool is already mounted using mountpoint command and unmount the pool if already mounted.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

I got the following output in agent.log in a KVM hypervisor host:

2018-06-06 19:51:26,059 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-5:null) (logid:293e0c5c) Attempting to unmount old mount at /mnt/00ec8deb-14b2-3264-bd61-4bf38f4bccc5
2018-06-06 19:51:26,090 INFO  [kvm.storage.LibvirtStorageAdaptor] (agentRequest-Handler-5:null) (logid:293e0c5c) Succeeded in unmounting /mnt/00ec8deb-14b2-3264-bd61-4bf38f4bccc5

This output indicates already-mounted mountpoint /mnt/00ec8deb-14b2-3264-bd61-4bf38f4bccc5 is successfully unmounted.

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
  Testing
• I have added tests to cover my changes.
• All relevant new and existing integration tests have passed.
• A full integration testsuite with all test that can run on my environment has passed.

[yadvr]

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[wido]

I'm wondering why we do this manual unmount. Isn't libvirt supposed to handle this for us? It manages the storage pool and mounts NFS for us. Why do we care about this?

I rather leave this to libvirt

[blueorangutan]

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2094

[yadvr]

@blueorangutan test

[blueorangutan]

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[DaanHoogland]

@wido if @cl-k-takahashi reports that it doesn't work in non English environments that could be a problem with libvirt.
@cl-k-takahashi can you add a more extensive environment description? So we can reproduce. Most of us use English environments.

[wido]

@DaanHoogland Yes, I understand, we check for "already mounted" in the code.

However, my main point is, why do we manually unmount. Destroying the pool in libvirt should make sure the mountpoint is gone.

[DaanHoogland]

so @wido your objection is to Script.runSimpleBashScript("umount -l " + targetPath);? I would agree to that but it is not part of this change. Do you propose to change this change to deal with that in one go?

[wido]

Hmm, yes, I understand you. It would be better if libvirt would handle all this, but it's indeed not part of this PR.

[cl-k-takahashi]

@DaanHoogland The current implementation detects "already mounted" in e.toString() but e.toString() is language configuration dependent. In Japanese environment, it gives "すでにマウントされています" instead of "already mounted" and the KVM agent doesn't unmount the pool.

[blueorangutan]

Trillian test result (tid-2735)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23346 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2696-t2735-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	Error	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	Error	120.44	test_privategw_acl.py
test_05_rvpc_multi_tiers	Failure	355.42	test_vpc_redundant.py
test_05_rvpc_multi_tiers	Error	378.29	test_vpc_redundant.py
test_hostha_enable_ha_when_host_in_maintenance	Error	3.47	test_hostha_kvm.py

[DaanHoogland]

yes @cl-k-takahashi we understand that.
@wido do you accept the change as is?

[wido]

LGTM

[yadvr]

LGTM. I checked that the mountpoint tool is available on centos6, centos7 and ubuntu, therefore this way of checking if something is indeed mounted should work okay.

[yadvr]

Test LGTM, merging this based on code reviews and test results, as well as manual testing of mountpoint tool on centos6, centos7 and ubuntu 16.04/18.04.