[proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757

UnderGreen · 2023-06-21T03:16:56Z

SUMMARY

Fixes #5579.
As per documentation from proxmoxer requests_toolbelt is required to upload large files.

Can't find the exact size of that large file definition, but from my personal testing on Darwin x86_64 it is around 260/261MB. Not sure where it is coming from, so can't explain it.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

proxmox_template

ADDITIONAL INFORMATION

Integration test output with updated module:

❯ ansible-test integration --allow-unsupported --local proxmox_template
Running proxmox_template integration test role

PLAY [testhost] ****************************************************************

TASK [Gathering Facts] *********************************************************
ok: [testhost]

TASK [proxmox_template : Create dummy ISO file] ********************************
changed: [testhost]

TASK [proxmox_template : Delete requests_toolbelt module if it is installed] ***
ok: [testhost]

TASK [proxmox_template : Install latest proxmoxer] *****************************
ok: [testhost]

TASK [proxmox_template : Upload ISO as template to Proxmox VE cluster should fail] ***
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: Failed to import the required Python library (requests_toolbelt) on santipov-ltm.internal.salesforce.com's Python /Users/s.antipov/src/ansible/collections/ansible_collections/community/general/.venv/bin/python3.11. Please read the module documentation and install it in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter
fatal: [testhost]: FAILED! => {"changed": false, "msg": "'requests_toolbelt' module is required to upload files larger than 256MB"}
...ignoring

TASK [proxmox_template : assert] ***********************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [proxmox_template : Install old (1.1.2) version of proxmoxer] *************
changed: [testhost]

TASK [proxmox_template : Upload ISO as template to Proxmox VE cluster should be successful] ***
changed: [testhost]

TASK [proxmox_template : assert] ***********************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [proxmox_template : Install latest proxmoxer] *****************************
changed: [testhost]

TASK [proxmox_template : Make smaller dummy file] ******************************
changed: [testhost]

TASK [proxmox_template : Upload ISO as template to Proxmox VE cluster should be successful] ***
changed: [testhost]

TASK [proxmox_template : assert] ***********************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [proxmox_template : Install requests_toolbelt] ****************************
changed: [testhost]

TASK [proxmox_template : Make big dummy file] **********************************
changed: [testhost]

TASK [proxmox_template : Upload ISO as template to Proxmox VE cluster should be successful] ***
changed: [testhost]

TASK [proxmox_template : assert] ***********************************************
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [proxmox_template : Delete ISO file from host] ****************************
changed: [testhost]

PLAY RECAP *********************************************************************
testhost                   : ok=18   changed=10   unreachable=0    failed=0    skipped=0    rescued=0    ignored=1

ansibullbot · 2023-06-21T03:20:12Z

cc @Ajpantuso @Thulium-Drake @joshainglis @karmab @tleguern
click here for bot help

felixfontein

One question: was requests_toolbelt already required in the past, i.e. did the module not work when it was around in all situations; or is this a new requirement? If it is a new requirement, this is a breaking change.

(If that's the case, it's probably better to warn about it missing when the size of the file is larger than a certain limit, instead of failing.)

Also I don't see how the toolbelt is being used. Is it used implicitly by proxmoxer or some other code in the module?

UnderGreen · 2023-06-26T21:41:32Z

@felixfontein looks like a problem with the proxmoxer module itself and I can reproduce it using plain proxmoxer. It is started with proxmoxer 1.2.0 and still failing in the master branch. Installing requests_toolbelt somehow resolves it(I am digging into it, but it looks like so far requesting another dependency is the way out).

UnderGreen · 2023-06-26T21:43:21Z

Also I don't see how the toolbelt is being used. Is it used implicitly by proxmoxer or some other code in the module?

Yes, it is used in proxmoxer.

UnderGreen · 2023-06-28T00:22:09Z

@felixfontein I added notes about file size and usage of requests_toolbelt.

ansibullbot · 2023-06-28T00:25:57Z

@UnderGreen this PR contains the following merge commits:

c635411

Please rebase your branch to remove these commits.

click here for bot help

…uster

felixfontein · 2023-06-28T20:29:45Z

I killed the config that the bot apparently uses in #6804, so it should no longer push unwanted commits.

Unfortunately it still makes CI fail by adding a failed check... I don't have sufficient privileges to get rid of the bot myself, but I've asked on IRC, hopefully it will be gone soon...

felixfontein · 2023-06-28T20:45:08Z

I've removed the commit; please now ignore the failing pre-commit.ci check :)

webknjaz · 2023-06-28T22:20:55Z

do you know why, very spontaneously, I would say, the pre-commit.ci bot committed for my PR?

I requested the bot app install for the repos with the config having seen people running it in GHA in some subprojects which is inefficient, but didn't take into account that since it didn't run in some CIs the violations might've accumulated over time. I also forgot that by default it commits the changes to PRs. This one is solvable through a configuration change, but the bot would still report the check for as long as the config exists.

The change was sudden, likely because it happened when somebody approved the GH App install which was like a day later after the request.

Unfortunately it still makes CI fail by adding a failed check...

That shouldn't be happening. Sounds like a race condition, or maybe the PR wasn't rebased after that? Looks like it's the latter — the Checks widget says:

It’s 1 commit behind (base commit: c4a2801)

felixfontein · 2023-06-29T03:59:46Z

That shouldn't be happening. Sounds like a race condition, or maybe the PR wasn't rebased after that? Looks like it's the latter — the Checks widget says:

It also happened for a new PR (#6808) from main branch. I guess it somehow caches that a project should have a config.

ansibullbot · 2023-06-29T04:15:03Z

cc @gundalow
click here for bot help

tests/utils/constraints.txt

UnderGreen · 2023-06-29T14:22:14Z

@felixfontein any insights on why it is still failing for 2.6?
importlib is in requirements and see in logs it is downloaded(but not installed?).

UnderGreen · 2023-06-30T04:01:01Z

I skip 2.6 Python as it was never supported by proxmoxer library.

patchback · 2023-07-02T19:48:30Z

Backport to stable-6: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 2d6e369 on top of patchback/backports/stable-6/2d6e369d81441279df812eea495710fc7abad4ad/pr-6757

Backporting merged PR #6757 into main

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/ansible-collections/community.general.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/stable-6/2d6e369d81441279df812eea495710fc7abad4ad/pr-6757 upstream/stable-6

Now, cherry-pick PR [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757 contents into that branch:
```
$ git cherry-pick -x 2d6e369d81441279df812eea495710fc7abad4ad
```
If it'll yell at you with something like fatal: Commit 2d6e369d81441279df812eea495710fc7abad4ad is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 2d6e369d81441279df812eea495710fc7abad4ad
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/stable-6/2d6e369d81441279df812eea495710fc7abad4ad/pr-6757

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

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

patchback · 2023-07-02T19:48:36Z

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/2d6e369d81441279df812eea495710fc7abad4ad/pr-6757

Backported as #6831

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

…uster (#6757) * [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster * Fix pep8 test * Add changelog fragment * Add notes about requests_toolbelt * Check versions and file size * Fix typo in notes * Add unit test. Move try inside of each function. * Fix sanity tests * Add proxmoxer in requirements file * Update integration tests * Add proxmoxer into constraints.txt * Address review comments * Don't run tests on 2.6 python * Disable Python 2.6 tests for other proxmox modules (cherry picked from commit 2d6e369)

felixfontein · 2023-07-02T19:48:58Z

@UnderGreen thanks a lot for your contribution!

…hile uploading big ISO to Proxmox VE cluster (#6831) [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster (#6757) * [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster * Fix pep8 test * Add changelog fragment * Add notes about requests_toolbelt * Check versions and file size * Fix typo in notes * Add unit test. Move try inside of each function. * Fix sanity tests * Add proxmoxer in requirements file * Update integration tests * Add proxmoxer into constraints.txt * Address review comments * Don't run tests on 2.6 python * Disable Python 2.6 tests for other proxmox modules (cherry picked from commit 2d6e369) Co-authored-by: Sergei Antipov <greendayonfire@gmail.com>

This comment was marked as outdated.

Sign in to view

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 backport-7 Automatically create a backport for the stable-7 branch labels Jun 21, 2023

felixfontein reviewed Jun 21, 2023

View reviewed changes

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jun 28, 2023

UnderGreen added 5 commits June 27, 2023 22:33

[proxmox_template] Fix error while uploading big ISO to Proxmox VE cl…

3f419b7

…uster

Fix pep8 test

3ef65d2

Add changelog fragment

75a4cb1

Add notes about requests_toolbelt

3a70c17

Check versions and file size

4868065

UnderGreen force-pushed the proxmox-template/fix-upload-error branch from c635411 to 4868065 Compare June 28, 2023 02:33

Fix typo in notes

796cca5

ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jun 28, 2023

felixfontein force-pushed the proxmox-template/fix-upload-error branch from 1d5368e to 02512ed Compare June 28, 2023 20:44

ansibullbot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 28, 2023

ansibullbot added the traceback label Jun 29, 2023

Update integration tests

fcbd929

Add proxmoxer into constraints.txt

4fcf265

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jun 29, 2023

felixfontein reviewed Jun 29, 2023

View reviewed changes

tests/utils/constraints.txt Outdated Show resolved Hide resolved

Address review comments

9ab6481

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jun 29, 2023

Don't run tests on 2.6 python

cb79cbd

Disable Python 2.6 tests for other proxmox modules

23459c6

UnderGreen requested a review from felixfontein June 30, 2023 14:37

felixfontein approved these changes Jul 2, 2023

View reviewed changes

felixfontein merged commit 2d6e369 into ansible-collections:main Jul 2, 2023
151 checks passed

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jul 2, 2023

patchback bot mentioned this pull request Jul 2, 2023

[PR #6757/2d6e369d backport][stable-7] [proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6831

Merged

UnderGreen deleted the proxmox-template/fix-upload-error branch July 5, 2023 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757

[proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757

UnderGreen commented Jun 21, 2023 •

edited

Loading

This comment was marked as outdated.

ansibullbot commented Jun 21, 2023

felixfontein left a comment

UnderGreen commented Jun 26, 2023

UnderGreen commented Jun 26, 2023

UnderGreen commented Jun 28, 2023

ansibullbot commented Jun 28, 2023

felixfontein commented Jun 28, 2023

felixfontein commented Jun 28, 2023

webknjaz commented Jun 28, 2023 •

edited

Loading

felixfontein commented Jun 29, 2023

ansibullbot commented Jun 29, 2023

UnderGreen commented Jun 29, 2023

UnderGreen commented Jun 30, 2023

patchback bot commented Jul 2, 2023 •

edited

Loading

patchback bot commented Jul 2, 2023 •

edited

Loading

felixfontein commented Jul 2, 2023

[proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757

[proxmox_template] Fix error while uploading big ISO to Proxmox VE cluster #6757

Conversation

UnderGreen commented Jun 21, 2023 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

ansibullbot commented Jun 21, 2023

felixfontein left a comment

Choose a reason for hiding this comment

UnderGreen commented Jun 26, 2023

UnderGreen commented Jun 26, 2023

UnderGreen commented Jun 28, 2023

ansibullbot commented Jun 28, 2023

felixfontein commented Jun 28, 2023

felixfontein commented Jun 28, 2023

webknjaz commented Jun 28, 2023 • edited Loading

felixfontein commented Jun 29, 2023

ansibullbot commented Jun 29, 2023

UnderGreen commented Jun 29, 2023

UnderGreen commented Jun 30, 2023

patchback bot commented Jul 2, 2023 • edited Loading

Backport to stable-6: 💔 cherry-picking failed — conflicts found

Backporting merged PR #6757 into main

patchback bot commented Jul 2, 2023 • edited Loading

Backport to stable-7: 💚 backport PR created

felixfontein commented Jul 2, 2023

UnderGreen commented Jun 21, 2023 •

edited

Loading

webknjaz commented Jun 28, 2023 •

edited

Loading

patchback bot commented Jul 2, 2023 •

edited

Loading

patchback bot commented Jul 2, 2023 •

edited

Loading