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

detach the volume when pod is terminated #45286

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions cmd/kube-controller-manager/app/controllermanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,7 @@ func StartControllers(controllers map[string]InitFunc, s *options.CMServer, root
cloud,
ProbeAttachableVolumePlugins(s.VolumeConfiguration),
s.DisableAttachDetachReconcilerSync,
s.ReconcilerSyncLoopPeriod.Duration,
)
s.ReconcilerSyncLoopPeriod.Duration)
if attachDetachControllerErr != nil {
return fmt.Errorf("failed to start attach/detach controller: %v", attachDetachControllerErr)
}
Expand Down
3 changes: 2 additions & 1 deletion hack/local-up-cluster.sh
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ CPU_CFS_QUOTA=${CPU_CFS_QUOTA:-true}
ENABLE_HOSTPATH_PROVISIONER=${ENABLE_HOSTPATH_PROVISIONER:-"false"}
CLAIM_BINDER_SYNC_PERIOD=${CLAIM_BINDER_SYNC_PERIOD:-"15s"} # current k8s default
ENABLE_CONTROLLER_ATTACH_DETACH=${ENABLE_CONTROLLER_ATTACH_DETACH:-"true"} # current default
KEEP_TERMINATED_POD_VOLUMES=${KEEP_TERMINATED_POD_VOLUMES:-"true"}
Copy link
Member

Choose a reason for hiding this comment

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

why is this defaulting to true? i don't think you should do this change.

Copy link
Member

Choose a reason for hiding this comment

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

actually, i see below it was defaulting to true previously... i think that was a mistake... can we make that false?

Copy link
Contributor

Choose a reason for hiding this comment

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

@derekwaynecarr I think that change should be a part of #45615

# This is the default dir and filename where the apiserver will generate a self-signed cert
# which should be able to be used as the CA to verify itself
CERT_DIR=${CERT_DIR:-"/var/run/kubernetes"}
Expand Down Expand Up @@ -638,7 +639,7 @@ function start_kubelet {
--enable-controller-attach-detach="${ENABLE_CONTROLLER_ATTACH_DETACH}" \
--cgroups-per-qos=${CGROUPS_PER_QOS} \
--cgroup-driver=${CGROUP_DRIVER} \
--keep-terminated-pod-volumes=true \
--keep-terminated-pod-volumes=${KEEP_TERMINATED_POD_VOLUMES} \
--eviction-hard=${EVICTION_HARD} \
--eviction-soft=${EVICTION_SOFT} \
--eviction-pressure-transition-period=${EVICTION_PRESSURE_TRANSITION_PERIOD} \
Expand Down
51 changes: 38 additions & 13 deletions pkg/controller/volume/attachdetach/attach_detach_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,7 @@ func (adc *attachDetachController) populateActualStateOfWorld() error {
continue
}
adc.processVolumesInUse(nodeName, node.Status.VolumesInUse, true /* forceUnmount */)
if _, exists := node.Annotations[volumehelper.ControllerManagedAttachAnnotation]; exists {
// Node specifies annotation indicating it should be managed by
// attach detach controller. Add it to desired state of world.
adc.desiredStateOfWorld.AddNode(types.NodeName(node.Name)) // Needed for DesiredStateOfWorld population
}
adc.addNodeToDswp(node, types.NodeName(node.Name))
}
}
return nil
Expand Down Expand Up @@ -385,7 +381,12 @@ func (adc *attachDetachController) podAdd(obj interface{}) {
return
}

util.ProcessPodVolumes(pod, true, /* addVolumes */
volumeActionFlag := util.DetermineVolumeAction(
pod,
adc.desiredStateOfWorld,
true /* default volume action */)

util.ProcessPodVolumes(pod, volumeActionFlag, /* addVolumes */
adc.desiredStateOfWorld, &adc.volumePluginMgr, adc.pvcLister, adc.pvLister)
}

Expand All @@ -395,8 +396,22 @@ func (adc *attachDetachController) GetDesiredStateOfWorld() cache.DesiredStateOf
}

func (adc *attachDetachController) podUpdate(oldObj, newObj interface{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

podUpdate still only happens once every 12 hours though right?

I think we have to do it in one of the reconciler loops

Copy link
Member Author

Choose a reason for hiding this comment

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

I would assume that, podUpdate should get called when pod's status for example, changes from Running to Complete - not every 12 hours.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, regardless of this event though - I have also updated DSWP to remove the pod from DSW if pod is terminated.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah...brain fart my b 🥇

// The flow for update is the same as add.
adc.podAdd(newObj)
pod, ok := newObj.(*v1.Pod)
if pod == nil || !ok {
return
}
if pod.Spec.NodeName == "" {
// Ignore pods without NodeName, indicating they are not scheduled.
return
}

volumeActionFlag := util.DetermineVolumeAction(
pod,
adc.desiredStateOfWorld,
true /* default volume action */)

util.ProcessPodVolumes(pod, volumeActionFlag, /* addVolumes */
adc.desiredStateOfWorld, &adc.volumePluginMgr, adc.pvcLister, adc.pvLister)
}

func (adc *attachDetachController) podDelete(obj interface{}) {
Expand Down Expand Up @@ -433,11 +448,7 @@ func (adc *attachDetachController) nodeUpdate(oldObj, newObj interface{}) {
}

nodeName := types.NodeName(node.Name)
if _, exists := node.Annotations[volumehelper.ControllerManagedAttachAnnotation]; exists {
// Node specifies annotation indicating it should be managed by attach
// detach controller. Add it to desired state of world.
adc.desiredStateOfWorld.AddNode(nodeName)
}
adc.addNodeToDswp(node, nodeName)
adc.processVolumesInUse(nodeName, node.Status.VolumesInUse, false /* forceUnmount */)
}

Expand Down Expand Up @@ -539,3 +550,17 @@ func (adc *attachDetachController) GetSecretFunc() func(namespace, name string)
return nil, fmt.Errorf("GetSecret unsupported in attachDetachController")
}
}

