feat(storage): add new resource `proxmox_virtual_environment_download_file` #837

rafsaf · 2023-12-25T23:48:00Z

Contributor's Note

I have added / updated documentation in /docs for any user-facing features or additions.
I have added / updated templates in /example for any new or updated resources / data sources.
I have ran make example to verify that the change works as expected.

Proof of Work

Hi, I am aware this is big. Sorry about that.

My justification of new resource instead of trying to change proxmox_virtual_environment_file is that later is using different approach to upload files (using upload API endpoint) and new resource is using only API download-url ednpoint. This has also the benefit that we will not break existing usage, but rather give another option that should be better suited and scalable in many nodes for vm and lxc images.

Autogenerated documentation for new resource
I updated documentation of vm, lxc and file docs in a manner that new resource should be 1st choice for vm/lxc images. It is faster, simpler, less error prone and does not require SSH.
Added some resources in example and gave it hard debugging until everything is right (hopefully).
Added basic acceptance tests, I created some fake gist files with .iso and qcow2 extenstion to save bandwith.
Refactored storage.go file into separate module with few files -> ideally this should not cause any issues.
allow_unsupported_types param is somewhat suspicious, but I didn't have any problems when using qcow2 or raw images, they both seem to work when have added iso postfix.
no ImportState code -> proxmox does not save URL from which file was uploaded, checksum or algorithm so (for me, I might be wrong) it has little sense here?

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #638

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

…ther docs and examples Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

bpg · 2023-12-26T15:38:31Z

Hi @rafsaf! That's awesome, thanks bunch for the PR!

I think having a separate resource to download files make sense, it matches PVE functionality quite nicely. Later on we can rename existing proxmox_virtual_environment_file -> proxmox_virtual_environment_upload_file for more clear separation in the provider's API.

I just skimmed the code and will do a proper review later in the week. One thing I noticed tho is the resource fails when uploading file already exists:

