PV remapClaimRefNS was being skipped when there was no snapshot

[sseago]

Signed-off-by: Scott Seago sseago@redhat.com

Thank you for contributing to Velero!

Please add a summary of your change

Previously (#3007 ) a fix was submitted which handles PV ClaimRef remapping and PV renaming when there is a snapshot, but it did not handle the default case of no snapshot, no restic, but still restoring the PV. This extends the same behavior to the default case (no snapshot, no restic, no delete reclaim policy): the ClaimRef gets its namespace remapped if necessary, and the PV gets renamed if necessary.

Tests included to confirm the fix.

Does your change fix a particular issue?

Fixes #3707

Please indicate you've done the following:

• [ x] Accepted the DCO. Commits without the DCO will delay acceptance.
• [ x] Created a changelog file or added /kind changelog-not-required.
• Updated the corresponding documentation in site/content/docs/main.

[nrb]

I think it looks ok, but it's mostly a copy paste from what I can tell.

Is it possible to move the remap logic to a function that is shared?

[nrb]

Hmm, since this is the same logic as the first case in the switch, I'm wondering if we should consolidate this into a function, or maybe do some of the logic before the switch.

The downside of moving it before is that it may be more work even in cases where there is no mapping to take place.

[sseago]

@nrb So I looked into this, but it looks like it would be difficult to drop all of the logic here into one function. Basically the logic added to the default case does 3 things:

1. Check whether we should rename the PV
2. Rename the PV if 1) is true
3. Remap the namespace if we need to

So 1+2 and 3 are somewhat independent of each other. Currently 2) and 3) are done in a different order in the two code paths, but there's really no need for that. However, the part that makes putting all of this code into a func is the fact that in the hasSnapshot case, there is some logic required between 1) and 2) which is not there in the default case. One thing I could consolidate is the block of code inside the if shouldRenamePV block -- I could put that into a a new func renamePV(...) which would be called inside the if block in both cases, if that would be enough of an improvement in terms of avoiding duplication.

[zubron]

While reviewing this change with others on the team, @dsu-igeek pointed out that although we are renaming the PV, remapping the ClaimRef namespace, and resetting the volume binding info, this doesn't clear the PV source, which means that the new renamed PV will still be pointing to the original backing storage currently in use by the original PV.

I experimented with this locally and saw that there doesn't seem to be anything to prevent multiple PVs from using the same backing storage (if they are on the same node). This means that data can be written by a pod in the original namespace and this can be read by a pod in the remapped namespace (as well as being able to perform other destructive operations).

I don't think it's possible to "clear" the source, as it is necessary to reference a volume type. Should we be provisioning a new PV in this case to go with the remapped namespace and PVC?

[dsu-igeek]

Without this do we get an error or do we get the PVC mapped to the original PV? I like the "principle of least surprise". To me, when I do a restore, I expect that I will get my original data back. In the case where we are restoring from a snapshot we get that. If we reconnect to the existing volume then we get that. Those are our existing cases. If we reconnect to the same volume but it's also in use somewhere else, that seems like a recipe for trouble when it works and a weird error when it doesn't (Bridget told me that she initially tested with the pods being on the same worker node and in that case the restore pod/PVC/PV was connected to the same directory as described above but if the pods wound up on different nodes, the restored pod would get an error that it couldn't attach to the storage). If we make a new empty volume that's probably not what is expected.

[sseago]

@dsu-igeek So without this, the following was the observed behavior (from the issue filed):

What steps did you take and what happened:
Restored a PV (without a snapshot) and the PVC it's bound to into a different namespace. The PVC was restored but "pending", with an error that the PV was bound to another claim. The reason for this is that the ClaimRef was not updated to the new namespace. The ClaimRef remapping is only currently done in restore/restore.go when there's a snapshot. It's also needed in the non-snapshot case.

[sseago]

So basically the PVC post-restore was not usable.

[sseago]

It was bound "to another claim" because its claimRef was different than the PVC that was restored with it.

[sseago]

@zubron Regarding the "not clearing the PV source", how is that different from a normal restore (without a namespace change) where the PV and PVC are restored. Since we're not using restic, a new PV isn't provisioned. Doesn't the "no namespace change" use case also result in the same scenario around two PVs using the same backing storage?

[sseago]

The particular use case where we hit this actually relies on the PV not changing its backing store data. We have a "Move PV/PVC" use case where in a migration, we do a backup of PV and PVC without restic or snapshots. Then after shutting down the application using the volume on the source cluster, we restore the backup to the destination cluster, and the transferred PV/PVC definitions allow the destination cluster to access the original (NFS) source volume. This works just fine in the "no namespace change" use case. The bug fixed by this PR was preventing it from working in the namespace change on restore scenario.

[dsu-igeek]

@sseago If we're restoring into the same namespace then the assumption would be that the PVC in that namespace had been removed or was still there (and we wouldn't overwrite it) and we wouldn't wind up sharing. I think it's still kind of dangerous and we should figure out these scenarios properly. Let's discuss in community meeting how to handle your case.

[sseago]

@dsu-igeek Sounds like a plan. So here are the two scenarios that we're dealing with (one works without this fix, one does not):

1. Backup namespace with PVCs and PVs in one cluster, no restic or snapshot copy; shut down applications using volumes; restore to different cluster. PV/PVC pair in new cluster points to same underlying storage as old cluster, effectively the PVC was "moved", so no data copy was needed. new cluster points to same (external) NFS share as the old cluster was using.
2. Same as 1) but change namespace on restore. With this fix, everything works just as in 1). Without this fix, the PVC remains "pending" forever. I suspect that the danger that's being discussed with this fix is still there in the same-namespace case, so I'm not certain that any new danger is introduced with this fix, but we can discuss it on the call next week.

[sseago]

@dsu-igeek @zubron I've updated the PR to remove the PV rename, so we're just fixing the pv/pvc mapping to handle namespace remapping. Unit tests updated accordingly, and we're also looking for the expected restore warning in the case where the PV already exists (since velero will discard it).

[dsu-igeek]

LGTM

[zubron]

Thanks, @sseago!