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

GCE and AWS provisioners, dynamic provisioning: admins can configure zone(s) where PVs shall be created #38505

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
10 changes: 6 additions & 4 deletions examples/persistent-volume-provisioning/README.md
Expand Up @@ -25,12 +25,13 @@ metadata:
provisioner: kubernetes.io/aws-ebs
parameters:
type: io1
zone: us-east-1d
zones: us-east-1d, us-east-1c
iopsPerGB: "10"
```

* `type`: `io1`, `gp2`, `sc1`, `st1`. See AWS docs for details. Default: `gp2`.
* `zone`: AWS zone. If not specified, a random zone from those where Kubernetes cluster has a node is chosen.
* `zone`: AWS zone. If neither zone nor zones is specified, volumes are generally round-robin-ed across all active zones where Kubernetes cluster has a node. Note: zone and zones parameters must not be used at the same time.
* `zones`: a comma separated list of AWS zone(s). If neither zone nor zones is specified, volumes are generally round-robin-ed across all active zones where Kubernetes cluster has a node. Note: zone and zones parameters must not be used at the same time.
* `iopsPerGB`: only for `io1` volumes. I/O operations per second per GiB. AWS volume plugin multiplies this with size of requested volume to compute IOPS of the volume and caps it at 20 000 IOPS (maximum supported by AWS, see AWS docs).
* `encrypted`: denotes whether the EBS volume should be encrypted or not. Valid values are `true` or `false`.
* `kmsKeyId`: optional. The full Amazon Resource Name of the key to use when encrypting the volume. If none is supplied but `encrypted` is true, a key is generated by AWS. See AWS docs for valid ARN value.
Expand All @@ -45,11 +46,12 @@ metadata:
provisioner: kubernetes.io/gce-pd
parameters:
type: pd-standard
zone: us-central1-a
zones: us-central1-a, us-central1-b
```

* `type`: `pd-standard` or `pd-ssd`. Default: `pd-ssd`
* `zone`: GCE zone. If not specified, a random zone in the same region as controller-manager will be chosen.
* `zone`: GCE zone. If neither zone nor zones is specified, volumes are generally round-robin-ed across all active zones where Kubernetes cluster has a node. Note: zone and zones parameters must not be used at the same time.
* `zones`: a comma separated list of GCE zone(s). If neither zone nor zones is specified, volumes are generally round-robin-ed across all active zones where Kubernetes cluster has a node. Note: zone and zones parameters must not be used at the same time.

#### vSphere

