-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resources that don't match the label selector are backed up #7750
Comments
This is related to a BIA called velero/pkg/backup/actions/pod_action.go Lines 47 to 50 in 6f7807c
It lists all pod's mounting PVCs and returns them as additional items. We may need to discuss whether this is the expected behavior. |
Generally speaking, the intent of the label selector on the backup is to back up only those resources that match the label. If the pod matches and the PVC doesn't, then only the pod is backed up not the volume. If you want both backed up, you should label both pod and PVC (and PV). If we automatically included every additional item from plugin response, even if it didn't match the backup spec (label selector, includes/excludes, etc.) then we'd break the use case where a user wants to back up only Pods. |
However if namespace have the labelselector, anything under that namespace would be backup as well per #7697 IIUC. |
@kaovilai @sseago |
/assign @blackpiglet |
@blackpiglet Actually, it should already work this way. When we call backupItem on the returned additional item, one of the first things we do is to check for exclusion. The only exception to this is if the mustInclude annotation is set -- if I'm remembering correctly, the CSI plugin sets this on the newly-created VolumeSnapshot resources since they're resources that Velero creates and the user may not know about (and thus may not include in backup). |
@blackpiglet but maybe we only check include/exclude resource types and not labels? I guess we check label selector matches in a different place, so maybe that doesn't happen on additional item return. So if it's not currently working this way, we may want to think about whether we want it to or not -- since any change would be a change for users who rely on current behavior. |
@blackpiglet thinking about this again, the pod_action returns PVCs, then the pvc_action returns PVs. At each of those points, we do check includes/excludes and only add those additional items to the backup if the includes/excludes call for it. In the above case, if we were excluding PVCs directly rather than using label selectors, it would have been excluded in the example above. I suspect that what we really need to do is just make sure that when we're checking includes/excludes we also check the label selector. Also note that there are cases where we bypass these checks for additional items, using the "backup.velero.io/must-include-additional-items" annotation -- we do this for CSI snapshots, since there's no way the user could have labeled the VolumeSnapshot properly and probably doesn't know to include it in the included items list. I think the case could be made that the pvc action which returns the bound PV should set this annotation, meaning including the PVC should always include the PV, regardless of whether the PV has the matching label. I don't think we want that behavior for Pod->PVC, as it should be possible for a user to back up a Pod without its volumes (and vice versa). |
Quicker summary -- I think the desired outcome is that, in general, backup label selector only backs up labeled items, not every additional item returned by BIAs for labeled items, but we need certain exceptions using the mustInclude annotation. We already have exceptions in place using this annotation for CSI plugin additional items. We probably need this for PVC additional items returned in pvc_action.go. Main reason for wanting this is that since PVs are dynamically provisioned, if a user creates their PVCs with labels already applied, the PVs won't have the labels, so it's an extra (and error-prone) step to have to remember to do this. Also, I can't really see any use cases that would call for backing up a PVC without a PV, while the same is not true for other plugin additional item returns (Pod->PVC, for example, should require the label on the PVC for that to be included). |
@sseago |
After thinking twice, allowing users to pick the labeled resources is reasonable. The suggested change is valuable, but it requires more knowledge from the user, and it's breaking change. |
The workload:
The backup command:
The backup result:
The PVC/PV isn't labeled with
app=etcd
but is still included in the backup.The issue also happens in restore.
The text was updated successfully, but these errors were encountered: