Skip to content
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

Allow to exclude storageclasses from the annotations #10

Closed
wants to merge 1 commit into from

Conversation

Gui13
Copy link

@Gui13 Gui13 commented Aug 18, 2020

Fixes #7

I follow up on @fredgate and implemented the filtering of storage classes.

Tell me what you think of it.

@Gui13
Copy link
Author

Gui13 commented Aug 19, 2020

WAIT THIS IS NOT WORKING!

Copy link
Owner

@duyanghao duyanghao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WAIT THIS IS NOT WORKING!

What do you mean by "WAIT THIS IS NOT WORKING!"?

@@ -69,10 +69,11 @@ The following table lists the configurable parameters of the `velero-volume-cont
| `veleroVolumeCfg.includeVolumeTypes` | The comma-separated list of [volume types](https://kubernetes.io/docs/concepts/storage/volumes/) to include in the backup annotation addition (default: all volume types). | `persistentVolumeClaim` |
| `veleroVolumeCfg.excludeVolumeTypes` | The comma-separated list of [volume types](https://kubernetes.io/docs/concepts/storage/volumes/) to exclude from the backup annotation addition. | |
| `veleroVolumeCfg.excludeJobs` | The comma-separated list of [job names](https://kubernetes.io/docs/concepts/workloads/controllers/job/) to exclude from the backup annotation addition (support [basic string globs](https://github.com/ryanuber/go-glob)). | |
| `veleroVolumeCfg.excludeStorageClasses` | The comma-separated list of [storage classes](https://kubernetes.io/docs/concepts/storage/storage-classes/) to exclude from the backup annotation addition. This allows you to exclude storage classes that support snapshoting, since they should be handled by issuing a snapshot creation rather than go through restic. | |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma-separated list of storage classes to exclude from the backup annotation addition. This allows you to exclude storage classes that support snapshot feature since they could choose to be handled by issuing a snapshot creation rather than go through restic.

@@ -66,7 +67,7 @@ func main() {
run := func() {
kubeInformerFactory := kubeinformers.NewSharedInformerFactory(kubeClient, 0)

controller := velerovolume.NewController(cfg.VeleroVolumeCfg, kubeClient, kubeInformerFactory.Core().V1().Pods())
controller := velerovolume.NewController(cfg.VeleroVolumeCfg, kubeClient, kubeInformerFactory.Core().V1().Pods(), kubeInformerFactory.Core().V1().PersistentVolumeClaims())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to use PersistentVolumeClaimInformer since we don't watch this kind of resource, and use kubeclientset directly to get pvc information.

@@ -48,6 +48,8 @@ type Controller struct {
podsLister corelisters.PodLister
podsSynced cache.InformerSynced

pvcLister corelisters.PersistentVolumeClaimLister
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this as explained above.

@@ -8,4 +8,5 @@ veleroVolumeCfg:
excludeNamespaces:
includeVolumeTypes: persistentVolumeClaim
excludeVolumeTypes:
excludeJobs: *
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excludeJobs: *

@@ -15,3 +15,4 @@ data:
includeVolumeTypes: persistentVolumeClaim
excludeVolumeTypes:
excludeJobs: *
excludeStorageClasses:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excludeStorageClasses: azurefiles,azurefiles-premium

pvcName := volume.PersistentVolumeClaim.ClaimName
pvc, err := c.pvcLister.PersistentVolumeClaims(namespace).Get(pvcName)
if err != nil {
klog.Warningf("Did not find the pvc with name %v: %v, cannot proceed", pvcName, err)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

klog.Warningf("ingore volume %s since failed to get its relevant pvc %s, error: %s", volume.Name, pvcName.Name, err.Error())

@@ -364,6 +368,21 @@ func (c *Controller) checkVolumeTypeRequirements(volumeType string) bool {
return false
}
}
} else if c.cfg.ExcludeStorageClasses != "" {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use if instead of else if

@Gui13
Copy link
Author

Gui13 commented Aug 20, 2020

Mine wasn't working at first because I had the logic wrong. I have iterated on it yesterday but I couldn't test it, so @duyanghao If you want to proceed with the MR from @benosman feel free, I think he got a bit further than I.

@duyanghao
Copy link
Owner

Close this since @benosman has opened another PR for this feature. And welcome any constructive comments about this PR @Gui13 .

@duyanghao duyanghao closed this Aug 21, 2020
@Gui13 Gui13 deleted the filterStorageClass branch September 5, 2020 07:56
@Gui13 Gui13 restored the filterStorageClass branch September 5, 2020 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore PVC according to their storage class
2 participants