Add support for multiple disks to testcloud plugin #2767

happz · 2024-03-15T13:07:32Z

TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \
    provision --how virtual --image fedora-39 \
              --hardware 'disk[1].size=20GB' \
              --hardware 'disk[0].size=15GB' \
    login -s provision \
    finish

Fixes #2765

Pull Request Checklist

implement the feature
extend the test coverage
mention the version
include a release note

thrix · 2024-05-28T15:19:43Z

@happz this looks weird:

❯ TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default     provision --how virtual --image fedora-39               --hardware 'disk[1].size=20GB'               --hardware 'disk[10].size=15GB'     login -s provision     finish
/var/home/mvadkert/.local/share/tmt/run-002
Found 1 plan.

/default/plan
    provision
        queued provision.provision task #1: default-0
        
        provision.provision task #1: default-0
        how: virtual
        hardware:
            disk:
              - {}
              - size: = 20GB
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - size: = 15GB
        image: fedora-39
        memory: 2048 MB
        disk: 40 GB
        qcow: Fedora-Cloud-Base-39-1.5.x86_64.qcow2
        effective hardware:
            disk:
              - {}
              - size: = 20GB
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - size: = 15GB

thrix · 2024-05-28T15:22:33Z

@happz this would deserve a better error:

❯ TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default     provision --how virtual --image fedora-39               --hardware 'disk.size=20GB'               --hardware 'disk[1].size=15GB'     login -s provision     finish
/var/home/mvadkert/.local/share/tmt/run-006
Found 1 plan.

/default/plan

