Feat add snapshot description field

[heldergomes]

No description provided.

[heldergomes]

This PR implements #2254

[heldergomes]

because this MR is quite long, I will try to explain a little bit about some change decisions and few concerns!

storage_volume_snapshot
Despite the requirement is quite simple, the change is painful because I had to change many piece of code, therefore I decided to change only instance snapshot on this PR, but I could open a new pull request changing volume snapshot afterward.

shared/api/instance_snapshot.go
I'd expected to refactor a little bit the InstanceSnapshot struct, snapshot data such as (expiry_at and description) should be on root struct and source instance data like (instance description) should be on nested object like InstanceProperties IMHO, this would design better the model to handle better with more instance properties in the future, however this change would break API, threfore I just add SnapshotDescription field on the model.

instance_args
Handle this struct was quite challenge mostly because incus handle snapshots and instance in the same way with instance_args, so although qemu/lxc driver and shared/api/instance_snapshot I ended up handle snapshot fields such as description and expiry on root struct, I took decision to create snapshot_args as nested object of instance_args, in order to make easier add new fields on this highly used struct.

I'd rather use snapshot as a nested struct of instance on every place like driver, api, db, etc, but this change is painful, so it's what it is!

instance_snapshot_property
I tried do use the term instance_snapshot_properties, however generate_database script only knew how to create mapper like list if tables name was singular, tables with suffix ies is considered plural on the script.

I think that's all, let me know if have any question or improvements.

[heldergomes]

Hi @stgraber, I think It's done this PR.
It was a bit challenge understand this whole change and I end up making a little mess with the past PR, so I closed the one that I had open and I made a clean branch with this one.

[stgraber]

@heldergomes sorry for the delay, I've been dealing with quite a bit of backlog :)
I'm now finally at the stage where I'm reviewing this. I'll do a quick rebase and push to get it building again and then start the review and tweaks on my end.

[heldergomes]

No worries @stgraber
I will work on the same feature for volumes snapshots, sooner or later I will open the MR.
Only one thing that I probably need help is the migration part, I made few tests and I worry that once a out-of-date incus update to the incus version after this change, the snapshots are not migrate properly.
I'd like to ask to double check carefully the /db/update.go

[stgraber]

Back to this one now that I've got the rest of the Incus 6.22 backlog taken care of ;)

[stgraber]

I'll most likely get that one merged right after 6.22 so we have a whole month with the DB change and can deal with any unexpected consequences :)

[stgraber]

I've pushed an initial edit. Basically drops the check in client/, re-orders and re-titles a bunch of commits. I'll look at the pointer in the Args struct next.

I also want to think a bit more about how we expose this stuff in the API as having ExpiresAt and SnapshotDescription feels pretty inconsistent. I need to go back to the issue and see if there's some alternative way to handle that which still doesn't break API ;)

[stgraber]

@heldergomes sorry for all the delays...
I was looking into this one again today after a rebase and it's now failing to build because of the dependent volume logic we've since merged.

It's not obvious to me how to change that code path to work with your new logic.
If you have a bit of time, it'd be great if you could take a look!

I've pushed what I've got here into your branch.

[heldergomes]

Hello @stgraber I will working on this fix!
I'm on this!

[stgraber]

Thanks!