Expand Down
30 changes: 23 additions & 7 deletions pkg/cloudprovider/providers/aws/aws.go
Expand Up @@ -287,11 +287,14 @@ const (

// VolumeOptions specifies capacity and tags for a volume.
type VolumeOptions struct {
CapacityGB int
Tags map[string]string
PVCName string
VolumeType string
AvailabilityZone string
CapacityGB int
Tags map[string]string
PVCName string
VolumeType string
ZonePresent bool
Copy link
Member

Choose a reason for hiding this comment

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

Do you need these two booleans? Can you just infer from if the string is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, I can approach it using one of the below ways:

A, riskier: infer from the empty string AvailabilityZone or AvailabilityZones that the optional parameter zone or zones wasn't present. As the Persistent Volume Claim parameters are provided as map[string]string the values "zone": "" or "zones": "" are perfectly valid values in Go lang even though K8s yaml file parsing shall never provide such values. So in case I relay on the K8s yaml file parsing I can use the AvailabilityZone or AvailabilityZones empty string values as an indication that the zone or zones parameter were not specified in a Persistent Volume Claim. However, if my code is "fed" with unexpected input the incorrect input value "zone": "" or "zones": "" will remain undetected.

B, defensive: use a dedicated booleans to indicate whether the zone or zones optional parameters were present or not.

I chose the defensive approach.

Copy link
Member

Choose a reason for hiding this comment

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

So if I understand correctly, zone or zones == "" is invalid, and should return an error, while zone && zones == nil is valid. Can you add unit tests for all these permutations of zone/zones == nil, "", and value?

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 check for zones == "" was already present in the func ZonesToSet.

I added func ValidateZone to check for zone == "".

ZonesPresent bool
AvailabilityZone string
AvailabilityZones string
// IOPSPerGB x CapacityGB will give total IOPS of the volume to create.
// Calculated total IOPS will be capped at MaxTotalIOPS.
IOPSPerGB int
Expand Down Expand Up @@ -1675,10 +1678,23 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (KubernetesVolumeID, er
return "", fmt.Errorf("error querying for all zones: %v", err)
}

createAZ := volumeOptions.AvailabilityZone
if createAZ == "" {
var createAZ string
if !volumeOptions.ZonePresent && !volumeOptions.ZonesPresent {
createAZ = volume.ChooseZoneForVolume(allZones, volumeOptions.PVCName)
}
if !volumeOptions.ZonePresent && volumeOptions.ZonesPresent {
if adminSetOfZones, err := volume.ZonesToSet(volumeOptions.AvailabilityZones); err != nil {
return "", err
} else {
createAZ = volume.ChooseZoneForVolume(adminSetOfZones, volumeOptions.PVCName)
}
}
if volumeOptions.ZonePresent && !volumeOptions.ZonesPresent {
if err := volume.ValidateZone(volumeOptions.AvailabilityZone); err != nil {
return "", err
}
createAZ = volumeOptions.AvailabilityZone
}

var createType string
var iops int64
Expand Down
10 changes: 10 additions & 0 deletions pkg/volume/aws_ebs/aws_util.go
Expand Up @@ -91,12 +91,18 @@ func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (aws.K
}
// Apply Parameters (case-insensitive). We leave validation of
// the values to the cloud provider.
volumeOptions.ZonePresent = false
volumeOptions.ZonesPresent = false
for k, v := range c.options.Parameters {
switch strings.ToLower(k) {
case "type":
volumeOptions.VolumeType = v
case "zone":
volumeOptions.ZonePresent = true
volumeOptions.AvailabilityZone = v
case "zones":
volumeOptions.ZonesPresent = true
volumeOptions.AvailabilityZones = v
case "iopspergb":
volumeOptions.IOPSPerGB, err = strconv.Atoi(v)
if err != nil {
Expand All @@ -114,6 +120,10 @@ func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (aws.K
}
}

if volumeOptions.ZonePresent && volumeOptions.ZonesPresent {
return "", 0, nil, fmt.Errorf("both zone and zones StorageClass parameters must not be used at the same time")
}

// TODO: implement PVC.Selector parsing
if c.options.PVC.Spec.Selector != nil {
return "", 0, nil, fmt.Errorf("claim.Spec.Selector is not supported for dynamic provisioning on AWS")
Expand Down
36 changes: 29 additions & 7 deletions pkg/volume/gce_pd/gce_util.go
Expand Up @@ -86,33 +86,55 @@ func (gceutil *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner) (strin
// Apply Parameters (case-insensitive). We leave validation of
// the values to the cloud provider.
diskType := ""
zone := ""
configuredZone := ""
configuredZones := ""
zonePresent := false
zonesPresent := false
for k, v := range c.options.Parameters {
switch strings.ToLower(k) {
case "type":
diskType = v
case "zone":
zone = v
zonePresent = true
configuredZone = v
case "zones":
zonesPresent = true
configuredZones = v
default:
return "", 0, nil, fmt.Errorf("invalid option %q for volume plugin %s", k, c.plugin.GetPluginName())
}
}

if zonePresent && zonesPresent {
return "", 0, nil, fmt.Errorf("both zone and zones StorageClass parameters must not be used at the same time")
}

// TODO: implement PVC.Selector parsing
if c.options.PVC.Spec.Selector != nil {
return "", 0, nil, fmt.Errorf("claim.Spec.Selector is not supported for dynamic provisioning on GCE")
}

if zone == "" {
// No zone specified, choose one randomly in the same region as the
// node is running.
zones, err := cloud.GetAllZones()
var zones sets.String
if !zonePresent && !zonesPresent {
zones, err = cloud.GetAllZones()
if err != nil {
glog.V(2).Infof("error getting zone information from GCE: %v", err)
return "", 0, nil, err
}
zone = volume.ChooseZoneForVolume(zones, c.options.PVC.Name)
}
if !zonePresent && zonesPresent {
if zones, err = volume.ZonesToSet(configuredZones); err != nil {
return "", 0, nil, err
}
}
if zonePresent && !zonesPresent {
if err := volume.ValidateZone(configuredZone); err != nil {
return "", 0, nil, err
}
zones = make(sets.String)
zones.Insert(configuredZone)
}
zone := volume.ChooseZoneForVolume(zones, c.options.PVC.Name)

err = cloud.CreateDisk(name, diskType, zone, int64(requestGB), *c.options.CloudTags)
if err != nil {
Expand Down
24 changes: 24 additions & 0 deletions pkg/volume/util.go
Expand Up @@ -410,3 +410,27 @@ func JoinMountOptions(userOptions []string, systemOptions []string) []string {
}
return allMountOptions.UnsortedList()
}

// ZonesToSet converts a string containing a comma separated list of zones to set
func ZonesToSet(zonesString string) (sets.String, error) {
zonesSlice := strings.Split(zonesString, ",")
zonesSet := make(sets.String)
for _, zone := range zonesSlice {
trimmedZone := strings.TrimSpace(zone)
Copy link
Member

Choose a reason for hiding this comment

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

If we defined it as a space separated string, we could just use strings.Fields() and maybe not need a helper function at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary to convert the input string with list of zones to a set because the func ChooseZoneForVolume requires a set as one of the input parameters. As the strings.Fields converts a string to a slice a helper function will be needed anyway. However, the line trimmedZone := strings.TrimSpace(zone) could be deleted if strings.Fields is used.
IMHO, comma separated list feels better than a space separated list. That's why I prefer to keep it as it is.

Copy link
Member

Choose a reason for hiding this comment

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

ack

if trimmedZone == "" {
return make(sets.String), fmt.Errorf("comma separated list of zones (%q) must not contain an empty zone", zonesString)
}
zonesSet.Insert(trimmedZone)
}
return zonesSet, nil
}

// ValidateZone returns:
// - an error in case zone is an empty string or contains only any combination of spaces and tab characters
// - nil otherwise
func ValidateZone(zone string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we in-line this at call-sites? It's sort of silly to out-of-line this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, it's a matter of subjective preference.
I prefer to express my intention at call-sites rather than to in-line the implementation at call-sites even though the current implementation of the validation is trivial.
In addition, the function ValidateZone is not called at only 1 place.
That's why I prefer to keep it as it is.

if strings.TrimSpace(zone) == "" {
return fmt.Errorf("the provided %q zone is not valid, it's an empty string or contains only spaces and tab characters", zone)
}
return nil
}
54 changes: 54 additions & 0 deletions pkg/volume/util_test.go
Expand Up @@ -523,3 +523,57 @@ func TestChooseZoneForVolume(t *testing.T) {
}
}
}

func TestZonesToSet(t *testing.T) {
functionUnderTest := "ZonesToSet"
// First part: want an error
sliceOfZones := []string{"", ",", "us-east-1a, , us-east-1d", ", us-west-1b", "us-west-2b,"}
for _, zones := range sliceOfZones {
if got, err := ZonesToSet(zones); err == nil {
t.Errorf("%v(%v) returned (%v), want (%v)", functionUnderTest, zones, got, "an error")
}
}

// Second part: want no error
tests := []struct {
zones string
want sets.String
}{
{
zones: "us-east-1a",
want: sets.String{"us-east-1a": sets.Empty{}},
},
{
zones: "us-east-1a, us-west-2a",
want: sets.String{
"us-east-1a": sets.Empty{},
"us-west-2a": sets.Empty{},
},
},
}
for _, tt := range tests {
if got, err := ZonesToSet(tt.zones); err != nil || !got.Equal(tt.want) {
t.Errorf("%v(%v) returned (%v), want (%v)", functionUnderTest, tt.zones, got, tt.want)
}
}
}

func TestValidateZone(t *testing.T) {
functionUnderTest := "ValidateZone"

// First part: want an error
errCases := []string{"", " "}
for _, errCase := range errCases {
if got := ValidateZone(errCase); got == nil {
t.Errorf("%v(%v) returned (%v), want (%v)", functionUnderTest, errCase, got, "an error")
}
}

// Second part: want no error
succCases := []string{" us-east-1a "}
for _, succCase := range succCases {
if got := ValidateZone(succCase); got != nil {
t.Errorf("%v(%v) returned (%v), want (%v)", functionUnderTest, succCase, got, nil)
}
}
}