╷
│ Error: Error creating Download File interface
│ 
│   with proxmox_virtual_environment_download_file.ubuntu_20_lxc_image,
│   on resource_virtual_environment_download_file.tf line 12, in resource "proxmox_virtual_environment_download_file" "ubuntu_20_lxc_image":
│   12: resource "proxmox_virtual_environment_download_file" "ubuntu_20_lxc_image" {
│ 
│ Could not DownloadFileByURL: `ubuntu-20.04-standard_20.04-1_amd64.tar.gz`, unexpected error: error download file to datastore local: failed waiting for url download -
│ task "UPID:pve:0022E8D2:05ED8B6B:658AEF0A:download:ubuntu-20.04-standard_20.04-1_amd64.tar.gz:root@pam:" failed to complete with exit code: refusing to override
│ existing file '/var/lib/vz/template/cache/ubuntu-20.04-standard_20.04-1_amd64.tar.gz'
╵
╷
│ Error: Error creating Download File interface
│ 
│   with proxmox_virtual_environment_download_file.ubuntu_noble_image,
│   on resource_virtual_environment_download_file.tf line 19, in resource "proxmox_virtual_environment_download_file" "ubuntu_noble_image":
│   19: resource "proxmox_virtual_environment_download_file" "ubuntu_noble_image" {
│ 
│ Could not get GetQueryURLMetadata, unexpected error: error retrieving Query URL metadata configuration: received an HTTP 500 response - Reason: invalid server response:
│ '404 Not Found'

We should probably handle this error more gracefully, and also add a flag to the resource to allow overwrite of the existing file (it should be false by default)

rafsaf · 2023-12-26T17:22:50Z

Sure, this can be handled better, I will look into this

…esource download file Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

…load task on error Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

rafsaf · 2023-12-28T00:55:58Z

if resource is not created and file already exists:

 Error: File already exists in a datastore, it was created outside of Terraform or is managed by another resource.
│ 
│   with proxmox_virtual_environment_download_file.ubuntu_20_lxc_image,
│   on resource_virtual_environment_download_file.tf line 12, in resource "proxmox_virtual_environment_download_file" "ubuntu_20_lxc_image":
│   12: resource "proxmox_virtual_environment_download_file" "ubuntu_20_lxc_image" {
│ 
│ File already exists in a datastore: `ubuntu-20.04-standard_20.04-1_amd64.tar.gz`, error: error download file
│ to datastore local: failed waiting for url download - task
│ "UPID:pve:0000C9ED:0020AB12:658CC467:download:ubuntu-20.04-standard_20.04-1_amd64.tar.gz:root@pam:" failed to
│ complete with exit code: refusing to override existing file
│ '/var/lib/vz/template/cache/ubuntu-20.04-standard_20.04-1_amd64.tar.gz'

error, based on
https://developer.hashicorp.com/terraform/plugin/framework/resources/create#recommendations

if resource is created and file does not exists or was removed:

no error, resource recreated https://developer.hashicorp.com/terraform/plugin/framework/resources/read#recommendations
if resource is deleted and file does not already exists

no error, only warning, https://developer.hashicorp.com/terraform/plugin/framework/resources/delete#recommendations

I added some validation so checksum and checksum_algorithm must be both null or strings, and also when someone interrupt terraform apply, upload task will be cancelled instead of going on.

As of overwrite, I think files with same names will be very very rare, since usually people will download debian/ubuntu/windows/other "official" images that always have long unique names, this is different than snippets from local filesystem.

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

bpg

Thanks again @rafsaf for taking on this feature, really good work!

I have a few comments and suggestions, could you please take a look?

bpg · 2023-12-28T19:22:40Z

docs/resources/virtual_environment_download_file.md

+
+### Optional
+
+- `allow_unsupported_types` (Boolean) By default `false`. If `true`, content formats `qcow2` and `raw` can be downloaded, though it is not supported by proxmox.


We should probably rework this a bit.

Technically, PVE supports qcow2 and raw, it is just a matter of renaming the file during upload. For example, I was able to download a debian's raw file without issues:

I see in the code below you're utilizing this "feature" by adding .iso extension, but I think we can replacing this argument with file_name, that allows to override the original remote file name. This will also match the current behaviour of the *_file resource, and could save some time for people migrating to *_download_file.

done, file_name is now computed and optional (if not provided by user, we will use default name),

bpg · 2023-12-28T19:23:47Z

docs/resources/virtual_environment_download_file.md

+
+- `content_type` (String) The file content type. Must be `iso` | `vztmpl`.
+- `datastore_id` (String) The identifier for the target datastore.
+- `download_url` (String) The URL to download the file from. Format `https?://.*`.


I'd probably remove the download_ prefix from it, seems redundant in the context of the resource.

bpg · 2023-12-28T19:43:37Z

docs/resources/virtual_environment_download_file.md

+
+### Read-Only
+
+- `filename` (String) The file name.


It should be file_name to match convention in the other resource.

bpg · 2023-12-28T19:56:52Z

example/resource_virtual_environment_download_file.tf

+  content_type       = "iso"
+  datastore_id       = "local"
+  node_name          = "pve"
+  download_url       = "https://cloud-images.ubuntu.com/noble/20231218/noble-server-cloudimg-amd64.img"


This file got disappeared, so make example throws an error:

│ Error: Error creating Download File interface │ │ with proxmox_virtual_environment_download_file.ubuntu_noble_image, │ on resource_virtual_environment_download_file.tf line 19, in resource "proxmox_virtual_environment_download_file" "ubuntu_noble_image": │ 19: resource "proxmox_virtual_environment_download_file" "ubuntu_noble_image" { │ │ Could not get GetQueryURLMetadata, unexpected error: error retrieving Query URL metadata configuration: received an HTTP 500 response - Reason: invalid server response: '404 Not Found'

I guess the intention for this resource is to verify the checksum calculation, that means we can't use "current" or "latest" downloads.
You can pick up something else from http://download.proxmox.com/images/system/, or just add checksum to the resource above.

every link from previous work was checked, i either used "latest" / "current" images or release ones, anyway they will not disappear

bpg · 2023-12-28T19:59:26Z

docs/resources/virtual_environment_download_file.md

+parent: Resources
+subcategory: Virtual Environment
+description: |-
+  Manages files upload using directly proxmox download-url API. It can be a full replacement for ISO files created using proxmox_virtual_environment_file and it does not use SSH.


Sorry for picking up on this, but it will be better to use "Proxmox VE", or "PVE", instead of "proxmox" -- there are other products under the Proxmox "umbrella".

bpg · 2023-12-28T20:18:42Z

fwprovider/resource_download_file.go

+		Description: "Manages files upload using directly proxmox download-url API. " +
+			"It can be a full replacement for ISO files created using " +


This should probably be MarkdownDescription, and leave the first line for the regular Description:

Suggested change

Description: "Manages files upload using directly proxmox download-url API. " +

"It can be a full replacement for ISO files created using " +

Description: "Manages files upload using directly proxmox download-url API.",

MarkdownDescription: "Manages files upload using directly proxmox download-url API. " +

"It can be a full replacement for ISO files created using " +

bpg · 2023-12-28T20:20:01Z

fwprovider/resource_download_file.go

+		Attributes: map[string]schema.Attribute{
+			"id": structure.IDAttribute(),
+			"content_type": schema.StringAttribute{
+				MarkdownDescription: "The file content type. Must be `iso` | `vztmpl`.",


I think just Description for the inner attribute would work.

bpg · 2023-12-28T21:13:45Z

fwprovider/resource_download_file.go

+	)
+	if err != nil {
+		resp.Diagnostics.AddError(
+			"Error creating Download File interface",


TBH, I was really confused with the error message when saw it first time 😅

Suggested change

"Error creating Download File interface",

"Error initiating file download",

bpg · 2023-12-28T21:28:34Z

proxmox/nodes/nodestorage/client.go

+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+package nodestorage


Perhaps rename nodestorage to just storage? Similar to what we have for node/containers, node/tasks, etc.

There is already storage package at proxmox/storage/client.go for example, for "global" storage api I suppose, this is why i came up with nodestorage.

But technically it can be named the same I think? so no problem

Yeah, I think /proxmox/nodes/storage package will be fine. In in some places we need to import both "storages", when we can use nodestorage import alias.

bpg · 2023-12-28T21:32:38Z

proxmox/nodes/tasks/client.go

@@ -23,13 +23,27 @@ func (c *Client) ExpandPath(_ string) string {
 	panic("ExpandPath of tasks.Client must not be used. Use BuildPath instead.")
 }

+// BaseTaskPath builds base task path using Task ID.
+func (c *Client) BaseTaskPath(taskID string) (string, error) {


this function probably doesn't need to be exported.

bpg · 2023-12-28T21:44:03Z

As of overwrite, I think files with same names will be very very rare, since usually people will download debian/ubuntu/windows/other "official" images that always have long unique names, this is different than snippets from local filesystem.

That may not always be the case, some people may decide to use the "latest" version of their distro. Or copy/paste the file resource in multiple templates. For sure, it's better to have a dedicated resource for each image, but in certain scenarios it is less headache just to overwrite "whatever is cached on PVE" with the "latest and greatest" as part of the current template.

rafsaf · 2023-12-28T22:09:33Z

Thanks for review!!!

I will be on it in a few days, I'm not only 100% sure about this nodestorage conflict with storage package name, but the rest is clear.

The overwrite indeed makes sense for "latest" version of some image.

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

…quiresReplaceModifier Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): resource download file fixes after review

rafsaf · 2024-01-01T23:20:53Z

Happy new year :p Okay, so here we go again.

I think now everything should be fine...

There is quite big change regarding overwrite option, first it is true by default and it is "file size based", so we check "remote" size (both datastore size and url size) every time on read.

Second, I spent some hours digging around, checking docs, issues, threads and eventually achieved what I wanted :P My idea is that we if current state size of file doesn't match any of above two (datastore or url), this means that someone manually changed the file (deleted in UI and uploaded something else) or there was change in file size from url (new release in "latest" image url). I want to recreate resource if that happens. Unfortunately, not so quick... I believe even when terraform state is changed on read and computed + not optional attribute has int64planmodifier.RequiresReplace(), there will be no plan change... 😠 At least one attribute would need to be updated.

The only fully working solution I tried is to use custom planmodifier and pass there data from Read func using Private attr... This is kind of slippery solution, but seems to work fine and does exactly what described.

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

bpg

Happy New Year @rafsaf!

Thanks for addressing all comments, it looks great!

I just updated the LXC file download resource to use PVE-compatible container image (it requires a proper root filesystem), and fixed few typos.

LGTM! 🚀

ratiborusx · 2024-01-09T11:46:26Z

Hey, lads!
Didn't want to open a new issue because it's quite a minor one. I'm trying to refactor my modules now to use this new resource and i noticed documentation about "old" resource "proxmox_virtual_environment_file" mentions this one but incorrectly. It misses "virtual" in it's name:
https://github.com/bpg/terraform-provider-proxmox/pull/837/files#diff-5caa47f7405edcf5a14491001c3be764f12c9859948e9ae1eeb3e46b702b142f

rafsaf · 2024-01-09T17:16:32Z

@ratiborusx

👋 Hello, nice catch. I am off till next week, but feel free to submit PR if you are in a position to create a fix.

This should be straightforward as only markdown files in the docs folder need to be changed.

bpg · 2024-01-10T02:41:25Z

Good catch indeed! :) Fixed in #872

Rafał Safin added 8 commits December 26, 2023 00:16

feat(core): add query url metadata endpoint to nodes client api

e4a2f5b

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(core): move storage api code to nodestorage folder

7480657

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(core): add download url api

3714ca1

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): add resource_download_file

dd954c0

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): finish new resource_download_file

d45a3e2

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): generate docs for new download file resource, update o…

08a9ad0

…ther docs and examples Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): add basic acc tests for resource download_file

d53a1cf

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(test): lint new test file

de55e1a

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

pull-request-size bot added the size/XXL label Dec 25, 2023

bpg changed the title ~~feat(storage): add new resource proxmox_virtual_environment_download_file~~ feat(storage): add new resource proxmox_virtual_environment_download_file Dec 26, 2023

Rafał Safin added 3 commits December 27, 2023 20:10

feat(storage): warning instead of error when file already exists on r…

b64c1a5

…esource download file Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): better validation in resource download file, delete up…

423c137

…load task on error Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): better err message in resource download file

