Translate Or constraint properly for mrack

[skycastlelily]

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• adjust plugin docstring
• modify the json schema
• mention the version
• include a release note

[skycastlelily]

Fix #3274 ^^

[skycastlelily]

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: ***@***.***>

[happz]

constraint_to_beaker_filter() should take care of this

ah, yeah, and my fix was ugly:( Updated^^

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()
                 }

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🤣🤔

Absolutely, do add even a simple test, e.g. the one you started with, or with two hostname items, that should be more than enough to verify the fix.

[skycastlelily]

Just to be clear, we need transformed_without_variants approach or transformed_with_variants,not both,right?
Btw, you may not notice that your transformed_without_variants approach is right the content of this merge request:)
AYMK, the transformed(with_variants) approach would produce some duplication elements,here are for following reviewers and maybe you:

For the following requirements:

        and:
          - beaker:
                pool: '!= foo.*'
          - or:
              - cpu:
                   processors: "> 2"
              - cpu:
                   cores: ">= 2"
              - cpu:
                   model-name: "~ wee.*"
              - cpu:
                   hyper-threading: true
          - disk:
              size: ">= 200"

The without approach would translate it to:

<hostRequires>
      <and>
        <pool op="!=" value="foo.*"/>
        <or>
          <cpu>
            <processors op="&gt;" value="2"/>
          </cpu>
          <cpu>
            <cores op="&gt;=" value="2"/>
          </cpu>
          <cpu>
            <model_name op="like" value="wee%"/>
          </cpu>
          <cpu>
            <hyper op="==" value="True"/>
          </cpu>
        </or>
        <disk>
          <size op="&gt;=" value="200"/>
        </disk>
      </and>
      <system_type value="Machine"/>
    </hostRequires>

The with one would translate it to:

    <hostRequires>
        <or>
          <and>
            <cpu>
              <processors op="&gt;" value="2"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <cores op="&gt;=" value="2"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <model_name op="like" value="wee%"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <hyper op="==" value="True"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
        </or>
        <system_type value="Machine"/>
      </hostRequires>

The latter one could be much longer than the first one,plus, the first one is more clearer.

but until we better test this approach, stick to variants

I analysed and tested it thoroughly, and IMO,it's very safe for us to switch to no variants approach:)

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🤣🤔
or with two hostname items, that should be more >than enough to verify the fix.

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?
However,this mr is supposed to fix _translate_tmt_hw error(so I guess I need to import TmtBeakerTransformer anyway, unless you professional has better approach?) , while the tests in tests/unit/provision/mrack/test_hw.py is testing constraint_to_beaker_filter:)

[happz]

(Good to see you are starting to use the full power of Github markdown, it's much, much easier to follow your thoughts :)

Just to be clear, we need transformed_without_variants approach or transformed_with_variants,not both,right?

Correct, we need just one of them, not both.

Btw, you may not notice that your transformed_without_variants approach is right the content of this merge request:)

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.

AYMK, the transformed(with_variants) approach would produce some duplication elements,here are for following reviewers and maybe you:

For the following requirements:

        and:
          - beaker:
                pool: '!= foo.*'
          - or:
              - cpu:
                   processors: "> 2"
              - cpu:
                   cores: ">= 2"
              - cpu:
                   model-name: "~ wee.*"
              - cpu:
                   hyper-threading: true
          - disk:
              size: ">= 200"

The without approach would translate it to:

<hostRequires>
      <and>
        <pool op="!=" value="foo.*"/>
        <or>
          <cpu>
            <processors op="&gt;" value="2"/>
          </cpu>
          <cpu>
            <cores op="&gt;=" value="2"/>
          </cpu>
          <cpu>
            <model_name op="like" value="wee%"/>
          </cpu>
          <cpu>
            <hyper op="==" value="True"/>
          </cpu>
        </or>
        <disk>
          <size op="&gt;=" value="200"/>
        </disk>
      </and>
      <system_type value="Machine"/>
    </hostRequires>

The with one would translate it to:

    <hostRequires>
        <or>
          <and>
            <cpu>
              <processors op="&gt;" value="2"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <cores op="&gt;=" value="2"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <model_name op="like" value="wee%"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
          <and>
            <cpu>
              <hyper op="==" value="True"/>
            </cpu>
            <pool op="!=" value="foo.*"/>
            <disk>
              <size op="&gt;=" value="200"/>
            </disk>
          </and>
        </or>
        <system_type value="Machine"/>
      </hostRequires>

Correct again, one of the approaches does emit more elements, because some constraints are part of multiple variants.

but until we better test this approach, stick to variants

I analysed and tested it thoroughly, and IMO,it's very safe for us to switch to no variants approach:)

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...

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🤣🤔
or with two hostname items, that should be more >than enough to verify the fix.

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? However,this mr is supposed to fix _translate_tmt_hw error(so I guess I need to import TmtBeakerTransformer anyway, unless you professional has better approach?) , while the tests in tests/unit/provision/mrack/test_hw.py is testing constraint_to_beaker_filter:)

tests/unit/provision/mrack/test_hw.py exists to contain tests of how beaker plugin handles HW requirements. I'd say this new test belongs to this file, tests/unit/test_hardware.py is for more generic HW stuff.

[skycastlelily]

(Good to see you are starting to use the full power of Github markdown, it's much, much easier to follow your thoughts :)

Thanks for your patience, mentor^^

Well, I didn't test it fully. If you did, then we're fine, let's use the direct approach, without variants.

✨

tests/unit/provision/mrack/test_hw.py exists to contain tests of how beaker plugin handles HW requirements. I'd say this new test belongs to this file, tests/unit/test_hardware.py is for more generic HW stuff.

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^^