cluster value in vm_attr for compute profiles expect string

[cmeissner]

SUMMARY

If we want to create compute profiles for vsphere compute resource the value of cluster within vm_attrs can't be an integer. We need to put an integer in quotes.
As we can use numbers for cluster and or datacenter in Sphere configuration the module should also accept integers and may be do a type casting inside the module.

ISSUE TYPE

• Bug Report

ANSIBLE VERSION

ansible >= 2.9.0, <= 2.9.10

COLLECTION VERSION

theforeman.foreman:2.1.0

KATELLO/FOREMAN VERSION

foreman-server 2.1.4

STEPS TO REPRODUCE

This code should reproduce the issue.

- name: setup compute profiles
  vars:
    compute_profile:
      name: "generic"
      compute_attributes:
        - compute_resource: "vsphere.example.com"
          vm_attrs:
            cpus: 1
            sockets: 1
            memory_mb: '2048'
            firmware: automatic
            cluster: 1337
            resource_pool: Resources
            path: "/Datacenters/1337/vm/1337_VM"
            guest_id: otherGuest64
            hardware_version: Default
            memoryHotAddEnabled: '0'
            cpuHotAddEnabled: '0'
            add_cdrom: '0'
            boot_order:
              - network
              - disk
            annotation: 'Foreman controlled vm'
            scsi_controllers:
              - type: VirtualLsiLogicController
                key: 1000
            interfaces_attributes:
              '0':
                type: VirtualE1000
                network: 1337_Service_PROD_1200
            volumes_attributes:
              '0':
                name: Hard disk
                thin: true
                mode: persistent
                controller_key: 1000
                size_gb: 10
                datastore: 1337_DS01
  theforeman.foreman.compute_profile:
    username: "{{ foreman_admin }}"
    password: "{{ foreman_admin_password }}"
    server_url: "https://foreman.example.com"
    validate_certs: "{{ validate_certs }}"
    name: "{{ compute_profile.name }}"
    compute_attributes: "{{ compute_profile.compute_attributes | default(omit) }}"
    state: "present"

EXPECTED RESULTS

Foreman should manage compute profile with a cluster name with integers only without complaining about.

ACTUAL RESULTS

TASK [config_foreman : Manage compute profile generic] **************************
70 fatal: [localhost]: FAILED! => {"changed": false, "msg": "Could not find clusters '1894' on compute resource 'vcenter.example.com'."}

[evgeni]

Hey @cmeissner, it's been a while!

You say that cluster: "1337" works, while cluster: 1337 does not, right?

I'd argue that putting it in quotes is more correct, after all that's the name of the cluster, which is (technically) a string, but I also see that there should be no reason for it not to work without.

[evgeni]

You could try changing the following line:

foreman-ansible-modules/plugins/module_utils/foreman_helper.py

Line 826 in 6c6d891

part = next((part for part in available_parts if part['name'] == name or part['id'] == name), None)

to be something like:

part = next((part for part in available_parts if part['name'] == str(name) or part['id'] == str(name)), None)

Or maybe also wrap the part[...] im str()?

[evgeni]

And I bet @mdellweg will kill me if I propose yet another "but this is nicer for the users" change that will break something else in 6 months time (#1240)

[cmeissner]

And I bet @mdellweg will kill me if I propose yet another "but this is nicer for the users" change that will break something else in 6 months time (#1240)

Haha, understand. But as names consisting only numbers are valid in vsphere it leads to confusion if the ansible module is complaining about missing cluster. For our real life example it leads to an extra effort of about 1h for debugging.

So from my point of view this is not only a convenient change. But I'm open for discussions.

[mdellweg]

Would it help to add type validation at the ansible parameter parsing?
I believe it should be ok, if the module complains it got an int instead of a string.

[cmeissner]

Would it help to add type validation at the ansible parameter parsing?
I believe it should be ok, if the module complains it got an int instead of a string.

This can save some time and effort. For me it is okay but I guess this will lead to confusion and somehow frustrating situation. But good error Messages are a good compromise.

[evgeni]

The Ansible parameter spec is, uh, complicated.
We can't partially define which parameters are allowed inside the vm_attrs, and having a list of all possible ones sounds crazy.
And Ansible wouldn't complain, but cast (well, with a warning, but still).

We could yield a warning ourself. But then we could also just cast ourself. Aaaah, decisions.

[mdellweg]

Is this problem limited to cluster? We can just proactively cast that specific value to str before any other processing.

[cmeissner]

Is this problem limited to cluster? We can just proactively cast that specific value to str before any other processing.

We seen it only at cluster.

[evgeni]

I would think it's a problem for any entry we do translation from name to id for (so cluster, network, etc)

[evgeni]

from triage meeting: if we cast both sides (foreman reply and the value in the playbook) to strings, this should be safe to compare and not break (too much gg)