or/and will override other hardware constraints in Beaker job XML

[skycastlelily]

Here is the hardware part of a reproducer plan file:

    hardware:
        disk:
            - size: "> 32 GiB"
            - size: "> 16 GiB"
        memory: "> 6 GiB"
        or:
          - cpu:
               model: 65
          - cpu:
               model: 67

ie, a server with two disks, memory> 6GiB, cpu model is 65 or 67 And here is the hostRequires part of the beaker job xml

      <hostRequires>
        <and>
          <cpu>
            <model op="==" value="65"/>
          </cpu>
        </and>
        <system_type value="Machine"/>
      </hostRequires>

disk.size and memory requires are abandoned somehow

[happz]

and/or shall not be mixed with other requirements in one block. tmt will indeed ignore requirements because of how this is parsed:

@ungroupify
def _parse_block(spec: Spec) -> BaseConstraint:
    ...

    if 'and' in spec:
        return _parse_and(spec['and'])

    if 'or' in spec:
        return _parse_or(spec['or'])

    return _parse_generic_spec(spec)

This is how it should be rewritten:

hardware:
  and:
    - disk:
        - size: "> 32 GiB"
        - size: "> 16 GiB"
    - memory: "> 6 GiB"
    - or:
      - cpu:
          model: 65
      - cpu:
          model: 67

The question is, did tmt complain about plan being incorrect? If it didn't that's the bug to fix. AFAICT, tmt should have complained because schema allows just one of the options:

  hardware:
    oneOf:
      - "$ref": "#/definitions/block"
      - "$ref": "#/definitions/and"
      - "$ref": "#/definitions/or"

So this seems like a question of better warning rather than and/or processing being broken.

[skycastlelily]

Tmt doesn't complaint,and here is the merge request make it complaint: https://github.com/teemtee/tmt/pull/3275😁

On Thu, Oct 10, 2024 at 5:20 PM Miloš Prchlík @.***> wrote:

and/or shall not be mixed with other requirements in one block. tmt will indeed ignore requirements because of how this is parsed:

@ungroupifydef _parse_block(spec: Spec) -> BaseConstraint: ...

if 'and' in spec:
    return _parse_and(spec['and'])

if 'or' in spec:
    return _parse_or(spec['or'])

return _parse_generic_spec(spec)

This is how it should be rewritten:

hardware: and:

• disk:
  • size: "> 32 GiB"
  • size: "> 16 GiB"
• memory: "> 6 GiB"
• or:
  • cpu: model: 65
  • cpu: model: 67

The question is, did tmt complain about plan being incorrect? If it didn't that's the bug to fix. AFAICT, tmt should have complained because schema allows just one of the options:

hardware: oneOf:

• "$ref": "#/definitions/block"
• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

So this seems like a question of better warning rather than and/or processing being broken.

— Reply to this email directly, view it on GitHub https://github.com/teemtee/tmt/issues/3273#issuecomment-2404558061, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23GH6IGHNO2BPWLIFXDZ2ZBDXAVCNFSM6AAAAABPWJJN4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBUGU2TQMBWGE . You are receiving this because you authored the thread.Message ID: @.***>

[happz]

But tmt does complain:

happz@multivac /tmp/foo $ cat plans.fmf 
discover:
  how: shell
  tests:
    - name: foo
      script: /bin/true

execute:
  how: tmt

provision:
    hardware:
        disk:
            - size: "> 32 GiB"
            - size: "> 16 GiB"
        memory: "> 6 GiB"
        or:
          - cpu:
               model: 65
          - cpu:
               model: 67
happz@multivac /tmp/foo $ tmt lint /plans
    warn: /plans:discover - {'how': 'shell', 'tests': [{'name': 'foo', 'script': '/bin/true'}]} is not valid under any of the given schemas
    warn: /plans:provision - {'hardware': {'disk': [{'size': '> 32 GiB'}, {'size': '> 16 GiB'}], 'memory': '> 6 GiB', 'or': [{'cpu': {'model': 65}}, {'cpu': {'model': 67}}]}, 'how': 'virtual'} is not valid under any of the given schemas
