From 4ae0eecb343a13137ed0aabe075f462d66512fed Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 31 Aug 2021 08:39:55 +0200 Subject: [PATCH] controller: use generic ephemeral volume helper functions The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them. There also was a missing owner check in the attach controller. --- .../volume/attachdetach/util/util.go | 27 ++++++++++++------- pkg/controller/volume/ephemeral/controller.go | 24 ++++++++--------- .../pvc_protection_controller.go | 5 ++-- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/pkg/controller/volume/attachdetach/util/util.go b/pkg/controller/volume/attachdetach/util/util.go index 034bce8fc33..c811e94b4cd 100644 --- a/pkg/controller/volume/attachdetach/util/util.go +++ b/pkg/controller/volume/attachdetach/util/util.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" utilfeature "k8s.io/apiserver/pkg/util/feature" corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache" "k8s.io/kubernetes/pkg/features" @@ -46,8 +47,10 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName = pvcSource.ClaimName readOnly = pvcSource.ReadOnly } + isEphemeral := false if ephemeralSource := podVolume.VolumeSource.Ephemeral; ephemeralSource != nil && utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) { - claimName = pod.Name + "-" + podVolume.Name + claimName = ephemeral.VolumeClaimName(pod, &podVolume) + isEphemeral = true } if claimName != "" { klog.V(10).Infof( @@ -56,8 +59,7 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName) // If podVolume is a PVC, fetch the real PV behind the claim - pvName, pvcUID, err := getPVCFromCacheExtractPV( - pod.Namespace, claimName, pvcLister) + pvc, err := getPVCFromCache(pod.Namespace, claimName, pvcLister) if err != nil { return nil, fmt.Errorf( "error processing PVC %q/%q: %v", @@ -65,7 +67,13 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName, err) } + if isEphemeral { + if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil { + return nil, err + } + } + pvName, pvcUID := pvc.Spec.VolumeName, pvc.UID klog.V(10).Infof( "Found bound PV for PVC (ClaimName %q/%q pvcUID %v): pvName=%q", pod.Namespace, @@ -119,20 +127,19 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, return spec, nil } -// getPVCFromCacheExtractPV fetches the PVC object with the given namespace and -// name from the shared internal PVC store extracts the name of the PV it is -// pointing to and returns it. +// getPVCFromCache fetches the PVC object with the given namespace and +// name from the shared internal PVC store. // This method returns an error if a PVC object does not exist in the cache // with the given namespace/name. // This method returns an error if the PVC object's phase is not "Bound". -func getPVCFromCacheExtractPV(namespace string, name string, pvcLister corelisters.PersistentVolumeClaimLister) (string, types.UID, error) { +func getPVCFromCache(namespace string, name string, pvcLister corelisters.PersistentVolumeClaimLister) (*v1.PersistentVolumeClaim, error) { pvc, err := pvcLister.PersistentVolumeClaims(namespace).Get(name) if err != nil { - return "", "", fmt.Errorf("failed to find PVC %s/%s in PVCInformer cache: %v", namespace, name, err) + return nil, fmt.Errorf("failed to find PVC %s/%s in PVCInformer cache: %v", namespace, name, err) } if pvc.Status.Phase != v1.ClaimBound || pvc.Spec.VolumeName == "" { - return "", "", fmt.Errorf( + return nil, fmt.Errorf( "PVC %s/%s has non-bound phase (%q) or empty pvc.Spec.VolumeName (%q)", namespace, name, @@ -140,7 +147,7 @@ func getPVCFromCacheExtractPV(namespace string, name string, pvcLister coreliste pvc.Spec.VolumeName) } - return pvc.Spec.VolumeName, pvc.UID, nil + return pvc, nil } // getPVSpecFromCache fetches the PV object with the given name from the shared diff --git a/pkg/controller/volume/ephemeral/controller.go b/pkg/controller/volume/ephemeral/controller.go index e4fb9749fdf..b5ad0cf685e 100644 --- a/pkg/controller/volume/ephemeral/controller.go +++ b/pkg/controller/volume/ephemeral/controller.go @@ -37,10 +37,10 @@ import ( kcache "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/kubernetes/pkg/controller/volume/common" ephemeralvolumemetrics "k8s.io/kubernetes/pkg/controller/volume/ephemeral/metrics" "k8s.io/kubernetes/pkg/controller/volume/events" - "k8s.io/kubernetes/pkg/volume/util" ) // Controller creates PVCs for ephemeral inline volumes in a pod spec. @@ -241,24 +241,22 @@ func (ec *ephemeralController) syncHandler(key string) error { // handleEphemeralVolume is invoked for each volume of a pod. func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error { klog.V(5).Infof("ephemeral: checking volume %s", vol.Name) - ephemeral := vol.Ephemeral - if ephemeral == nil { + if vol.Ephemeral == nil { return nil } - pvcName := pod.Name + "-" + vol.Name + pvcName := ephemeral.VolumeClaimName(pod, &vol) pvc, err := ec.pvcLister.PersistentVolumeClaims(pod.Namespace).Get(pvcName) if err != nil && !errors.IsNotFound(err) { return err } if pvc != nil { - if metav1.IsControlledBy(pvc, pod) { - // Already created, nothing more to do. - klog.V(5).Infof("ephemeral: volume %s: PVC %s already created", vol.Name, pvcName) - return nil + if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil { + return err } - return fmt.Errorf("PVC %q (uid: %q) was not created for the pod", - util.GetPersistentVolumeClaimQualifiedName(pvc), pvc.UID) + // Already created, nothing more to do. + klog.V(5).Infof("ephemeral: volume %s: PVC %s already created", vol.Name, pvcName) + return nil } // Create the PVC with pod as owner. @@ -276,10 +274,10 @@ func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error { BlockOwnerDeletion: &isTrue, }, }, - Annotations: ephemeral.VolumeClaimTemplate.Annotations, - Labels: ephemeral.VolumeClaimTemplate.Labels, + Annotations: vol.Ephemeral.VolumeClaimTemplate.Annotations, + Labels: vol.Ephemeral.VolumeClaimTemplate.Labels, }, - Spec: ephemeral.VolumeClaimTemplate.Spec, + Spec: vol.Ephemeral.VolumeClaimTemplate.Spec, } ephemeralvolumemetrics.EphemeralVolumeCreateAttempts.Inc() _, err = ec.kubeClient.CoreV1().PersistentVolumeClaims(pod.Namespace).Create(context.TODO(), pvc, metav1.CreateOptions{}) diff --git a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go index cd971c395f9..b85961383fe 100644 --- a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go +++ b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go @@ -32,6 +32,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" "k8s.io/component-base/metrics/prometheus/ratelimiter" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller/volume/common" "k8s.io/kubernetes/pkg/controller/volume/protectionutil" @@ -299,7 +300,7 @@ func (c *Controller) podUsesPVC(pod *v1.Pod, pvc *v1.PersistentVolumeClaim) bool if pod.Spec.NodeName != "" { for _, volume := range pod.Spec.Volumes { if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvc.Name || - c.genericEphemeralVolumeFeatureEnabled && !podIsShutDown(pod) && volume.Ephemeral != nil && pod.Name+"-"+volume.Name == pvc.Name && metav1.IsControlledBy(pvc, pod) { + c.genericEphemeralVolumeFeatureEnabled && !podIsShutDown(pod) && volume.Ephemeral != nil && ephemeral.VolumeClaimName(pod, &volume) == pvc.Name && ephemeral.VolumeIsForPod(pod, pvc) == nil { klog.V(2).InfoS("Pod uses PVC", "pod", klog.KObj(pod), "PVC", klog.KObj(pvc)) return true } @@ -407,7 +408,7 @@ func (c *Controller) enqueuePVCs(pod *v1.Pod, deleted bool) { case volume.PersistentVolumeClaim != nil: c.queue.Add(pod.Namespace + "/" + volume.PersistentVolumeClaim.ClaimName) case c.genericEphemeralVolumeFeatureEnabled && volume.Ephemeral != nil: - c.queue.Add(pod.Namespace + "/" + pod.Name + "-" + volume.Name) + c.queue.Add(pod.Namespace + "/" + ephemeral.VolumeClaimName(pod, &volume)) } } }