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

Improve Proxmox volume handling #8542

Merged

Conversation

Lithimlin
Copy link
Contributor

@Lithimlin Lithimlin commented Jun 19, 2024

SUMMARY

Fixes #8407

Makes the creation of new volumes idempotent and adds new keys with better readability.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

proxmox

ADDITIONAL INFORMATION

Here is a sample playbook to test the new features/bug fixes:

---
- name: Create LXC
  hosts: localhost
  become: false
  gather_facts: false
  vars_files:
    - proxmox_credentials.yml
  tasks:
    - name: Create LXC
      community.general.proxmox:
        api_host: "{{ api_host }}"
        api_user: "{{ api_user }}"
        api_password: "{{ api_password }}"
        # api_token_id: "{{ api_token_id }}"
        # api_token_secret: "{{ api_token_secret }}"
        node: "{{ node }}"
        hostname: test-node
        ostemplate: local:vztmpl/debian-12-standard_12.2-1_amd64.tar.zst
        disk: local-lvm:8
      register: create_res

    - name: Update LXC config [new - create volumes]
      community.general.proxmox:
        api_host: "{{ api_host }}"
        api_user: "{{ api_user }}"
        api_password: "{{ api_password }}"
        # api_token_id: "{{ api_token_id }}"
        # api_token_secret: "{{ api_token_secret }}"
        node: "{{ node }}"
        hostname: test-node
        cores: 1
        vmid: "{{ create_res.vmid }}"
        disk_volume:
          storage: local-lvm
          size: 14
        mount_volumes:
          - id: mp0
            storage: local-lvm
            size: 3
            mountpoint: /mnt/test
          - id: mp1
            host_path: /root/test
            mountpoint: /mnt/host
            options:
              backup: "0"
        update: true

    - name: Update LXC config [new - explicit volumes]
      community.general.proxmox:
        api_host: "{{ api_host }}"
        api_user: "{{ api_user }}"
        api_password: "{{ api_password }}"
        # api_token_id: "{{ api_token_id }}"
        # api_token_secret: "{{ api_token_secret }}"
        node: "{{ node }}"
        hostname: test-node
        cores: 1
        vmid: "{{ create_res.vmid }}"
        disk_volume:
          storage: local-lvm
          volume: "vm-{{ create_res.vmid }}-disk-0"
          size: 14
        mount_volumes:
          - id: mp0
            storage: local-lvm
            volume: "vm-{{ create_res.vmid }}-disk-1"
            size: 3
            mountpoint: /mnt/test
          - id: mp1
            host_path: /root/test
            mountpoint: /mnt/host
            options:
              backup: "0"
        update: true

    - name: Update LXC config [old - implicit volumes]
      community.general.proxmox:
        api_host: "{{ api_host }}"
        api_user: "{{ api_user }}"
        api_password: "{{ api_password }}"
        # api_token_id: "{{ api_token_id }}"
        # api_token_secret: "{{ api_token_secret }}"
        node: "{{ node }}"
        hostname: test-node
        cores: 1
        vmid: "{{ create_res.vmid }}"
        disk: local-lvm:14
        mounts: '{"mp0":"local-lvm:3,mp=/mnt/test","mp1":"/root/test,mp=/mnt/host,backup=0"}'
        update: true

    - name: Update LXC config [old - explicit volumes]
      community.general.proxmox:
        api_host: "{{ api_host }}"
        api_user: "{{ api_user }}"
        api_password: "{{ api_password }}"
        # api_token_id: "{{ api_token_id }}"
        # api_token_secret: "{{ api_token_secret }}"
        node: "{{ node }}"
        hostname: test-node
        cores: 1
        vmid: "{{ create_res.vmid }}"
        disk: "local-lvm:vm-{{ create_res.vmid }}-disk-0,size=14G"
        mounts: '{"mp0":"local-lvm:vm-{{ create_res.vmid }}-disk-1,size=3G,mp=/mnt/test","mp1":"/root/test,mp=/mnt/host,backup=0"}'
        update: true

Note: To test bind mounting, we actually need to use the root@pam user with its password. Even a token owned by that user will not work due to missing permissions.

