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

Feature gate for regional PDs #60337

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Feature gate for regional PDs #60337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions pkg/cloudprovider/providers/gce/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ go_library(
"//pkg/cloudprovider/providers/gce/cloud/filter:go_default_library",
"//pkg/cloudprovider/providers/gce/cloud/meta:go_default_library",
"//pkg/controller:go_default_library",
"//pkg/features:go_default_library",
"//pkg/kubelet/apis:go_default_library",
"//pkg/master/ports:go_default_library",
"//pkg/util/net/sets:go_default_library",
Expand Down Expand Up @@ -79,6 +80,7 @@ go_library(
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/k8s.io/client-go/informers:go_default_library",
"//vendor/k8s.io/client-go/kubernetes:go_default_library",
"//vendor/k8s.io/client-go/kubernetes/scheme:go_default_library",
Expand Down
3 changes: 0 additions & 3 deletions pkg/cloudprovider/providers/gce/gce_alpha.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,12 @@ const (
// tier to use. Currently supports "Standard" and "Premium" (default).
AlphaFeatureNetworkTiers = "NetworkTiers"

AlphaFeatureGCEDisk = "DiskAlphaAPI"

Copy link
Member

Choose a reason for hiding this comment

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

Double check removing this feature does not cause any failures if old code still passes the flag in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only spot that passes in an alpha features list that could contain "DiskAlphaAPI", and it just logs the error if the feature doesn't exist.

kubernetes/pkg/cloudprovider/providers/gce/gce.go

Line 294 in 6a7656b

glog.Errorf("Encountered error for creating alpha feature gate: %v", err)

AlphaFeatureNetworkEndpointGroup = "NetworkEndpointGroup"
)

// All known alpha features
var knownAlphaFeatures = map[string]bool{
AlphaFeatureNetworkTiers: true,
AlphaFeatureGCEDisk: true,
AlphaFeatureNetworkEndpointGroup: true,
}

Expand Down
83 changes: 49 additions & 34 deletions pkg/cloudprovider/providers/gce/gce_disks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
computealpha "google.golang.org/api/compute/v0.alpha"
compute "google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/features"
)

type DiskType string
Expand Down Expand Up @@ -143,7 +145,7 @@ func (manager *gceServiceManager) CreateRegionalDiskOnCloudProvider(
diskType string,
replicaZones sets.String) (gceObject, error) {

if manager.gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
Copy link
Member

Choose a reason for hiding this comment

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

Update the error below. It is no longer valid:
The regional PD feature is only available via the GCE Alpha API. Enable \"DiskAlphaAPI\" in the list of \"alpha-features\" in \"gce.conf\" to use the feature.")

Ditto below.

Copy link
Member

Choose a reason for hiding this comment

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

Also since you have a k8s feature gate, maybe you can do the gating at the GCE Volume Plugin layer now instead of in the cloud provider code? Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it's gated at volume plugin layer as well. It's left here as a safeguard.

diskTypeURI, err := manager.getDiskTypeURI(
manager.gce.region /* diskRegion */, multiZone{replicaZones}, diskType, true /* useAlphaAPI */)
if err != nil {
Expand All @@ -166,7 +168,7 @@ func (manager *gceServiceManager) CreateRegionalDiskOnCloudProvider(
manager.gce.projectID, manager.gce.region, diskToCreateAlpha).Do()
}

return nil, fmt.Errorf("The regional PD feature is only available via the GCE Alpha API. Enable \"DiskAlphaAPI\" in the list of \"alpha-features\" in \"gce.conf\" to use the feature.")
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
}

