Use `testcloud` domain API v2

[frantisekz]

Refactors domain configuration interface between tmt and testcloud, to allow greater flexibility and superior api stability when making changes and additions in the future.

Apart from that, doesn't expose support for (will come as a separate PR):

• tpm emulation
• uefi
• cpu core count setting (more advanced topologies to follow https://tmt.readthedocs.io/en/stable/spec/hardware.html#cpu TBD)

testcloud counterparts:

• https://pagure.io/testcloud/c/6f0942cb3ba31ff0927ee8b151218e8dc8b34d7d?branch=master
• https://pagure.io/testcloud/pull-request/155
• https://pagure.io/testcloud/c/7ee6dc839a46c72db6d23bcd2e057c2de6fe5ea8?branch=master
• ...

(compatible testcloud built at): https://copr.fedorainfracloud.org/coprs/frantisekz/testcloud-wip/

[lukaszachy]

@kkaarreell FYI - This adds TPM flag for provision -h virtual

[kkaarreell]

Hi @frantisekz , I would like to test this code but may I ask you to rebase first? For proper testing using my test I need the multi-host related code so it would help me a lot if this would be up to date. Thank you.

[frantisekz]

Hi @frantisekz , I would like to test this code but may I ask you to rebase first? For proper testing using my test I need the multi-host related code so it would help me a lot if this would be up to date. Thank you.

Sure, done!

[frantisekz]

@thrix both addressed, thanks!

[happz]

@frantisekz any chance you might be willing to delay your PR to 1.27 to be merged together with https://github.com/teemtee/tmt/pull/1316?

My main concern is the new options added by your patch, --cpus, --tpm, and --uefi. If we do indeed get rid of the EPEL8 limitation in 1.27, the HW requirements PR could get merged and I would invest my time into helping transition testcloud plugin to use the --hardware key/option & follow the existing specification. Obviously, these new features are something you wish to see supported by the plugin, same here, yet I'm worried about the number of options duplicating what's already on the verge of being ready. EPEL8 was a long-time blocker, but since things started to move very recently, we might actually get rid of the limitation in 1.27, unblocking the consolidation.

I already updated the Beaker plugin in https://github.com/teemtee/tmt/pull/1316/, to "understand" common HW requirements as specified in hardware key. There is a CLI option, --hardware, mirroring the hardware key, allowing the same degree of CLI extensibility as --disk, --memory or --cpus, but the same language accepted by the hardware key: --hardware "cpu.processors>=4", --hardware "disk[0].size >= 100 GiB", --hardware "tpm.version == 2".

I understand it would be yet another delay, but it would make the Universe a much better and tidier place :) Switching the plugin's implementation to one used by the beaker and artemis plugins would be a huge boost to tmt's power of running a plan on top of many different infrastructures.

[lukaszachy]

I'm sorry this didn't get into 1.26, see above for reasons.

[frantisekz]

@frantisekz any chance you might be willing to delay your PR to 1.27 to be merged together with #1316?

Sure sure, no problem.

My main concern is the new options added by your patch, --cpus, --tpm, and --uefi. If we do indeed get rid of the EPEL8 limitation in 1.27, the HW requirements PR could get merged and I would invest my time into helping transition testcloud plugin to use the --hardware key/option & follow the existing specification.

Obviously, these new features are something you wish to see supported by the plugin, same here, yet I'm worried about the number of options duplicating what's already on the verge of being ready. EPEL8 was a long-time blocker, but since things started to move very recently, we might actually get rid of the limitation in 1.27, unblocking the consolidation.

I already updated the Beaker plugin in #1316, to "understand" common HW requirements as specified in hardware key. There is a CLI option, --hardware, mirroring the hardware key, allowing the same degree of CLI extensibility as --disk, --memory or --cpus, but the same language accepted by the hardware key: --hardware "cpu.processors>=4", --hardware "disk[0].size >= 100 GiB", --hardware "tpm.version == 2".

I'll remove exposure of --cpus, --tpm, and --uefi from this PR and make a separate one for it with the migration to a more standardized approach. From the testcloud development perspective, the far more important is the migration to the new api, the exposure of other params is a nice "side effect" (I do realize this is more important to tmt consumers). IMO, it'd also make more sense from commit/PR perspective to have these two split.

wdyt @happz ?

[happz]

My main concern is the new options added by your patch, --cpus, --tpm, and --uefi. If we do indeed get rid of the EPEL8 limitation in 1.27, the HW requirements PR could get merged and I would invest my time into helping transition testcloud plugin to use the --hardware key/option & follow the existing specification. Obviously, these new features are something you wish to see supported by the plugin, same here, yet I'm worried about the number of options duplicating what's already on the verge of being ready. EPEL8 was a long-time blocker, but since things started to move very recently, we might actually get rid of the limitation in 1.27, unblocking the consolidation. I already updated the Beaker plugin in #1316, to "understand" common HW requirements as specified in hardware key. There is a CLI option, --hardware, mirroring the hardware key, allowing the same degree of CLI extensibility as --disk, --memory or --cpus, but the same language accepted by the hardware key: --hardware "cpu.processors>=4", --hardware "disk[0].size >= 100 GiB", --hardware "tpm.version == 2".

I'll remove exposure of --cpus, --tpm, and --uefi from this PR and make a separate one for it with the migration to a more standardized approach.

