From 6cdb97f2a033f8ddbff76e27e3bdacb18641f64f Mon Sep 17 00:00:00 2001 From: Marc Villacorta Date: Mon, 1 Jul 2019 17:22:45 +0200 Subject: [PATCH] Created PVs are owned by their respective Nodes --- cmd/local-volume-provisioner/main.go | 3 +- helm/README.md | 3 +- helm/provisioner/templates/provisioner.yaml | 3 ++ helm/provisioner/values.yaml | 9 +++- pkg/common/common.go | 16 +++++- pkg/deleter/deleter_test.go | 14 +++++- pkg/discovery/discovery.go | 35 +++++++++++-- pkg/discovery/discovery_test.go | 54 +++++++++++++++------ 8 files changed, 113 insertions(+), 24 deletions(-) diff --git a/cmd/local-volume-provisioner/main.go b/cmd/local-volume-provisioner/main.go index d69556adb..7e7276e54 100644 --- a/cmd/local-volume-provisioner/main.go +++ b/cmd/local-volume-provisioner/main.go @@ -33,7 +33,7 @@ import ( "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/metrics" "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/metrics/collectors" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" ) @@ -95,6 +95,7 @@ func main() { Namespace: namespace, JobContainerImage: jobImage, LabelsForPV: provisionerConfig.LabelsForPV, + SetPVOwnerRef: provisionerConfig.SetPVOwnerRef, }) klog.Infof("Starting metrics server at %s\n", optListenAddress) diff --git a/helm/README.md b/helm/README.md index eba39f671..21713256a 100644 --- a/helm/README.md +++ b/helm/README.md @@ -35,7 +35,7 @@ And there are [a lot of examples](TODO) to help you get started quickly. Helm templating is used to generate the provisioner's DaemonSet, ConfigMap and other necessary objects' specs. The generated specs can be further customized -as needed (usually not necessary), and then deployed using kubectl. +as needed (usually not necessary), and then deployed using kubectl. **helm template** uses 3 sources of information: @@ -100,6 +100,7 @@ provisioner chart and their default values. | common.minResyncPeriod | Resync period in reflectors will be random between `minResyncPeriod` and `2*minResyncPeriod`. | str | `5m0s` | | common.configMapName | Provisioner ConfigMap name. | str | `local-provisioner-config` | | common.podSecurityPolicy | Whether to create pod security policy or not. | bool | `false` | +| common.setPVOwnerRef | If set to true, PVs are set to be dependents of the owner Node. | bool | `false` | | classes.[n].name | StorageClass name. | str | `-` | | classes.[n].hostDir | Path on the host where local volumes of this storage class are mounted under. | str | `-` | | classes.[n].mountDir | Optionally specify mount path of local volumes. By default, we use same path as hostDir in container. | str | `-` | diff --git a/helm/provisioner/templates/provisioner.yaml b/helm/provisioner/templates/provisioner.yaml index e00c8fd7f..b098b0d6c 100644 --- a/helm/provisioner/templates/provisioner.yaml +++ b/helm/provisioner/templates/provisioner.yaml @@ -17,6 +17,9 @@ data: {{- if .Values.common.useAlphaAPI }} useAlphaAPI: "true" {{- end }} +{{- if .Values.common.setPVOwnerRef }} + setPVOwnerRef: "true" +{{- end }} {{- if .Values.common.useJobForCleaning }} useJobForCleaning: "yes" {{- end}} diff --git a/helm/provisioner/values.yaml b/helm/provisioner/values.yaml index f2c3ed5be..35a4732a2 100644 --- a/helm/provisioner/values.yaml +++ b/helm/provisioner/values.yaml @@ -12,13 +12,18 @@ common: namespace: default # # Defines whether to create provisioner namespace - # + # createNamespace: false + # # Beta PV.NodeAffinity field is used by default. If running against pre-1.10 # k8s version, the `useAlphaAPI` flag must be enabled in the configMap. # useAlphaAPI: false # + # Indicates if PVs should be dependents of the owner Node. + # + setPVOwnerRef: false + # # Provisioner clean volumes in process by default. If set to true, provisioner # will use Jobs to clean. # @@ -78,7 +83,7 @@ classes: # reclaimPolicy: Delete # Available reclaim policies: Delete/Retain, defaults: Delete. # # Configure DaemonSet for provisioner. -# +# daemonset: # # Defines the name of a Provisioner diff --git a/pkg/common/common.go b/pkg/common/common.go index 7a8be841c..9b5b113d1 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -31,7 +31,7 @@ import ( "hash/fnv" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/informers" @@ -107,6 +107,8 @@ type UserConfig struct { UseNodeNameOnly bool // LabelsForPV stores additional labels added to provisioned PVs LabelsForPV map[string]string + // SetPVOwnerRef indicates if PVs should be dependents of the owner Node + SetPVOwnerRef bool } // MountConfig stores a configuration for discoverying a specific storageclass @@ -165,6 +167,8 @@ type LocalPVConfig struct { MountOptions []string FsType *string Labels map[string]string + SetPVOwnerRef bool + OwnerReference *metav1.OwnerReference } // BuildConfigFromFlags being defined to enable mocking during unit testing @@ -199,6 +203,8 @@ type ProvisionerConfiguration struct { // LabelsForPV could be used to specify additional labels that will be // added to PVs created by static provisioner. LabelsForPV map[string]string `json:"labelsForPV" yaml:"labelsForPV"` + // SetPVOwnerRef indicates if PVs should be dependents of the owner Node, default to false + SetPVOwnerRef bool `json:"setPVOwnerRef" yaml:"setPVOwnerRef"` } // CreateLocalPVSpec returns a PV spec that can be used for PV creation @@ -230,11 +236,19 @@ func CreateLocalPVSpec(config *LocalPVConfig) *v1.PersistentVolume { MountOptions: config.MountOptions, }, } + if config.UseAlphaAPI { pv.ObjectMeta.Annotations[AlphaStorageNodeAffinityAnnotation] = config.AffinityAnn } else { pv.Spec.NodeAffinity = config.NodeAffinity } + + if config.SetPVOwnerRef { + pv.ObjectMeta.OwnerReferences = []metav1.OwnerReference{ + *config.OwnerReference, + } + } + return pv } diff --git a/pkg/deleter/deleter_test.go b/pkg/deleter/deleter_test.go index 8654bf4e2..283da5cb2 100644 --- a/pkg/deleter/deleter_test.go +++ b/pkg/deleter/deleter_test.go @@ -29,7 +29,7 @@ import ( "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/util" batch_v1 "k8s.io/api/batch/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,6 +42,8 @@ import ( const ( testHostDir = "/mnt/disks" testMountDir = "/discoveryPath" + testNodeName = "somehost.acme.com" + testNodeUID = "d9607e19-f88f-11e6-a518-42010a800195" testStorageClass = "sc1" ) @@ -558,6 +560,11 @@ func testSetup(t *testing.T, config *testConfig, cleanupCmd []string, useJobForC Name: pvName, HostPath: fakePath, StorageClass: testStorageClass, + OwnerReference: &meta_v1.OwnerReference{ + Kind: "Node", + Name: testNodeName, + UID: testNodeUID, + }, } // If volume mode has been explicitly specified in the volume config, then explicitly set it in the PV. switch vol.VolumeMode { @@ -598,7 +605,10 @@ func testSetup(t *testing.T, config *testConfig, cleanupCmd []string, useJobForC BlockCleanerCommand: cleanupCmd, }, }, - Node: &v1.Node{ObjectMeta: meta_v1.ObjectMeta{Name: "somehost.acme.com"}}, + Node: &v1.Node{ObjectMeta: meta_v1.ObjectMeta{ + Name: testNodeName, + UID: testNodeUID, + }}, UseJobForCleaning: useJobForCleaning, JobContainerImage: containerImage, Namespace: ns, diff --git a/pkg/discovery/discovery.go b/pkg/discovery/discovery.go index 9c864a163..258450138 100644 --- a/pkg/discovery/discovery.go +++ b/pkg/discovery/discovery.go @@ -27,7 +27,8 @@ import ( "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/common" "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/metrics" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" storagev1listers "k8s.io/client-go/listers/storage/v1" "k8s.io/client-go/tools/cache" esUtil "sigs.k8s.io/sig-storage-lib-external-provisioner/util" @@ -45,6 +46,7 @@ type Discoverer struct { nodeAffinityAnn string nodeAffinity *v1.VolumeNodeAffinity classLister storagev1listers.StorageClassLister + ownerReference *metav1.OwnerReference } // NewDiscoverer creates a Discoverer object that will scan through @@ -72,6 +74,12 @@ func NewDiscoverer(config *common.RuntimeConfig, cleanupTracker *deleter.Cleanup labelMap[labelName] = labelValue } + // Generate owner reference + ownerRef, err := generateOwnerReference(config.Node) + if err != nil { + return nil, fmt.Errorf("Failed to generate owner reference: %v", err) + } + if config.UseAlphaAPI { nodeAffinity, err := generateNodeAffinity(config.Node) if err != nil { @@ -87,7 +95,8 @@ func NewDiscoverer(config *common.RuntimeConfig, cleanupTracker *deleter.Cleanup Labels: labelMap, CleanupTracker: cleanupTracker, classLister: sharedInformer.Lister(), - nodeAffinityAnn: tmpAnnotations[common.AlphaStorageNodeAffinityAnnotation]}, nil + nodeAffinityAnn: tmpAnnotations[common.AlphaStorageNodeAffinityAnnotation], + ownerReference: ownerRef}, nil } volumeNodeAffinity, err := generateVolumeNodeAffinity(config.Node) @@ -100,7 +109,25 @@ func NewDiscoverer(config *common.RuntimeConfig, cleanupTracker *deleter.Cleanup Labels: labelMap, CleanupTracker: cleanupTracker, classLister: sharedInformer.Lister(), - nodeAffinity: volumeNodeAffinity}, nil + nodeAffinity: volumeNodeAffinity, + ownerReference: ownerRef}, nil +} + +func generateOwnerReference(node *v1.Node) (*metav1.OwnerReference, error) { + if node.GetName() == "" { + return nil, fmt.Errorf("Node does not have name") + } + + if node.GetUID() == "" { + return nil, fmt.Errorf("Node does not have UID") + } + + return &metav1.OwnerReference{ + Kind: "Node", + APIVersion: "v1", + Name: node.GetName(), + UID: node.UID, + }, nil } func generateNodeAffinity(node *v1.Node) (*v1.NodeAffinity, error) { @@ -314,6 +341,8 @@ func (d *Discoverer) createPV(file, class string, reclaimPolicy v1.PersistentVol VolumeMode: volMode, Labels: d.Labels, MountOptions: mountOptions, + SetPVOwnerRef: d.SetPVOwnerRef, + OwnerReference: d.ownerReference, } if d.UseAlphaAPI { diff --git a/pkg/discovery/discovery_test.go b/pkg/discovery/discovery_test.go index a3fb5882e..28c7d6c38 100644 --- a/pkg/discovery/discovery_test.go +++ b/pkg/discovery/discovery_test.go @@ -29,7 +29,7 @@ import ( "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/deleter" "sigs.k8s.io/sig-storage-local-static-provisioner/pkg/util" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -45,6 +45,7 @@ const ( testHostDir = "/mnt/disks" testMountDir = "/discoveryPath" testNodeName = "test-node" + testNodeUID = "d9607e19-f88f-11e6-a518-42010a800195" testProvisionerName = "test-provisioner" ) @@ -74,6 +75,7 @@ var testNode = &v1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: testNodeName, Labels: nodeLabels, + UID: testNodeUID, }, } @@ -117,6 +119,8 @@ type testConfig struct { expectedVolumes map[string][]*util.FakeDirEntry // True if testing api failure apiShouldFail bool + // True if PVs should be dependents of the owner Node + testPVOwnerRef bool // The rest are set during setup volUtil *util.FakeVolumeUtil client *fake.Clientset @@ -139,8 +143,9 @@ func TestDiscoverVolumes_Basic(t *testing.T) { test := &testConfig{ dirLayout: vols, expectedVolumes: vols, + testPVOwnerRef: true, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, true) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) @@ -161,7 +166,7 @@ func TestDiscoverVolumes_BasicTwice(t *testing.T) { dirLayout: vols, expectedVolumes: vols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) @@ -178,7 +183,7 @@ func TestDiscoverVolumes_NoDir(t *testing.T) { dirLayout: vols, expectedVolumes: vols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) @@ -192,7 +197,7 @@ func TestDiscoverVolumes_EmptyDir(t *testing.T) { dirLayout: vols, expectedVolumes: vols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) @@ -213,7 +218,7 @@ func TestDiscoverVolumes_NewVolumesLater(t *testing.T) { dirLayout: vols, expectedVolumes: vols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() @@ -250,7 +255,7 @@ func TestDiscoverVolumes_CreatePVFails(t *testing.T) { dirLayout: vols, expectedVolumes: map[string][]*util.FakeDirEntry{}, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() @@ -268,7 +273,7 @@ func TestDiscoverVolumes_BadVolume(t *testing.T) { dirLayout: vols, expectedVolumes: map[string][]*util.FakeDirEntry{}, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() @@ -302,7 +307,7 @@ func TestDiscoverVolumes_CleaningInProgress(t *testing.T) { dirLayout: vols, expectedVolumes: expectedVols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) // Mark dir1/mount2 PV as being cleaned. This one should not get created pvName := getPVName(vols["dir1"][1]) @@ -338,13 +343,13 @@ func TestDiscoverVolumes_InvalidMode(t *testing.T) { dirLayout: vols, expectedVolumes: expectedVols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) } -func testSetup(t *testing.T, test *testConfig, useAlphaAPI bool) *Discoverer { +func testSetup(t *testing.T, test *testConfig, useAlphaAPI, setPVOwnerRef bool) *Discoverer { test.cache = cache.NewVolumeCache() test.volUtil = util.NewFakeVolumeUtil(false /*deleteShouldFail*/, map[string][]*util.FakeDirEntry{}) test.volUtil.AddNewDirEntries(testMountDir, test.dirLayout) @@ -370,6 +375,7 @@ func testSetup(t *testing.T, test *testConfig, useAlphaAPI bool) *Discoverer { NodeLabelsForPV: nodeLabelsForPV, UseAlphaAPI: useAlphaAPI, LabelsForPV: labelsForPV, + SetPVOwnerRef: setPVOwnerRef, } objects := make([]runtime.Object, 0) for _, o := range testStorageClasses { @@ -555,6 +561,23 @@ func verifyMountOptions(t *testing.T, createdPV *v1.PersistentVolume) { } } +func verifyOwnerReference(t *testing.T, pv *v1.PersistentVolume) { + ownerReference := &pv.ObjectMeta.OwnerReferences[0] + if ownerReference == nil { + t.Errorf("No owner reference found") + } + + if ownerReference.Name != testNodeName { + t.Errorf("Owner reference name is %s, expected %s", ownerReference.Name, testNodeName) + return + } + + if ownerReference.UID != testNodeUID { + t.Errorf("Owner reference UID is %s, expected %s", ownerReference.UID, testNodeUID) + return + } +} + // testPVInfo contains all the fields we are intested in validating. type testPVInfo struct { pvName string @@ -615,6 +638,9 @@ func verifyCreatedPVs(t *testing.T, test *testConfig) { verifyCapacity(t, createdPV, expectedPV) verifyVolumeMode(t, createdPV, expectedPV) verifyMountOptions(t, createdPV) + if test.testPVOwnerRef { + verifyOwnerReference(t, createdPV) + } // TODO: Verify volume type once that is supported in the API. } } @@ -676,7 +702,7 @@ func TestDiscoverVolumes_NotMountPoint(t *testing.T) { dirLayout: vols, expectedVolumes: expectedVols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) d.DiscoverLocalVolumes() verifyCreatedPVs(t, test) @@ -688,7 +714,7 @@ func TestUseAlphaAPI(t *testing.T) { dirLayout: vols, expectedVolumes: vols, } - d := testSetup(t, test, false) + d := testSetup(t, test, false, false) if d.UseAlphaAPI { t.Fatal("UseAlphaAPI should be false") } @@ -696,7 +722,7 @@ func TestUseAlphaAPI(t *testing.T) { t.Fatal("the value nodeAffinityAnn shouldn't be set while nodeAffinity should") } - d = testSetup(t, test, true) + d = testSetup(t, test, true, false) if !d.UseAlphaAPI { t.Fatal("UseAlphaAPI should be true") }