Skip to content

Fix virtual template size for managed storage for KVM / refactor cloud-install-sys-tmplt#3371

Merged
svenvogel merged 4 commits intoapache:masterfrom
svenvogel:template_rewrite
Dec 17, 2019
Merged

Fix virtual template size for managed storage for KVM / refactor cloud-install-sys-tmplt#3371
svenvogel merged 4 commits intoapache:masterfrom
svenvogel:template_rewrite

Conversation

@svenvogel
Copy link
Copy Markdown
Contributor

@svenvogel svenvogel commented Jun 3, 2019

Description

  • cloud-install-sys-tmplt: add function to calculate the virtual size of template for managed storage like netapp solidfire so that the copy process is working from NFS to iSCSI correctly
  • add qemu-img (CentOS) / qemu-utils (Debian) packages to management build dependency
  • refactor cloud-install-sys-tmplt to be more verbose and user friendly help
  • createtmplt.sh: remove unused chksum flag

image

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)

Screenshots (if appropriate):

How Has This Been Tested?

tested manually in our production environment with different files like vhd, qcow2 and ova

@svenvogel svenvogel requested review from DennisKonrad and yadvr June 3, 2019 12:29
@svenvogel
Copy link
Copy Markdown
Contributor Author

@rhtyd please run your check again so that we can find the problem.

@DennisKonrad
Copy link
Copy Markdown
Contributor

@rhtyd From my point of view the code from sven is a cleanup/improvement. How can one see now what causes the problem?

For me the problem is now, how can I decide if there's a problem with svens code or is there something that should change in Trillian?

@DennisKonrad
Copy link
Copy Markdown
Contributor

DennisKonrad commented Jun 3, 2019

https://github.com/shapeblue/Trillian

Is the code here up to date? There's no way to set something like this up in a few days but if we would choose to do it: Would an installation using the code in the repo above behave (more or less) the same as blueorangutan?

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 3, 2019

I'll have to review if the flags have changed. From a users point of view, the command should be backward compatible. Trillian is working OK with other PRs so likely the issue is in the PR, or the script was changed such that the usage is not backward compatible.
@blueorangutan package

@apache apache deleted a comment from blueorangutan Jun 3, 2019
@apache apache deleted a comment from blueorangutan Jun 3, 2019
@apache apache deleted a comment from blueorangutan Jun 3, 2019
@apache apache deleted a comment from blueorangutan Jun 3, 2019
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 3, 2019

@DennisKonrad you can test against a local kvm based nested env in few hours: https://github.com/rhtyd/monkeybox
@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

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

Comment thread scripts/storage/secondary/createtmplt.sh
@DennisKonrad
Copy link
Copy Markdown
Contributor

@DennisKonrad you can test against a local kvm based nested env in few hours: https://github.com/rhtyd/monkeybox
@blueorangutan package

Isn't the whole use case of Trillian to spin up an cloudstack env on the fly to run the smoke tests? Also you pointed out the merge guideline states the smoketests should pass. So it seems to me there's not much use in manually setting up monkey box to test.

Maybe I don't understand the difference between Monkeybox and Trillian. Could you elaborate on this?

Also the question would be of importance to me: Can one use the trillian repo to get (more or less) the same results (as blueorangutan)?

@blueorangutan
Copy link
Copy Markdown

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 3, 2019

@DennisKonrad yes, but Trillian requires a lot of effort to setup and manage, and can span multiple hosts/machines. Monkeybox is aimed for developers who can setup a nested kvm based dev-test env on a single machine. Marvin based smoketests can run on both env.
The merge guideline require that smoketests should pass and the author/CI-runner to prove that no regressions were seen, irrespective of whether the tests were run manually or via a script/job.

BO is a github bot account that triggers jobs in our internal Jenkins which uses Trillian to deploy and run tests against an env.

@blueorangutan
Copy link
Copy Markdown

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

@DennisKonrad
Copy link
Copy Markdown
Contributor

@rhtyd Ok, that clears up things a lot. But what about those two. How can one make sense of the test result there?