937e82d

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

Rafał Safin added 2 commits December 28, 2023 02:06

fix(core): removed unnecessary toint in custom type bool

2a485c3

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(storage): typo in resource download file error

3e35e1c

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

bpg reviewed Dec 28, 2023

View reviewed changes

Rafał Safin and others added 9 commits January 1, 2024 20:14

feat(storage): download file resource review - 1

fcf8d8b

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

feat(storage): finish resource download file after review

877b460

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(storage): error instead of warnings in parse int errors in sizeRe…

543561d

…quiresReplaceModifier Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(docs): remove unwanted changes in virtual_environment_file.md

714e997

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(test): fix download file base acceptance tests

b592bea

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(test): fix resource download file acc tests

582fe3c

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

fix(test): last fix resource download file acc test

5b2229b

Signed-off-by: Rafał Safin <rafal.safin@rafsaf.pl>

Merge pull request #1 from rafsaf/new-resource-download-file-2

f3f4079

feat(storage): resource download file fixes after review

Merge branch 'main' into new-resource-download-file

57d6e8e

fix: use PVE-compatible jammy LXC image, fix few typos

6c3201f

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

bpg approved these changes Jan 3, 2024

View reviewed changes

bpg merged commit 58347c0 into bpg:main Jan 3, 2024
8 checks passed

