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

Recover expansion failure #106154

Merged
merged 3 commits into from
Nov 16, 2021
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
13 changes: 12 additions & 1 deletion api/openapi-spec/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions pkg/api/persistentvolumeclaim/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,21 @@ func EnforceDataSourceBackwardsCompatibility(pvcSpec, oldPVCSpec *core.Persisten
}
}

func DropDisabledFieldsFromStatus(pvc, oldPVC *core.PersistentVolumeClaim) {
if !utilfeature.DefaultFeatureGate.Enabled(features.ExpandPersistentVolumes) && oldPVC.Status.Conditions == nil {
pvc.Status.Conditions = nil
}

if !utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure) {
if !allocatedResourcesInUse(oldPVC) {
pvc.Status.AllocatedResources = nil
}
if !resizeStatusInUse(oldPVC) {
pvc.Status.ResizeStatus = nil
}
}
}

func dataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) bool {
if oldPVCSpec == nil {
return false
Expand Down Expand Up @@ -118,3 +133,25 @@ func NormalizeDataSources(pvcSpec *core.PersistentVolumeClaimSpec) {
pvcSpec.DataSource = pvcSpec.DataSourceRef.DeepCopy()
}
}

func resizeStatusInUse(oldPVC *core.PersistentVolumeClaim) bool {
if oldPVC == nil {
return false
}
if oldPVC.Status.ResizeStatus != nil {
return true
}
return false
}

func allocatedResourcesInUse(oldPVC *core.PersistentVolumeClaim) bool {
if oldPVC == nil {
return false
}

if oldPVC.Status.AllocatedResources != nil {
return true
}

return false
}
109 changes: 109 additions & 0 deletions pkg/api/persistentvolumeclaim/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"k8s.io/apimachinery/pkg/api/resource"

utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
Expand Down Expand Up @@ -288,3 +289,111 @@ func TestDataSourceRef(t *testing.T) {
})
}
}

func TestDropDisabledFieldsFromStatus(t *testing.T) {
tests := []struct {
name string
feature bool
pvc *core.PersistentVolumeClaim
oldPVC *core.PersistentVolumeClaim
expected *core.PersistentVolumeClaim
}{
{
name: "for:newPVC=hasAllocatedResource,oldPVC=doesnot,featuregate=false; should drop field",
feature: false,
pvc: withAllocatedResource("5G"),
oldPVC: getPVC(),
expected: getPVC(),
},
{
name: "for:newPVC=hasAllocatedResource,oldPVC=doesnot,featuregate=true; should keep field",
feature: true,
pvc: withAllocatedResource("5G"),
oldPVC: getPVC(),
expected: withAllocatedResource("5G"),
},
{
name: "for:newPVC=hasAllocatedResource,oldPVC=hasAllocatedResource,featuregate=true; should keep field",
feature: true,
pvc: withAllocatedResource("5G"),
oldPVC: withAllocatedResource("5G"),
expected: withAllocatedResource("5G"),
},
{
name: "for:newPVC=hasAllocatedResource,oldPVC=hasAllocatedResource,featuregate=false; should keep field",
feature: false,
pvc: withAllocatedResource("10G"),
oldPVC: withAllocatedResource("5G"),
expected: withAllocatedResource("10G"),
},
{
name: "for:newPVC=hasAllocatedResource,oldPVC=nil,featuregate=false; should drop field",
feature: false,
pvc: withAllocatedResource("5G"),
oldPVC: nil,
expected: getPVC(),
},
{
name: "for:newPVC=hasResizeStatus,oldPVC=nil, featuregate=false should drop field",
feature: false,
pvc: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
oldPVC: nil,
expected: getPVC(),
},
{
name: "for:newPVC=hasResizeStatus,oldPVC=doesnot,featuregate=true; should keep field",
feature: true,
pvc: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
oldPVC: getPVC(),
expected: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
},
{
name: "for:newPVC=hasResizeStatus,oldPVC=hasResizeStatus,featuregate=true; should keep field",
feature: true,
pvc: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
oldPVC: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
expected: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
},
{
name: "for:newPVC=hasResizeStatus,oldPVC=hasResizeStatus,featuregate=false; should keep field",
feature: false,
pvc: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
oldPVC: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
expected: withResizeStatus(core.PersistentVolumeClaimNodeExpansionFailed),
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, test.feature)()

DropDisabledFieldsFromStatus(test.pvc, test.oldPVC)

if !reflect.DeepEqual(*test.expected, *test.pvc) {
t.Errorf("Unexpected change: %+v", cmp.Diff(test.expected, test.pvc))
}
})
}
}

func getPVC() *core.PersistentVolumeClaim {
return &core.PersistentVolumeClaim{}
}

func withAllocatedResource(q string) *core.PersistentVolumeClaim {
return &core.PersistentVolumeClaim{
Status: core.PersistentVolumeClaimStatus{
AllocatedResources: core.ResourceList{
core.ResourceStorage: resource.MustParse(q),
},
},
}
}

