Remove dataSource from PVC on backup for prior CSI restore case

[eemcmullan]

Thank you for contributing to Velero!

Please add a summary of your change

Removes the dataSource field from PVCs on backup to resolve issue #6106

Does your change fix a particular issue?

Fixes #(issue)
#6106

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 as a comment on this pull request.
• Updated the corresponding documentation in site/content/docs/main.

[kaovilai]

Lgtm

[blackpiglet]

Please also add a changelog file.

[Lyndon-Li]

I suggest we move the same to restore, specifically:
During restore, we judge if the DataSource/DataSourceRef is required (for CSI restore, it is required, for others it is not), if it is required, leave the fields there, otherwise, remove them as what we are doing here

Reasons:

1. In backup, we may have multiple BIAs, a BIA doesn't know what others is doing or will do, so if not necessary, a BIA should not change the data comparing to the source, otherwise, we don't know what other BIAs will react, especially, DataSource/DataSourceRef are used by various data mover purposes
2. As the principle, if not necessary, the backup procedure should not change anything comparing to the source. This principle guarantees the consistency and coherence of the restore data and finally guarantees the validation of the restore
3. The current problem is right indicating the requirement that we need to take care of the DataSource/DataSourceRef in restore, just like what we do for Status fields. Therefore, it looks more appropriate to put it into restore

On the other hand, I am not seeing any direct error the current change will cause, so I will leave this to more members for judgements, @reasonerjt @blackpiglet @shubham-pampattiwar

[blackpiglet]

Keeping the resources unchanged in backup is reasonable, but I also couldn't spot any scenarios broken by the current change.

[sseago]

@Lyndon-Li the problem with clearing it on restore is that restore is where some plugins might add it. For example, CSI restore plugin adds it, so if the built-in RIA clears it, we could have a race condition, and there's no way of knowing within one plugin action whether another action has been called and/or will be called. Are there any use cases where having this in backup is needed? This field is only used for initial content creation for a new PVC. This source provided the content for the PVC at creation time. It will not have any content that was added/modified after creation -- for that we need the current content, via kopia/restic, CSI snapshot, or native snapshot. So I'm not sure we ever need/want this in the backup.

[Lyndon-Li]

@sseago Yes, indeed, if we want to put the clear code in restore, we need to make sure that the fields are not added by a CSI RIA plugin. One idea is we put the clear code at early stage of restoreItem, e.g., somewhere around here, none of the RIAs has been called at this place, so it is safe to remove the fields without a check.

Are there any use cases where having this in backup is needed?

Personally, I don't see any need for them during in backup at present, so technically, I don't see any error will happen if we keep the current clear code in backup.
Merely, I just think it is more appropriate to have the code in restore (as the reasons mentioned above). Therefore, I raise this as a suggestion, I am fine with either way as the final decision.

[sseago]

If it were in the core restore logic rather than in a RestoreItemAction, it would work as long as it was processed before any plugins run. However, the restore workflow is already full of if blocks ("if groupResource ==", etc.) and this would add to the complexity, while doing it on backup as in this PR is putting it in code already filtered for PVCs. Also, if we're clearing the field before running any plugins, then it means the field from backup will never be used anyway, so we might as well remove it on backup. In general, we don't want to change much data on backup, as for most use cases, restore is more appropriate, but in cases where a field will never be useful for restore (or for RIA funcs), then removing on backup is fine.

[Lyndon-Li]

as for most use cases, restore is more appropriate, but in cases where a field will never be useful for restore (or for RIA funcs), then removing on backup is fine

OK, this sounds reasonable

[kaovilai]

Velero add label to items it restored

velero/pkg/restore/restore.go

Line 1853 in 65f99c1

labels[velerov1api.BackupNameLabel] = label.GetValidName(backupName)

Do we wanna limit this action to items we are certain are restored by velero?
Or perhaps limit to just those restored by velero-csi-plugin?

[sseago]

I don't think prior restore is necessarily relevant. While the specific failure case we've seen involves prior restore, I think these fields would almost always result in restoring a PVC with stale or invalid content.

[kaovilai]

Ok then retitle the PR to a broader scope, then suggest CSI restore as one of the failure case.