/plans
warn C000 key "tests" not recognized by schema /schemas/discover/fmf
warn C000 value of "how" is not "fmf"
warn C000 key "script" not recognized by schema, and does not match "^extra-" pattern
warn C000 value of "how" is not "artemis"
warn C000 value of "how" is not "beaker"
warn C000 key "hardware" not recognized by schema /schemas/provision/connect
warn C000 value of "how" is not "connect"
warn C000 key "hardware" not recognized by schema /schemas/provision/container
warn C000 value of "how" is not "container"
warn C000 key "hardware" not recognized by schema /schemas/provision/local
warn C000 value of "how" is not "local"
warn C000 key "hardware" not recognized by schema /schemas/provision/minute
warn C000 value of "how" is not "minute"
warn C000 key "hardware" not recognized by schema /schemas/provision/virtual
warn C000 fmf node failed schema validation
warn C001 summary key is missing

It reports plan fails the schema validation, and hardware key does not match the schema. So the problem is reported.

I wouldn't put a new extra special check into the code, we should instead try to focus on making the schema validation reporting better and easier to understand.

[happz]

I have some experiments with better JSON schema error rendering in my stash, I'll try to resurrect it & test it with the example above.

[skycastlelily]

It does not warn on my side. @.*** tmt]$ cat plans/mrack.fmf summary: Basic smoke test provision: how: beaker hardware: disk:

• size: "> 4096"

• size: "> 16 GiB" memory: "> 6 GiB"

  or:
    - hostname: dummy1.redhat.com
    - hostname: dummy2.redhat.com

discover:

• name: server-setup how: fmf test:
  • tests/multi-select @.*** tmt]$ tmt lint plans/mrack /plans/mrack pass C000 fmf node passes schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known pass P004 discover step methods are all known skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

The warns your plan get have nothing to do with or/and issue,

warn C000 key "hardware" not recognized by schema /schemas/provision/virtual

Because cpu.model is not supported by the default provision method,virtual,and if I add how: artemis, the warn is gone.

And I have modified the plan file quickly with the guide of warn, and here is the pass one:

@.*** tmt]$ tmt lint plans/check /plans/check pass C000 fmf node passes schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known skip P004 discover step is not defined skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

@.*** tmt]$ cat plans/check.fmf summary: Basic smoke test execute: how: tmt

provision: how: artemis hardware: disk:

• size: "> 32 GiB"
• size: "> 16 GiB" memory: "> 6 GiB" or:
  • cpu: model: 65
  • cpu: model: 67

I wouldn't put a new extra special check into the code, we should instead try to focus on making the schema validation reporting better and easier to understand.

Make sense, we should also work on making lint warn, but some users may don't know they need to lint the plan,and they end up with getting a machine they don't want, that could be a serious problem,it could prevent QEs find bugs, they thought the test pass on a nvme server,as tmt told them so,but may the pass got from a normal server,what do you think?😁

[happz]

It does not warn on my side.

Well, that's a problem, I suppose, it really should complain about something. I see now that how: beaker would remove the virtual note, but that's it - there should be a complaint about failed validation. It's not good or acceptable that there is no warning emitted by schema validation.

Make sense, we should also work on making lint warn, but some users may don't know they need to lint the plan,and they end up with getting a machine they don't want, that could be a serious problem,it could prevent QEs find bugs, they thought the test pass on a nvme server,as tmt told them so,but may the pass got from a normal server,what do you think?😁

Yeah, this is all valid - but we are not disagreeing on the outcome, just on the method. I say the schema validation must report this, it doesn't, and that's a bug. Adding an extra check to tmt.hardware does not fix this bug, just hides it. It's as simple as that, the bug lies elsewhere, not in tmt.hardware.

[happz]

It might be as easy as this:

diff --git a/tmt/schemas/provision/hardware.yaml b/tmt/schemas/provision/hardware.yaml
index 53ce43a4..495be6c9 100644
--- a/tmt/schemas/provision/hardware.yaml
+++ b/tmt/schemas/provision/hardware.yaml
@@ -440,6 +440,8 @@ definitions:
             - "$ref": "#/definitions/and"
             - "$ref": "#/definitions/or"