func withResizeStatus(status core.PersistentVolumeClaimResizeStatus) *core.PersistentVolumeClaim {
return &core.PersistentVolumeClaim{
Status: core.PersistentVolumeClaimStatus{
ResizeStatus: &status,
},
}
}
1 change: 1 addition & 0 deletions pkg/apis/apps/v1/zz_generated.defaults.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/apis/apps/v1beta1/zz_generated.defaults.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/apis/apps/v1beta2/zz_generated.defaults.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions pkg/apis/core/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,9 @@ type PersistentVolumeClaimSpec struct {
// +optional
Selector *metav1.LabelSelector
// Resources represents the minimum resources required
// If RecoverVolumeExpansionFailure feature is enabled users are allowed to specify resource requirements
// that are lower than previous value but must still be higher than capacity recorded in the
// status field of the claim.
// +optional
Resources ResourceRequirements
// VolumeName is the binding reference to the PersistentVolume backing this
Expand Down Expand Up @@ -486,6 +489,26 @@ const (
PersistentVolumeClaimFileSystemResizePending PersistentVolumeClaimConditionType = "FileSystemResizePending"
)

// +enum
type PersistentVolumeClaimResizeStatus string
Copy link
Member

Choose a reason for hiding this comment

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

Should we also update comments on the PVC.spec.capacity field with the new feature-gated behavior for shrinking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated


const (
// When expansion is complete, the empty string is set by resize controller or kubelet.
Copy link
Member

Choose a reason for hiding this comment

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

Is this also the status in the beginning when nothing has happened yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

If a PVC has not been expanded before, then this field will be nil.

PersistentVolumeClaimNoExpansionInProgress PersistentVolumeClaimResizeStatus = ""
// State set when resize controller starts expanding the volume in control-plane
PersistentVolumeClaimControllerExpansionInProgress PersistentVolumeClaimResizeStatus = "ControllerExpansionInProgress"
// State set when expansion has failed in resize controller with a terminal error.
// Transient errors such as timeout should not set this status and should leave ResizeStatus
// unmodified, so as resize controller can resume the volume expansion.
PersistentVolumeClaimControllerExpansionFailed PersistentVolumeClaimResizeStatus = "ControllerExpansionFailed"
// State set when resize controller has finished expanding the volume but further expansion is needed on the node.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe mention kubelet has not yet started node expansion to differentiate from NodeExpansionInProgress?

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 think that current comment is accurate, in the sense that resize controller in control-plane is finished with the work but mentioning the fact that "kubelet has not yet started node expansion" borders into an implementation detail which is subject to change and I am not comfortable putting it in API docs. What do you think?

PersistentVolumeClaimNodeExpansionPending PersistentVolumeClaimResizeStatus = "NodeExpansionPending"
// State set when kubelet starts expanding the volume.
PersistentVolumeClaimNodeExpansionInProgress PersistentVolumeClaimResizeStatus = "NodeExpansionInProgress"
// State set when expansion has failed in kubelet with a terminal error. Transient errors don't set NodeExpansionFailed.
PersistentVolumeClaimNodeExpansionFailed PersistentVolumeClaimResizeStatus = "NodeExpansionFailed"
)

// PersistentVolumeClaimCondition represents the current condition of PV claim
type PersistentVolumeClaimCondition struct {
Type PersistentVolumeClaimConditionType
Expand Down Expand Up @@ -513,6 +536,24 @@ type PersistentVolumeClaimStatus struct {
Capacity ResourceList
// +optional
Conditions []PersistentVolumeClaimCondition
// The storage resource within AllocatedResources tracks the capacity allocated to a PVC. It may
// be larger than the actual capacity when a volume expansion operation is requested.
// For storage quota, the larger value from allocatedResources and PVC.spec.resources is used.
// If allocatedResources is not set, PVC.spec.resources alone is used for quota calculation.
// If a volume expansion capacity request is lowered, allocatedResources is only
// lowered if there are no expansion operations in progress and if the actual volume capacity
// is equal or lower than the requested capacity.
jsafrane marked this conversation as resolved.
Show resolved Hide resolved
// This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature.
// +featureGate=RecoverVolumeExpansionFailure
// +optional
AllocatedResources ResourceList
// ResizeStatus stores status of resize operation.
// ResizeStatus is not set by default but when expansion is complete resizeStatus is set to empty
// string by resize controller or kubelet.
// This is an alpha field and requires enabling RecoverVolumeExpansionFailure feature.
// +featureGate=RecoverVolumeExpansionFailure
// +optional
ResizeStatus *PersistentVolumeClaimResizeStatus
}

// PersistentVolumeAccessMode defines various access modes for PV.
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/core/v1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pkg/apis/core/v1/zz_generated.defaults.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.