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.

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

Allow tags to be supplied to proxmox_kvm on QEMU VM create #1989

Closed
1 task done
KMaheshBhat opened this issue Mar 10, 2021 · 8 comments · Fixed by #2000
Closed
1 task done

Allow tags to be supplied to proxmox_kvm on QEMU VM create #1989

KMaheshBhat opened this issue Mar 10, 2021 · 8 comments · Fixed by #2000
Labels
cloud feature This issue/PR relates to a feature request has_pr module module plugins plugin (any type)

Comments

@KMaheshBhat
Copy link

KMaheshBhat commented Mar 10, 2021

Summary

I checked the parameter list defined at

and per the documentation, the tags is not a parameter that can be passed via community.general.proxmox_kvm module.

It is available in the Proxmox VE API - https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/qemu/{vmid}/config - and seems to be available under POST and PUT. I understand this uses Proxmoxer ( https://github.com/proxmoxer/proxmoxer ) but my understanding is that it is a wrapper and should pass the parameter if sent from proxmox_kvm.

Issue Type

Feature Idea

Component Name

proxmox_kvm

Additional Information

Example use:

- name: Create new VM with minimal options (with tags)
  community.general.proxmox_kvm:
    api_user: root@pam
    api_password: secret
    api_host: helldorado
    name: spynal
    node: sabrewulf
    tags: "tag1"

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@ansibullbot
Copy link
Collaborator

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 cloud feature This issue/PR relates to a feature request module module needs_triage plugins plugin (any type) labels Mar 10, 2021
@Ajpantuso
Copy link
Collaborator

I recently submitted PR #1949 to fix the handling of tags strings in the inventory plugin and add an additional parsed list as a fact.
Some words of caution:

The last point is the only concerning one since I envision someone with a create task that works in every respect except for an improper tags string which will cause them much frustration.

If the proxmox team can supply a spec for the tags string and that validation is put upfront in this module to better inform the user then this should be trivial to implement.

@KMaheshBhat
Copy link
Author

@Ajpantuso I found the following snippet in the current HEAD of the proxmox/pve-common defining pve-tag:

# used for pve-tag-list in e.g., guest configs
register_format('pve-tag', \&pve_verify_tag);
sub pve_verify_tag {
    my ($value, $noerr) = @_;

    return $value if $value =~ m/^[a-z0-9_][a-z0-9_\-\+\.]*$/i;

    return undef if $noerr;

    die "invalid characters in tag\n";
}

The said pve-tag-list is used in proxmox/qemu-server :

    tags => {
	type => 'string', format => 'pve-tag-list',
	description => 'Tags of the VM. This is only meta information.',
	optional => 1,
    },

But I could not locate the pve-tag-list definition. Comma separated would seem to be the case, with each pve-tag defined by the RegEx in pve-common.

@Ajpantuso
Copy link
Collaborator

Ajpantuso commented Mar 11, 2021

@Ajpantuso I found the following snippet in the current HEAD of the proxmox/pve-common defining pve-tag:

# used for pve-tag-list in e.g., guest configs
register_format('pve-tag', \&pve_verify_tag);
sub pve_verify_tag {
    my ($value, $noerr) = @_;

    return $value if $value =~ m/^[a-z0-9_][a-z0-9_\-\+\.]*$/i;

    return undef if $noerr;

    die "invalid characters in tag\n";
}

The said pve-tag-list is used in proxmox/qemu-server :

    tags => {
	type => 'string', format => 'pve-tag-list',
	description => 'Tags of the VM. This is only meta information.',
	optional => 1,
    },

But I could not locate the pve-tag-list definition. Comma separated would seem to be the case, with each pve-tag defined by the RegEx in pve-common.

Ah thank you.

Not fun digging through their perl modules, but from what I see appending "-list" to a format name will trigger value parsing:

sub check_format {
    my ($format, $value, $path) = @_;

    if (ref($format) eq 'HASH') {
	# hash ref cannot have validator/list/opt handling attached
	return parse_property_string($format, $value, $path);
    }

    if (ref($format) eq 'CODE') {
	# we are the (sole, old-style) validator
	return $format->($value);
    }

    return if $format eq 'regex';

    my $parsed;
    $format =~ m/^(.*?)(?:-a?(list|opt))?$/;
    my ($format_name, $format_type) = ($1, $2 // 'none');
    my $registered = get_format($format_name);
    die "undefined format '$format'\n" if !$registered;

    die "'-$format_type' format must have code ref, not hash\n"
	if $format_type ne 'none' && ref($registered) ne 'CODE';

    if ($format_type eq 'list') {
	# Note: we allow empty lists
	foreach my $v (split_list($value)) {
	    $parsed = $registered->($v);
	}
    } elsif ($format_type eq 'opt') {
	$parsed = $registered->($value) if $value;
   } else {
	if (ref($registered) eq 'HASH') {
	    # Note: this is the only case where a validator function could be
	    # attached, hence it's safe to handle that in parse_property_string.
	    # We do however have to call it with $format_name instead of
	    # $registered, so it knows about the name (and thus any validators).
	    $parsed = parse_property_string($format, $value, $path);
	} else {
	    $parsed = $registered->($value);
	}
    }

    return $parsed;
}

And the split_list helper comes from Tools.pm which looks like it allows either ',' or ';' as a seperator.

sub split_list {
    my $listtxt = shift // '';

    return split (/\0/, $listtxt) if $listtxt =~ m/\0/;

    $listtxt =~ s/[,;]/ /g;
    $listtxt =~ s/^\s+//;

    my @data = split (/\s+/, $listtxt);

    return @data;
}

@KMaheshBhat
Copy link
Author

👍 Nice find!

So if we do end up adding the tags to the community.general.proxmox_kvm module, then we should specify for the parameter that:

  • tags available only in Proxmox 6+
  • tags should be comma or semi-colon separated
  • tags can contain a-z or 0-9 or _ or - or + or .
  • tags should start with a-z or 0-9 or _

Is the documentation for the modules in this asnible-collections/community.general same repository? The 'Edit on Github' link in the documentation site seems broken - https://docs.ansible.com/ansible/latest/collections/community/general/

@felixfontein
Copy link
Collaborator

Is the documentation for the modules in this asnible-collections/community.general same repository?

Yes. The documentation is auto-generated from the modules and plugins.

The 'Edit on Github' link in the documentation site seems broken - https://docs.ansible.com/ansible/latest/collections/community/general/

The auto-generated index pages should not have that link - that's a bug. I've passed that on.

@Ajpantuso
Copy link
Collaborator

👍 Nice find!

So if we do end up adding the tags to the community.general.proxmox_kvm module, then we should specify for the parameter that:

  • tags available only in Proxmox 6+
  • tags should be comma or semi-colon separated
  • tags can contain a-z or 0-9 or _ or - or + or .
  • tags should start with a-z or 0-9 or _

Is the documentation for the modules in this asnible-collections/community.general same repository? The 'Edit on Github' link in the documentation site seems broken - https://docs.ansible.com/ansible/latest/collections/community/general/

Yes, one suggestion however is that instead of asking for the raw tags string in the parameter it would make sense to accept a yaml list of tags. (Seems more straightforward to work with from an ansible perspective)

Example:

tags:
  - tag1
  - tag2
  - tag3

or

tags: ['tag1', 'tag2', 'tag3']

The module would then validate each tag and join them with commas or semi-colons to pass on to proxmoxer.

I would also suggest being strict and failing the task if tags are invalid. (Allowing the task to proceed without complete tags may cause unexpected results if the tags are used in the same playbook or sequence of plays)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud feature This issue/PR relates to a feature request has_pr module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants