Fix bug: if the snapshot is no longer in engine CR, don't block the removal process

[PhanLe1010]

longhorn/longhorn#6298

[ejweber]

I didn't realize you were already on a PR for this, so I drafted #2075. It is similar in concept (allow us to proceed to remove the finalizer if the snapshot is not in the engine). However, I was hoping to just reorder things without adding additional checks.

I'll defer to you since you know this section of code best. LGTM, but need to test it.

[PhanLe1010]

@ejweber I think we still need to check with the engine process before removing the snapshot CR's finalizer. This PR will do that. What do you think?

[ejweber]

We discussed my "competing" PR offline and are in agreement that this is the right approach.

We don't have a simple recreate, but I can use the same iterative test I opened the issue with to check whether my cluster continues to accrue snapshots with this fix.

[james-munson]

After substantial discussion and explanation, this makes sense to me.

[innobead]

LGTM.

nit: is it possible to have an orphaned snapshot always unable to delete because it can't be deleted at a replica side somehow, so the volume keeps in an auto-attaching state (surely, ticket type snapshot should be interruptable, so it should not impact other operations ideally)?

cc @shuo-wu @derekbit

[innobead]

@mergify backport v1.5.x

[mergify]

backport v1.5.x

✅ Backports have been created

• #2077 Fix bug: if the snapshot is no longer in engine CR, don't block the removal process (backport #2074) has been created for branch v1.5.x

[innobead]

Also, auto-cleanup-system-generated-snapshot was introduced in 1.4 as well, so we don't need to tackle this issue even thought the implementation is different than 1.5 which is using longhorn VA.

[ejweber]

Tested as suggested in #2074 (comment). After 15 iterations my cluster has only 50 snapshots, and each snapshot is only less than five minutes old. This is as expected.

[PhanLe1010]

LGTM.

nit: is it possible to have an orphaned snapshot always unable to delete because it can't be deleted at a replica side somehow, so the volume keeps in an auto-attaching state (surely, ticket type snapshot should be interruptable, so it should not impact other operations ideally)?

cc @shuo-wu @derekbit

If the snapshot is stuck in the removed state in the replica, yes, the volume will remain attached due to the snapshot-controller attachment ticket.

If workload starts on different node, it will interrupt the snapshot AD ticket. For other operations that require attachment, they will request the same node as the snapshot AD ticket. So I think it is fine

[PhanLe1010]

Also, auto-cleanup-system-generated-snapshot was introduced in 1.4 as well, so we don't need to tackle this issue even thought the implementation is different than 1.5 which is using longhorn VA.

Sorry, could you elaborate more on this?

I think this issue should happen in 1.5.x only because of the new AD mechanism

[innobead]

Sounds good.

[innobead]

Also, auto-cleanup-system-generated-snapshot was introduced in 1.4 as well, so we don't need to tackle this issue even thought the implementation is different than 1.5 which is using longhorn VA.

Sorry, could you elaborate more on this?

I think this issue should happen in 1.5.x only because of the new AD mechanism

It has been clarified. All good.