rhtyd commented on issue #3371: Fix template size for managed storage / refactor cloud-install-sys-tmplt and createtmplt.sh URL: https://github.com/apache/cloudstack/pull/3371#issuecomment-498611909 @svenvogel @DennisKonrad here are bunch of issues with this PR: - It breaks the cloud-install-sys-tmplt usage which is not backward compatible anymore, to simply setup/seed the initial systemvmtemplate before deploying a zone fails (it should not need to check if qemu-img etc is available). Trillian logs for reference: ``` 20:08:11 TASK [cloudstack-config : Install System VM templates] ************************* 20:08:12 failed: [pr3371-t3639-kvm-centos7-mgmt1] (item=-u http://staging.yadav.xyz/systemvmtemplate/custom/4113/systemvmtemplate-4.11.3-kvm.qcow2.bz2 -h kvm -F) => {"changed": false, "cmd": ["/usr/share/cloudstack-common/scripts/storage/secondary/cloud-install-sys-tmplt", "-m", "/mnt/secstoragetmp", "-u", "http://staging.yadav.xyz/systemvmtemplate/custom/4113/systemvmtemplate-4.11.3-kvm.qcow2.bz2";, "-h", "kvm", "-F"], "delta": "0:00:00.195024", "end": "2019-06-03 14:38:29.618615", "failed": true, "item": "-u http://staging.yadav.xyz/systemvmtemplate/custom/4113/systemvmtemplate-4.11.3-kvm.qcow2.bz2 -h kvm -F", "rc": 2, "start": "2019-06-03 14:38:29.423591", "stderr": "which: no qemu-img in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin)", "stderr_lines": ["which: no qemu-img in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin)"], "stdout": "Please install qemu-img: command\non CentOS run \"yum install qemu-img\"\non Ubuntu/Debian run \"apt-get install qemu-utils\"\nInstallation failed", "stdout_lines": ["Please install qemu-img: command", "on CentOS run \"yum install qemu-img\"", "on Ubuntu/Debian run \"apt-get install qemu-utils\"", "Installation failed"]} ``` - You can find usage of the second script using `git grep createtmplt.sh`, I found that the script is used both the ssvm agent and kvm to process templates. By removing flags that once were passed, it breaks the usage of the script: https://github.com/apache/cloudstack/blob/master/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java#L723 Given this is a refactoring/cleanup PR, this ought to be backward compatible wrt script execution usages, i.e. don't remove flags and don't replace old flags, without proper documentation don't change how script is used.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
