Skip to content

Commit

Permalink
Always Start pvc-protection-controller and pv-protection-controller
Browse files Browse the repository at this point in the history 
StorageObjectInUseProtection feature is enabled by default in K8s 1.10+. Assume K8s cluster is used with this feature enabled, i.e. finalizers are added to all PVs and PVCs. In case the K8s cluster admin disables the StorageObjectInUseProtection feature and a user deletes a PVC that is not in active use by a pod then the PVC is not removed from the system because of the finalizer. Therefore, the user will have to remove the finalizer manually in order to have the PVC removed from the system. Note: deleted PVs won't be removed from the system also because of finalizers.

That's why pvc-protection-controller is always started because the pvc-protection-controller removes finalizers from PVCs automatically when a PVC is not in active use by a pod. Also the pv-protection-controller is always started to remove finalizers from PVs automatically when a PV is not Bound to a PVC.

Related issue:
kubernetes#60764

Related PRs:
kubernetes#61370
kubernetes#61324
  • Loading branch information
@pospispa
pospispa committed Apr 21, 2018
1 parent 7195f79 commit 9e8f00a
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 58 deletions.
30 changes: 13 additions & 17 deletions cmd/kube-controller-manager/app/core.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,24 +395,20 @@ func startGarbageCollectorController(ctx ControllerContext) (bool, error) {
}

func startPVCProtectionController(ctx ControllerContext) (bool, error) {
if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
go pvcprotection.NewPVCProtectionController(
ctx.InformerFactory.Core().V1().PersistentVolumeClaims(),
ctx.InformerFactory.Core().V1().Pods(),
ctx.ClientBuilder.ClientOrDie("pvc-protection-controller"),
).Run(1, ctx.Stop)
return true, nil
}
return false, nil
go pvcprotection.NewPVCProtectionController(
ctx.InformerFactory.Core().V1().PersistentVolumeClaims(),
ctx.InformerFactory.Core().V1().Pods(),
ctx.ClientBuilder.ClientOrDie("pvc-protection-controller"),
utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection),
).Run(1, ctx.Stop)
return true, nil
}

func startPVProtectionController(ctx ControllerContext) (bool, error) {
if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
go pvprotection.NewPVProtectionController(
ctx.InformerFactory.Core().V1().PersistentVolumes(),
ctx.ClientBuilder.ClientOrDie("pv-protection-controller"),
).Run(1, ctx.Stop)
return true, nil
}
return false, nil
go pvprotection.NewPVProtectionController(
ctx.InformerFactory.Core().V1().PersistentVolumes(),
ctx.ClientBuilder.ClientOrDie("pv-protection-controller"),
utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection),
).Run(1, ctx.Stop)
return true, nil
}
10 changes: 9 additions & 1 deletion pkg/controller/volume/pvcprotection/pvc_protection_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,17 @@ type Controller struct {
podListerSynced cache.InformerSynced

queue workqueue.RateLimitingInterface

// allows overriding of StorageObjectInUseProtection feature Enabled/Disabled for testing
storageObjectInUseProtectionEnabled bool
}