ghost mentioned this pull request Dec 29, 2023

chore(main): release 0.43.0 #849

Merged

bpg mentioned this pull request Jan 10, 2024

fix(docs): typos in proxmox_virtual_environment_file resource #872

Merged

3 tasks

seankoneill mentioned this pull request Oct 3, 2024

fix(docs): correct url for template download in container resource example #1563

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): add new resource `proxmox_virtual_environment_download_file` #837

feat(storage): add new resource `proxmox_virtual_environment_download_file` #837

rafsaf commented Dec 25, 2023

bpg commented Dec 26, 2023

rafsaf commented Dec 26, 2023

rafsaf commented Dec 28, 2023 •

edited

Loading

bpg left a comment

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg Dec 28, 2023

rafsaf Dec 28, 2023

bpg Dec 29, 2023

bpg Dec 28, 2023

rafsaf Jan 1, 2024

bpg commented Dec 28, 2023

rafsaf commented Dec 28, 2023

rafsaf commented Jan 1, 2024 •

edited

Loading

bpg left a comment

ratiborusx commented Jan 9, 2024

rafsaf commented Jan 9, 2024

bpg commented Jan 10, 2024


		### Optional

		- `allow_unsupported_types` (Boolean) By default `false`. If `true`, content formats `qcow2` and `raw` can be downloaded, though it is not supported by proxmox.

		Description: "Manages files upload using directly proxmox download-url API. " +
		"It can be a full replacement for ISO files created using " +

	"Error creating Download File interface",
	"Error initiating file download",

feat(storage): add new resource proxmox_virtual_environment_download_file #837

feat(storage): add new resource proxmox_virtual_environment_download_file #837

Conversation

rafsaf commented Dec 25, 2023

Contributor's Note

Proof of Work

Community Note

bpg commented Dec 26, 2023

rafsaf commented Dec 26, 2023

rafsaf commented Dec 28, 2023 • edited Loading

bpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpg commented Dec 28, 2023

rafsaf commented Dec 28, 2023

rafsaf commented Jan 1, 2024 • edited Loading

bpg left a comment

Choose a reason for hiding this comment

ratiborusx commented Jan 9, 2024

rafsaf commented Jan 9, 2024

bpg commented Jan 10, 2024

feat(storage): add new resource `proxmox_virtual_environment_download_file` #837

feat(storage): add new resource `proxmox_virtual_environment_download_file` #837

rafsaf commented Dec 28, 2023 •

edited

Loading

rafsaf commented Jan 1, 2024 •

edited

Loading