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

file modified
+9
@@ -55,6 +55,10 @@ 

      m: 3G

      net_nic:

        model: e1000

+   drive:

+   - size: 11000000000

+     path: /home/vader

+   - size: 2000000000

  

  standard-inventory-docker:

    dumb_option: dumb_parameter
@@ -67,6 +71,11 @@ 

  * `qemu.m` - RAM size in megabytes. Optionally, a suffix of `M` or `G`.

  * `qemu.net_nic.model` - Use `qemu-system-x86_64 -net nic,model=help` for a list

     of available devices.

+ * `drive` - has to contain dict of additional drive(s)

+ * `drive.size` - allows to specify additional drive with size in bytes

+                  (default size: 2G)

+ * `drive.path` - allows to specify additional drive with custom path to its backing file

+                  (default path: /tmp)

  

  You can open a RFE ticket to extend supported parameters according to

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

@@ -1,6 +1,7 @@ 

  #!/usr/bin/python2

  

  import argparse

+ import atexit

Nice!

  import errno

  import json

  import os
@@ -248,6 +249,52 @@ 

      return value

  

  

+ class AdditionalDrives(object):

+     """Prepare additional drives options for qemu

+ 

+     Based on FMF config creates temporary sparse files and returns

+     corresponding qemu command options.

+     cleanup() will be called eventually to close the files.

+     """

+ 

+     _tempfiles = list()

+ 

+     @classmethod

+     def generate(cls):

+         """Generate sparse files and return drive qemu options

+         Returns

+         -------

+         list of str

+                 qemu -drive options

+         """

+         drives = fmf_get(['qemu', 'drive'], list())

+         result = []

+         for drive in drives:

+             # create temporary sparse file

+             size = int(drive.get('size', 2 * 1024 ** 3))  # default size: 2G

+             path = drive.get('path', None)

+             path = str(path) if path is not None else None

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

+             drive_file = tempfile.NamedTemporaryFile(dir=path)

+             drive_file.truncate(size)

+             cls._tempfiles.append({'file': drive_file, 'path': path})

+             logger.info("Created temporary sparse file '%s'." %drive_file.name)

+ 

+             # translate data into qemu command options

+             result += ["-drive", "file=%s,media=disk,if=virtio" % drive_file.name]

+         atexit.register(cls.cleanup)

+         return result

+ 

+     @classmethod

+     def cleanup(cls):

+         """Close all temporary files created by this class

+         """

+         for tempfile in cls._tempfiles:

+             fullname = os.path.join(tempfile['path'], tempfile['file'].name)

+             logger.info("Closing and removing temporary sparse file '%s'" % fullname)

+             if os.path.isfile(fullname):

+                 tempfile['file'].close()

+ 

+ 

  def start_qemu(image, cloudinit, portrange=(2222, 5555)):

      for _ in range(10):

          port = random.randint(*portrange)
@@ -270,10 +317,11 @@ 

      # Log from qemu itself.

      log_qemu = log_guest.replace(".guest.log", ".qemu.log")

      # Parameters from FMF:

-     param_m = fmf_get(['qemu', 'm'], "1024")

-     param_net_nic_model = fmf_get(['qemu', 'net_nic', 'model'], 'virtio')

+     param_m = str(fmf_get(['qemu', 'm'], "1024"))

Not sure why str() was added.

+     param_net_nic_model = str(fmf_get(['qemu', 'net_nic', 'model'], 'virtio'))

      # Use -cpu host and -smp by default.

      # virtio-rng-pci: https://wiki.qemu.org/Features/VirtIORNG

+ 

      qemu_cmd = ["/usr/bin/qemu-system-x86_64",

                  "-cpu", "host", "-smp", get_qemu_smp_arg(),

                  "-m", param_m, image, "-enable-kvm", "-snapshot", "-cdrom", cloudinit,
@@ -282,10 +330,13 @@ 

                  "-device", "isa-serial,chardev=pts2", "-chardev", "file,id=pts2,path=" + log_guest,

                  "-display", "none"]

  

+     qemu_cmd += AdditionalDrives.generate()

+ 

      if diagnose:

          qemu_cmd += ["-vnc", DEF_HOST + ":1,to=4095"]

  

      qemu_proc = subprocess.Popen(qemu_cmd, stdout=open(log_qemu, 'a'), stderr=subprocess.STDOUT)

+     time.sleep(5)

  

      if qemu_proc and diagnose:

          logger.info("qemu-kvm is running with VNC server. PID: {}".format(qemu_proc.pid))

  • 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 bdcee4d

5 years ago

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.

Ok, looks good. Thank you for contribution.

Commit 3f6c747 fixes this pull-request

Pull-Request has been merged by astepano

5 years ago

Pull-Request has been merged by astepano

5 years ago