+    additionalProperties: false
+
     required:
       - "and"

@@ -454,6 +456,8 @@ definitions:
             - "$ref": "#/definitions/and"
             - "$ref": "#/definitions/or"

+    additionalProperties: false
+
     required:
       - "or"

Both and and or allowed other properties to appear - after forbidding that, suddenly, it's either one of three variants, no overlaps. With this patch, you should see a warning.

[skycastlelily]

Wow, learned, thanks for your mentor^^ and so efficient, tbo, I was complacent 🤣about finding your plan file problem at a glance,and quickly find a truth to support my merge request.

On Thu, Oct 10, 2024 at 11:09 PM Miloš Prchlík @.***> wrote:

It might be as easy as this:

diff --git a/tmt/schemas/provision/hardware.yaml b/tmt/schemas/provision/hardware.yaml index 53ce43a4..495be6c9 100644--- a/tmt/schemas/provision/hardware.yaml+++ b/tmt/schemas/provision/hardware.yaml@@ -440,6 +440,8 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"
  • additionalProperties: false+ required:
    • "and" @@ -454,6 +456,8 @@ definitions:
• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"
  • additionalProperties: false+ required:
    • "or"

Both and and or allowed other properties to appear - after forbidding that, suddenly, it's either one of three variants, no overlaps. With this patch, you should see a warning.

— Reply to this email directly, view it on GitHub https://github.com/teemtee/tmt/issues/3273#issuecomment-2405392573, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23DZMYQE73X4JYMWRZLZ22KA5AVCNFSM6AAAAABPWJJN4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBVGM4TENJXGM . You are receiving this because you authored the thread.Message ID: @.***>

[happz]

Wow, learned, thanks for your mentor^^ and so efficient, tbo, I was complacent 🤣about finding your plan file problem at a glance,and quickly find a truth to support my merge request.

Yeah, I'm a bloody genius, and I managed to mistake the virtual-related warnings for those related to the actual problem, which costed me half a day ;)

Anyway, will you update the PR, or shall I submit a new one?

[skycastlelily]

which costed me half a day ;)

I spent one more hour on #3274, still didn't get any luck🥹😁

Anyway, will you update the PR, or shall I submit a new one?

I'd prefer update the PR,AYK,I'm eagerly to send as more as possible merge requests😍

On Fri, Oct 11, 2024 at 1:46 PM Miloš Prchlík @.***> wrote:

Wow, learned, thanks for your mentor^^ and so efficient, tbo, I was complacent 🤣about finding your plan file problem at a glance,and quickly find a truth to support my merge request.

Yeah, I'm a bloody genius, and I managed to mistake the virtual-related warnings for those related to the actual problem, which costed me half a day ;)

Anyway, will you update the PR, or shall I submit a new one?

— Reply to this email directly, view it on GitHub https://github.com/teemtee/tmt/issues/3273#issuecomment-2406595810, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23AWFJ26SK45M7CL4WDZ25Q3BAVCNFSM6AAAAABPWJJN4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBWGU4TKOBRGA . You are receiving this because you authored the thread.Message ID: @.***>

[skycastlelily]

After add additionalProperties: false as you suggested,tmt run complaints as expected,but neither tmt lint nor tmt run give enough information for non-tmt-professional users understand what is going on,some users may ignore the warning.

(dev) @.*** tmt]$ tmt run plans --name plans/mrack$ /var/tmp/tmt/run-158 warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk': [{'size': '> 4096'}, {'size': '> 16 GiB'}], 'or': [{'hostname': ' hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com'}, {'hostname': ' koza-4.khw.eng.rdu2.dc.redhat.com'}]}} is not valid under any of the given schemas

/plans/mrack discover how: shell summary: 1 test selected provision queued provision.provision task #1: default-0

    provision.provision task #1: default-0
    how: beaker
    hardware:
        disk:
          - size: '> 4096'
          - size: '> 16 GiB'
        or:
          - hostname: hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com
          - hostname: koza-4.khw.eng.rdu2.dc.redhat.com

