Specification for reusable hardware configurations#4833
Conversation
hostfilter for reusable hardware configurations
There was a problem hiding this comment.
Code Review
This pull request introduces a new specification for referencing external hardware filter configurations via the hostfilter key in .fmf files. The feedback suggests ensuring the description field starts with the summary text to maintain consistency with existing documentation standards.
hostfilter for reusable hardware configurations9c8fe30 to
9fcc235
Compare
|
@AthreyVinay so this is just adding the schema right? In that case I would update the description. |
yes, spec-only, following the same pattern as |
9fcc235 to
19b30be
Compare
LecrisUT
left a comment
There was a problem hiding this comment.
Minor comments, otherwise lgtm. Re the note, fine with or without it.
| memory: ">= 64 GB" | ||
|
|
||
| A filter reference expands to all of its defined constraints combined | ||
| with AND, and can be used anywhere a hardware constraint can appear. |
There was a problem hiding this comment.
This part reads ambiguous. The AND there relates to the sibling keys, but this can read as if everything in the preset yaml is anded (whatever that would mean)
There was a problem hiding this comment.
Wording has been updated, @LecrisUT, please re-review.
There was a problem hiding this comment.
@LecrisUT Hi! Most of your comments must be addressed in latest spec:)
There was a problem hiding this comment.
So just to confirm, there is no more implicit and gluing and something like
provision:
hardware:
import-preset:
path: ~/.config/tmt/presets.yml
preset: BIG_MEMORY
cpu:
model: 97is forbidden? The current state is ambiguous about this
There was a problem hiding this comment.
This is the default way how hardware constraints are handled:
When multiple environment requirements are provided the provision implementation should attempt to satisfy all of them. It is also possible to write this explicitly using the and operator containing a list of dictionaries with individual requirements.
There was a problem hiding this comment.
Being explicit if it is supported or not and how it behaves would be appreciated
There was a problem hiding this comment.
Merging still happens, as it always did, preset is just another key, and just like and or or keys it translates into a group of constraints. Same as the following:
provision:
hardware:
import-preset:
path: ~/.config/tmt/presets.yml
preset: BIG_MEMORY
cpu:
model: 97
memory: 4 GBtranslates to:
hardware:
and:
- whatever-is-behind-BIG_MEMORY
- cpu:
model: 97
- memory: 4 GB
psss
left a comment
There was a problem hiding this comment.
Thanks for outlining the spec! I'm a bit hesitant about separating the preset fetch key from the hardware section. Wouldn't it be a bit better to group all hardware-related stuff under a single umbrella? Also, should we support multiple presets as well?
@bmeneg, could you please have a look at the proposed syntax? Any suggestions from your side?
Please, see the previous threads, namely #4833 (comment) and #4833 (comment).
We already do. A single file can define multiple presets, and they can be used with the help of hardware:
- and:
- preset: AMD__ROME
- preset: BIG_MEMORY |
| # Use a remote preset | ||
| provision: | ||
| hardware-preset: | ||
| url: https://github.com/my-team/presets.yml |
There was a problem hiding this comment.
Or alternatively it could be moved into
hardwarelevel? That way we could also merge thefilterinto itI'd prefer not to have it there, although it would make some sense for sure.
I see that both approaches have some advantages. Although from implementation point of view the separate key could be (in some respect) a bit simpler, I'm trying to look at it from the user perspective and having all hardware-related stuff under one section currently wins.
Also from the separation of concerns perspective I'd say it makes a perfect sense if the Hardware class handles everything needed for the full hardware requirements parsing, including fetching the presets.
Allowing it under
hardwarewould open new can of worms: wouldpresetbe allowed to used everywhere in the tree, should it be resolved first, what about backreferences?
It might not be such a big can of worms if we clearly state that the import-preset keyword needs to be only at the top level of the hardware key. From implementation point of view it would be just popping the key as the first step and then handling the rest as constraints.
There was a problem hiding this comment.
So, to summarize so @AthreyVinay can continue:
hardware:
import-preset:
- url: https://github.com/my-team/project/presets.yml
- path: ~/.config/tmt/presets.yml
and:
- preset: AMD__ROME
- preset: BIG_MEMORY
- cpu:
model: 79
import-presetandpresetimport-presetallowed only on the top-levelpresetis a string, not a string or list of stringsimport-presetcan be a mapping or list of mappings
Did I miss anything?
There was a problem hiding this comment.
Yeah, exactly. Thanks for the summary.
There was a problem hiding this comment.
I have a patch that extends hardware key, allowing user to use a URL or a file (hardware: @foo.yaml, or --hardware http://...). I followed the syntax we use for envvar files (--environment @foo.env). Would it make sense, instead of path and url keys, use the same approach?
hardware:
import-preset:
- https://github.com/my-team/project/presets.yml
- @~/.config/tmt/presets.ymlOr without @, that would work as well, since we have tmt.utils.is_url() helper we can use to tell whether the item looks like an URL or not, and handle it accordingly.
While I was hacking on that hardware patch, I noticed there's an opportunity for "load me content from this source" operation, used at several places, environment and hardware being two examples, this one seems like another. It would be nice to present a consistent input format eventually.
There was a problem hiding this comment.
Sure +1 for consistency.
There was a problem hiding this comment.
Good idea. +1 from me to drop the keys and just have a list of links. I would vote for not using @ as I understand it as instead of defining the value here check the file which is not the case here. We could also support the standard file:// prefix. So what about these three options?
hardware:
import-preset:
- https://github.com/my-team/project/presets.yml
- file:///home/user/config/tmt/presets.yml
- ~/.config/tmt/presets.ymlWhile I was hacking on that
hardwarepatch, I noticed there's an opportunity for "load me content from this source" operation, used at several places,environmentandhardwarebeing two examples, this one seems like another. It would be nice to present a consistent input format eventually.
Agreed, would be nice to handle these consistently.
There was a problem hiding this comment.
That's also a +1 from my side on dropping the keywords and use links directly.
c03ce60 to
f3d0ec1
Compare
f3d0ec1 to
ac88bda
Compare
| model: 49 | ||
| - name: MEMORY_MIN_64G | ||
| hardware: | ||
| memory: ">= 64 GB" |
There was a problem hiding this comment.
Speaking about simplification, what about using dictionary layout for the presets.yaml files? For the above-mentioned example it would look like this:
AMD__ROME:
cpu:
family: 23
model: 49
BIG_MEMORY:
memory: ">= 64 GB"It's basically mapping a preset name to dictionary content. No need to repeat hardware again and again. Everything is a hardware preset.
There was a problem hiding this comment.
I like the explicitness of having the hardware key. It reads more clearly how it connects to provision.hardware
There was a problem hiding this comment.
I don't have a strong opinion here, but I like the explicitness too, making it clear it accepts only hardware definitions (seems obvious, but we never know...). However I also understand why removing the hardware does make sense: it gives the impression that the resulting mapping would be:
provision:
hardware:
# preset expanded here
hardware:
cpu:
family: 23
model: 49
resolves: #4693
Pull Request Checklist