Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Commit

Permalink
Merge pull request #521 from wongma7/megafix
Browse files Browse the repository at this point in the history 
Fix provisioning for >=1.5 to recognize annotation as authoritative
  • Loading branch information
@wongma7
wongma7 authored Dec 20, 2017
2 parents e82f2f9 + be9beaa commit c3ee08f
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 63 deletions.
34 changes: 19 additions & 15 deletions lib/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,25 +570,29 @@ func (ctrl *ProvisionController) shouldProvision(claim *v1.PersistentVolumeClaim
}

// Kubernetes 1.5 provisioning with annStorageProvisioner
if provisioner, found := claim.Annotations[annStorageProvisioner]; found {
if provisioner == ctrl.provisionerName {
return true
if ctrl.kubeVersion.AtLeast(utilversion.MustParseSemantic("v1.5.0")) {
if provisioner, found := claim.Annotations[annStorageProvisioner]; found {
if provisioner == ctrl.provisionerName {
return true
}
return false
}
} else {
// Kubernetes 1.4 provisioning, evaluating class.Provisioner
claimClass := helper.GetPersistentVolumeClaimClass(claim)
provisioner, _, err := ctrl.getStorageClassFields(claimClass)
if err != nil {
glog.Errorf("Error getting claim %q's StorageClass's fields: %v", claimToClaimKey(claim), err)
return false
}
if provisioner != ctrl.provisionerName {
return false
}
return false
}

// Kubernetes 1.4 provisioning, evaluating class.Provisioner
claimClass := helper.GetPersistentVolumeClaimClass(claim)
provisioner, _, err := ctrl.getStorageClassFields(claimClass)
if err != nil {
glog.Errorf("Error getting claim %q's StorageClass's fields: %v", claimToClaimKey(claim), err)
return false
}
if provisioner != ctrl.provisionerName {
return false
return true
}

return true
return false
}

func (ctrl *ProvisionController) shouldDelete(volume *v1.PersistentVolume) bool {
Expand Down
119 changes: 71 additions & 48 deletions lib/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ import (
)

const (
resyncPeriod = 100 * time.Millisecond
resyncPeriod = 100 * time.Millisecond
defaultServerVersion = "v1.5.0"
)

// TODO clean this up, e.g. remove redundant params (provisionerName: "foo.bar/baz")
Expand All @@ -65,13 +66,13 @@ func TestController(t *testing.T) {
objs: []runtime.Object{
newStorageClass("class-1", "foo.bar/baz"),
newStorageClass("class-2", "abc.def/ghi"),
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-2", "uid-1-2", "class-2", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
newClaim("claim-2", "uid-1-2", "class-2", "abc.def/ghi", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newTestProvisioner(),
expectedVolumes: []v1.PersistentVolume{
*newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "", nil)),
*newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)),
},
},
{
Expand All @@ -89,7 +90,7 @@ func TestController(t *testing.T) {
{
name: "don't provision for claim-1 because it's already bound",
objs: []runtime.Object{
newClaim("claim-1", "uid-1-1", "class-1", "volume-1", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "volume-1", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newTestProvisioner(),
Expand All @@ -98,7 +99,7 @@ func TestController(t *testing.T) {
{
name: "don't provision for claim-1 because its class doesn't exist",
objs: []runtime.Object{
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newTestProvisioner(),
Expand Down Expand Up @@ -130,7 +131,7 @@ func TestController(t *testing.T) {
name: "provisioner fails to provision for claim-1: no pv is created",
objs: []runtime.Object{
newStorageClass("class-1", "foo.bar/baz"),
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newBadTestProvisioner(),
Expand All @@ -151,7 +152,7 @@ func TestController(t *testing.T) {
name: "try to provision for claim-1 but fail to save the pv object",
objs: []runtime.Object{
newStorageClass("class-1", "foo.bar/baz"),
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newTestProvisioner(),
Expand Down Expand Up @@ -180,26 +181,26 @@ func TestController(t *testing.T) {
name: "provision for claim-1 but not claim-2, because it is ignored",
objs: []runtime.Object{
newStorageClass("class-1", "foo.bar/baz"),
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-2", "uid-1-2", "class-1", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
newClaim("claim-2", "uid-1-2", "class-1", "foo.bar/baz", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newIgnoredProvisioner(),
expectedVolumes: []v1.PersistentVolume{
*newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "", nil)),
*newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)),
},
},
{
name: "provision with Retain reclaim policy",
objs: []runtime.Object{
newStorageClassWithSpecifiedReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimRetain),
newClaim("claim-1", "uid-1-1", "class-1", "", nil),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil),
},
provisionerName: "foo.bar/baz",
provisioner: newTestProvisioner(),
serverVersion: "v1.8.0",
expectedVolumes: []v1.PersistentVolume{
*newProvisionedVolumeWithSpecifiedReclaimPolicy(newStorageClassWithSpecifiedReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimRetain), newClaim("claim-1", "uid-1-1", "class-1", "", nil)),
*newProvisionedVolumeWithSpecifiedReclaimPolicy(newStorageClassWithSpecifiedReclaimPolicy("class-1", "foo.bar/baz", v1.PersistentVolumeReclaimRetain), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)),
},
},
}
Expand All @@ -211,7 +212,7 @@ func TestController(t *testing.T) {
}
}

serverVersion := "v1.5.0"
serverVersion := defaultServerVersion
if test.serverVersion != "" {
serverVersion = test.serverVersion
}
Expand Down Expand Up @@ -259,7 +260,7 @@ func TestMultipleControllers(t *testing.T) {
lock: sync.Mutex{},
claimSource: claimSource,
}
reactor.claims["claim-1"] = newClaim("claim-1", "uid-1-1", "class-1", "", nil)
reactor.claims["claim-1"] = newClaim("claim-1", "uid-1-1", "class-1", test.provisionerName, "", nil)
client.PrependReactor("update", "persistentvolumeclaims", reactor.React)
client.PrependReactor("get", "persistentvolumeclaims", reactor.React)

Expand All @@ -278,15 +279,15 @@ func TestMultipleControllers(t *testing.T) {
ctrls := make([]*ProvisionController, test.numControllers)
stopChs := make([]chan struct{}, test.numControllers)
for i := 0; i < test.numControllers; i++ {
ctrls[i] = NewProvisionController(client, test.provisionerName, provisioner, "v1.5.0", CreateProvisionedPVInterval(10*time.Millisecond))
ctrls[i] = NewProvisionController(client, test.provisionerName, provisioner, defaultServerVersion, CreateProvisionedPVInterval(10*time.Millisecond))
ctrls[i].claimSource = claimSource
ctrls[i].claims.Add(newClaim("claim-1", "uid-1-1", "class-1", "", nil))
ctrls[i].classes.Add(newStorageClass("class-1", "foo.bar/baz"))
ctrls[i].claims.Add(newClaim("claim-1", "uid-1-1", "class-1", test.provisionerName, "", nil))
ctrls[i].classes.Add(newStorageClass("class-1", test.provisionerName))
stopChs[i] = make(chan struct{})
}

for i := 0; i < test.numControllers; i++ {
go ctrls[i].addClaim(newClaim("claim-1", "uid-1-1", "class-1", "", nil))
go ctrls[i].addClaim(newClaim("claim-1", "uid-1-1", "class-1", test.provisionerName, "", nil))
}

// Sleep for 3 election retry periods
Expand All @@ -305,63 +306,79 @@ func TestMultipleControllers(t *testing.T) {

func TestShouldProvision(t *testing.T) {
tests := []struct {
name string
provisionerName string
class *storagebeta.StorageClass
claim *v1.PersistentVolumeClaim
expectedShould bool
name string
provisionerName string
class *storagebeta.StorageClass
claim *v1.PersistentVolumeClaim
serverGitVersion string
expectedShould bool
}{
{
name: "should provision",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-1", "", nil),
claim: newClaim("claim-1", "1-1", "class-1", "foo.bar/baz", "", nil),
expectedShould: true,
},
{
name: "claim already bound",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-1", "foo", nil),
claim: newClaim("claim-1", "1-1", "class-1", "foo.bar/baz", "foo", nil),
expectedShould: false,
},
{
name: "no such class",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-2", "", nil),
claim: newClaim("claim-1", "1-1", "class-2", "", "", nil),
expectedShould: false,
},
{
name: "not this provisioner's job",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "abc.def/ghi"),
claim: newClaim("claim-1", "1-1", "class-1", "", nil),
claim: newClaim("claim-1", "1-1", "class-1", "abc.def/ghi", "", nil),
expectedShould: false,
},
// Kubernetes 1.5 provisioning - annStorageProvisioner is set
// and only this annotation is evaluated
{
name: "should provision 1.5",
name: "unknown provisioner 1.5",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-2", "abc.def/ghi"),
claim: newClaim("claim-1", "1-1", "class-1", "",
map[string]string{annStorageProvisioner: "foo.bar/baz"}),
expectedShould: true,
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-1", "", "",
map[string]string{annStorageProvisioner: "abc.def/ghi"}),
expectedShould: false,
},
// Kubernetes 1.4 provisioning - annStorageProvisioner is set but ignored
{
name: "unknown provisioner 1.5",
name: "should provision, unknown provisioner annotation but 1.4",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-1", "",
claim: newClaim("claim-1", "1-1", "class-1", "", "",
map[string]string{annStorageProvisioner: "abc.def/ghi"}),
expectedShould: false,
serverGitVersion: "v1.4.0",
expectedShould: true,
},
// Kubernetes 1.4 provisioning - annStorageProvisioner is not set nor needed
{
name: "should provision, no provisioner annotation needed",
provisionerName: "foo.bar/baz",
class: newStorageClass("class-1", "foo.bar/baz"),
claim: newClaim("claim-1", "1-1", "class-1", "", "", nil),
serverGitVersion: "v1.4.0",
expectedShould: true,
},
}
for _, test := range tests {
client := fake.NewSimpleClientset(test.claim)
provisioner := newTestProvisioner()
ctrl := newTestProvisionController(client, test.provisionerName, provisioner, "v1.5.0")
serverVersion := defaultServerVersion
if test.serverGitVersion != "" {
serverVersion = test.serverGitVersion
}
ctrl := newTestProvisionController(client, test.provisionerName, provisioner, serverVersion)

err := ctrl.classes.Add(test.class)
if err != nil {
Expand Down Expand Up @@ -450,26 +467,26 @@ func TestIsOnlyRecordUpdate(t *testing.T) {
}{
{
name: "is only record update",
old: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
old: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
expectedIs: true,
},
{
name: "is seen as only record update, stayed exactly the same",
old: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
old: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
expectedIs: true,
},
{
name: "isn't only record update, class changed as well",
old: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-2", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
old: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-2", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
expectedIs: false,
},
{
name: "isn't only record update, only class changed",
old: newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-2", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
old: newClaim("claim-1", "1-1", "class-1", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
new: newClaim("claim-1", "1-1", "class-2", "", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
expectedIs: false,
},
}
Expand Down Expand Up @@ -532,14 +549,14 @@ func newStorageClassWithSpecifiedReclaimPolicy(name, provisioner string, reclaim
}
}

func newClaim(name, claimUID, provisioner, volumeName string, annotations map[string]string) *v1.PersistentVolumeClaim {
func newClaim(name, claimUID, class, provisioner, volumeName string, annotations map[string]string) *v1.PersistentVolumeClaim {
claim := &v1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: v1.NamespaceDefault,
UID: types.UID(claimUID),
ResourceVersion: "0",
Annotations: map[string]string{annClass: provisioner},
Annotations: map[string]string{},
SelfLink: "/api/v1/namespaces/" + v1.NamespaceDefault + "/persistentvolumeclaims/" + name,
},
Spec: v1.PersistentVolumeClaimSpec{
Expand All @@ -555,6 +572,12 @@ func newClaim(name, claimUID, provisioner, volumeName string, annotations map[st
Phase: v1.ClaimPending,
},
}
// TODO remove annClass according to version of Kube.
claim.Annotations[annClass] = class
if provisioner != "" {
claim.Annotations[annStorageProvisioner] = provisioner
}
// Allow overwriting of above annotations
for k, v := range annotations {
claim.Annotations[k] = v
}
Expand Down Expand Up @@ -715,7 +738,7 @@ func (i *ignoredProvisioner) Provision(options VolumeOptions) (*v1.PersistentVol
return nil, &IgnoredError{"Ignored"}
}

return newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "", nil)), nil
return newProvisionedVolume(newStorageClass("class-1", "foo.bar/baz"), newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", nil)), nil
}

func (i *ignoredProvisioner) Delete(volume *v1.PersistentVolume) error {
Expand Down

0 comments on commit c3ee08f

Please sign in to comment.