func (adc *attachDetachController) addNodeToDswp(node *v1.Node, nodeName types.NodeName) {
if _, exists := node.Annotations[volumehelper.ControllerManagedAttachAnnotation]; exists {
keepTerminatedPodVolumes := false

if t, ok := node.Annotations[volumehelper.KeepTerminatedPodVolumesAnnotation]; ok {
keepTerminatedPodVolumes = (t == "true")
}

// Node specifies annotation indicating it should be managed by attach
// detach controller. Add it to desired state of world.
adc.desiredStateOfWorld.AddNode(nodeName, keepTerminatedPodVolumes)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ func attachDetachRecoveryTestCase(t *testing.T, extraPods1 []*v1.Pod, extraPods2
plugins,
false,
time.Second*1)

if err != nil {
t.Fatalf("Run failed with error. Expected: <no error> Actual: <%v>", err)
}
Expand Down
34 changes: 30 additions & 4 deletions pkg/controller/volume/attachdetach/cache/desired_state_of_world.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ type DesiredStateOfWorld interface {
// AddNode adds the given node to the list of nodes managed by the attach/
// detach controller.
// If the node already exists this is a no-op.
AddNode(nodeName k8stypes.NodeName)
// keepTerminatedPodVolumes is a property of the node that determines
// if for terminated pods volumes should be mounted and attached.
AddNode(nodeName k8stypes.NodeName, keepTerminatedPodVolumes bool)

// AddPod adds the given pod to the list of pods that reference the
// specified volume and is scheduled to the specified node.
Expand Down Expand Up @@ -95,6 +97,10 @@ type DesiredStateOfWorld interface {
// GetPodToAdd generates and returns a map of pods based on the current desired
// state of world
GetPodToAdd() map[types.UniquePodName]PodToAdd

// GetKeepTerminatedPodVolumesForNode determines if node wants volumes to be
// mounted and attached for terminated pods
GetKeepTerminatedPodVolumesForNode(k8stypes.NodeName) bool
}

// VolumeToAttach represents a volume that should be attached to a node.
Expand Down Expand Up @@ -144,6 +150,10 @@ type nodeManaged struct {
// attached to this node. The key in the map is the name of the volume and
// the value is a pod object containing more information about the volume.
volumesToAttach map[v1.UniqueVolumeName]volumeToAttach

// keepTerminatedPodVolumes determines if for terminated pods(on this node) - volumes
// should be kept mounted and attached.
keepTerminatedPodVolumes bool
}

// The volume object represents a volume that should be attached to a node.
Expand Down Expand Up @@ -173,14 +183,15 @@ type pod struct {
podObj *v1.Pod
}

func (dsw *desiredStateOfWorld) AddNode(nodeName k8stypes.NodeName) {
func (dsw *desiredStateOfWorld) AddNode(nodeName k8stypes.NodeName, keepTerminatedPodVolumes bool) {
dsw.Lock()
defer dsw.Unlock()

if _, nodeExists := dsw.nodesManaged[nodeName]; !nodeExists {
dsw.nodesManaged[nodeName] = nodeManaged{
nodeName: nodeName,
volumesToAttach: make(map[v1.UniqueVolumeName]volumeToAttach),
nodeName: nodeName,
volumesToAttach: make(map[v1.UniqueVolumeName]volumeToAttach),
keepTerminatedPodVolumes: keepTerminatedPodVolumes,
}
}
}
Expand Down Expand Up @@ -313,6 +324,21 @@ func (dsw *desiredStateOfWorld) VolumeExists(
return false
}

// GetKeepTerminatedPodVolumesForNode determines if node wants volumes to be
// mounted and attached for terminated pods
func (dsw *desiredStateOfWorld) GetKeepTerminatedPodVolumesForNode(nodeName k8stypes.NodeName) bool {
dsw.RLock()
defer dsw.RUnlock()

if nodeName == "" {
return false
}
if node, ok := dsw.nodesManaged[nodeName]; ok {
return node.keepTerminatedPodVolumes
}
return false
}

func (dsw *desiredStateOfWorld) GetVolumesToAttach() []VolumeToAttach {
dsw.RLock()
defer dsw.RUnlock()
Expand Down