https://builds.apache.org/job/cloudstack-pr-analysis
https://travis-ci.org/apache/cloudstack

The jenkins fails with some code that doesnt even belong to the PR and the Travis tests mostly seem to pass this time even though they didn't with the same code before??

@blueorangutan
Copy link
Copy Markdown

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 3, 2019

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 3, 2019

@DennisKonrad master needs stabilisation, which is why you're seeing intermittent errors. The jenkins job is not reliable, I'm not sure how to manage that, I usually ignore that like most people. I check Travis test results and smoketests via BO.

4.11 for example is most stable branch where Travis mostly passes along with smoketests. We'll need to put some effort on master to ensure Travis test runs are reliable, this would require either fixing the failing tests or the code that caused that regression.

@DennisKonrad
Copy link
Copy Markdown
Contributor

@rhtyd So to resolve this issue can you provide the calls to those scripts to @svenvogel so he can maybe adapt this PR to work with the BO that's currently in place?

I see no other way to resolve this issue.

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 4, 2019

@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"]}

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.

@svenvogel
Copy link
Copy Markdown
Contributor Author

@rhtyd @syed thanks for help. i will introduce a flag for managed storage and revert the necessary changes back for createtemplt.sh script. after that there should be not a problem. after that we can run the next test.

@svenvogel
Copy link
Copy Markdown
Contributor Author

@rhtyd how can i test it by self with "@blue...."?

@svenvogel
Copy link
Copy Markdown
Contributor Author

@svenvogel I will test it. it looks ok.
by the way, you need to "git rebase" with latest master,or "Resolve conflicts" above.

@weizhouapache thanks for checking. yes you are sure. now it should be rebased 👍

@svenvogel svenvogel self-assigned this Dec 4, 2019
@syed
Copy link
Copy Markdown
Contributor

syed commented Dec 5, 2019

LGTM 👍 Thanks @svenvogel

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Dec 7, 2019

I'll get back on this soon to review, let me kick tests in the meanwhile
@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-433

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Dec 8, 2019

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tested following actions on kvm/ubuntu18.04

deploy a new zone with these changes
register template
download template
create vm
download volume
create template from volume

@svenvogel
Copy link
Copy Markdown
Contributor Author

4 LGTM, checks passed tested in different environments CentOS and Ubuntu.

@svenvogel svenvogel merged commit a0efbf9 into apache:master Dec 17, 2019
andrijapanicsb added a commit that referenced this pull request Dec 17, 2019
…tor cloud-install-sys-tmplt (#3371)"

This reverts commit a0efbf9.
andrijapanicsb added a commit that referenced this pull request Dec 17, 2019
…tor cloud-install-sys-tmplt (#3371)" (#3771)

This reverts commit a0efbf9.
@andrijapanicsb
Copy link
Copy Markdown
Contributor

Apologies @svenvogel , I have reverted this PR/merge - since tests have NOT complete (there is no output) - let me kick the tests again, and if all fine, you can merge it after that, please.

@andrijapanicsb
Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

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

andrijapanicsb added a commit that referenced this pull request Dec 17, 2019
… / refactor cloud-install-sys-tmplt (#3371)" (#3771)"

This reverts commit e319c8b.
@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-633)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 26613 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3371-t633-kvm-centos7.zip
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

andrijapanicsb added a commit that referenced this pull request Dec 18, 2019
… / refactor cloud-install-sys-tmplt (#3371)" (#3771)"

This reverts commit e319c8b.
andrijapanicsb added a commit that referenced this pull request Dec 19, 2019
… / refactor cloud-install-sys-tmplt (#3371)" (#3771)" (#3772)

This reverts commit e319c8b.
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
…d-install-sys-tmplt (apache#3371)

* remove unused chksum flag
* refactor cloud-install-sys-tmplt / add qemu-img command to calculate correct virtual size
* add qemu-utils dependency to debian build control file
* add qemu-utils dependency to centos spec file
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
ustcweizhou pushed a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.