Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
skycastlelily opened this issue Oct 10, 2024 · 11 comments
Open

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

skycastlelily opened this issue Oct 10, 2024 · 11 comments
Labels
area | hardware Implementation of hardware requirements plugin | mrack The beaker provision plugin

Comments

@skycastlelily
Copy link
Collaborator

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 happz changed the title or/and will override other hardware constraints or/and will override other hardware constraints in Beaker job XML Oct 10, 2024
@happz happz added area | hardware Implementation of hardware requirements plugin | mrack The beaker provision plugin labels Oct 10, 2024
@happz
Copy link
Collaborator

happz commented Oct 10, 2024

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
Copy link
Collaborator Author

skycastlelily commented Oct 10, 2024 via email

@happz
Copy link
Collaborator

happz commented Oct 10, 2024

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
Copy link
Collaborator

happz commented Oct 10, 2024

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
Copy link
Collaborator Author

skycastlelily commented Oct 10, 2024 via email •
edited
Loading

@happz
Copy link
Collaborator

happz commented Oct 10, 2024

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
Copy link
Collaborator

happz commented Oct 10, 2024

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
Copy link
Collaborator Author

skycastlelily commented Oct 10, 2024 via email

@happz
Copy link
Collaborator

happz commented Oct 11, 2024

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
Copy link
Collaborator Author

skycastlelily commented Oct 11, 2024 via email

@skycastlelily
Copy link
Collaborator Author

skycastlelily commented Oct 14, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area | hardware Implementation of hardware requirements plugin | mrack The beaker provision plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@happz @skycastlelily