Velero: exclude-from-backup not working for PersistentVolume

[chris-ng-scmp]

What steps did you take and what happened:
Exclude a PersistentVolume to backup

Step 1:

kubectl  label -n kube-tools pv/d-j6cai1gtm341t3ur48v2 velero.io/exclude-from-backup=true

Step 2:

velero backup  create test

Step 3:

velero backup describe test --details 

Output

...
  v1/PersistentVolume:
    - d-j6cai1gtm341t3ur48v2
...
Persistent Volumes:
  d-j6cai1gtm341t3ur48v2:
    Snapshot ID:        s-j6cfjrz9szlt3kmit2s6
    Type:               cloud_ssd
    Availability Zone:  cn-hongkong-b
    IOPS:               <N/A>
...

What did you expect to happen:

Should no snapshoot taken for the PV

Environment:

• Velero version (use velero version):

Client:
    Version: v1.1.0
    Git commit: a357f21aec6b39a8244dd23e469cc4519f1fe608
Server:
    Version: v1.1.0

• Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.3", GitCommit:"2d3c76f9091b6bec110a5e63777c332469e0cba2", GitTreeState:"clean", BuildDate:"2019-08-19T12:36:28Z", GoVersion:"go1.12.9", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"12+", GitVersion:"v1.12.6-aliyun.1", GitCommit:"8cb561c", GitTreeState:"", BuildDate:"2019-04-22T11:34:20Z", GoVersion:"go1.10.8", Compiler:"gc", Platform:"linux/amd64"}

• Cloud provider or hardware configuration:
  Alicloud

[skriss]

thanks for the report @chris-ng-scmp. It looks like the problem is that when we process "additional items", we're not excluding items based on this label. Since PVCs/PVs are backed up as "additional items" for the pod that they're used by, they're affected by this.

[prydonius]

Yeah we are excluding by this label when we list resources (https://github.com/heptio/velero/blob/3618e51633eca3eed5600217df5e5473f65ab111/pkg/backup/resource_backupper.go#L219-L225) but don't use the label selector again when we fetch additional items. @skriss it looks like we can just add the same label selector to this call (https://github.com/heptio/velero/blob/3618e51633eca3eed5600217df5e5473f65ab111/pkg/backup/item_backupper.go#L329)?

[skriss]

I don't think you can use a label selector with a .Get() -- we might want to do it around https://github.com/heptio/velero/blob/3618e51633eca3eed5600217df5e5473f65ab111/pkg/backup/item_backupper.go#L126 where we do similar re-checks of filters, but we'd have to use the labels package to check if it matches (we do something similar in the restore code since we're checking in-memory items against a label selector).

[skriss]

@prydonius do you want to take fixing this for 1.2?

[prydonius]

@skriss ah you're right. If we do it there, do you think we should remove it from the resource_backupper list, since we'll just be checking it twice? Although it does save iterating through objects we know have that label.

[prydonius]

Another option is to replace the Get with a List with the label selector and field selector to get the resource by name

[skriss]

I'd say it's probably fine to remove it from resource_backupper and just do it once at the top of backupItem -- avoids duplication and the minor inefficiency is probably negligible.

[prydonius]

sgtm, I can send a PR to fix this later today

[chris-ng-scmp]

Thanks for the quick response!