bpg / terraform-provider-proxmox

Terraform Provider for Proxmox
https://registry.terraform.io/providers/bpg/proxmox
Mozilla Public License 2.0
771 stars 127 forks source link

proxmox_virtual_environment_file doesn't notice when the local file has been changed #677

Open TheNotary opened 10 months ago

TheNotary commented 10 months ago

Describe the bug When a file that has been uploaded to proxmox via proxmox_virtual_environment_file is changed, terraform doesn't notice this and mark the old upload.

To Reproduce Steps to reproduce the behavior:

  1. Create a proxmox_virtual_environment_file
  2. make an arbitrary change
  3. do terraform plan
  4. See that tf says the infrastructure is up-to-date even though the file on proxmox is now stale

Expected behavior I think it's supposed to keep track of the state of the local file (at the very least, probably comparing timestamps?) and when it changes, re-apply it so everything is synced up.

ratiborusx commented 10 months ago

Yup, i mentioned something like this somewhere in here in PART II of my poem: https://github.com/bpg/terraform-provider-proxmox/issues/586#issuecomment-1779596266

And now about the issue with the first implementation i mentioned earlier. If you change your external file that is getting imported into Proxmox's snippets datastore these changes are not visible for provider and resource will stay the same. Most likely because provider checks only existence of a file by its name. But if we "import" raw source even as rendered external file then provider sees changes and acts accordingly. The same thing probably happens with cloud images being imported into ISO datastore. Maybe if provider checked and compared checksum of source and target files every plan and tried to recreate resource (re-download file) if changes are detected it would work better. We could also re-download any distribution's cloud images when they get updated. But i'm not sure if it's something feasible or easily implemented or even wanted by most people.

I believe a timestamp won't help if for example you just overwrote the same file - there's no changes but it could trigger resource re-creation. I was thinking about checksum but it looks like there's already "source_file.checksum" argument. I probably need to ponder more in that direction. I'm mostly concerned with keeping my downloaded cloud images up-to-date or re-download them in case someone introduces any changes on the node's storage.

If you're concerned about changes to small files in "snippets" datastore (like cloud-config files) then i'd recommend to use terraform's "file()" function with "source_raw". That way content gets parsed and all changes are detected. Won't be a good solution for big files i guess :)

Hopefully i got it right because its a bit unclear what "local" means. In my case when i upload cloud-config.yml which resides in the same environment on my machine where all terraform's manifests are i would call it local. But when i download cloud images i consider them local when they reside on pve's storage. Maybe "source" (which could be local or remote) and "target" would reflect it better?

bpg commented 10 months ago

There is a logic in the file resource to detect local file changes (i.e. for source_file pointing to file in a file system, not URL). Though it may not work in all use cases.

@TheNotary what is the OS you're running TF from?

TheNotary commented 10 months ago

I noticed this bug on a Mac (13.6). It looks like it's using APFS as the default file system, I don't think I did any tinkering with it.

bpg commented 10 months ago

Thanks! I remember this feature worked for me, a caused unexpected recreation of a working VM because the source disk image has changed 🤦🏼‍♂️ I'll try adding some acceptance tests around it, should be able to pinpoint the issue.

TheNotary commented 10 months ago

Thanks, I was taking a stab at it just now, and it looks like on my system it's setting last_timestamp to be whatever the current timestamp is rather than looking it up from the state file (or something like that). I'll share my logs below. lmk if you have any trouble reproducing.

(line ~770)

tflog.Debug(ctx, "Comparing file attributes", map[string]interface{}{
            "last_file_md":   lastFileMD,
            "last_file_size": lastFileSize,
            "last_file_tag":  lastFileTag,
            "file_md":        fileModificationDate,
            "file_size":      fileSize,
            "file_tag":       fileTag,
        })

Logs on mac

2023-11-07T21:48:08.668-0600 [DEBUG] provider.terraform-provider-proxmox_v0.37.0: Comparing file attributes: last_file_md=2023-11-08T03:47:18Z tf_provider_addr=registry.terraform.io/bpg/proxmox tf_resource_type=proxmox_virtual_environment_file @caller=terraform-provider-proxmox/proxmoxtf/resource/file.go:774 last_file_size=11 @module=proxmox file_md=2023-11-08T03:47:18Z file_size=11 last_file_tag=654b04c6-b tf_req_id=c949c3ab-5e86-f7d2-fc8d-0ba05a5f6e3f tf_mux_provider=tf5to6server.v5tov6Server tf_rpc=ReadResource file_tag=654b04c6-b timestamp=2023-11-07T21:48:08.668-0600

Btw, do you have any tips for debug logging, I find TF's defaults to be virtually unreadable the way they are.

Also, @ratiborusx, I meant to get back to you after I did some work towards this but have come down with a bug and haven't gotten far at all. I think with the checksum approach, with most file systems you need to perform a full read of the file and then calculate the checksum before you know whether the file is up-to-date or now which wouldn't be terribly significant for small files, but with larger files, say in the 9gig range, that could put a significant delay on the act of terraform plan so hopefully bpg can get to the bottom of why things aren't updating.

bpg-autobot[bot] commented 4 months ago

Marking this issue as stale due to inactivity in the past 180 days. This helps us focus on the active issues. If this issue is reproducible with the latest version of the provider, please comment. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!