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

move GCERegionalPersistentDisk feature to k8s.io/cloud-provider/features #73604

Merged
merged 1 commit into from
Feb 2, 2019
Merged

move GCERegionalPersistentDisk feature to k8s.io/cloud-provider/features #73604

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: 1 addition & 1 deletion pkg/cloudprovider/providers/gce/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ go_library(
importpath = "k8s.io/kubernetes/pkg/cloudprovider/providers/gce",
deps = [
"//pkg/api/v1/service: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 All @@ -74,6 +73,7 @@ go_library(
"//staging/src/k8s.io/client-go/tools/record:go_default_library",
"//staging/src/k8s.io/client-go/util/flowcontrol:go_default_library",
"//staging/src/k8s.io/cloud-provider:go_default_library",
"//staging/src/k8s.io/cloud-provider/features:go_default_library",
"//vendor/cloud.google.com/go/compute/metadata:go_default_library",
"//vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud:go_default_library",
"//vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/filter:go_default_library",
Expand Down
28 changes: 14 additions & 14 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 @@ -38,8 +38,8 @@ import (
compute "google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
utilfeature "k8s.io/apiserver/pkg/util/feature"
cloudfeatures "k8s.io/cloud-provider/features"
"k8s.io/klog"
"k8s.io/kubernetes/pkg/features"
)

// DiskType defines a specific type for holding disk types (eg. pd-ssd)
Expand Down Expand Up @@ -146,8 +146,8 @@ func (manager *gceServiceManager) CreateRegionalDiskOnCloudProvider(
diskType string,
replicaZones sets.String) error {

if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", cloudfeatures.GCERegionalPersistentDisk)
}

diskTypeURI, err := manager.getDiskTypeURI(
Expand Down Expand Up @@ -247,8 +247,8 @@ func (manager *gceServiceManager) GetDiskFromCloudProvider(
func (manager *gceServiceManager) GetRegionalDiskFromCloudProvider(
diskName string) (*Disk, error) {

if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return nil, fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", cloudfeatures.GCERegionalPersistentDisk)
}

ctx, cancel := cloud.ContextWithCallTimeout()
Expand Down Expand Up @@ -284,8 +284,8 @@ func (manager *gceServiceManager) DeleteDiskOnCloudProvider(
func (manager *gceServiceManager) DeleteRegionalDiskOnCloudProvider(
diskName string) error {

if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", cloudfeatures.GCERegionalPersistentDisk)
}

ctx, cancel := cloud.ContextWithCallTimeout()
Expand Down Expand Up @@ -411,8 +411,8 @@ func (manager *gceServiceManager) ResizeDiskOnCloudProvider(disk *Disk, sizeGb i
}

func (manager *gceServiceManager) RegionalResizeDiskOnCloudProvider(disk *Disk, sizeGb int64) error {
if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", features.GCERegionalPersistentDisk)
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return fmt.Errorf("the regional PD feature is only available with the %s Kubernetes feature gate enabled", cloudfeatures.GCERegionalPersistentDisk)
}

resizeServiceRequest := &compute.RegionDisksResizeRequest{
Expand Down Expand Up @@ -532,7 +532,7 @@ func (g *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bo
// Try fetching as regional PD
var disk *Disk
var mc *metricContext
if regional && utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if regional && utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
disk, err = g.getRegionalDiskByName(diskName)
if err != nil {
return err
Expand Down Expand Up @@ -768,7 +768,7 @@ func (g *Cloud) ResizeDisk(diskToResize string, oldSize resource.Quantity, newSi
}
return newSizeQuant, mc.Observe(err)
case multiZone:
if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return oldSize, fmt.Errorf("disk.ZoneInfo has unexpected type %T", zoneInfo)
}

Expand Down Expand Up @@ -811,7 +811,7 @@ func (g *Cloud) GetAutoLabelsForPD(name string, zone string) (map[string]string,
// 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)
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
zoneSet, err := volumeutil.LabelZonesToSet(zone)
if err != nil {
klog.Warningf("Failed to parse zone field: %q. Will use raw field.", zone)
Expand Down Expand Up @@ -916,7 +916,7 @@ func (g *Cloud) getRegionalDiskByName(diskName string) (*Disk, error) {
// Prefer getDiskByName, if the zone can be established
// Return cloudprovider.DiskNotFound if the given disk cannot be found in any zone
func (g *Cloud) GetDiskByNameUnknownZone(diskName string) (*Disk, error) {
if utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
regionalDisk, err := g.getRegionalDiskByName(diskName)
if err == nil {
return regionalDisk, err
Expand Down Expand Up @@ -996,7 +996,7 @@ func (g *Cloud) doDeleteDisk(diskToDelete string) error {
mc = newDiskMetricContextZonal("delete", disk.Region, zoneInfo.zone)
return mc.Observe(g.manager.DeleteDiskOnCloudProvider(zoneInfo.zone, disk.Name))
case multiZone:
if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.GCERegionalPersistentDisk) {
return fmt.Errorf("disk.ZoneInfo has unexpected type %T", zoneInfo)
}

Expand Down
1 change: 1 addition & 0 deletions pkg/features/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ go_library(
"//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/features:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/cloud-provider/features:go_default_library",
],
)

Expand Down
9 changes: 2 additions & 7 deletions pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features"
genericfeatures "k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
cloudfeatures "k8s.io/cloud-provider/features"
)

const (
Expand Down Expand Up @@ -282,12 +283,6 @@ const (
// Enable container log rotation for cri container runtime
CRIContainerLogRotation utilfeature.Feature = "CRIContainerLogRotation"

// owner: @verult
// GA: v1.13
//
// Enables the regional PD feature on GCE.
GCERegionalPersistentDisk utilfeature.Feature = "GCERegionalPersistentDisk"

// owner: @krmayankk
// alpha: v1.10
//
Expand Down Expand Up @@ -460,7 +455,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
TokenRequestProjection: {Default: true, PreRelease: utilfeature.Beta},
BoundServiceAccountTokenVolume: {Default: false, PreRelease: utilfeature.Alpha},
CRIContainerLogRotation: {Default: true, PreRelease: utilfeature.Beta},
GCERegionalPersistentDisk: {Default: true, PreRelease: utilfeature.GA},
cloudfeatures.GCERegionalPersistentDisk: {Default: true, PreRelease: utilfeature.GA},
CSIMigration: {Default: false, PreRelease: utilfeature.Alpha},
CSIMigrationGCE: {Default: false, PreRelease: utilfeature.Alpha},
CSIMigrationAWS: {Default: false, PreRelease: utilfeature.Alpha},
Expand Down
1 change: 1 addition & 0 deletions pkg/volume/gcepd/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ go_library(
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
"//staging/src/k8s.io/cloud-provider:go_default_library",
"//staging/src/k8s.io/cloud-provider/features:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
"//vendor/k8s.io/utils/exec:go_default_library",
"//vendor/k8s.io/utils/path:go_default_library",
Expand Down
6 changes: 3 additions & 3 deletions pkg/volume/gcepd/gce_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
utilfeature "k8s.io/apiserver/pkg/util/feature"
cloudprovider "k8s.io/cloud-provider"
cloudfeatures "k8s.io/cloud-provider/features"
"k8s.io/klog"
gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
"k8s.io/kubernetes/pkg/features"
kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis"
"k8s.io/kubernetes/pkg/util/mount"
"k8s.io/kubernetes/pkg/volume"
Expand Down Expand Up @@ -127,10 +127,10 @@ func (util *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner, node *v1.
return "", 0, nil, "", err
}
case "replication-type":
if !utilfeature.DefaultFeatureGate.Enabled(features.GCERegionalPersistentDisk) {
if !utilfeature.DefaultFeatureGate.Enabled(cloudfeatures.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)
k, c.plugin.GetPluginName(), cloudfeatures.GCERegionalPersistentDisk)
}
replicationType = strings.ToLower(v)
case volume.VolumeParameterFSType:
Expand Down
1 change: 1 addition & 0 deletions staging/publishing/import-restrictions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@
allowedImports:
- k8s.io/api
- k8s.io/apimachinery
- k8s.io/apiserver
- k8s.io/client-go
- k8s.io/klog

Expand Down
1 change: 1 addition & 0 deletions staging/src/k8s.io/cloud-provider/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ filegroup(
name = "all-srcs",
srcs = [
":package-srcs",
"//staging/src/k8s.io/cloud-provider/features:all-srcs",
"//staging/src/k8s.io/cloud-provider/node:all-srcs",
],
tags = ["automanaged"],
Expand Down
8 changes: 8 additions & 0 deletions staging/src/k8s.io/cloud-provider/Godeps/Godeps.json
View file
Open in desktop

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

24 changes: 24 additions & 0 deletions staging/src/k8s.io/cloud-provider/features/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = ["gce.go"],
importmap = "k8s.io/kubernetes/vendor/k8s.io/cloud-provider/features",
importpath = "k8s.io/cloud-provider/features",
visibility = ["//visibility:public"],
deps = ["//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library"],
)

filegroup(
name = "package-srcs",
srcs = glob(["**"]),
tags = ["automanaged"],
visibility = ["//visibility:private"],
)

filegroup(
name = "all-srcs",
srcs = [":package-srcs"],
tags = ["automanaged"],
visibility = ["//visibility:public"],
)
33 changes: 33 additions & 0 deletions staging/src/k8s.io/cloud-provider/features/gce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
Copyright 2019 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 features

import (
utilfeature "k8s.io/apiserver/pkg/util/feature"
)

// TODO: this file should ideally live in k8s.io/cloud-provider-gcp, but it is
// temporarily placed here to remove dependencies to k8s.io/kubernetes in the
// in-tree GCE cloud provider. Move this to k8s.io/cloud-provider-gcp as soon
// as it's ready to be used
const (
// owner: @verult
// GA: v1.13
//
// Enables the regional PD feature on GCE.
GCERegionalPersistentDisk utilfeature.Feature = "GCERegionalPersistentDisk"
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 understand this is not the ideal place for this, but this would unblock the cloud provider extraction work and I think it's worth the trade-off, happy to consider alternatives though.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

as long as this is just a string and not pulling in GCE specific code, this is good. +1

Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency on k8s.io/apiserver is really not what we want. Can we move the generic feature gate code (not the actual instance) to component-base? /cc @luxas @mtaufen @stewart-yu

Copy link
Member Author

Choose a reason for hiding this comment

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

No objections from me :), also happy to help do the work for this with some guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for delay, back from Spring Festival now.
and no objections 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.

Awesome, thanks for following up :). I'll see if I can get around to this in the next few days

)