// NewPVCProtectionController returns a new *{VCProtectionController.
func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimInformer, podInformer coreinformers.PodInformer, cl clientset.Interface) *Controller {
func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimInformer, podInformer coreinformers.PodInformer, cl clientset.Interface, storageObjectInUseProtectionFeatureEnabled bool) *Controller {
e := &Controller{
client: cl,
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvcprotection"),
storageObjectInUseProtectionEnabled: storageObjectInUseProtectionFeatureEnabled,
}
if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
Expand Down Expand Up @@ -176,6 +180,10 @@ func (c *Controller) processPVC(pvcNamespace, pvcName string) error {
}

func (c *Controller) addFinalizer(pvc *v1.PersistentVolumeClaim) error {
// Skip adding Finalizer in case the StorageObjectInUseProtection feature is not enabled
if !c.storageObjectInUseProtectionEnabled {
return nil
}
claimClone := pvc.DeepCopy()
claimClone.ObjectMeta.Finalizers = append(claimClone.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer)
_, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
Expand Down
46 changes: 36 additions & 10 deletions pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,22 +162,31 @@ func TestPVCProtectionController(t *testing.T) {
deletedPod *v1.Pod
// List of expected kubeclient actions that should happen during the
// test.
expectedActions []clienttesting.Action
expectedActions []clienttesting.Action
storageObjectInUseProtectionEnabled bool
}{
//
// PVC events
//
{
name: "PVC without finalizer -> finalizer is added",
name: "StorageObjectInUseProtection Enabled, PVC without finalizer -> finalizer is added",
updatedPVC: pvc(),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, withProtectionFinalizer(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "PVC with finalizer -> no action",
updatedPVC: withProtectionFinalizer(pvc()),
expectedActions: []clienttesting.Action{},
name: "StorageObjectInUseProtection Disabled, PVC without finalizer -> finalizer is added",
updatedPVC: pvc(),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: false,
},
{
name: "PVC with finalizer -> no action",
updatedPVC: withProtectionFinalizer(pvc()),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: true,
},
{
name: "saving PVC finalizer fails -> controller retries",
Expand All @@ -197,13 +206,23 @@ func TestPVCProtectionController(t *testing.T) {
// This succeeds
clienttesting.NewUpdateAction(pvcVer, defaultNS, withProtectionFinalizer(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "StorageObjectInUseProtection Enabled, deleted PVC with finalizer -> finalizer is removed",
updatedPVC: deleted(withProtectionFinalizer(pvc())),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted PVC with finalizer -> finalizer is removed",
name: "StorageObjectInUseProtection Disabled, deleted PVC with finalizer -> finalizer is removed",
updatedPVC: deleted(withProtectionFinalizer(pvc())),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: false,
},
{
name: "finalizer removal fails -> controller retries",
Expand All @@ -223,6 +242,7 @@ func TestPVCProtectionController(t *testing.T) {
// Succeeds
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted PVC with finalizer + pods with the PVC exists -> finalizer is not removed",
Expand All @@ -241,16 +261,18 @@ func TestPVCProtectionController(t *testing.T) {
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted PVC with finalizer + pods with the PVC andis finished -> finalizer is removed",
name: "deleted PVC with finalizer + pods with the PVC and is finished -> finalizer is removed",
initialObjects: []runtime.Object{
withStatus(v1.PodFailed, withPVC(defaultPVCName, pod())),
},
updatedPVC: deleted(withProtectionFinalizer(pvc())),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
//
// Pod events
Expand All @@ -260,8 +282,9 @@ func TestPVCProtectionController(t *testing.T) {
initialObjects: []runtime.Object{
deleted(withProtectionFinalizer(pvc())),
},
updatedPod: withStatus(v1.PodRunning, withPVC(defaultPVCName, pod())),
expectedActions: []clienttesting.Action{},
updatedPod: withStatus(v1.PodRunning, withPVC(defaultPVCName, pod())),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: true,
},
{
name: "updated finished Pod -> finalizer is removed",
Expand All @@ -272,6 +295,7 @@ func TestPVCProtectionController(t *testing.T) {
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "updated unscheduled Pod -> finalizer is removed",
Expand All @@ -282,6 +306,7 @@ func TestPVCProtectionController(t *testing.T) {
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted running Pod -> finalizer is removed",
Expand All @@ -292,6 +317,7 @@ func TestPVCProtectionController(t *testing.T) {
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
},
storageObjectInUseProtectionEnabled: true,
},
}

Expand Down Expand Up @@ -330,7 +356,7 @@ func TestPVCProtectionController(t *testing.T) {
}

// Create the controller
ctrl := NewPVCProtectionController(pvcInformer, podInformer, client)
ctrl := NewPVCProtectionController(pvcInformer, podInformer, client, test.storageObjectInUseProtectionEnabled)

// Start the test by simulating an event
if test.updatedPVC != nil {
Expand Down
10 changes: 9 additions & 1 deletion pkg/controller/volume/pvprotection/pv_protection_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,17 @@ type Controller struct {
pvListerSynced cache.InformerSynced

queue workqueue.RateLimitingInterface

// allows overriding of StorageObjectInUseProtection feature Enabled/Disabled for testing
storageObjectInUseProtectionEnabled bool
}

// NewPVProtectionController returns a new *Controller.
func NewPVProtectionController(pvInformer coreinformers.PersistentVolumeInformer, cl clientset.Interface) *Controller {
func NewPVProtectionController(pvInformer coreinformers.PersistentVolumeInformer, cl clientset.Interface, storageObjectInUseProtectionFeatureEnabled bool) *Controller {
e := &Controller{
client: cl,
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvprotection"),
storageObjectInUseProtectionEnabled: storageObjectInUseProtectionFeatureEnabled,
}
if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolume_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
Expand Down Expand Up @@ -151,6 +155,10 @@ func (c *Controller) processPV(pvName string) error {
}

func (c *Controller) addFinalizer(pv *v1.PersistentVolume) error {
// Skip adding Finalizer in case the StorageObjectInUseProtection feature is not enabled
if !c.storageObjectInUseProtectionEnabled {
return nil
}
pvClone := pv.DeepCopy()
pvClone.ObjectMeta.Finalizers = append(pvClone.ObjectMeta.Finalizers, volumeutil.PVProtectionFinalizer)
_, err := c.client.CoreV1().PersistentVolumes().Update(pvClone)
Expand Down
41 changes: 31 additions & 10 deletions pkg/controller/volume/pvprotection/pv_protection_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,21 +111,30 @@ func TestPVProtectionController(t *testing.T) {
updatedPV *v1.PersistentVolume
// List of expected kubeclient actions that should happen during the
// test.
expectedActions []clienttesting.Action
expectedActions []clienttesting.Action
storageObjectInUseProtectionEnabled bool
}{
// PV events
//
{
name: "PV without finalizer -> finalizer is added",
name: "StorageObjectInUseProtection Enabled, PV without finalizer -> finalizer is added",
updatedPV: pv(),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvVer, "", withProtectionFinalizer(pv())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "PVC with finalizer -> no action",
updatedPV: withProtectionFinalizer(pv()),
expectedActions: []clienttesting.Action{},
name: "StorageObjectInUseProtection Disabled, PV without finalizer -> finalizer is added",
updatedPV: pv(),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: false,
},
{
name: "PVC with finalizer -> no action",
updatedPV: withProtectionFinalizer(pv()),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: true,
},
{
name: "saving PVC finalizer fails -> controller retries",
Expand All @@ -145,13 +154,23 @@ func TestPVProtectionController(t *testing.T) {
// This succeeds
clienttesting.NewUpdateAction(pvVer, "", withProtectionFinalizer(pv())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "StorageObjectInUseProtection Enabled, deleted PV with finalizer -> finalizer is removed",
updatedPV: deleted(withProtectionFinalizer(pv())),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted PV with finalizer -> finalizer is removed",
name: "StorageObjectInUseProtection Disabled, deleted PV with finalizer -> finalizer is removed",
updatedPV: deleted(withProtectionFinalizer(pv())),
expectedActions: []clienttesting.Action{
clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
},
storageObjectInUseProtectionEnabled: false,
},
{
name: "finalizer removal fails -> controller retries",
Expand All @@ -171,11 +190,13 @@ func TestPVProtectionController(t *testing.T) {
// Succeeds
clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
},
storageObjectInUseProtectionEnabled: true,
},
{
name: "deleted PVC with finalizer + PV is bound -> finalizer is not removed",
updatedPV: deleted(withProtectionFinalizer(boundPV())),
expectedActions: []clienttesting.Action{},
name: "deleted PVC with finalizer + PV is bound -> finalizer is not removed",
updatedPV: deleted(withProtectionFinalizer(boundPV())),
expectedActions: []clienttesting.Action{},
storageObjectInUseProtectionEnabled: true,
},
}

Expand Down Expand Up @@ -209,7 +230,7 @@ func TestPVProtectionController(t *testing.T) {
}

// Create the controller
ctrl := NewPVProtectionController(pvInformer, client)
ctrl := NewPVProtectionController(pvInformer, client, test.storageObjectInUseProtectionEnabled)

// Start the test by simulating an event
if test.updatedPV != nil {
Expand Down
34 changes: 15 additions & 19 deletions plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,25 +324,21 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
eventsRule(),
},
})
if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pvc-protection-controller"},
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
rbac.NewRule("list", "watch", "get").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
})
}
if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pv-protection-controller"},
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
eventsRule(),
},
})
}
addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pvc-protection-controller"},
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
rbac.NewRule("list", "watch", "get").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
})
addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pv-protection-controller"},
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
eventsRule(),
},
})

return controllerRoles, controllerRoleBindings
}
Expand Down

0 comments on commit 9e8f00a

Please sign in to comment.