plan failed

    Traceback (most recent call last):
      File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/__main__.py", line 18, in run_cli
        tmt.cli.main()
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
        return self.main(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 1078, in main
        rv = self.invoke(ctx)
             ^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
        return _process_result(sub_ctx.command.invoke(sub_ctx))
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 1720, in invoke
        return _process_result(rv)
               ^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 1657, in _process_result
        value = ctx.invoke(self._result_callback, value, **ctx.params)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/core.py", line 783, in invoke
        return __callback(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/.local/share/hatch/env/virtual/tmt/Hq3GU5T-/dev/lib/python3.11/site-packages/click/decorators.py", line 33, in new_func
        return f(get_current_context(), *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/cli.py", line 508, in finito
        click_context.obj.run.go()
      File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/base.py", line 3576, in go
        raise tmt.utils.GeneralError(
    tmt.utils.GeneralError: plan failed

The exception was caused by 1 earlier exceptions

Cause number 1:

    Failed to load step data for ProvisionTestcloudData: unsupported operand type(s) for +=: 'dict' and 'list'

        Traceback (most recent call last):
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/base.py", line 3572, in go
            plan.go()
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/base.py", line 2271, in go
            self.wake()
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/base.py", line 2240, in wake
            step.wake()
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/provision/__init__.py", line 2212, in wake
            for data in self.data:
                        ^^^^^^^^^
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/__init__.py", line 517, in data
            self._data = self._normalize_data(self._raw_data, self._logger)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/__init__.py", line 478, in _normalize_data
            plugin = self._plugin_base_class.delegate(self, raw_data=raw_datum)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/__init__.py", line 1385, in delegate
            raise tmt.utils.GeneralError(
        tmt.utils.GeneralError: Failed to load step data for ProvisionTestcloudData: unsupported operand type(s) for +=: 'dict' and 'list'

    The exception was caused by 1 earlier exceptions

    Cause number 1:

        unsupported operand type(s) for +=: 'dict' and 'list'

            Traceback (most recent call last):
              File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/__init__.py", line 1382, in delegate
                data = plugin_data_class.from_spec(raw_data, step._logger)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/__init__.py", line 299, in from_spec
                data._load_keys(cast(dict[str, Any], raw_data), cls.__name__, logger)
              File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/utils.py", line 6397, in _load_keys
                value = dataclass_normalize_field(self, key_address, keyname, value, logger)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/utils.py", line 5915, in dataclass_normalize_field
                value = metadata.normalize_callback(key_address, raw_value, logger)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              File "/var/home/mvadkert/git/github.com/teemtee/tmt/tmt/steps/provision/__init__.py", line 511, in normalize_hardware
                merged[components.name] += [
            TypeError: unsupported operand type(s) for +=: 'dict' and 'list'

thrix · 2024-05-28T15:23:43Z

@happz this does not fail:

❯ TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default     provision --how virtual --image fedora-39               --hardware 'disk=20GiB' login -s provision     finish
/var/home/mvadkert/.local/share/tmt/run-008
Found 1 plan.

/default/plan
    provision
        queued provision.provision task #1: default-0
        
        provision.provision task #1: default-0
        how: virtual
        hardware: disk: = 20GiB

        image: fedora-39
        memory: 2048 MB
        disk: 40 GB
        qcow: Fedora-Cloud-Base-39-1.5.x86_64.qcow2
        effective hardware: disk: = 20GiB
        name: tmt-008-AmhIKWiF
        key: /var/home/mvadkert/.local/share/tmt/run-008/default/plan/provision/default-0/id_ecdsa
        progress: booting...
        primary address: 127.0.0.1
        topology address: 127.0.0.1
        port: 10022

thrix

Some behavior is odd, kindly asking for review

happz · 2024-05-28T15:31:44Z

@happz this looks weird:

❯ TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default     provision --how virtual --image fedora-39               --hardware 'disk[1].size=20GB'               --hardware 'disk[10].size=15GB'     login -s provision     finish
/var/home/mvadkert/.local/share/tmt/run-002
Found 1 plan.

/default/plan
    provision
        queued provision.provision task #1: default-0
        
        provision.provision task #1: default-0
        how: virtual
        hardware:
            disk:
              - {}
              - size: = 20GB
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - size: = 15GB
        image: fedora-39
        memory: 2048 MB
        disk: 40 GB
        qcow: Fedora-Cloud-Base-39-1.5.x86_64.qcow2
        effective hardware:
            disk:
              - {}
              - size: = 20GB
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - {}
              - size: = 15GB

I'm open to suggestions for improvements.

happz · 2024-05-28T15:56:25Z

@thrix right, I’ll add some test coverage, and I’ll use your examples too.

thrix · 2024-05-28T20:01:55Z

@happz

(shortened a bit for brewity)

/default/plan
provision
queued provision.provision task #1: default-0

    provision.provision task #1: default-0
    how: virtual
    hardware:
        disk:
          - {}
          - size: = 20GB
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - size: = 15GB

I'm open to suggestions for improvements.

I kinda understand why it looks like it, but from user perspective I am wondering if it is important that one disk is with index 1 and second with 10, looking at the booted machine, seems it is the same as if I would use index 0 and index 1?

Because of that I would go with:

/default/plan
provision
queued provision.provision task #1: default-0
    provision.provision task #1: default-0
    how: virtual
    hardware:
        disk:
          - size: = 20GB
          - size: = 15GB

happz · 2024-05-28T20:17:48Z

@happz

(shortened a bit for brewity)
/default/plan
provision
queued provision.provision task #1: default-0
    provision.provision task #1: default-0
    how: virtual
    hardware:
        disk:
          - {}
          - size: = 20GB
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - {}
          - size: = 15GB
I'm open to suggestions for improvements.
I kinda understand why it looks like it, but from user perspective I am wondering if it is important that one disk is with index 1 and second with 10, looking at the booted machine, seems it is the same as if I would use index 0 and index 1?

Well, then you should have used --hardware 'disk[0].size=15GB' and --hardware 'disk[1].size=15GB', not --hardware 'disk[10].size=15GB' ;)

Yeah, this is not well thought out, the order is much less important than it might look like. This might be actually a good idea, merge all inputs and then remove all empty slots. We need the index when defining the requirements, so you can refer to the same disk from multiple options (--hardware 'disk[10].size=15GB' --hardware 'disk[10].vendor-name=Seagate'), but after that, it's pointless.

Because of that I would go with:
/default/plan
provision
queued provision.provision task #1: default-0
    provision.provision task #1: default-0
    how: virtual
    hardware:
        disk:
          - size: = 20GB
          - size: = 15GB

I'd like to achieve this:

     hardware:
         disk[0].size: = 20GB
         disk[1].size: = 15GB

but it turns out to be hard to achieve with simple tools I have, it'd require way more work, so I will probably go with removing the empty slots.

happz · 2024-05-28T20:35:25Z

@thrix hopefully all three points were addressed in 2cf44e5

martinhoyer

Nice, feel free to close the nitpicking comments.

tmt/hardware.py

tmt/steps/provision/__init__.py

thrix

LGTM

happz · 2024-06-11T12:59:05Z

@thrix I'm putting placeholders back into output, I can't fix #3004 without them for now. Good news is, you won't see them in real life that much, disk[10] is a very artificial example, therefore the output won't be as bad as it was here.

Added an issue to track it, and will follow up, not just to get rid of placeholders but also to clear up the position of "root disk" (or "boot disk"?).

lukaszachy · 2024-06-11T15:50:48Z

@happz Test failures are not related to this PR, right?

martinhoyer · 2024-06-11T16:16:21Z

@happz Test failures are not related to this PR, right?

I don't think so fwiw.

happz · 2024-06-11T19:14:56Z

Unrelated, but the results for the provision were affected by something, and the test I updated got no results at all, which is weird and suspicious. Restarting rather than merging the PR.

``` TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \ provision --how virtual --image fedora-39 \ --hardware 'disk[1].size=20GB' \ --hardware 'disk[0].size=15GB' \ login -s provision \ finish ``` Fixes #2765

Co-authored-by: Martin Hoyer <[email protected]>

happz · 2024-06-12T15:06:48Z

All failures are unrelated now, merging.

``` TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \ provision --how virtual --image fedora-39 \ --hardware 'disk[1].size=20GB' \ --hardware 'disk[2].size=15GB' \ login -s provision \ finish ``` Fixes #2765 Co-authored-by: Martin Hoyer <[email protected]>

``` TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \ provision --how virtual --image fedora-39 \ --hardware 'disk[1].size=20GB' \ --hardware 'disk[2].size=15GB' \ login -s provision \ finish ``` Fixes teemtee#2765 Co-authored-by: Martin Hoyer <[email protected]>

happz added step | provision Stuff related to the provision step plugin | testcloud The testcloud virtual provision plugin area | hardware Implementation of hardware requirements labels Mar 15, 2024

happz added this to the 1.33 milestone Mar 15, 2024

happz requested review from frantisekz, psss, lukaszachy, thrix and janhavlin as code owners March 15, 2024 13:07

happz force-pushed the testcloud-multiple-disks branch from 6d356d9 to c2bcbd0 Compare March 27, 2024 07:39

happz added the ci | full test Pull request is ready for the full test execution label Mar 27, 2024

happz modified the milestones: 1.33, 1.34 Apr 30, 2024

happz force-pushed the testcloud-multiple-disks branch from c2bcbd0 to 2f630e0 Compare May 21, 2024 09:07

happz force-pushed the testcloud-multiple-disks branch from 2f630e0 to 162d457 Compare May 28, 2024 13:36

thrix requested changes May 28, 2024

View reviewed changes

happz force-pushed the testcloud-multiple-disks branch from 2cf44e5 to d78fca0 Compare June 3, 2024 08:06

bajertom modified the milestones: 1.34, 1.35 Jun 4, 2024

therazix approved these changes Jun 6, 2024

View reviewed changes

KwisatzHaderach approved these changes Jun 6, 2024

View reviewed changes

martinhoyer approved these changes Jun 10, 2024

View reviewed changes

tmt/hardware.py Outdated Show resolved Hide resolved

tmt/steps/provision/__init__.py Outdated Show resolved Hide resolved

happz force-pushed the testcloud-multiple-disks branch from e0a030c to 416ff35 Compare June 10, 2024 19:28

thrix self-requested a review June 11, 2024 11:35

happz added the status | blocked The merging of PR is blocked on some other issue label Jun 11, 2024

thrix approved these changes Jun 11, 2024

View reviewed changes

happz mentioned this pull request Jun 11, 2024

Avoid overwriting "root disk" properties with --hardware disk[N] #3004

Open

happz force-pushed the testcloud-multiple-disks branch from 953982c to 6b07646 Compare June 11, 2024 12:56

happz added status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. and removed status | blocked The merging of PR is blocked on some other issue labels Jun 11, 2024

happz and others added 8 commits June 12, 2024 14:03

Add support for multiple disks to testcloud plugin

542ab2c

``` TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \ provision --how virtual --image fedora-39 \ --hardware 'disk[1].size=20GB' \ --hardware 'disk[0].size=15GB' \ login -s provision \ finish ``` Fixes #2765

squash: address comments

de577a1

squash: Update tmt/hardware.py

2e043bd

Co-authored-by: Martin Hoyer <[email protected]>

squash: Update tmt/steps/provision/__init__.py

f4d3523

Co-authored-by: Martin Hoyer <[email protected]>

squash: post-rebase fix

07867c4

squash: add test

a02e744

squash: add release note

bc89080

squash: fix test

f4933f5

happz force-pushed the testcloud-multiple-disks branch from e8290de to f4933f5 Compare June 12, 2024 12:03

happz merged commit e1e6898 into main Jun 12, 2024
16 of 19 checks passed

happz deleted the testcloud-multiple-disks branch June 12, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple disks to testcloud plugin #2767

Add support for multiple disks to testcloud plugin #2767

happz commented Mar 15, 2024 •

edited

Loading

thrix commented May 28, 2024

thrix commented May 28, 2024

thrix commented May 28, 2024

thrix left a comment

happz commented May 28, 2024

happz commented May 28, 2024

thrix commented May 28, 2024 •

edited

Loading

happz commented May 28, 2024

happz commented May 28, 2024

martinhoyer left a comment

thrix left a comment

happz commented Jun 11, 2024

lukaszachy commented Jun 11, 2024

martinhoyer commented Jun 11, 2024

happz commented Jun 11, 2024

happz commented Jun 12, 2024

Add support for multiple disks to testcloud plugin #2767

Add support for multiple disks to testcloud plugin #2767

Conversation

happz commented Mar 15, 2024 • edited Loading

thrix commented May 28, 2024

thrix commented May 28, 2024

thrix commented May 28, 2024

thrix left a comment

Choose a reason for hiding this comment

happz commented May 28, 2024

happz commented May 28, 2024

thrix commented May 28, 2024 • edited Loading

happz commented May 28, 2024

happz commented May 28, 2024

martinhoyer left a comment

Choose a reason for hiding this comment

thrix left a comment

Choose a reason for hiding this comment

happz commented Jun 11, 2024

lukaszachy commented Jun 11, 2024

martinhoyer commented Jun 11, 2024

happz commented Jun 11, 2024

happz commented Jun 12, 2024

happz commented Mar 15, 2024 •

edited

Loading

thrix commented May 28, 2024 •

edited

Loading