Re-organize snapshot handling, allow for snapshot-specific properties (description, expiry, ...)

[LOTRg]

Is there an existing issue for this?

• [x] There is no existing issue for this feature

What are you currently unable to do

Hello All , First of all ... thank you so much for your hard work on such a quality, well-thought project which is now my default container platform specially that I don't like docker and the likes.

I have a feature request to please add snapshot description field, it would be nice to know what was this snapshot before or after, or the state or other interesting information that can be entered and later be viewed with snapshot list or something, much like other software like VMWare products, KVM or even incus images where description field is available ... this will greatly help with workflow knowing exactly what each snapshot represent.

Once again, thank you for a quality software and the hard work you do .

What do you think would need to be added

A description flag to the snapshot create verb which can be stored in the DB or stored along with the snapshot as a Key/value .

[LOTRg]

Any feedback on this or if it's planned ?

[stgraber]

We actually have a description field in the database and API, so should just be a matter of correctly exposing it.

[stgraber]

Ah, hmm, not quite, the current description field we have is meant to snapshot the instance's own description and then restore it, so we shouldn't use that.

I think we should do a bit of re-shuffling so that instances_snapshots holds data about the snapshot themselves and then use a instances_snapshots_properties or something along those line to hold the instance properties, avoiding mixing the two.

At the API level, we'd then probably want to put the snapshot-specific stuff into a separate struct.

So anyway, that will be a fair amount of work, also because we'd want to clean up this for both instances and volumes, but it's certainly worth doing.

[LOTRg]

Very much appreciated @stgraber and thank you for the great work ! looking forward to when this get implemented .

[heldergomes]

I'd like to take this one!

@stgraber may I ask for help about which components I should digging deep to understand how to make those changes

[heldergomes]

I spent some time understanding the snapshot code in Incus and came to the following conclusion regarding this change:

• Need to add new version on update.go to add instances_snapshots_properties, storage_volumes_snapshots_properties and migrate description field to the new table for past snapshots.
• Add description flag on snapshot creation (CLI and API).
• Add new struct "properties" on snapshot retrieve.
• Parse properties model on snapshot creation and restore.

I'm still studying snapshot's feature to undestand if scheduler might have any side effect.

I was thinking to add only description on properties' table for now, is there any other field that should be moved to there?

[stgraber]

I think the first step should be to create instances_snapshots_properties and storage_volumes_snapshots_properties and as part of that, move just about all existing fields from the main table over to that.

Looking at instances_snapshots we currently have:

• id
• instance_id
• name
• creation_date
• stateful
• description
• expiry_date

And an instances entry has:

• id
• node_id
• name
• architecture
• type
• ephemeral
• creation_date
• stateful
• last_use_date
• description
• project_id
• expiry_date

Not all the fields in instances make sense to snapshot, but those do:

• description
• ephemeral
• stateful

So those are the initial ones we'd want in instances_snapshots_properties.

With that change, we can then:

• Add a instances_snapshots_properties_id foreign key to instances_snapshots
• Drop stateful from instances_snapshots
• Re-purpose description in instances_snapshots to be the description of the snapshot and not the description of the instance
• Add a snapshot struct to the data returned at /1.0/instances/NAME/snapshots/NAME which would expose the snapshot fields:
  • creation_date
  • description
  • expiry_date

On top of that, I've also noticed that we have an expiry_date field in the instances table. That one doesn't really make any sense to me and was most likely there from back in the day when instances and instance snapshots were in the same table. Since we're doing a bunch of cleanup around those tables, we should also drop that expiry_date column from the instances table.

[heldergomes]

@stgraber Thank you for your help! It's clear for me now 👍

[heldergomes]

@stgraber I'm checking if would really make sense move stateful from instances_snapshot to properties' table, since there's stateful flag on snapshot creation

Examples:
  incus snapshot create u1 snap0
  	Create a snapshot of "u1" called "snap0".

  incus snapshot create u1 snap0 < config.yaml
  	Create a snapshot of "u1" called "snap0" with the configuration from "config.yaml".

Flags:
      --expiry string   Expiry date or time span for the new snapshot
      --no-expiry       Ignore any configured auto-expiry for the instance
      --reuse           If the snapshot name already exists, delete and create a new one
      --stateful        Whether or not to snapshot the instance's running state

I'm trying to understand if it's possible create a stateful snapshot from a non-stateful instance, if so I think we should remain stateful on instance_snapshot, however I got an error when I try to create a stateful snapshot from a non-stateful snapshot

incus snapshot create tomato-test i3u4 --stateful=true
Error: Stateful snapshot requires migration.stateful to be set to true

Could you help me here? why it has stateful flag on snapshot's creation if stateful is always the same as the source instance?

[stgraber]

The stateful flag on the instance is an indication of whether there is state that will be restored on next boot.

So it will be true if:

• Looking at a snapshot created with incus snapshot create --stateful
• Looking at a stopped instance which was stopped with incus stop --stateful

So for snapshots, we just need the flag in instances_snapshots_properties and we'll be able to tell if a snapshot is stateful or not based on whether that property is set to true.

[stgraber]

(Yeah it's a bit weird as it's effectively a read-only property on the instance, we don't exactly have many of those in our API)

[heldergomes]

ok, thanks

[heldergomes]

Hi @stgraber, I need a help again rsrs - quite challenge this change :-) I notice that DROP A COLUMN is not recognized by generate-database, I had check on incus source code and it seems anyone did this before, so I'm struggling to understand how could I remove stateful from instance struct and mapper through generate instead straight in the code. may you help me?

[stgraber]

That's not supported by sqlite3, the way we've handled it in the past and you'll see quite a few examples of that in the DB update logic is by creating a new version of the table and moving the data over.

https://github.com/lxc/incus/blob/main/internal/server/db/cluster/update.go#L256

This is the most recent example I could find of it.

[heldergomes]

Hi @stgraber, I'm almost done with this change! I'd like to ask for help!

Basically I'm planning add InstanceProperties struct nested on InstanceSnapshot struct in order to handle in a better way the snapshot API requests, however if I include ephemeral and stateful on InstanceProperties, would be better I remove those fields from InstanceSnapshot to avoid duplicated data on snapshot's endpoint.

If I clean up ephemeral and stateful from InstanceSnapshot, Would be a breaking change, right? Is there any other option to avoid breaking change?

[stgraber]

What structs are you talking about exactly?

Changes to structs in shared/api would be an API break, but I don't see why we'd need to mess with those much. If you're talking about structs within the db package, then that's internal and we can do whatever we want there.