From the testcloud development perspective, the far more important is the migration to the new api, the exposure of other params is a nice "side effect" (I do realize this is more important to tmt consumers). IMO, it'd also make more sense from commit/PR perspective to have these two split.

wdyt @happz ?

TBH, that would be the best. I'm not opposed to having these options per se, but I'd like to see them:

1. beeing the same across plugins that support hardware, i.e. -h virtual --memory 8Gib vs -h beaker --ram 8GiB would be a defeat, and
2. following the specification, i.e. --cpu-processors (or --cpu.processors?) rather than --cpus as cpus is not defined by https://tmt.readthedocs.io/en/stable/spec/hardware.html#cpu, --memory instead of --ram, `--tpm-version=2.0", and so on.

I'm pretty sure we can amend explicit "shortcuts" like --memory with whatever is in a plan's hardware field. My current idea would be to use and to merge these together

provision:
  hardware:
    cpu:
      processors: >= 6
    boot:
      method: uefi

$ tmt run -a provision --memory "> 8GiB" --disk "120 GiB" ...

could result in the following set of constraints:

and:
  - cpu:
      processors: >= 6
    boot:
      method: uefi
  - memory: > 8GiB
  - disk:
      size: 120 GiB

I bet there can be other ways, but in general, no problem with having these options. Adding them to artemis, beaker, and virtual plugins in a standalone set of patches would make sense to me. And don't worry, I'm perfectly fine spending my time on this, after all, I'm making things more complicated now, it's absolutely fitting for me to get rid of all the wrinkles in all those plugins later :)

[psss]

From the testcloud development perspective, the far more important is the migration to the new api...

Makes a complete sense to me as well to cover api changes first in a separate PR.

[happz]

@frantisekz I noticed you dropped the extra features, is your PR stable now, can I try to refactor arch/memory/disk to use the new HW requirements code on top of this patch? I'd start with these existing ones, then we can add tpm, CPU core, and the rest.

Edit: after reading the patch, I'm really looking forward to this! Code's now much cleaner and easier to navigate for me, and I can clearly see where testcloud and HW requirements would interact. This would be the first time the HW requirements code ported from Artemis would meet a plugin actually creating a guest, so far we were matching requirements with pre-existing instance types, and while in theory everything is clear, who knows how it all turns out :) But I'm confident we're going to make it work :)

[frantisekz]

@frantisekz I noticed you dropped the extra features, is your PR stable now, can I try to refactor arch/memory/disk to use the new HW requirements code on top of this patch? I'd start with these existing ones, then we can add tpm, CPU core, and the rest.

Yep, it's stable now. I had to look into CoreOS regression which is now resolved (I'll submit a newer testcloud build into the WIP copr).

Edit: after reading the patch, I'm really looking forward to this! Code's now much cleaner and easier to navigate for me, and I can clearly see where testcloud and HW requirements would interact. This would be the first time the HW requirements code ported from Artemis would meet a plugin actually creating a guest, so far we were matching requirements with pre-existing instance types, and while in theory everything is clear, who knows how it all turns out :) But I'm confident we're going to make it work :)

Thanks! There's still a long road ahead, but the baseline was reaaaally low :) . There is also bunch of stuff pending cleanup once I can remove back/forward compatibility from testcloud.

Thanks for the review items, I believe I'd resolved everything but the mypy which is being worked on right now.

[frantisekz]

Okay, all should be addressed and a new testcloud (0.9.2-40) has been built in the COPR.

[frantisekz]

Looks good and works nicely! Thanks! Added just one minor suggestion. Plus, I guess, we should also bump the testcloud require to make sure the new api is installed on the user system, right?

Updated, feedback addressed, thanks!

[psss]

Updated, feedback addressed, thanks!

Nice, thanks!

Hint: During the pull request review it's better to add new commits rather then amending existing ones. It makes it easier to see what actually changed, especially when the force-push is combined with a rebase.

[frantisekz]

Hint: During the pull request review it's better to add new commits rather then amending existing ones. It makes it easier to see what actually changed, especially when the force-push is combined with a rebase.

Yeah, sorry for that, I probably took it as too trivial no do it the proper way around, will pay attention to it the next time.

[psss]

Yeah, sorry for that, I probably took it as too trivial no do it the proper way around, will pay attention to it the next time.

Sure, no problemo.

[packit-as-a-service[bot]]

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'tmt': {'value': {'targets': ["Expected 'list[str]' or 'dict[str,dict]', got {'fedora-all': None, 'epel-9': None, 'fedora-39': {'additional_repos': 'https://download.copr.fedorainfracloud.org/results/frantisekz/testcloud-wip/fedora-39-x86_64/'}} (type <class 'dict'>)."]}}})}}}).

For more info, please check out the documentation or contact the Packit team.

[packit-as-a-service[bot]]

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'tmt': {'value': {'targets': ["Expected 'list[str]' or 'dict[str,dict]', got {'fedora-all': None, 'epel-9': None, 'fedora-39': {'additional_repos': ['https://download.copr.fedorainfracloud.org/results/frantisekz/testcloud-wip/fedora-39-x86_64/']}} (type <class 'dict'>)."]}}})}}}).

For more info, please check out the documentation or contact the Packit team.

[psss]

/packit test

[psss]

/packit build