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.

Sign up for GitHub

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

Jump to bottom

Add VolumeBindingMode to StorageClass API #54436

Merged
merged 3 commits into from
Nov 15, 2017
Merged

Add VolumeBindingMode to StorageClass API #54436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b60bd37
StorageClass API changes for VolumeBindingMode
msau42 Oct 4, 2017
86dd5e8
generated files
msau42 Nov 6, 2017
207d13f
Add defaulting to VolumeBindingMode
msau42 Nov 10, 2017
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
8 changes: 8 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions api/swagger-spec/storage.k8s.io_v1.json
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions api/swagger-spec/storage.k8s.io_v1beta1.json
View file
Open in desktop

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

11 changes: 11 additions & 0 deletions docs/api-reference/storage.k8s.io/v1/definitions.html
View file
Open in desktop

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

11 changes: 11 additions & 0 deletions docs/api-reference/storage.k8s.io/v1beta1/definitions.html
View file
Open in desktop

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

2 changes: 0 additions & 2 deletions hack/.golint_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,8 @@ pkg/apis/settings
pkg/apis/settings/v1alpha1
pkg/apis/storage
pkg/apis/storage/util
pkg/apis/storage/v1
pkg/apis/storage/v1/util
pkg/apis/storage/v1alpha1
pkg/apis/storage/v1beta1
pkg/apis/storage/v1beta1/util
pkg/auth/authorizer/abac
pkg/capabilities
Expand Down
22 changes: 22 additions & 0 deletions pkg/apis/storage/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ type StorageClass struct {
// for all PVs created from this storageclass.
// +optional
AllowVolumeExpansion *bool

// VolumeBindingMode indicates how PersistentVolumeClaims should be
// provisioned and bound. When unset, VolumeBindingImmediate is used.
// This field is alpha-level and is only honored by servers that enable
// the VolumeScheduling feature.
// +optional
VolumeBindingMode *VolumeBindingMode
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down Expand Up @@ -187,3 +194,18 @@ type VolumeError struct {
// +optional
Message string
}

// VolumeBindingMode indicates how PersistentVolumeClaims should be bound.
type VolumeBindingMode string

const (
// VolumeBindingImmediate indicates that PersistentVolumeClaims should be
// immediately provisioned and bound.
VolumeBindingImmediate VolumeBindingMode = "Immediate"

// VolumeBindingWaitForFirstConsumer indicates that PersistentVolumeClaims
// should not be provisioned and bound until the first Pod is created that
// references the PeristentVolumeClaim. The volume provisioning and
// binding will occur during Pod scheduing.
VolumeBindingWaitForFirstConsumer VolumeBindingMode = "WaitForFirstConsumer"
)
24 changes: 22 additions & 2 deletions pkg/apis/storage/util/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,22 @@ package(default_visibility = ["//visibility:public"])
load(
"@io_bazel_rules_go//go:def.bzl",
"go_library",
"go_test",
)

go_library(
name = "go_default_library",
srcs = ["helpers.go"],
srcs = [
"helpers.go",
"util.go",
],
importpath = "k8s.io/kubernetes/pkg/apis/storage/util",
deps = ["//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library"],
deps = [
"//pkg/apis/storage:go_default_library",
"//pkg/features:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

filegroup(
Expand All @@ -24,3 +33,14 @@ filegroup(
srcs = [":package-srcs"],
tags = ["automanaged"],
)

go_test(
name = "go_default_test",
srcs = ["util_test.go"],
importpath = "k8s.io/kubernetes/pkg/apis/storage/util",
library = ":go_default_library",
deps = [
"//pkg/apis/storage:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)
30 changes: 30 additions & 0 deletions pkg/apis/storage/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
Copyright 2017 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/apis/storage"
"k8s.io/kubernetes/pkg/features"
)

// DropDisabledAlphaFields removes disabled fields from the StorageClass object.
func DropDisabledAlphaFields(class *storage.StorageClass) {
if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
class.VolumeBindingMode = nil
}
}
52 changes: 52 additions & 0 deletions pkg/apis/storage/util/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
Copyright 2017 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"testing"

utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/apis/storage"
)

func TestDropAlphaFields(t *testing.T) {
bindingMode := storage.VolumeBindingWaitForFirstConsumer

// Test that field gets dropped when feature gate is not set
class := &storage.StorageClass{
VolumeBindingMode: &bindingMode,
}
DropDisabledAlphaFields(class)
if class.VolumeBindingMode != nil {
t.Errorf("VolumeBindingMode field didn't get dropped: %+v", class.VolumeBindingMode)
}

// Test that field does not get dropped when feature gate is set
class = &storage.StorageClass{
VolumeBindingMode: &bindingMode,
}
if err := utilfeature.DefaultFeatureGate.Set("VolumeScheduling=true"); err != nil {
t.Fatalf("Failed to set feature gate for VolumeScheduling: %v", err)
}
DropDisabledAlphaFields(class)
if class.VolumeBindingMode != &bindingMode {
t.Errorf("VolumeBindingMode field got unexpectantly modified: %+v", class.VolumeBindingMode)
}
if err := utilfeature.DefaultFeatureGate.Set("VolumeScheduling=false"); err != nil {
t.Fatalf("Failed to disable feature gate for VolumeScheduling: %v", err)
}
}
16 changes: 16 additions & 0 deletions pkg/apis/storage/v1/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
load(
"@io_bazel_rules_go//go:def.bzl",
"go_library",
"go_test",
)

go_library(
Expand All @@ -18,11 +19,13 @@ go_library(
deps = [
"//pkg/apis/core:go_default_library",
"//pkg/apis/storage:go_default_library",
"//pkg/features:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/api/storage/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

Expand All @@ -41,3 +44,16 @@ filegroup(
],
tags = ["automanaged"],
)

go_test(
name = "go_default_xtest",
srcs = ["defaults_test.go"],
importpath = "k8s.io/kubernetes/pkg/apis/storage/v1_test",
deps = [
"//pkg/api/legacyscheme:go_default_library",
"//pkg/apis/storage/install:go_default_library",
"//vendor/k8s.io/api/storage/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)
7 changes: 7 additions & 0 deletions pkg/apis/storage/v1/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/runtime"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/features"
)

func addDefaultingFuncs(scheme *runtime.Scheme) error {
Expand All @@ -31,4 +33,9 @@ func SetDefaults_StorageClass(obj *storagev1.StorageClass) {
obj.ReclaimPolicy = new(v1.PersistentVolumeReclaimPolicy)
*obj.ReclaimPolicy = v1.PersistentVolumeReclaimDelete
}

if obj.VolumeBindingMode == nil && utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't work with upgrades. Need to discuss with @kubernetes/sig-api-machinery-pr-reviews if it's possible to default new alpha fields.

Copy link
Member

Choose a reason for hiding this comment

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

When gate disabled, they should not be persisted or returned from the API, so… not really

Copy link
Member Author

@msau42 msau42 Nov 13, 2017 •
edited

Choose a reason for hiding this comment

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

Discussed with @thockin and @lavalamp and verified that defaults are applied on read to objects that were stored with a nil field. So we can default new fields under a feature gate as long as we also access the fields under the feature gate.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't that mean the default values are serialized back out and returned to API clients even when the gate is disabled? If so, that isn't what we want

Copy link
Member

@liggitt liggitt Nov 13, 2017 •
edited

Choose a reason for hiding this comment

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

So we can default new fields under a feature gate

Ah, you're saying the defaulting itself would be gated. I thought I looked into that at one point and it resulted in weird (and possibly circular) dependencies, but maybe I'm misremembering

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a scenario I'm not sure how to handle:

  1. 1.9 feature disabled, create new object, nil field not defaulted
  2. 1.10 feature is enabled, update object, defaulting will kick in. How does validation detect the difference between nil field -> defaulting vs nil field -> user set some value. I want this field to be immutable.

Copy link
Member

Choose a reason for hiding this comment

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

How does validation detect the difference between nil field -> defaulting vs nil field -> user set some value

it doesn't (and shouldn't). the old object decoded from etcd will get the default, get sent to the user, and any update coming from the user without that value will also get the default. from validation's perspective, both the old and new object will have the default value, so the field will not appear to have changed.

obj.VolumeBindingMode = new(storagev1.VolumeBindingMode)
*obj.VolumeBindingMode = storagev1.VolumeBindingImmediate
}
}