#257 Now posible to use additional disks
Merged by astepano. Opened by japokorn.
japokorn/standard-test-roles master-extra_disks  into  master

Download 257.patch
  • additional disks for qemu can be specified using FMF

Resolves issue #247[1]

[1] https://pagure.io/standard-test-roles/issue/247

ansible-playbook is called from /tests directory.
Not sure why this change is here.

Please do not intro new newlines without necessity or code style violence.

https://pagure.io/standard-test-roles/blob/master/f/README.md
Section : Flexible Metadata Format for default provisioner(s)

and : https://qemu.weilnetz.de/doc/qemu-doc.html

Let's name keys as options to qemu.

How about to rename 'add_drives' to 'drive' which will be yaml list.

Please udate your RP and also add new keys to :
https://pagure.io/standard-test-roles/blob/master/f/README.md
Section : Flexible Metadata Format for default provisioner(s)

How about adding a key to Metadata drive[X].path ?
So user can specify where to create such drive, at which path.
Different instances can have different configurations.
And there is no guaranty that file located at tempfile.NamedTemporaryFile() can grow to GB.

Just a though aloud: can we specify some key or ENV var that says: keep drivers, do not remove them.

can we put this code inside to start_qemu(), do not introduce extra_opts.

and register cleanup function for drivers in some official way?

Ping me if you need more explanation.

Also please check https://jenkins-fedora-atomic-process.apps.ci.centos.org/job/str_pr_test/195/console

PR fails with Python style coding, look at something like:

/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:275:64: E225 missing whitespace around operator
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:276:53: E261 at least two spaces before inline comment
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:276:54: E262 inline comment should start with '# '
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:277:38: E261 at least two spaces before inline comment
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:281:56: E225 missing whitespace around operator
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:285:5: E303 too many blank lines (2)
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:290:76: E225 missing whitespace around operator
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:294:68: E251 unexpected spaces around keyword / parameter equals
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:294:70: E251 unexpected spaces around keyword / parameter equals
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:322:5: E303 too many blank lines (2)
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:340:5: E303 too many blank lines (2)
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:388:65: E251 unexpected spaces around keyword / parameter equals
/home/jenkins/workspace/str_pr_test/inventory/standard-inventory-qcow2:388:67: E251 unexpected spaces around keyword / parameter equals

Question is: with this approach we will have block device in qemu. But it will be un-initialised.
VM will still format it and mount.

For now, you can look at script: https://pagure.io/standard-test-roles/blob/master/f/scripts/qcow2-grow
You can call it manually on your Fedora 2x system. It will grow gcow2 image. It doesn't work on Centos.

rebased onto bdcee4d4acbd5ec629a95b00f7947eec28389b52

Updated the PR:
- removed unwanted artifacts (empty line, "tests")
- changed key name from add_drives to drive
- added new "path" key allowing user to specify different path for drives' backing file.
- updated README.md
- additional drives cleanup refactored - now it uses atexit
- fixed PEP8 issues

I think that option of keeping the drives is currently above range of this PR, though adding it might prove useful in the future.

Maybe make it more explicit with: isinstance(path, basestring)
But, ok.

Not sure why str() was added.

Ok, looks good. Thank you for contribution.

Commit 3f6c7479 fixes this pull-request

Pull-Request has been merged by astepano

Pull-Request has been merged by astepano

Metadata