^C^C finish warn: Nothing to finish, no guests provisioned.

Aborted! (dev) @.*** tmt]$ tmt lint plans/mrack warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk': [{'size': '> 4096'}, {'size': '> 16 GiB'}], 'or': [{'hostname': ' hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com'}, {'hostname': ' koza-4.khw.eng.rdu2.dc.redhat.com'}]}} is not valid under any of the given schemas /plans/mrack warn C000 value of "how" is not "artemis" warn C000 key "hardware" not recognized by schema /schemas/provision/connect warn C000 value of "how" is not "connect" warn C000 key "hardware" not recognized by schema /schemas/provision/container warn C000 value of "how" is not "container" warn C000 key "hardware" not recognized by schema /schemas/provision/local warn C000 value of "how" is not "local" warn C000 key "hardware" not recognized by schema /schemas/provision/minute warn C000 value of "how" is not "minute" warn C000 key "hardware" not recognized by schema /schemas/provision/virtual warn C000 value of "how" is not "virtual" warn C000 fmf node failed schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known skip P004 discover step is not defined skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk':

[{'size': '> 4096'}, {'size': '> 16 GiB'}], 'or': [{'hostname': ' hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com'}, {'hostname': ' koza-4.khw.eng.rdu2.dc.redhat.com'}]}, 'name': 'default-0'} is not valid under any of the given schemas Lint checks on all pass G001 no duplicate ids detected

How about making schema validation report this,and add additional check in tmt.hardware,and offer a user-friendly message ?:)

Here is the output with both implemented^^

(dev) @.*** tmt]$ tmt run plans --name plans/mrack$ /var/tmp/tmt/run-161 warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk': [{'size': '> 4096'}, {'size': '> 16 GiB'}], 'or': [{'hostname': ' hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com'}, {'hostname': ' koza-4.khw.eng.rdu2.dc.redhat.com'}]}} is not valid under any of the given schemas

/plans/mrack

plan failed

The exception was caused by 1 earlier exceptions

Cause number 1:

Failed to load step data for ProvisionBeakerData: only one of

block/and/or is allowed

The exception was caused by 1 earlier exceptions

Cause number 1:

    only one of block/and/or is allowed

On Sat, Oct 12, 2024 at 12:09 AM Lili Nie @.***> wrote:

After add maxProperties: 1,tmt complaints, but not very clear, I guess we should tell users only one of any/and/block is allowed:)

(dev) @.*** tmt]$ git diff diff --git a/tmt/schemas/provision/hardware.yaml b/tmt/schemas/provision/hardware.yaml index 53ce43a4..b8181a83 100644 --- a/tmt/schemas/provision/hardware.yaml +++ b/tmt/schemas/provision/hardware.yaml @@ -440,6 +440,9 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false
• maxProperties: 1

• required:

  • "and"

@@ -454,6 +457,9 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false
• maxProperties: 1

• required:(dev) @.*** tmt]$ tmt lint plans/mrack warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk': [{'size': '> 4096'}, {'size': '> 16 GiB'}], 'memory': '> 6 GiB', 'or': [{'hostname': 'dummy1.redhat.com'}, {'hostname': 'dummy2.redhat.com'}]}} is not valid under any of the given schemas /plans/mrack warn C000 value of "how" is not "artemis" warn C000 key "hardware" not recognized by schema /schemas/provision/connect warn C000 value of "how" is not "connect" warn C000 key "hardware" not recognized by schema /schemas/provision/container warn C000 value of "how" is not "container" warn C000 key "hardware" not recognized by schema /schemas/provision/local warn C000 value of "how" is not "local" warn C000 key "hardware" not recognized by schema /schemas/provision/minute warn C000 value of "how" is not "minute" warn C000 key "hardware" not recognized by schema /schemas/provision/virtual warn C000 value of "how" is not "virtual" warn C000 fmf node failed schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known pass P004 discover step methods are all known skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

  warn: /plans/mrack:provision - {'how': 'beaker', 'hardware': {'disk': [{'size': '> 4096'}, {'size': '> 16 GiB'}], 'memory': '> 6 GiB', 'or': [{'hostname': 'hpe-dl385gen8-01.khw.eng.rdu2.dc.redhat.com'}, {'hostname': 'koza-4.khw.eng.rdu2.dc.redhat.com'}]}, 'name': 'default-0'} is not valid under any of the given schemas Lint checks on all pass G001 no duplicate ids detected

  • "or"

