New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[XenServer/XCP-ng] Pass the image store NFS version on storage commands #5886
Conversation
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2271 |
@blueorangutan test centos7 xcpng82 |
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
Trillian test result (tid-2938)
|
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2276 |
@blueorangutan test centos7 xcpng82 |
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
Trillian test result (tid-2945)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
private ImageStoreDao imageStoreDao; | ||
|
||
protected void setSecondaryStorageNfsVersionToParams(Long zoneId, Map<String, Object> params) { | ||
ImageStoreVO imageStoreInZone = imageStoreDao.findOneByZoneAndProtocol(zoneId, "nfs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvazquez Are multiple image stores with different nfs versions supported in a zone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be possible since the 'secstorage.nfs.version' setting has scope = ImageStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @nvazquez, will there be any issues if the nfs version is set based on one image store when multiple image stores are added (with different versions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially yes - suggesting we should pick the lower value set in this case? cc @DaanHoogland
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nvazquez @sureshanaparti I do not see a problem here. Unless I am looking at the code in isolation too much and should be looking at a bigger picture, what happens here:
a store is picked
if a store is found
- the nfs version for that store is retrieved and used
if not
- the default nfs version is used
what could go wrong here is only that the operator has not set the right nfs versions to the system/stores.
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
@blueorangutan package |
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2308 |
@blueorangutan test centos7 xcpng82 |
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, verified manually the latest changes
Trillian test result (tid-3012)
|
Trillian test result (tid-3015)
|
@blueorangutan test centos7 xcpng82 |
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
engine/storage/src/main/java/org/apache/cloudstack/storage/image/NfsImageStoreDriverImpl.java
Show resolved
Hide resolved
...rver/src/main/java/com/cloud/hypervisor/xenserver/resource/Xenserver625StorageProcessor.java
Show resolved
Hide resolved
Trillian test result (tid-3029)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cltgm, needs extensive testing though
@blueorangutan package |
@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2357 |
@blueorangutan test centos7 xcpng82 |
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests |
Trillian test result (tid-3047)
|
@sureshanaparti it seems that failing test was consistent across the previous failures, I’ll investigate it |
@nvazquez there is some issue connecting to hosts, seems to be environment issue. noticed this in other PR tests as well (not observed in the latest health check runs). |
Description
This PR fixes issues on mounting secondary storage from XCP hosts, controlling the NFS version set.
secstorage.nfs.version
setting of an image store, or if it is not set, the global value is usedmount -o vers=4....
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested on a NFS 3 server as image store
Deploy the environment
Set image store 'secstorage.nfs.version' = 4
Restart the management server
Deploy VM --> failure as secondary storage could not be mounted on the host
Set image store 'secstorage.nfs.version' = 3
Restart the management server
Deploy VM --> Success
Set image store 'secstorage.nfs.version' = '' and global 'secstorage.nfs.version' = ''
Restart the management server
Take backups, snapshots, attach ISO -> Success