-
Notifications
You must be signed in to change notification settings - Fork 600
feat: add resource storage template #1420
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
base: master
Are you sure you want to change the base?
feat: add resource storage template #1420
Conversation
|
@NemoDacremont This feature looks promising. One minor thing: We should have some computed id so the lxc resource can reference it directly. One major thing: Could we look into up streaming the logic to interact with the PVE API to the SDK project? As the Terraform provider should only be a facade, since it's quite difficult to test. |
proxmox/resource_storage_template.go
Outdated
| defer lock.unlock() | ||
|
|
||
| storage := strings.SplitN(d.Id(), ":", 2)[0] | ||
| templateURL := fmt.Sprintf("/nodes/%s/storage/%s/content/%s", d.Get("pve_node").(string), storage, d.Id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we look into up streaming the logic to interact with the PVE API to the SDK project? As the Terraform provider should only be a facade, since it's quite difficult to test.
I'm not sure to understand what you meant, are you talking about this part forging manually the request url ? For the other parts, I feel like it uses correctly the sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was about the url and https://github.com/Telmate/terraform-provider-proxmox/pull/1420/files#diff-94914ef253ac2ff7283aee16ba7efba607cded5e7eb3c5143e8d4fd96fbefcd8R95
The SDK has an old legacy part which is client.GetStorageContent as the SDK should never expose the raw API response, as it requires the consumer to understand the internals of the API across PVE versions.
I guess for now it's fine as those parts have to be improved in the SDK first.
I actually don't know terraform that much, what computed id ? Do you have an example I can refer to ? |
|
@Tinyblargon Thank you for your previous feedback, I finally took the time to understand what computed attributes are. I added the computed attribute
About that, I think I also get what you meant, is it about these two features ?
Can you confirm it is the logic that should be up streamed to the SDK project ? If this is the case, I can try working on it. Finally, I tested the computed attribute with the following config if you wish to test it (proxmox v9) : resource "proxmox_storage_template" "template_example" {
pve_node = "pve"
storage = "local"
template = "alpine-3.22-default_20250617_amd64.tar.xz"
}
resource "proxmox_lxc" "lxc_examplte" {
vmid = 123
cores = 1
memory = 1024
swap = 0
hostname = "test"
target_node = "pve"
ostemplate = proxmox_storage_template.template_example.os_template
password = "12345"
start = true
force = false
unprivileged = false
features {
nesting = true
}
rootfs {
size = "1G"
storage = "local-lvm"
}
network {
name = "eth0"
bridge = "vmbr2"
firewall = true
ip = "dhcp"
}
} |
Yes that's exactly the logic I meant. Preferably type LxcTemplate struct {
Node NodeName
Storage string
File string
} |
|
We do already have https://github.com/Telmate/proxmox-api-go/blob/0e194e6b413db3c7030797fe850dd5596b284948/proxmox/config__lxc__new.go#L766 not sure if we should extend that or not, as we don't need to know the node there, so adding it would just be confusing. But I'm feel having |
|
@Tinyblargon I think this patch starts to reach a state I like. Here's a summary of what happened and what I tested to help to reproduce. Latest commitsIn the last patches, I use the latest methods I introduced in my recent PR on Telmate/proxmox-api-go#495. I also introduced To summarize, the patch :
The CI fails obviously because the SDK version requires to be bumped before it can pass the tests. However, beware that it seems there is other issues with the latest commit as explained below TestsI tested this patch, and here is how you can reproduce them. I tested on the base Proxmox image v9.0.11 I build with NemoDacremont/proxmox-ve for VirtualBox. I followed your tip, and used However, I had issues while compiling the provider, and had to comment out lines defining Test: Create, Read, Delete, RecreateUsing this terraform config : resource "proxmox_storage_template" "template_example" {
pve_node = "pve"
storage = "local"
template = "alpine-3.22-default_20250617_amd64.tar.xz"
}I applied once to test it downloading the template, then I applied again to make sure it detected it already being there. I finished by removing it and applying to make sure it was deleted. I finally recreated it to make sure it could redownload it. Download template and a LXC using the computed attributeIn order to make sure the LXC could use the computed attribute resource "proxmox_storage_template" "template_example" {
pve_node = "pve"
storage = "local"
template = "alpine-3.22-default_20250617_amd64.tar.xz"
}
resource "proxmox_lxc" "lxc_servers" {
vmid = 123
cores = 1
memory = 1024
swap = 0
hostname = "test"
target_node = "pve"
ostemplate = proxmox_storage_template.template_example.os_template
password = "password"
unprivileged = true
features {
nesting = true
}
rootfs {
size = "1G"
storage = "local-lvm"
}
}It worked well, and I could start the LXC manually then. |
|
Hi @Tinyblargon I've been busy lately, and I couldn't find time to continue what I was doing on proxmox-api, I'll catch up later when I'll have more time. Meanwhile, I've seen that you bumped the proxmox-api version, and it seems like this feature should pass the CI now if you're interested in merging it |
|
@NemoDacremont There are 2 minor changes I'd make to the Terraform interface. If it's okay with you I can merge this as is and do the minor renames. Everything else looks good.
In the future I'd like to add options to download from a url, and support values from the template menu in PVE. resource "proxmox_storage_template" "template_example" {
node = "pve"
storage = "local"
template {
package = "debian-13-standard"
version = "13.1-2" # no version will download what is the latest version at resource creation.
}
} |
|
Ok no problem @Tinyblargon , I've just started working on it, I'll leave the download from url for a later PR if that's fine with you. The move from pve_node to node will be fairly straightforward, and I'll see what I can do for the template field, but it should be ok |
|
Hey @Tinyblargon, I could find time to finish the changes you requested
Well, maybe I shouldn't have read your message that fast, maybe it would have been simpler for you to just do it...
As expected, I didn't implement the download from url, but I introduced a new block "template" that supports "file" or package[+version] to define the template. The patch might be a little verbose, however I think it works fine and hope it meets your expectations with the package and version. I also updated the docs to define a resource template. Feel free to change the PR or merge and apply another fix. I tested with this terraform file : resource "proxmox_storage_template" "alpine322" {
node = "pve"
storage = "local"
template {
file = "alpine-3.22-default_20250617_amd64.tar.xz"
}
}
# resource "proxmox_storage_template" "alpine322" { # Should fail with explicit error message
# node = "pve"
# storage = "local"
# template {
# }
# }
# resource "proxmox_storage_template" "alpine322" {
# node = "pve"
# storage = "local"
# template {
# package = "alpine-3.22-default"
# }
# }
# resource "proxmox_storage_template" "alpine322" {
# node = "pve"
# storage = "local"
# template {
# package = "alpine-3.22-default"
# version = "20250617"
# }
# }
# resource "proxmox_storage_template" "alpine322" { # Should fail, version doesn't exists
# node = "pve"
# storage = "local"
# template {
# package = "alpine-3.22-default"
# version = "1337"
# }
# } |
Add resources for CT templates, it is heavily inspired of
resource_storage_iso.goresource.
Implemented :
This enables creating template by adding resources, removing template by removing the resource from the .tf file when it alread exists.
This can be tested with the following config (after a
make local-dev-install)fixes #1419