Output:

[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [Create LXC] *******************************************************************************************************************************************************************

TASK [Create LXC] *******************************************************************************************************************************************************************
ok: [localhost]

TASK [Update LXC config [new - create volumes]] *************************************************************************************************************************************
changed: [localhost]

TASK [Update LXC config [new - explicit volumes]] ***********************************************************************************************************************************
ok: [localhost]

TASK [Update LXC config [old - implicit volumes]] ***********************************************************************************************************************************
ok: [localhost]

TASK [Update LXC config [old - explicit volumes]] ***********************************************************************************************************************************
ok: [localhost]

PLAY RECAP **************************************************************************************************************************************************************************
localhost                  : ok=5    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0  

Lithimlin added 3 commits May 28, 2024 14:13
using black via trunk.io
- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management
@ansibullbot

This comment was marked as outdated.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug ci_verified Push fixes to PR branch to re-run CI module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) labels Jun 19, 2024
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 19, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Jun 20, 2024
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

changelogs/fragments/8542-fix-proxmox-volume-handling.yml Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Show resolved Hide resolved
plugins/modules/proxmox.py Show resolved Hide resolved
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 27, 2024
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jun 27, 2024
- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 27, 2024
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 27, 2024
Lithimlin and others added 4 commits July 5, 2024 10:44
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Lithimlin
Thanks for your contribution! I got a few comments on it, see below.

plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
Lithimlin and others added 2 commits July 8, 2024 11:50
Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <[email protected]>
Accept suggested review change

Co-authored-by: Alexei Znamensky <[email protected]>
@ansibullbot
Copy link
Collaborator

@Lithimlin This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 8, 2024
@Lithimlin Lithimlin force-pushed the proxmox-volume-handling branch from cdcc844 to 2d364aa Compare July 8, 2024 10:09
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 8, 2024
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@felixfontein
Copy link
Collaborator

If nobody objects I'll merge this tomorrow.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jul 14, 2024
@felixfontein felixfontein merged commit 6cefde6 into ansible-collections:main Jul 14, 2024
150 checks passed
Copy link

patchback bot commented Jul 14, 2024

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/6cefde622cac8bf0d20203dd32e0e8fd96ba68fe/pr-8542

Backported as #8622

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jul 14, 2024
* proxmox: basic linting

using black via trunk.io

