diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index bb4e2f62a67..25b35b73f10 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -62,7 +62,6 @@ type CMServer struct { ResourceQuotaSyncPeriod time.Duration NamespaceSyncPeriod time.Duration PVClaimBinderSyncPeriod time.Duration - EnablePVCClaimBinder bool RegisterRetryCount int MachineList util.StringList SyncNodeList bool @@ -125,7 +124,6 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet) { fs.DurationVar(&s.ResourceQuotaSyncPeriod, "resource-quota-sync-period", s.ResourceQuotaSyncPeriod, "The period for syncing quota usage status in the system") fs.DurationVar(&s.NamespaceSyncPeriod, "namespace-sync-period", s.NamespaceSyncPeriod, "The period for syncing namespace life-cycle updates") fs.DurationVar(&s.PVClaimBinderSyncPeriod, "pvclaimbinder-sync-period", s.PVClaimBinderSyncPeriod, "The period for syncing persistent volumes and persistent volume claims") - fs.BoolVar(&s.EnablePVCClaimBinder, "enable-alpha-pvclaimbinder", s.EnablePVCClaimBinder, "Optionally enable persistent volume claim binding. This feature is experimental and expected to change.") fs.DurationVar(&s.PodEvictionTimeout, "pod-eviction-timeout", s.PodEvictionTimeout, "The grace peroid for deleting pods on failed nodes.") fs.Float32Var(&s.DeletingPodsQps, "deleting-pods-qps", 0.1, "Number of nodes per second on which pods are deleted in case of node failure.") fs.IntVar(&s.DeletingPodsBurst, "deleting-pods-burst", 10, "Number of nodes on which pods are bursty deleted in case of node failure. For more details look into RateLimiter.") @@ -247,10 +245,8 @@ func (s *CMServer) Run(_ []string) error { namespaceManager := namespace.NewNamespaceManager(kubeClient, s.NamespaceSyncPeriod) namespaceManager.Run() - if s.EnablePVCClaimBinder { - pvclaimBinder := volumeclaimbinder.NewPersistentVolumeClaimBinder(kubeClient, s.PVClaimBinderSyncPeriod) - pvclaimBinder.Run() - } + pvclaimBinder := volumeclaimbinder.NewPersistentVolumeClaimBinder(kubeClient, s.PVClaimBinderSyncPeriod) + pvclaimBinder.Run() if len(s.ServiceAccountKeyFile) > 0 { privateKey, err := serviceaccount.ReadPrivateKey(s.ServiceAccountKeyFile) diff --git a/pkg/api/types.go b/pkg/api/types.go index beaa6217aea..569b24f03f2 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -252,7 +252,9 @@ type PersistentVolumeSpec struct { PersistentVolumeSource `json:",inline"` // AccessModes contains all ways the volume can be mounted AccessModes []AccessModeType `json:"accessModes,omitempty"` - // holds the binding reference to a PersistentVolumeClaim + // ClaimRef is part of a bi-directional binding between PersistentVolume and PersistentVolumeClaim. + // ClaimRef is expected to be non-nil when bound. + // claim.VolumeName is the authoritative bind between PV and PVC. ClaimRef *ObjectReference `json:"claimRef,omitempty"` } @@ -292,6 +294,8 @@ type PersistentVolumeClaimSpec struct { AccessModes []AccessModeType `json:"accessModes,omitempty"` // Resources represents the minimum resources required Resources ResourceRequirements `json:"resources,omitempty"` + // VolumeName is the binding reference to the PersistentVolume backing this claim + VolumeName string `json:"volumeName,omitempty"` } type PersistentVolumeClaimStatus struct { @@ -301,8 +305,6 @@ type PersistentVolumeClaimStatus struct { AccessModes []AccessModeType `json:"accessModes,omitempty"` // Represents the actual resources of the underlying volume Capacity ResourceList `json:"capacity,omitempty"` - // VolumeRef is a reference to the PersistentVolume bound to the PersistentVolumeClaim - VolumeRef *ObjectReference `json:"volumeRef,omitempty"` } type AccessModeType string diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index aa634dc7d49..52a74c04be0 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -269,8 +269,10 @@ type PersistentVolumeSpec struct { PersistentVolumeSource `json:",inline" description:"the actual volume backing the persistent volume"` // AccessModes contains all ways the volume can be mounted AccessModes []AccessModeType `json:"accessModes,omitempty" description:"all ways the volume can be mounted"` - // holds the binding reference to a PersistentVolumeClaim - ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"the binding reference to a persistent volume claim"` + // ClaimRef is part of a bi-directional binding between PersistentVolume and PersistentVolumeClaim. + // ClaimRef is expected to be non-nil when bound. + // claim.VolumeName is the authoritative bind between PV and PVC. + ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"when bound, a reference to the bound claim"` } type PersistentVolumeStatus struct { @@ -309,6 +311,8 @@ type PersistentVolumeClaimSpec struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the desired access modes the volume should have"` // Resources represents the minimum resources required Resources ResourceRequirements `json:"resources,omitempty" description:"the desired resources the volume should have"` + // VolumeName is the binding reference to the PersistentVolume backing this claim + VolumeName string `json:"volumeName,omitempty" description:"the binding reference to the persistent volume backing this claim"` } type PersistentVolumeClaimStatus struct { @@ -318,8 +322,6 @@ type PersistentVolumeClaimStatus struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the actual access modes the volume has"` // Represents the actual resources of the underlying volume Capacity ResourceList `json:"capacity,omitempty" description:"the actual resources the volume has"` - // VolumeRef is a reference to the PersistentVolume bound to the PersistentVolumeClaim - VolumeRef *ObjectReference `json:"volumeRef,omitempty" description:"a reference to the backing persistent volume, when bound"` } type AccessModeType string diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index e3375209ec8..aeae7121515 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -174,8 +174,8 @@ type PersistentVolumeSpec struct { PersistentVolumeSource `json:",inline" description:"the actual volume backing the persistent volume"` // AccessModes contains all ways the volume can be mounted AccessModes []AccessModeType `json:"accessModes,omitempty" description:"all ways the volume can be mounted"` - // holds the binding reference to a PersistentVolumeClaim - ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"the binding reference to a persistent volume claim"` + // ClaimRef is a non-binding reference to the claim bound to this volume + ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"when bound, a reference to the bound claim"` } type PersistentVolumeStatus struct { @@ -211,6 +211,8 @@ type PersistentVolumeClaimSpec struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the desired access modes the volume should have"` // Resources represents the minimum resources required Resources ResourceRequirements `json:"resources,omitempty" description:"the desired resources the volume should have"` + // VolumeName is the binding reference to the PersistentVolume backing this claim + VolumeName string `json:"volumeName,omitempty" description:"the binding reference to the persistent volume backing this claim"` } type PersistentVolumeClaimStatus struct { @@ -220,8 +222,6 @@ type PersistentVolumeClaimStatus struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the actual access modes the volume has"` // Represents the actual resources of the underlying volume Capacity ResourceList `json:"capacity,omitempty" description:"the actual resources the volume has"` - // VolumeRef is a reference to the PersistentVolume bound to the PersistentVolumeClaim - VolumeRef *ObjectReference `json:"volumeRef,omitempty" description:"a reference to the backing persistent volume, when bound"` } type AccessModeType string diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index ae524183f35..474618f0b6b 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -136,8 +136,10 @@ type PersistentVolumeSpec struct { PersistentVolumeSource `json:",inline" description:"the actual volume backing the persistent volume"` // AccessModes contains all ways the volume can be mounted AccessModes []AccessModeType `json:"accessModes,omitempty" description:"all ways the volume can be mounted"` - // holds the binding reference to a PersistentVolumeClaim - ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"the binding reference to a persistent volume claim"` + // ClaimRef is part of a bi-directional binding between PersistentVolume and PersistentVolumeClaim. + // ClaimRef is expected to be non-nil when bound. + // claim.VolumeName is the authoritative bind between PV and PVC. + ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"when bound, a reference to the bound claim"` } type PersistentVolumeStatus struct { @@ -173,6 +175,8 @@ type PersistentVolumeClaimSpec struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the desired access modes the volume should have"` // Resources represents the minimum resources required Resources ResourceRequirements `json:"resources,omitempty" description:"the desired resources the volume should have"` + // VolumeName is the binding reference to the PersistentVolume backing this claim + VolumeName string `json:"volumeName,omitempty" description:"the binding reference to the persistent volume backing this claim"` } type PersistentVolumeClaimStatus struct { @@ -182,8 +186,6 @@ type PersistentVolumeClaimStatus struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the actual access modes the volume has"` // Represents the actual resources of the underlying volume Capacity ResourceList `json:"capacity,omitempty" description:"the actual resources the volume has"` - // VolumeRef is a reference to the PersistentVolume bound to the PersistentVolumeClaim - VolumeRef *ObjectReference `json:"volumeRef,omitempty" description:"a reference to the backing persistent volume, when bound"` } type AccessModeType string diff --git a/pkg/api/v1beta3/conversion_generated.go b/pkg/api/v1beta3/conversion_generated.go index 26069484303..83cb55384c1 100644 --- a/pkg/api/v1beta3/conversion_generated.go +++ b/pkg/api/v1beta3/conversion_generated.go @@ -2060,6 +2060,7 @@ func convert_v1beta3_PersistentVolumeClaimSpec_To_api_PersistentVolumeClaimSpec( if err := convert_v1beta3_ResourceRequirements_To_api_ResourceRequirements(&in.Resources, &out.Resources, s); err != nil { return err } + out.VolumeName = in.VolumeName return nil } @@ -2078,6 +2079,7 @@ func convert_api_PersistentVolumeClaimSpec_To_v1beta3_PersistentVolumeClaimSpec( if err := convert_api_ResourceRequirements_To_v1beta3_ResourceRequirements(&in.Resources, &out.Resources, s); err != nil { return err } + out.VolumeName = in.VolumeName return nil } @@ -2106,14 +2108,6 @@ func convert_v1beta3_PersistentVolumeClaimStatus_To_api_PersistentVolumeClaimSta } else { out.Capacity = nil } - if in.VolumeRef != nil { - out.VolumeRef = new(newer.ObjectReference) - if err := convert_v1beta3_ObjectReference_To_api_ObjectReference(in.VolumeRef, out.VolumeRef, s); err != nil { - return err - } - } else { - out.VolumeRef = nil - } return nil } @@ -2142,14 +2136,6 @@ func convert_api_PersistentVolumeClaimStatus_To_v1beta3_PersistentVolumeClaimSta } else { out.Capacity = nil } - if in.VolumeRef != nil { - out.VolumeRef = new(ObjectReference) - if err := convert_api_ObjectReference_To_v1beta3_ObjectReference(in.VolumeRef, out.VolumeRef, s); err != nil { - return err - } - } else { - out.VolumeRef = nil - } return nil } diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 3e70761b41c..d4e77963821 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -269,8 +269,10 @@ type PersistentVolumeSpec struct { PersistentVolumeSource `json:",inline" description:"the actual volume backing the persistent volume"` // AccessModes contains all ways the volume can be mounted AccessModes []AccessModeType `json:"accessModes,omitempty" description:"all ways the volume can be mounted"` - // holds the binding reference to a PersistentVolumeClaim - ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"the binding reference to a persistent volume claim"` + // ClaimRef is part of a bi-directional binding between PersistentVolume and PersistentVolumeClaim. + // ClaimRef is expected to be non-nil when bound. + // claim.VolumeName is the authoritative bind between PV and PVC. + ClaimRef *ObjectReference `json:"claimRef,omitempty" description:"when bound, a reference to the bound claim"` } type PersistentVolumeStatus struct { @@ -309,6 +311,8 @@ type PersistentVolumeClaimSpec struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the desired access modes the volume should have"` // Resources represents the minimum resources required Resources ResourceRequirements `json:"resources,omitempty" description:"the desired resources the volume should have"` + // VolumeName is the binding reference to the PersistentVolume backing this claim + VolumeName string `json:"volumeName,omitempty" description:"the binding reference to the persistent volume backing this claim"` } type PersistentVolumeClaimStatus struct { @@ -318,8 +322,6 @@ type PersistentVolumeClaimStatus struct { AccessModes []AccessModeType `json:"accessModes,omitempty" description:"the actual access modes the volume has"` // Represents the actual resources of the underlying volume Capacity ResourceList `json:"capacity,omitempty" description:"the actual resources the volume has"` - // VolumeRef is a reference to the PersistentVolume bound to the PersistentVolumeClaim - VolumeRef *ObjectReference `json:"volumeRef,omitempty" description:"a reference to the backing persistent volume, when bound"` } type AccessModeType string diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 9fb8d533434..8cd15a2bffd 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -20,7 +20,6 @@ import ( "fmt" "net" "path" - "reflect" "strings" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -29,7 +28,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" errs "github.com/GoogleCloudPlatform/kubernetes/pkg/util/fielderrors" - "github.com/GoogleCloudPlatform/kubernetes/pkg/volume" "github.com/golang/glog" ) @@ -447,6 +445,10 @@ func ValidatePersistentVolume(pv *api.PersistentVolume) errs.ValidationErrorList allErrs := errs.ValidationErrorList{} allErrs = append(allErrs, ValidateObjectMeta(&pv.ObjectMeta, false, ValidatePersistentVolumeName).Prefix("metadata")...) + if len(pv.Spec.AccessModes) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("persistentVolume.AccessModes")) + } + if len(pv.Spec.Capacity) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("persistentVolume.Capacity")) } @@ -455,6 +457,10 @@ func ValidatePersistentVolume(pv *api.PersistentVolume) errs.ValidationErrorList allErrs = append(allErrs, errs.NewFieldInvalid("", pv.Spec.Capacity, fmt.Sprintf("only %s is expected", api.ResourceStorage))) } + for _, qty := range pv.Spec.Capacity { + allErrs = append(allErrs, validateBasicResource(qty)...) + } + numVolumes := 0 if pv.Spec.HostPath != nil { numVolumes++ @@ -517,16 +523,6 @@ func ValidatePersistentVolumeClaim(pvc *api.PersistentVolumeClaim) errs.Validati func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *api.PersistentVolumeClaim) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} allErrs = ValidatePersistentVolumeClaim(newPvc) - if oldPvc.Status.VolumeRef != nil { - oldModesAsString := volume.GetAccessModesAsString(oldPvc.Spec.AccessModes) - newModesAsString := volume.GetAccessModesAsString(newPvc.Spec.AccessModes) - if oldModesAsString != newModesAsString { - allErrs = append(allErrs, errs.NewFieldInvalid("spec.AccessModes", oldPvc.Spec.AccessModes, "field is immutable")) - } - if !reflect.DeepEqual(oldPvc.Spec.Resources, newPvc.Spec.Resources) { - allErrs = append(allErrs, errs.NewFieldInvalid("spec.Resources", oldPvc.Spec.Resources, "field is immutable")) - } - } newPvc.Status = oldPvc.Status return allErrs } @@ -537,6 +533,12 @@ func ValidatePersistentVolumeClaimStatusUpdate(newPvc, oldPvc *api.PersistentVol if newPvc.ResourceVersion == "" { allErrs = append(allErrs, errs.NewFieldRequired("resourceVersion")) } + if len(newPvc.Spec.AccessModes) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("persistentVolume.AccessModes")) + } + for _, qty := range newPvc.Status.Capacity { + allErrs = append(allErrs, validateBasicResource(qty)...) + } newPvc.Spec = oldPvc.Spec return allErrs } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 3fa3d07a34e..2d61d86ea0e 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -228,6 +228,7 @@ func TestValidatePersistentVolumes(t *testing.T) { Capacity: api.ResourceList{ api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), }, + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, PersistentVolumeSource: api.PersistentVolumeSource{ HostPath: &api.HostPathVolumeSource{Path: "/foo"}, }, @@ -239,6 +240,7 @@ func TestValidatePersistentVolumes(t *testing.T) { Capacity: api.ResourceList{ api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), }, + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, PersistentVolumeSource: api.PersistentVolumeSource{ HostPath: &api.HostPathVolumeSource{Path: "/foo"}, }, @@ -250,11 +252,11 @@ func TestValidatePersistentVolumes(t *testing.T) { Capacity: api.ResourceList{ api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), }, + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, PersistentVolumeSource: api.PersistentVolumeSource{ HostPath: &api.HostPathVolumeSource{Path: "/foo"}, }, - }, - ), + }), }, "missing-name": { isExpectedFailure: true, @@ -262,12 +264,24 @@ func TestValidatePersistentVolumes(t *testing.T) { Capacity: api.ResourceList{ api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), }, + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, }), }, "missing-capacity": { isExpectedFailure: true, volume: testVolume("foo", "", api.PersistentVolumeSpec{}), }, + "missing-accessmodes": { + isExpectedFailure: true, + volume: testVolume("goodname", "missing-accessmodes", api.PersistentVolumeSpec{ + Capacity: api.ResourceList{ + api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), + }, + PersistentVolumeSource: api.PersistentVolumeSource{ + HostPath: &api.HostPathVolumeSource{Path: "/foo"}, + }, + }), + }, "too-many-sources": { isExpectedFailure: true, volume: testVolume("", "", api.PersistentVolumeSpec{ @@ -379,136 +393,6 @@ func TestValidatePersistentVolumeClaim(t *testing.T) { } } -func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { - - pvcA := &api.PersistentVolumeClaim{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: "ns", - Labels: map[string]string{ - "nice-label": "fizzbuzz", - }, - }, - Spec: api.PersistentVolumeClaimSpec{ - AccessModes: []api.AccessModeType{ - api.ReadWriteOnce, - }, - Resources: api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), - }, - }, - }, - } - - // AccessModes differ from pvcA - pvcB := &api.PersistentVolumeClaim{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: "ns", - Labels: map[string]string{ - "nice-label": "fizzbuzz", - }, - }, - Spec: api.PersistentVolumeClaimSpec{ - AccessModes: []api.AccessModeType{ - api.ReadWriteMany, - }, - Resources: api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), - }, - }, - }, - } - - // Resources differ from pvcA - pvcC := &api.PersistentVolumeClaim{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: "ns", - Labels: map[string]string{ - "nice-label": "fizzbuzz", - }, - }, - Spec: api.PersistentVolumeClaimSpec{ - AccessModes: []api.AccessModeType{ - api.ReadWriteOnce, - }, - Resources: api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceName(api.ResourceStorage): resource.MustParse("7G"), - }, - }, - }, - } - - // Labels differ from pvcA - pvcD := &api.PersistentVolumeClaim{ - ObjectMeta: api.ObjectMeta{ - Name: "foo", - Namespace: "ns", - Labels: map[string]string{ - "nice-label": "buzzfizz", - }, - }, - Spec: api.PersistentVolumeClaimSpec{ - AccessModes: []api.AccessModeType{ - api.ReadWriteOnce, - }, - Resources: api.ResourceRequirements{ - Requests: api.ResourceList{ - api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), - }, - }, - }, - } - - scenarios := map[string]struct { - isExpectedFailure bool - oldClaim *api.PersistentVolumeClaim - newClaim *api.PersistentVolumeClaim - }{ - "invalid-accessmodes-change": { - isExpectedFailure: true, - oldClaim: pvcA, - newClaim: pvcB, - }, - "invalid-resources-change": { - isExpectedFailure: true, - oldClaim: pvcA, - newClaim: pvcC, - }, - "valid-label-change": { - isExpectedFailure: false, - oldClaim: pvcA, - newClaim: pvcD, - }, - } - - // validation errors on Update only occur if the Claim is already bound. - // failures are only expected after binding - for name, scenario := range scenarios { - errs := ValidatePersistentVolumeClaimUpdate(scenario.newClaim, scenario.oldClaim) - if len(errs) > 0 && !scenario.isExpectedFailure { - t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs) - } - } - - // 2 of 3 scenarios above are invalid if the old PVC has a bound PV - for name, scenario := range scenarios { - scenario.oldClaim.Status.VolumeRef = &api.ObjectReference{Name: "foo", Namespace: "ns"} - errs := ValidatePersistentVolumeClaimUpdate(scenario.newClaim, scenario.oldClaim) - if len(errs) == 0 && scenario.isExpectedFailure { - t.Errorf("Unexpected success for scenario: %s", name) - } - if len(errs) > 0 && !scenario.isExpectedFailure { - t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs) - } - } - -} - func TestValidateVolumes(t *testing.T) { successCase := []api.Volume{ {Name: "abc", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{"/mnt/path1"}}}, diff --git a/pkg/kubectl/describe.go b/pkg/kubectl/describe.go index 255721082d5..1e0e650bf68 100644 --- a/pkg/kubectl/describe.go +++ b/pkg/kubectl/describe.go @@ -321,15 +321,15 @@ type PersistentVolumeClaimDescriber struct { func (d *PersistentVolumeClaimDescriber) Describe(namespace, name string) (string, error) { c := d.PersistentVolumeClaims(namespace) - psd, err := c.Get(name) + pvc, err := c.Get(name) if err != nil { return "", err } return tabbedString(func(out io.Writer) error { - fmt.Fprintf(out, "Name:\t%s\n", psd.Name) - fmt.Fprintf(out, "Status:\t%d\n", psd.Status.Phase) - fmt.Fprintf(out, "Volume:\t%d\n", psd.Status.VolumeRef.UID) + fmt.Fprintf(out, "Name:\t%s\n", pvc.Name) + fmt.Fprintf(out, "Status:\t%d\n", pvc.Status.Phase) + fmt.Fprintf(out, "Volume:\t%d\n", pvc.Spec.VolumeName) return nil }) diff --git a/pkg/kubectl/resource_printer.go b/pkg/kubectl/resource_printer.go index c3ca097b718..9b0b0730993 100644 --- a/pkg/kubectl/resource_printer.go +++ b/pkg/kubectl/resource_printer.go @@ -653,9 +653,9 @@ func printNodeList(list *api.NodeList, w io.Writer) error { func printPersistentVolume(pv *api.PersistentVolume, w io.Writer) error { claimRefUID := "" if pv.Spec.ClaimRef != nil { + claimRefUID += pv.Spec.ClaimRef.Namespace + claimRefUID += "/" claimRefUID += pv.Spec.ClaimRef.Name - claimRefUID += " / " - claimRefUID += string(pv.Spec.ClaimRef.UID) } modesStr := volume.GetAccessModesAsString(pv.Spec.AccessModes) @@ -677,11 +677,7 @@ func printPersistentVolumeList(list *api.PersistentVolumeList, w io.Writer) erro } func printPersistentVolumeClaim(pvc *api.PersistentVolumeClaim, w io.Writer) error { - volumeRefUID := "" - if pvc.Status.VolumeRef != nil { - volumeRefUID = string(pvc.Status.VolumeRef.UID) - } - _, err := fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", pvc.Name, pvc.Labels, pvc.Status.Phase, volumeRefUID) + _, err := fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", pvc.Name, pvc.Labels, pvc.Status.Phase, pvc.Spec.VolumeName) return err } diff --git a/pkg/registry/persistentvolume/etcd/etcd_test.go b/pkg/registry/persistentvolume/etcd/etcd_test.go index 802af462a0f..30e3c1d528a 100644 --- a/pkg/registry/persistentvolume/etcd/etcd_test.go +++ b/pkg/registry/persistentvolume/etcd/etcd_test.go @@ -55,6 +55,7 @@ func validNewPersistentVolume(name string) *api.PersistentVolume { Capacity: api.ResourceList{ api.ResourceName(api.ResourceStorage): resource.MustParse("10G"), }, + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, PersistentVolumeSource: api.PersistentVolumeSource{ HostPath: &api.HostPathVolumeSource{Path: "/foo"}, }, diff --git a/pkg/registry/persistentvolumeclaim/etcd/etcd_test.go b/pkg/registry/persistentvolumeclaim/etcd/etcd_test.go index 08da9c11de3..9de76bf994e 100644 --- a/pkg/registry/persistentvolumeclaim/etcd/etcd_test.go +++ b/pkg/registry/persistentvolumeclaim/etcd/etcd_test.go @@ -327,12 +327,20 @@ func TestEtcdUpdateStatus(t *testing.T) { pvcStart := validNewPersistentVolumeClaim("foo", api.NamespaceDefault) fakeClient.Set(key, runtime.EncodeOrDie(latest.Codec, pvcStart), 1) - pvIn := &api.PersistentVolumeClaim{ + pvc := &api.PersistentVolumeClaim{ ObjectMeta: api.ObjectMeta{ Name: "foo", Namespace: api.NamespaceDefault, ResourceVersion: "1", }, + Spec: api.PersistentVolumeClaimSpec{ + AccessModes: []api.AccessModeType{api.ReadWriteOnce}, + Resources: api.ResourceRequirements{ + Requests: api.ResourceList{ + api.ResourceName(api.ResourceStorage): resource.MustParse("3Gi"), + }, + }, + }, Status: api.PersistentVolumeClaimStatus{ Phase: api.ClaimBound, }, @@ -340,10 +348,10 @@ func TestEtcdUpdateStatus(t *testing.T) { expected := *pvcStart expected.ResourceVersion = "2" - expected.Labels = pvIn.Labels - expected.Status = pvIn.Status + expected.Labels = pvc.Labels + expected.Status = pvc.Status - _, _, err := statusStorage.Update(ctx, pvIn) + _, _, err := statusStorage.Update(ctx, pvc) if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/pkg/volume/persistent_claim/persistent_claim.go b/pkg/volume/persistent_claim/persistent_claim.go index dea97c62688..77876ea696c 100644 --- a/pkg/volume/persistent_claim/persistent_claim.go +++ b/pkg/volume/persistent_claim/persistent_claim.go @@ -58,16 +58,26 @@ func (plugin *persistentClaimPlugin) NewBuilder(spec *volume.Spec, pod *api.Pod, return nil, err } - if claim.Status.VolumeRef == nil { + if claim.Spec.VolumeName == "" { return nil, fmt.Errorf("The claim %+v is not yet bound to a volume", claim.Name) } - pv, err := plugin.host.GetKubeClient().PersistentVolumes().Get(claim.Status.VolumeRef.Name) + pv, err := plugin.host.GetKubeClient().PersistentVolumes().Get(claim.Spec.VolumeName) if err != nil { glog.Errorf("Error finding persistent volume for claim: %+v\n", claim.Name) return nil, err } + if pv.Spec.ClaimRef == nil { + glog.Errorf("The volume is not yet bound to the claim. Expected to find the bind on volume.Spec.ClaimRef: %+v", pv) + return nil, err + } + + if pv.Spec.ClaimRef.UID != claim.UID { + glog.Errorf("Excepted volume.Spec.ClaimRef.UID %+v but have %+v", pv.Spec.ClaimRef.UID, claim.UID) + return nil, err + } + builder, err := plugin.host.NewWrapperBuilder(volume.NewSpecFromPersistentVolume(pv), pod, opts, mounter) if err != nil { glog.Errorf("Error creating builder for claim: %+v\n", claim.Name) diff --git a/pkg/volume/persistent_claim/persistent_claim_test.go b/pkg/volume/persistent_claim/persistent_claim_test.go index 461c0c5c120..0eed61e4817 100644 --- a/pkg/volume/persistent_claim/persistent_claim_test.go +++ b/pkg/volume/persistent_claim/persistent_claim_test.go @@ -65,11 +65,12 @@ func TestCanSupport(t *testing.T) { func TestNewBuilder(t *testing.T) { tests := []struct { - pv *api.PersistentVolume - claim *api.PersistentVolumeClaim - plugin volume.VolumePlugin - podVolume api.VolumeSource - testFunc func(builder volume.Builder, plugin volume.VolumePlugin) error + pv *api.PersistentVolume + claim *api.PersistentVolumeClaim + plugin volume.VolumePlugin + podVolume api.VolumeSource + testFunc func(builder volume.Builder, plugin volume.VolumePlugin) error + expectedFailure bool }{ { pv: &api.PersistentVolume{ @@ -90,11 +91,11 @@ func TestNewBuilder(t *testing.T) { Name: "claimA", Namespace: "nsA", }, + Spec: api.PersistentVolumeClaimSpec{ + VolumeName: "pvA", + }, Status: api.PersistentVolumeClaimStatus{ Phase: api.ClaimBound, - VolumeRef: &api.ObjectReference{ - Name: "pvA", - }, }, }, podVolume: api.VolumeSource{ @@ -110,6 +111,7 @@ func TestNewBuilder(t *testing.T) { } return nil }, + expectedFailure: false, }, { pv: &api.PersistentVolume{ @@ -130,10 +132,8 @@ func TestNewBuilder(t *testing.T) { Name: "claimB", Namespace: "nsB", }, - Status: api.PersistentVolumeClaimStatus{ - VolumeRef: &api.ObjectReference{ - Name: "pvB", - }, + Spec: api.PersistentVolumeClaimSpec{ + VolumeName: "pvA", }, }, podVolume: api.VolumeSource{ @@ -149,6 +149,88 @@ func TestNewBuilder(t *testing.T) { } return nil }, + expectedFailure: false, + }, + { + pv: &api.PersistentVolume{ + ObjectMeta: api.ObjectMeta{ + Name: "pvA", + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{}, + }, + }, + }, + claim: &api.PersistentVolumeClaim{ + ObjectMeta: api.ObjectMeta{ + Name: "claimA", + Namespace: "nsA", + }, + Spec: api.PersistentVolumeClaimSpec{ + VolumeName: "pvA", + }, + Status: api.PersistentVolumeClaimStatus{ + Phase: api.ClaimBound, + }, + }, + podVolume: api.VolumeSource{ + PersistentVolumeClaimVolumeSource: &api.PersistentVolumeClaimVolumeSource{ + ReadOnly: false, + ClaimName: "claimA", + }, + }, + plugin: gce_pd.ProbeVolumePlugins()[0], + testFunc: func(builder volume.Builder, plugin volume.VolumePlugin) error { + if builder != nil { + return fmt.Errorf("Unexpected non-nil builder: %+v", builder) + } + return nil + }, + expectedFailure: true, // missing pv.Spec.ClaimRef + }, + { + pv: &api.PersistentVolume{ + ObjectMeta: api.ObjectMeta{ + Name: "pvA", + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{}, + }, + ClaimRef: &api.ObjectReference{ + Name: "claimB", + UID: types.UID("abc123"), + }, + }, + }, + claim: &api.PersistentVolumeClaim{ + ObjectMeta: api.ObjectMeta{ + Name: "claimA", + Namespace: "nsA", + UID: types.UID("def456"), + }, + Spec: api.PersistentVolumeClaimSpec{ + VolumeName: "pvA", + }, + Status: api.PersistentVolumeClaimStatus{ + Phase: api.ClaimBound, + }, + }, + podVolume: api.VolumeSource{ + PersistentVolumeClaimVolumeSource: &api.PersistentVolumeClaimVolumeSource{ + ReadOnly: false, + ClaimName: "claimA", + }, + }, + plugin: gce_pd.ProbeVolumePlugins()[0], + testFunc: func(builder volume.Builder, plugin volume.VolumePlugin) error { + if builder != nil { + return fmt.Errorf("Unexpected non-nil builder: %+v", builder) + } + return nil + }, + expectedFailure: true, // mismatched pv.Spec.ClaimRef and pvc }, } @@ -171,17 +253,18 @@ func TestNewBuilder(t *testing.T) { } pod := &api.Pod{ObjectMeta: api.ObjectMeta{UID: types.UID("poduid")}} builder, err := plug.NewBuilder(spec, pod, volume.VolumeOptions{}, nil) - if err != nil { - t.Errorf("Failed to make a new Builder: %v", err) - } - if builder == nil { - t.Errorf("Got a nil Builder: %v", builder) + + if !item.expectedFailure { + if err != nil { + t.Errorf("Failed to make a new Builder: %v", err) + } + if builder == nil { + t.Errorf("Got a nil Builder: %v", builder) + } } - if builder != nil { - if err := item.testFunc(builder, item.plugin); err != nil { - t.Errorf("Unexpected error %+v", err) - } + if err := item.testFunc(builder, item.plugin); err != nil { + t.Errorf("Unexpected error %+v", err) } } } @@ -195,7 +278,6 @@ func TestNewBuilderClaimNotBound(t *testing.T) { PersistentVolumeSource: api.PersistentVolumeSource{ GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{}, }, - ClaimRef: nil, }, } claim := &api.PersistentVolumeClaim{ @@ -203,10 +285,6 @@ func TestNewBuilderClaimNotBound(t *testing.T) { Name: "claimC", Namespace: "nsA", }, - Status: api.PersistentVolumeClaimStatus{ - Phase: api.ClaimBound, - VolumeRef: nil, - }, } podVolume := api.VolumeSource{ PersistentVolumeClaimVolumeSource: &api.PersistentVolumeClaimVolumeSource{ diff --git a/pkg/volumeclaimbinder/persistent_volume_claim_binder.go b/pkg/volumeclaimbinder/persistent_volume_claim_binder.go index b0b9ad6149c..ecfd736d229 100644 --- a/pkg/volumeclaimbinder/persistent_volume_claim_binder.go +++ b/pkg/volumeclaimbinder/persistent_volume_claim_binder.go @@ -85,7 +85,7 @@ func NewPersistentVolumeClaimBinder(kubeClient client.Interface, syncPeriod time AddFunc: binder.addClaim, UpdateFunc: binder.updateClaim, // no DeleteFunc needed. a claim requires no clean-up. - // the missing claim itself is the release of the resource. + // syncVolume handles the missing claim }, ) @@ -175,15 +175,8 @@ func syncVolume(volumeIndex *persistentVolumeOrderedIndex, binderClient binderCl if volume.Spec.ClaimRef == nil { return fmt.Errorf("PersistentVolume[%s] expected to be bound but found nil claimRef: %+v", volume) } else { - claim, err := binderClient.GetPersistentVolumeClaim(volume.Spec.ClaimRef.Namespace, volume.Spec.ClaimRef.Name) - if err == nil { - // bound and active. Build claim status as needed. - if claim.Status.VolumeRef == nil { - // syncClaimStatus sets VolumeRef, attempts to persist claim status, - // and does a rollback as needed on claim.Status - syncClaimStatus(binderClient, volume, claim) - } - } else { + _, err := binderClient.GetPersistentVolumeClaim(volume.Spec.ClaimRef.Namespace, volume.Spec.ClaimRef.Name) + if err != nil { if errors.IsNotFound(err) { nextPhase = api.VolumeReleased } else { @@ -237,32 +230,54 @@ func syncClaim(volumeIndex *persistentVolumeOrderedIndex, binderClient binderCli return fmt.Errorf("A volume match does not exist for persistent claim: %s", claim.Name) } - claimRef, err := api.GetReference(claim) - if err != nil { - return fmt.Errorf("Unexpected error getting claim reference: %v\n", err) - } - // make a binding reference to the claim. - // triggers update of the volume in this controller, which builds claim status - volume.Spec.ClaimRef = claimRef - volume, err = binderClient.UpdatePersistentVolume(volume) + // triggers update of the claim in this controller, which builds claim status + claim.Spec.VolumeName = volume.Name + // TODO: make this similar to Pod's binding both with BindingREST subresource and GuaranteedUpdate helper in etcd.go + claim, err = binderClient.UpdatePersistentVolumeClaim(claim) if err == nil { nextPhase = api.ClaimBound - } - if err != nil { + glog.V(5).Infof("PersistentVolumeClaim[%s] is bound\n", claim.Name) + } else { // Rollback by unsetting the ClaimRef on the volume pointer. // the volume in the index will be unbound again and ready to be matched. - volume.Spec.ClaimRef = nil + claim.Spec.VolumeName = "" // Rollback by restoring original phase to claim pointer nextPhase = api.ClaimPending return fmt.Errorf("Error updating volume: %+v\n", err) } - // bound claims requires no maintenance. Deletion by the user is the last lifecycle phase. case api.ClaimBound: - // This is the end of a claim's lifecycle. - // After claim deletion, a volume is recycled when it verifies its claim is unbound - glog.V(5).Infof("PersistentVolumeClaime[%s] is bound\n", claim.Name) + volume, err := binderClient.GetPersistentVolume(claim.Spec.VolumeName) + if err != nil { + return fmt.Errorf("Unexpected error getting persistent volume: %v\n", err) + } + + if volume.Spec.ClaimRef == nil { + claimRef, err := api.GetReference(claim) + if err != nil { + return fmt.Errorf("Unexpected error getting claim reference: %v\n", err) + } + volume.Spec.ClaimRef = claimRef + _, err = binderClient.UpdatePersistentVolume(volume) + if err != nil { + return fmt.Errorf("Unexpected error saving PersistentVolume.Status: %+v", err) + } + } + + // all "actuals" are transferred from PV to PVC so the user knows what + // type of volume they actually got for their claim. + // Volumes cannot have zero AccessModes, so checking that a claim has access modes + // is sufficient to tell us if these values have already been set. + if len(claim.Status.AccessModes) == 0 { + claim.Status.Phase = api.ClaimBound + claim.Status.AccessModes = volume.Spec.AccessModes + claim.Status.Capacity = volume.Spec.Capacity + _, err := binderClient.UpdatePersistentVolumeClaimStatus(claim) + if err != nil { + return fmt.Errorf("Unexpected error saving claim status: %+v", err) + } + } } if currentPhase != nextPhase { @@ -272,29 +287,6 @@ func syncClaim(volumeIndex *persistentVolumeOrderedIndex, binderClient binderCli return nil } -func syncClaimStatus(binderClient binderClient, volume *api.PersistentVolume, claim *api.PersistentVolumeClaim) (err error) { - volumeRef, err := api.GetReference(volume) - if err != nil { - return fmt.Errorf("Unexpected error getting volume reference: %v\n", err) - } - - // all "actuals" are transferred from PV to PVC so the user knows what - // type of volume they actually got for their claim - claim.Status.Phase = api.ClaimBound - claim.Status.VolumeRef = volumeRef - claim.Status.AccessModes = volume.Spec.AccessModes - claim.Status.Capacity = volume.Spec.Capacity - - _, err = binderClient.UpdatePersistentVolumeClaimStatus(claim) - if err != nil { - claim.Status.Phase = api.ClaimPending - claim.Status.VolumeRef = nil - claim.Status.AccessModes = nil - claim.Status.Capacity = nil - } - return err -} - // Run starts all of this binder's control loops func (controller *PersistentVolumeClaimBinder) Run() { glog.V(5).Infof("Starting PersistentVolumeClaimBinder\n") diff --git a/pkg/volumeclaimbinder/persistent_volume_claim_binder_test.go b/pkg/volumeclaimbinder/persistent_volume_claim_binder_test.go index 57f4e91b41e..42cac5778ba 100644 --- a/pkg/volumeclaimbinder/persistent_volume_claim_binder_test.go +++ b/pkg/volumeclaimbinder/persistent_volume_claim_binder_test.go @@ -194,30 +194,21 @@ func TestBindingWithExamples(t *testing.T) { claim: claim, } - volumeIndex.Add(pv) + // adds the volume to the index, making the volume available syncVolume(volumeIndex, mockClient, pv) - if pv.Status.Phase != api.VolumeAvailable { t.Errorf("Expected phase %s but got %s", api.VolumeBound, pv.Status.Phase) } - if pv.Spec.ClaimRef != nil { - t.Errorf("Expected nil ClaimRef but got %+v\n", pv.Spec.ClaimRef) - } - + // an initial sync for a claim will bind it to an unbound volume, triggers state change syncClaim(volumeIndex, mockClient, claim) + // state change causes another syncClaim to update statuses + syncClaim(volumeIndex, mockClient, claim) + // claim updated volume's status, causing an update and syncVolume call + syncVolume(volumeIndex, mockClient, pv) if pv.Spec.ClaimRef == nil { - t.Errorf("Expected ClaimRef but got nil for volume: %+v\n", pv) - } - - // first sync verifies the new bound claim, advances state, triggering update - syncVolume(volumeIndex, mockClient, pv) - // second sync verifies claim, sees missing claim status and builds it - syncVolume(volumeIndex, mockClient, pv) - - if claim.Status.VolumeRef == nil { - t.Fatalf("Expected claim to be bound to volume") + t.Errorf("Expected ClaimRef but got nil for pv.Status.ClaimRef: %+v\n", pv) } if pv.Status.Phase != api.VolumeBound { diff --git a/test/integration/persistent_volumes_test.go b/test/integration/persistent_volumes_test.go index 1f8e0be39cf..d7b799d31ef 100644 --- a/test/integration/persistent_volumes_test.go +++ b/test/integration/persistent_volumes_test.go @@ -88,7 +88,7 @@ func TestPersistentVolumeClaimBinder(t *testing.T) { for { event := <-watch.ResultChan() claim := event.Object.(*api.PersistentVolumeClaim) - if claim.Status.VolumeRef != nil { + if claim.Spec.VolumeName != "" { boundCount++ } if boundCount == expectedBoundCount { @@ -102,10 +102,10 @@ func TestPersistentVolumeClaimBinder(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } - if (claim.Name == "claim01" || claim.Name == "claim02") && claim.Status.VolumeRef == nil { + if (claim.Name == "claim01" || claim.Name == "claim02") && claim.Spec.VolumeName == "" { t.Errorf("Expected claim to be bound: %+v", claim) } - if claim.Name == "claim03" && claim.Status.VolumeRef != nil { + if claim.Name == "claim03" && claim.Spec.VolumeName != "" { t.Errorf("Expected claim03 to be unbound: %v", claim) } }