Translate Or constraint properly for mrack
#3327
Conversation
|
Fix #3274 ^^ |
tmt/steps/provision/mrack.py
Outdated
| constraint_to_beaker_filter(constraint, logger) | ||
| for constraint in hw.constraint.variant() | ||
| ]) | ||
| if isinstance(hw.constraint, tmt.hardware.Or): |
There was a problem hiding this comment.
Why is this needed when constraint_to_beaker_filter() seems to do the same thing?
def constraint_to_beaker_filter(
constraint: tmt.hardware.BaseConstraint,
logger: tmt.log.Logger) -> MrackBaseHWElement:
""" Convert a hardware constraint into a Mrack-compatible filter """
if isinstance(constraint, tmt.hardware.And):
return MrackHWAndGroup(
children=[
constraint_to_beaker_filter(child_constraint, logger)
for child_constraint in constraint.constraints
]
)
if isinstance(constraint, tmt.hardware.Or):
return MrackHWOrGroup(
children=[
constraint_to_beaker_filter(child_constraint, logger)
for child_constraint in constraint.constraints
]
)IIUIC, when called, constraint_to_beaker_filter() runs isinstance() calls and handles And and Or when needed, so, to me, this is suspicious: constraint_to_beaker_filter() should take care of this, if it does not, then why, and how it needs to be fixed? Because adding a special bit of code here won't help, there may be nested Or uses, deeper in the tree of HW requirements, and this particular change would not handle those. That's why it's in constraint_to_beaker_filter(), it scans the whole tree and there shouldn't be a difference between the top-level one or a deeper one, i.e. this function is the one to debug and fix.
skycastlelily
force-pushed
the
or-mrack
branch
from
f333ffc
to
928b391
Compare
|
constraint_to_beaker_filter() should take care of this
ah, yeah, and my fix was ugly:(
Updated^^
Btw, I'm thinking to extend the test to cover the or situation, I need
import TmtBeakerTransformer,but it is in def import_and_load_mrack_deps
function🤣🤔
…On Wed, Oct 30, 2024 at 3:52 PM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tmt/steps/provision/mrack.py
<#3327 (comment)>:
> @@ -714,12 +714,14 @@ def _translate_tmt_hw(self, hw: tmt.hardware.Hardware) -> dict[str, Any]:
""" Return hw requirements from given hw dictionary """
assert hw.constraint
-
- transformed = MrackHWAndGroup(
- children=[
- constraint_to_beaker_filter(constraint, logger)
- for constraint in hw.constraint.variant()
- ])
+ if isinstance(hw.constraint, tmt.hardware.Or):
Why is this needed when constraint_to_beaker_filter() seems to do the
same thing?
def constraint_to_beaker_filter(
constraint: tmt.hardware.BaseConstraint,
logger: tmt.log.Logger) -> MrackBaseHWElement:
""" Convert a hardware constraint into a Mrack-compatible filter """
if isinstance(constraint, tmt.hardware.And):
return MrackHWAndGroup(
children=[
constraint_to_beaker_filter(child_constraint, logger)
for child_constraint in constraint.constraints
]
)
if isinstance(constraint, tmt.hardware.Or):
return MrackHWOrGroup(
children=[
constraint_to_beaker_filter(child_constraint, logger)
for child_constraint in constraint.constraints
]
)
IIUIC, when called, constraint_to_beaker_filter() runs isinstance() calls
and handles And and Or when needed, so, to me, this is suspicious:
constraint_to_beaker_filter() should take care of this, if it does not,
then why, and how it needs to be fixed? Because adding a special bit of
code here won't help, there may be nested Or uses, deeper in the tree of
HW requirements, and this particular change would not handle those. That's
why it's in constraint_to_beaker_filter(), it scans the whole tree and
there shouldn't be a difference between the top-level one or a deeper one,
i.e. this function is the one to debug and fix.
—
Reply to this email directly, view it on GitHub
<#3327 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23ERSAFGJ5YV7INDAMDZ6CFVPAVCNFSM6AAAAABQ3NHEU6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBTHA4DOMJXGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I played with it a bit, so I know why it's broken, and while your fix should work, it would be extremely useful if the future reader - other developers, or even your future self - would understand why. A comment here and there would help a lot. I ended up with the following, feel free to use the comments and/or the code: diff --git a/tmt/steps/provision/mrack.py b/tmt/steps/provision/mrack.py
index 82bd57ab..5d9b552c 100644
--- a/tmt/steps/provision/mrack.py
+++ b/tmt/steps/provision/mrack.py
@@ -715,14 +715,42 @@ def import_and_load_mrack_deps(workdir: Any, name: str, logger: tmt.log.Logger)
assert hw.constraint
- transformed = MrackHWAndGroup(
+ # For each constraint variant, create one `<and/>` element.
+ # Then group them all in a single, top-level `<or/>` element.
+ # This way, we tell Beaker to pick machines matching either
+ # of the allowed variants.
+ transformed = MrackHWOrGroup(
children=[
- constraint_to_beaker_filter(constraint, logger)
- for constraint in hw.constraint.variant()
- ])
+ MrackHWAndGroup(
+ children=[
+ constraint_to_beaker_filter(constraint, logger)
+ for constraint in variant
+ ])
+ for variant in hw.constraint.variants()
+ ])
logger.debug('Transformed hardware', tmt.utils.dict_to_yaml(transformed.to_mrack()))
+ # The transformation above should be perfectly valid, but:
+ # Beaker should be pretty happy when facing the whole
+ # constraint, without us splitting it into variants. Beaker,
+ # unlike instance-type-based infrastructures like AWS, does
+ # have the actual filtering, and can express `or` and `and`
+ # groups. And our `constraint_to_beaker_filter()` does that,
+ # for those groups nested deeper in the tree. On top-level,
+ # we still work with variants because it's aligned with the
+ # rest of the world (and it's how Artemis implemented this).
+ #
+ # For the sake of testing, let's translate the whole tree as
+ # well, and log it, but until we better test this approach,
+ # stick to variants. But eventually, we should probably
+ # switch to this approach, it better resembles the original
+ # HW requirement tree.
+ transformed_without_variants = constraint_to_beaker_filter(hw.constraint, logger)
+
+ logger.debug('Transformed hardware (without variants)',
+ tmt.utils.dict_to_yaml(transformed_without_variants.to_mrack()))
+
return {
'hostRequires': transformed.to_mrack()
}
Absolutely, do add even a simple test, e.g. the one you started with, |
skycastlelily
requested review from
psss,
lukaszachy,
thrix and
janhavlin
as code owners
|
Just to be clear, we need transformed_without_variants approach or transformed_with_variants,not both,right? For the following requirements: The without approach would translate it to: The with one would translate it to: The latter one could be much longer than the first one,plus, the first one is more clearer.
I analysed and tested it thoroughly, and IMO,it's very safe for us to switch to no variants approach:)
Considering you are in doctor's office(:hug),I guess,by saying that you may mean add test to tests/unit/test_hardware.py(#48b9811? |
|
(Good to see you are starting to use the full power of Github markdown, it's much, much easier to follow your thoughts :)
Correct, we need just one of them, not both.
I did notice :) I shared my comment mostly because of the comments: I reached the same solution as you did, I just added explanation of what's happening.
Correct again, one of the approaches does emit more elements, because some constraints are part of multiple variants.
Well, I didn't test it fully. If you did, then we're fine, let's use the direct approach, without variants. Which you did already, but PR lacks the comments explaining why. We can see what is happening, but we often do not recall why...
|
Thanks for your patience, mentor^^
✨
Yeah.#48b9811 is for more generic HW stuff, so I should put it in test_hardware.py ,right?And ,I need to add another new test to mrack/test_hw.py , I got stuck on importing TmtBeakerTransformer,and may need to ping you^^ |
Pull Request Checklist