* proxmox: refactor mount handling (#8407)

- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management

* proxmox: add changelog fragment

* proxmox(fix): fix occasional syntax error

* Update changelogs/fragments/8542-fix-proxmox-volume-handling.yml

Link to pull request

Co-authored-by: Felix Fontein <[email protected]>

* Update documentation

- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag

* Revert "proxmox: basic linting"

This reverts commit ca7214f.

* proxmox: Fix documentation

* Fix list identifier in documentation

* pass volume options as dict instead of list

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* proxmox: ensure values of `disk_volume` and `mount_volumes.*` dicts are strings

* proxmox(fix): correct indentation

* Apply suggestions from code review: punctuation

Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <[email protected]>

* Update plugins/modules/proxmox.py: vol_string building

Accept suggested review change

Co-authored-by: Alexei Znamensky <[email protected]>

* proxmox: Use better string check and conversion

---------

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Alexei Znamensky <[email protected]>
(cherry picked from commit 6cefde6)
@felixfontein
Copy link
Collaborator

@Lithimlin thanks for your contribution!
@russoz thanks for reviewing!

@Lithimlin Lithimlin deleted the proxmox-volume-handling branch July 14, 2024 10:16
felixfontein pushed a commit that referenced this pull request Jul 14, 2024
#8622)

Improve Proxmox volume handling (#8542)

* proxmox: basic linting

using black via trunk.io

* proxmox: refactor mount handling (#8407)

- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management

* proxmox: add changelog fragment

* proxmox(fix): fix occasional syntax error

* Update changelogs/fragments/8542-fix-proxmox-volume-handling.yml

Link to pull request

Co-authored-by: Felix Fontein <[email protected]>

* Update documentation

- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag

* Revert "proxmox: basic linting"

This reverts commit ca7214f.

* proxmox: Fix documentation

* Fix list identifier in documentation

* pass volume options as dict instead of list

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* proxmox: ensure values of `disk_volume` and `mount_volumes.*` dicts are strings

* proxmox(fix): correct indentation

* Apply suggestions from code review: punctuation

Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <[email protected]>

* Update plugins/modules/proxmox.py: vol_string building

Accept suggested review change

Co-authored-by: Alexei Znamensky <[email protected]>

* proxmox: Use better string check and conversion

---------

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Alexei Znamensky <[email protected]>
(cherry picked from commit 6cefde6)

Co-authored-by: JL Euler <[email protected]>
@Lithimlin
Copy link
Contributor Author

I noticed a problem that I hadn't tested after my last commit:
The string conversion turns all empty (None) entries into strings which messes up things down the line.
Additionally, I documented the size parameters as integers, but with that the module will always throw a warning, saying that all variables must be strings. This happens even if the values are passed as strings because the module converts them back to integers.

@felixfontein since this was your comment, I'd like to talk about the conversion again.
I don't think it was even necessary since build_volume() will always convert it to a string anyway.

aioue pushed a commit to aioue/community.general that referenced this pull request Oct 1, 2024
* proxmox: basic linting

using black via trunk.io

* proxmox: refactor mount handling (ansible-collections#8407)

- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management

* proxmox: add changelog fragment

* proxmox(fix): fix occasional syntax error

* Update changelogs/fragments/8542-fix-proxmox-volume-handling.yml

Link to pull request

Co-authored-by: Felix Fontein <[email protected]>

* Update documentation

- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag

* Revert "proxmox: basic linting"

This reverts commit ca7214f.

* proxmox: Fix documentation

* Fix list identifier in documentation

* pass volume options as dict instead of list

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* proxmox: ensure values of `disk_volume` and `mount_volumes.*` dicts are strings

* proxmox(fix): correct indentation

* Apply suggestions from code review: punctuation

Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <[email protected]>

* Update plugins/modules/proxmox.py: vol_string building

Accept suggested review change

Co-authored-by: Alexei Znamensky <[email protected]>

* proxmox: Use better string check and conversion

---------

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Alexei Znamensky <[email protected]>
TobiasZeuch181 pushed a commit to TobiasZeuch181/zypper_repository_add_list that referenced this pull request Oct 2, 2024
* proxmox: basic linting

using black via trunk.io

* proxmox: refactor mount handling (ansible-collections#8407)

- make mount creation idempotent: Mounts created using the special syntax "<storage>:<size>" no longer create a new volume each time
- add new keys for easier mount creation & management

* proxmox: add changelog fragment

* proxmox(fix): fix occasional syntax error

* Update changelogs/fragments/8542-fix-proxmox-volume-handling.yml

Link to pull request

Co-authored-by: Felix Fontein <[email protected]>

* Update documentation

- Fix options defined as values
- Document mutual exclusivity
- Fix option hierarchy
- Add version_added tag

* Revert "proxmox: basic linting"

This reverts commit ca7214f.

* proxmox: Fix documentation

* Fix list identifier in documentation

* pass volume options as dict instead of list

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/proxmox.py

Update documentation wording

Co-authored-by: Felix Fontein <[email protected]>

* proxmox: ensure values of `disk_volume` and `mount_volumes.*` dicts are strings

* proxmox(fix): correct indentation

* Apply suggestions from code review: punctuation

Add suggested punctuation to documentation

Co-authored-by: Alexei Znamensky <[email protected]>

* Update plugins/modules/proxmox.py: vol_string building

Accept suggested review change

Co-authored-by: Alexei Znamensky <[email protected]>

* proxmox: Use better string check and conversion

---------

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Alexei Znamensky <[email protected]>
markusj added a commit to markusj/community.general that referenced this pull request Oct 17, 2024
 and ansible-collections#8720

It is still not working and maybe my attempt to allow fractional volume sizes again broke something, too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxmox LXC - Mounts configuration causes repeated creation of volumes
4 participants