func (manager *gceServiceManager) AttachDiskOnCloudProvider(
Expand Down Expand Up @@ -238,7 +240,7 @@ func (manager *gceServiceManager) GetDiskFromCloudProvider(
func (manager *gceServiceManager) GetRegionalDiskFromCloudProvider(
diskName string) (*GCEDisk, error) {

if manager.gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
diskAlpha, err := manager.gce.serviceAlpha.RegionDisks.Get(
manager.gce.projectID, manager.gce.region, diskName).Do()
if err != nil {
Expand All @@ -260,7 +262,7 @@ func (manager *gceServiceManager) GetRegionalDiskFromCloudProvider(
}, nil
}

return nil, fmt.Errorf("The regional PD feature is only available via the GCE Alpha API. Enable \"DiskAlphaAPI\" in the list of \"alpha-features\" in \"gce.conf\" to use the feature.")
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
}

func (manager *gceServiceManager) DeleteDiskOnCloudProvider(
Expand All @@ -272,12 +274,12 @@ func (manager *gceServiceManager) DeleteDiskOnCloudProvider(

func (manager *gceServiceManager) DeleteRegionalDiskOnCloudProvider(
diskName string) (gceObject, error) {
if manager.gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return manager.gce.serviceAlpha.RegionDisks.Delete(
manager.gce.projectID, manager.gce.region, diskName).Do()
}

return nil, fmt.Errorf("DeleteRegionalDiskOnCloudProvider is a regional PD feature and is only available via the GCE Alpha API. Enable \"DiskAlphaAPI\" in the list of \"alpha-features\" in \"gce.conf\" to use the feature.")
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
}

func (manager *gceServiceManager) WaitForZoneOp(
Expand Down Expand Up @@ -417,13 +419,13 @@ func (manager *gceServiceManager) ResizeDiskOnCloudProvider(disk *GCEDisk, sizeG
}

func (manager *gceServiceManager) RegionalResizeDiskOnCloudProvider(disk *GCEDisk, sizeGb int64) (gceObject, error) {
if manager.gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
resizeServiceRequest := &computealpha.RegionDisksResizeRequest{
SizeGb: sizeGb,
}
return manager.gce.serviceAlpha.RegionDisks.Resize(manager.gce.projectID, disk.Region, disk.Name, resizeServiceRequest).Do()
}
return nil, fmt.Errorf("RegionalResizeDiskOnCloudProvider is a regional PD feature and is only available via the GCE Alpha API. Enable \"DiskAlphaAPI\" in the list of \"alpha-features\" in \"gce.conf\" to use the feature.")
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
}

// Disks is interface for manipulation with GCE PDs.
Expand Down Expand Up @@ -530,7 +532,7 @@ func (gce *GCECloud) AttachDisk(diskName string, nodeName types.NodeName, readOn
// Try fetching as regional PD
var disk *GCEDisk
var mc *metricContext
if regional {
if regional && utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
disk, err = gce.getRegionalDiskByName(diskName)
if err != nil {
glog.V(5).Infof("Could not find regional PD named %q to Attach. Will look for a zonal PD", diskName)
Expand Down Expand Up @@ -795,17 +797,20 @@ func (gce *GCECloud) ResizeDisk(diskToResize string, oldSize resource.Quantity,
}
return newSizeQuant, nil
case multiZone:
mc = newDiskMetricContextRegional("resize", disk.Region)
resizeOp, err := gce.manager.RegionalResizeDiskOnCloudProvider(disk, requestGB)
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
mc = newDiskMetricContextRegional("resize", disk.Region)
resizeOp, err := gce.manager.RegionalResizeDiskOnCloudProvider(disk, requestGB)

if err != nil {
return oldSize, mc.Observe(err)
}
waitErr := gce.manager.WaitForRegionalOp(resizeOp, mc)
if waitErr != nil {
return oldSize, waitErr
if err != nil {
return oldSize, mc.Observe(err)
}
waitErr := gce.manager.WaitForRegionalOp(resizeOp, mc)
if waitErr != nil {
return oldSize, waitErr
}
return newSizeQuant, nil
}
return newSizeQuant, nil
return oldSize, fmt.Errorf("disk.ZoneInfo has unexpected type %T", zoneInfo)
case nil:
return oldSize, fmt.Errorf("PD has nil ZoneInfo: %v", disk)
default:
Expand Down Expand Up @@ -838,19 +843,26 @@ func (gce *GCECloud) GetAutoLabelsForPD(name string, zone string) (map[string]st
// We could assume the disks exists; we have all the information we need
// However it is more consistent to ensure the disk exists,
// and in future we may gather addition information (e.g. disk type, IOPS etc)
zoneSet, err := volumeutil.LabelZonesToSet(zone)
if err != nil {
glog.Warningf("Failed to parse zone field: %q. Will use raw field.", zone)
}

if len(zoneSet) > 1 {
// Regional PD
disk, err = gce.getRegionalDiskByName(name)
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
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 be gating this? At this point the disk has already been created. Doing this will just not report the correct info back to the scheduler. But I can buy the argument, that disabling the feature means disabling all of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this check, a user can manually provision a PV with with zones label zone1__zone2 and still have it point to a regional PD. I don't think we should allow this if the flag gate is unset

Copy link
Member

Choose a reason for hiding this comment

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

Should it fall back to non-regional behavior in that case, or be completely unusable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The else clause here is the non-regional behavior before any regional logic was added

zoneSet, err := volumeutil.LabelZonesToSet(zone)
if err != nil {
return nil, err
glog.Warningf("Failed to parse zone field: %q. Will use raw field.", zone)
}

if len(zoneSet) > 1 {
// Regional PD
disk, err = gce.getRegionalDiskByName(name)
if err != nil {
return nil, err
}
} else {
// Zonal PD
disk, err = gce.getDiskByName(name, zone)
if err != nil {
return nil, err
}
}
} else {
// Zonal PD
disk, err = gce.getDiskByName(name, zone)
if err != nil {
return nil, err
Expand Down Expand Up @@ -936,7 +948,7 @@ func (gce *GCECloud) getRegionalDiskByName(diskName string) (*GCEDisk, error) {
// Prefer getDiskByName, if the zone can be established
// Return cloudprovider.DiskNotFound if the given disk cannot be found in any zone
func (gce *GCECloud) GetDiskByNameUnknownZone(diskName string) (*GCEDisk, error) {
if gce.AlphaFeatureGate.Enabled(AlphaFeatureGCEDisk) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
regionalDisk, err := gce.getRegionalDiskByName(diskName)
if err == nil {
return regionalDisk, err
Expand Down Expand Up @@ -1020,12 +1032,15 @@ func (gce *GCECloud) doDeleteDisk(diskToDelete string) error {
}
return gce.manager.WaitForZoneOp(deleteOp, zoneInfo.zone, mc)
case multiZone:
mc = newDiskMetricContextRegional("delete", disk.Region)
deleteOp, err := gce.manager.DeleteRegionalDiskOnCloudProvider(disk.Name)
if err != nil {
return mc.Observe(err)
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
mc = newDiskMetricContextRegional("delete", disk.Region)
deleteOp, err := gce.manager.DeleteRegionalDiskOnCloudProvider(disk.Name)
if err != nil {
return mc.Observe(err)
}
return gce.manager.WaitForRegionalOp(deleteOp, mc)
}
return gce.manager.WaitForRegionalOp(deleteOp, mc)
return fmt.Errorf("disk.ZoneInfo has unexpected type %T", zoneInfo)
case nil:
return fmt.Errorf("PD has nil ZoneInfo: %v", disk)
default:
Expand Down
5 changes: 0 additions & 5 deletions pkg/cloudprovider/providers/gce/gce_disks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,16 +100,11 @@ func TestCreateRegionalDisk_Basic(t *testing.T) {
gceRegion := "fake-region"
zonesWithNodes := []string{"zone1", "zone3", "zone2"}
fakeManager := newFakeManager(gceProjectId, gceRegion)
alphaFeatureGate, featureGateErr := NewAlphaFeatureGate([]string{AlphaFeatureGCEDisk})

if featureGateErr != nil {
t.Error(featureGateErr)
}
gce := GCECloud{
manager: fakeManager,
managedZones: zonesWithNodes,
projectID: gceProjectId,
AlphaFeatureGate: alphaFeatureGate,
nodeZones: createNodeZones(zonesWithNodes),
nodeInformerSynced: func() bool { return true },
}
Expand Down
7 changes: 7 additions & 0 deletions pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ const (
//
// Enable container log rotation for cri container runtime
CRIContainerLogRotation utilfeature.Feature = "CRIContainerLogRotation"

// owner: @verult
// beta: v1.10
//
// Enables the regional PD feature on GCE.
GCERegionalPersistentDisk utilfeature.Feature = "GCERegionalPersistentDisk"
)

func init() {
Expand Down Expand Up @@ -299,6 +305,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
NoDaemonSetScheduler: {Default: false, PreRelease: utilfeature.Alpha},
TokenRequest: {Default: false, PreRelease: utilfeature.Alpha},
CRIContainerLogRotation: {Default: false, PreRelease: utilfeature.Alpha},
GCERegionalPersistentDisk: {Default: true, PreRelease: utilfeature.Beta},

// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:
Expand Down
2 changes: 2 additions & 0 deletions pkg/volume/gce_pd/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ go_library(
deps = [
"//pkg/cloudprovider:go_default_library",
"//pkg/cloudprovider/providers/gce:go_default_library",
"//pkg/features:go_default_library",
"//pkg/kubelet/apis:go_default_library",
"//pkg/util/mount:go_default_library",
"//pkg/util/strings:go_default_library",
Expand All @@ -31,6 +32,7 @@ go_library(
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//vendor/k8s.io/utils/exec:go_default_library",
],
)
Expand Down
7 changes: 7 additions & 0 deletions pkg/volume/gce_pd/gce_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ import (
"github.com/golang/glog"
"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/cloudprovider"
gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
"k8s.io/kubernetes/pkg/features"
kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis"
"k8s.io/kubernetes/pkg/volume"
volumeutil "k8s.io/kubernetes/pkg/volume/util"
Expand Down Expand Up @@ -109,6 +111,11 @@ func (gceutil *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner) (strin
zonesPresent = true
configuredZones = v
case "replication-type":
if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return "", 0, nil, "",
fmt.Errorf("the %q option for volume plugin %v is only supported with the %q Kubernetes feature gate enabled",
k, c.plugin.GetPluginName(), features.GCERegionalPersistentDisk)
}
replicationType = strings.ToLower(v)
case volume.VolumeParameterFSType:
fstype = v
Expand Down
1 change: 0 additions & 1 deletion test/e2e/e2e.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ func setupProviderConfig() error {

gceAlphaFeatureGate, err := gcecloud.NewAlphaFeatureGate([]string{
gcecloud.AlphaFeatureNetworkEndpointGroup,
gcecloud.AlphaFeatureGCEDisk,
})
if err != nil {
glog.Errorf("Encountered error for creating alpha feature gate: %v", err)
Expand Down