Skip to content

Commit

Permalink
Allow volume attach limit overwrite via command line parameter
Browse files Browse the repository at this point in the history
  • Loading branch information
rfranzke committed Jun 8, 2020
1 parent c972094 commit 426c3e5
Show file tree
Hide file tree
Showing 19 changed files with 251 additions and 100 deletions.
3 changes: 2 additions & 1 deletion cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ func main() {
fs := flag.NewFlagSet("aws-ebs-csi-driver", flag.ExitOnError)
options := GetOptions(fs)

drv, err := driver.NewDriver(
drv, err := driver.New(
driver.WithEndpoint(options.ServerOptions.Endpoint),
driver.WithExtraVolumeTags(options.ControllerOptions.ExtraVolumeTags),
driver.WithMode(options.DriverMode),
driver.WithVolumeAttachLimitOverwrite(options.NodeOptions.VolumeAttachLimitOverwrite),
)
if err != nil {
klog.Fatalln(err)
Expand Down
4 changes: 2 additions & 2 deletions cmd/options/controller_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ type ControllerOptions struct {
ExtraVolumeTags map[string]string
}

func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) {
fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
func (o *ControllerOptions) AddFlags(fs *flag.FlagSet) {
fs.Var(cliflag.NewMapStringString(&o.ExtraVolumeTags), "extra-volume-tags", "Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
}
13 changes: 11 additions & 2 deletions cmd/options/node_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ import (
)

// NodeOptions contains options and configuration settings for the node service.
type NodeOptions struct{}
type NodeOptions struct {
// VolumeAttachLimitOverwrite specifies the value that shall be reported as "maximum number of attachable volumes"
// in CSINode objects. It is similar to https://kubernetes.io/docs/concepts/storage/storage-limits/#custom-limits
// which allowed administrators to specify custom volume limits. Note that this solution is just a "stop-gap"
// solution until a more sophisticated solution can be implemented (dynamically discovering the maximum number of
// attachable volume per EC2 machine type, see also https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/347).
VolumeAttachLimitOverwrite int64
}

func (s *NodeOptions) AddFlags(fs *flag.FlagSet) {}
func (o *NodeOptions) AddFlags(fs *flag.FlagSet) {
fs.Int64Var(&o.VolumeAttachLimitOverwrite, "volume-attach-limit-overwrite", -1, "Overwrite value for the maximum number of attachable volumes for all nodes")
}
5 changes: 5 additions & 0 deletions cmd/options/node_options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ func TestNodeOptions(t *testing.T) {
flag string
found bool
}{
{
name: "lookup desired flag",
flag: "volume-attach-limit-overwrite",
found: true,
},
{
name: "fail for non-desired flag",
flag: "some-flag",
Expand Down
4 changes: 2 additions & 2 deletions cmd/options/server_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ type ServerOptions struct {
Endpoint string
}

func (s *ServerOptions) AddFlags(fs *flag.FlagSet) {
fs.StringVar(&s.Endpoint, "endpoint", driver.DefaultCSIEndpoint, "Endpoint for the CSI driver server")
func (o *ServerOptions) AddFlags(fs *flag.FlagSet) {
fs.StringVar(&o.Endpoint, "endpoint", driver.DefaultCSIEndpoint, "Endpoint for the CSI driver server")
}
17 changes: 17 additions & 0 deletions cmd/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"flag"
"os"
"reflect"
"strconv"
"testing"

"github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver"
Expand All @@ -45,6 +46,9 @@ func TestGetOptions(t *testing.T) {
extraVolumeTagKey: extraVolumeTagValue,
}

volumeAttachLimitOverwriteFlagName := "volume-attach-limit-overwrite"
var volumeAttachLimitOverwrite int64 = 42

args := append([]string{
"aws-ebs-csi-driver",
}, additionalArgs...)
Expand All @@ -55,6 +59,9 @@ func TestGetOptions(t *testing.T) {
if withControllerOptions {
args = append(args, "-"+extraVolumeTagsFlagName+"="+extraVolumeTagKey+"="+extraVolumeTagValue)
}
if withNodeOptions {
args = append(args, "-"+volumeAttachLimitOverwriteFlagName+"="+strconv.FormatInt(volumeAttachLimitOverwrite, 10))
}

oldArgs := os.Args
defer func() { os.Args = oldArgs }()
Expand Down Expand Up @@ -82,6 +89,16 @@ func TestGetOptions(t *testing.T) {
}
}

if withNodeOptions {
volumeAttachLimitOverwriteFlag := flagSet.Lookup(volumeAttachLimitOverwriteFlagName)
if volumeAttachLimitOverwriteFlag == nil {
t.Fatalf("expected %q flag to be added but it is not", volumeAttachLimitOverwriteFlagName)
}
if options.NodeOptions.VolumeAttachLimitOverwrite != volumeAttachLimitOverwrite {
t.Fatalf("expected volumeAttachLimitOverwrite to be %d but it is %d", volumeAttachLimitOverwrite, options.NodeOptions.VolumeAttachLimitOverwrite)
}
}

return options
}

Expand Down
1 change: 1 addition & 0 deletions deploy/kubernetes/base/node.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ spec:
args:
- node
- --endpoint=$(CSI_ENDPOINT)
# - --volume-attach-limit-overwrite=42
- --logtostderr
- --v=5
env:
Expand Down
11 changes: 11 additions & 0 deletions docs/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,14 @@ Example 2: `AWS_REGION=us-west-1 /bin/aws-ebs-csi-driver controller --extra-volu

- `node`: This will only start the node service of the CSI driver.\
Example: `/bin/aws-ebs-csi-driver node --endpoint=unix://...`

## Custom volume limits

For the Kubernetes in-tree volume provisioners (including the `kubernetes.io/aws-ebs` provisioner) it was possible for administrators to provide a custom volume limit overwrite (see https://kubernetes.io/docs/concepts/storage/storage-limits/#custom-limits).
This solution is not working with CSI any longer.
As part of [#347](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/347) we discuss how we can implement a sophisticated computation of the volume attach limit per node (e.g., based on the used machine types or already attached network interfaces).
However, it turns out that such optimal implementation is not easily achievable.
In order to allow existing clusters that are leveraging/relying on this feature to migrate to CSI, the EBS CSI driver is supporting the `--volume-attach-limit-overwrite` flag.
It works similar to earlier supported the `KUBE_MAX_PD_VOLS` environment variable.
If provided, the EBS driver will report the configured value as "maximum number of attachable volumes" in the `CSINode` objects during driver registration.
Please consider using this flag as "stop gap" solution that is probably going to be replaced in the future with a more intelligent way of getting to know the attachment limits.
4 changes: 2 additions & 2 deletions pkg/driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ var (
// controllerService represents the controller service of CSI driver
type controllerService struct {
cloud cloud.Cloud
driverOptions *DriverOptions
driverOptions *Options
}

var (
Expand All @@ -68,7 +68,7 @@ var (

// newControllerService creates a new controller service
// it panics if failed to create the service
func newControllerService(driverOptions *DriverOptions) controllerService {
func newControllerService(driverOptions *Options) controllerService {
region := os.Getenv("AWS_REGION")
if region == "" {
metadata, err := NewMetadataFunc()
Expand Down
Loading

0 comments on commit 426c3e5

Please sign in to comment.