On Fri, Oct 11, 2024 at 11:48 PM Lili Nie @.***> wrote:

Turns out it's not that easy🤣🤔

@.*** tmt]$ tmt lint plans/mrack /plans/mrack pass C000 fmf node passes schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known pass P004 discover step methods are all known skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

@. tmt]$ vi plans/mrack.fmf @. tmt]$ git diff diff --git a/tmt/schemas/provision/hardware.yaml b/tmt/schemas/provision/hardware.yaml index 53ce43a4..495be6c9 100644 --- a/tmt/schemas/provision/hardware.yaml +++ b/tmt/schemas/provision/hardware.yaml @@ -440,6 +440,8 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false

• required:

  • "and"

@@ -454,6 +456,8 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false

• required:

  • "or"

@.*** tmt]$ cat plans/mrack.fmf summary: Basic smoke test provision: how: beaker hardware: disk:

• size: "> 4096"

• size: "> 16 GiB" memory: "> 6 GiB"

  or:
    - hostname: dummpy.redhat.com
    - hostname: ***@***.*** tmt]$ tmt lint plans/mrack

  /plans/mrack pass C000 fmf node passes schema validation pass C001 summary key is set and is reasonably long pass P001 correct keys are used pass P002 execute step defined with "how" pass P003 execute step methods are all known pass P004 discover step methods are all known skip P005 no remote fmf ids defined pass P006 phases have unique names pass P007 prepare phase 'default-0' does not require specific guest pass P007 prepare phase 'default-1' does not require specific guest pass P007 execute phase 'default-0' does not require specific guest

@. tmt]$ vi plans/mrack.fmf @. tmt]$ git diff diff --git a/tmt/schemas/provision/hardware.yaml b/tmt/schemas/provision/hardware.yaml index 53ce43a4..495be6c9 100644 --- a/tmt/schemas/provision/hardware.yaml +++ b/tmt/schemas/provision/hardware.yaml @@ -440,6 +440,8 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false

• required:

  • "and"

@@ -454,6 +456,8 @@ definitions:

• "$ref": "#/definitions/and"
• "$ref": "#/definitions/or"

• additionalProperties: false

• required:

  • "or"

@.*** tmt]$ cat plans/mrack.fmf summary: Basic smoke test provision: how: beaker hardware: disk:

• size: "> 4096"

• size: "> 16 GiB" memory: "> 6 GiB"

  or:
    - hostname: dummpy.redhat.com
    - hostname: dummpy2.redhat.com

On Fri, Oct 11, 2024 at 2:50 PM Lili Nie @.***> wrote:

which costed me half a day ;)

I spent one more hour on #3274, still didn't get any luck🥹😁

Anyway, will you update the PR, or shall I submit a new one?

I'd prefer update the PR,AYK,I'm eagerly to send as more as possible merge requests😍

On Fri, Oct 11, 2024 at 1:46 PM Miloš Prchlík @.***> wrote:

Wow, learned, thanks for your mentor^^ and so efficient, tbo, I was complacent 🤣about finding your plan file problem at a glance,and quickly find a truth to support my merge request.

Yeah, I'm a bloody genius, and I managed to mistake the virtual-related warnings for those related to the actual problem, which costed me half a day ;)

Anyway, will you update the PR, or shall I submit a new one?

— Reply to this email directly, view it on GitHub https://github.com/teemtee/tmt/issues/3273#issuecomment-2406595810, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23AWFJ26SK45M7CL4WDZ25Q3BAVCNFSM6AAAAABPWJJN4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBWGU4TKOBRGA . You are receiving this because you authored the thread.Message ID: @.***>