From 4497e74e5f531d39264c649dd6479a331e2e7e7f Mon Sep 17 00:00:00 2001 From: torredil Date: Tue, 9 Apr 2024 18:37:42 +0000 Subject: [PATCH] Address feedback Signed-off-by: torredil --- cmd/main.go | 4 +- pkg/driver/options.go | 97 ++++++++++++++++++------- pkg/driver/options_test.go | 143 +++++++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 30 deletions(-) create mode 100644 pkg/driver/options_test.go diff --git a/cmd/main.go b/cmd/main.go index fa68fb000f..3228adaba0 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -36,7 +36,6 @@ import ( ) var ( - osExit = os.Exit featureGate = featuregate.NewFeatureGate() ) @@ -104,7 +103,7 @@ func main() { klog.FlushAndExit(klog.ExitFlushTimeout, 1) } fmt.Println(versionInfo) - osExit(0) + os.Exit(0) } if *toStderr { @@ -148,7 +147,6 @@ func main() { region = metadata.GetRegion() } - klog.InfoS("batching", "status", options.Batching) cloud, err := cloud.NewCloud(region, options.AwsSdkDebugLog, options.UserAgentExtra, options.Batching) if err != nil { klog.ErrorS(err, "failed to create cloud service") diff --git a/pkg/driver/options.go b/pkg/driver/options.go index 45d185747b..1c9ab8fd36 100644 --- a/pkg/driver/options.go +++ b/pkg/driver/options.go @@ -24,37 +24,80 @@ import ( cliflag "k8s.io/component-base/cli/flag" ) +// Options contains options and configuration settings for the driver. type Options struct { - Mode Mode - Endpoint string - HttpEndpoint string - EnableOtelTracing bool - ExtraTags map[string]string - ExtraVolumeTags map[string]string - KubernetesClusterID string - AwsSdkDebugLog bool - WarnOnInvalidTag bool - UserAgentExtra string - Batching bool + Mode Mode + + // #### Server options #### + + //Endpoint is the endpoint for the CSI driver server + Endpoint string + // HttpEndpoint is the TCP network address where the HTTP server for metrics will listen + HttpEndpoint string + // EnableOtelTracing is a flag to enable opentelemetry tracing for the driver + EnableOtelTracing bool + + // #### Controller options #### + + // ExtraTags is a map of tags that will be attached to each dynamically provisioned + // resource. + ExtraTags map[string]string + // ExtraVolumeTags is a map of tags that will be attached to each dynamically provisioned + // volume. + // DEPRECATED: Use ExtraTags instead. + ExtraVolumeTags map[string]string + // ID of the kubernetes cluster. + KubernetesClusterID string + // flag to enable sdk debug log + AwsSdkDebugLog bool + // flag to warn on invalid tag, instead of returning an error + WarnOnInvalidTag bool + // flag to set user agent + UserAgentExtra string + // flag to enable batching of API calls + Batching bool + // flag to set the timeout for volume modification requests to be coalesced into a single + // volume modification call to AWS. ModifyVolumeRequestHandlerTimeout time.Duration - VolumeAttachLimit int64 - ReservedVolumeAttachments int + + // #### Node options ##### + + // VolumeAttachLimit 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 by configuring the kube-scheduler. Also, each AWS + // machine type has different volume limits. By default, the EBS CSI driver parses the machine type name and then + // decides the volume limit. However, this is only a rough approximation and not good enough in most cases. + // Specifying the volume attach limit via command line is the alternative until a more sophisticated solution presents + // itself (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). + VolumeAttachLimit int64 + // ReservedVolumeAttachments specifies number of volume attachments reserved for system use. + // Typically 1 for the root disk, but may be larger when more system disks are attached to nodes. + // This option is not used when --volume-attach-limit is specified. + // When -1, the amount of reserved attachments is loaded from instance metadata that captured state at node boot + // and may include not only system disks but also CSI volumes (and therefore it may be wrong). + ReservedVolumeAttachments int } -func (o *Options) AddFlags(fs *flag.FlagSet) { - fs.StringVar(&o.Endpoint, "endpoint", DefaultCSIEndpoint, "Endpoint for the CSI driver server") - fs.StringVar(&o.HttpEndpoint, "http-endpoint", "", "The TCP network address where the HTTP server for metrics will listen (example: `:8080`). The default is empty string, which means the server is disabled.") - fs.BoolVar(&o.EnableOtelTracing, "enable-otel-tracing", false, "To enable opentelemetry tracing for the driver. The tracing is disabled by default. Configure the exporter endpoint with OTEL_EXPORTER_OTLP_ENDPOINT and other env variables, see https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration.") - fs.Var(cliflag.NewMapStringString(&o.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '=,='") - fs.Var(cliflag.NewMapStringString(&o.ExtraVolumeTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '=,='") - fs.StringVar(&o.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).") - fs.BoolVar(&o.AwsSdkDebugLog, "aws-sdk-debug-log", false, "To enable the aws sdk debug log level (default to false).") - fs.BoolVar(&o.WarnOnInvalidTag, "warn-on-invalid-tag", false, "To warn on invalid tags, instead of returning an error") - fs.StringVar(&o.UserAgentExtra, "user-agent-extra", "", "Extra string appended to user agent.") - fs.BoolVar(&o.Batching, "batching", false, "To enable batching of API calls. This is especially helpful for improving performance in workloads that are sensitive to EC2 rate limits.") - fs.DurationVar(&o.ModifyVolumeRequestHandlerTimeout, "modify-volume-request-handler-timeout", DefaultModifyVolumeRequestHandlerTimeout, "Timeout for the window in which volume modification calls must be received in order for them to coalesce into a single volume modification call to AWS. This must be lower than the csi-resizer and volumemodifier timeouts") - fs.Int64Var(&o.VolumeAttachLimit, "volume-attach-limit", -1, "Value for the maximum number of volumes attachable per node. If specified, the limit applies to all nodes and overrides --reserved-volume-attachments. If not specified, the value is approximated from the instance type.") - fs.IntVar(&o.ReservedVolumeAttachments, "reserved-volume-attachments", -1, "Number of volume attachments reserved for system use. Not used when --volume-attach-limit is specified. The total amount of volume attachments for a node is computed as: - - . When -1, the amount of reserved attachments is loaded from instance metadata that captured state at node boot and may include not only system disks but also CSI volumes.") +func (o *Options) AddFlags(f *flag.FlagSet) { + // Server options + f.StringVar(&o.Endpoint, "endpoint", DefaultCSIEndpoint, "Endpoint for the CSI driver server") + f.StringVar(&o.HttpEndpoint, "http-endpoint", "", "The TCP network address where the HTTP server for metrics will listen (example: `:8080`). The default is empty string, which means the server is disabled.") + f.BoolVar(&o.EnableOtelTracing, "enable-otel-tracing", false, "To enable opentelemetry tracing for the driver. The tracing is disabled by default. Configure the exporter endpoint with OTEL_EXPORTER_OTLP_ENDPOINT and other env variables, see https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration.") + + // Controller options + f.Var(cliflag.NewMapStringString(&o.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '=,='") + f.Var(cliflag.NewMapStringString(&o.ExtraVolumeTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '=,='") + f.StringVar(&o.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).") + f.BoolVar(&o.AwsSdkDebugLog, "aws-sdk-debug-log", false, "To enable the aws sdk debug log level (default to false).") + f.BoolVar(&o.WarnOnInvalidTag, "warn-on-invalid-tag", false, "To warn on invalid tags, instead of returning an error") + f.StringVar(&o.UserAgentExtra, "user-agent-extra", "", "Extra string appended to user agent.") + f.BoolVar(&o.Batching, "batching", false, "To enable batching of API calls. This is especially helpful for improving performance in workloads that are sensitive to EC2 rate limits.") + f.DurationVar(&o.ModifyVolumeRequestHandlerTimeout, "modify-volume-request-handler-timeout", DefaultModifyVolumeRequestHandlerTimeout, "Timeout for the window in which volume modification calls must be received in order for them to coalesce into a single volume modification call to AWS. This must be lower than the csi-resizer and volumemodifier timeouts") + + // Node options + f.Int64Var(&o.VolumeAttachLimit, "volume-attach-limit", -1, "Value for the maximum number of volumes attachable per node. If specified, the limit applies to all nodes and overrides --reserved-volume-attachments. If not specified, the value is approximated from the instance type.") + f.IntVar(&o.ReservedVolumeAttachments, "reserved-volume-attachments", -1, "Number of volume attachments reserved for system use. Not used when --volume-attach-limit is specified. The total amount of volume attachments for a node is computed as: - - . When -1, the amount of reserved attachments is loaded from instance metadata that captured state at node boot and may include not only system disks but also CSI volumes.") } func (o *Options) Validate() error { diff --git a/pkg/driver/options_test.go b/pkg/driver/options_test.go new file mode 100644 index 0000000000..1b33c6d482 --- /dev/null +++ b/pkg/driver/options_test.go @@ -0,0 +1,143 @@ +package driver + +import ( + "testing" + "time" + + flag "github.com/spf13/pflag" +) + +func TestAddFlags(t *testing.T) { + o := &Options{} + + f := flag.NewFlagSet("test", flag.ExitOnError) + o.AddFlags(f) + + if err := f.Set("endpoint", "custom-endpoint"); err != nil { + t.Errorf("error setting endpoint: %v", err) + } + if err := f.Set("http-endpoint", ":8080"); err != nil { + t.Errorf("error setting http-endpoint: %v", err) + } + if err := f.Set("enable-otel-tracing", "true"); err != nil { + t.Errorf("error setting enable-otel-tracing: %v", err) + } + if err := f.Set("extra-tags", "key1=value1,key2=value2"); err != nil { + t.Errorf("error setting extra-tags: %v", err) + } + if err := f.Set("k8s-tag-cluster-id", "cluster-123"); err != nil { + t.Errorf("error setting k8s-tag-cluster-id: %v", err) + } + if err := f.Set("aws-sdk-debug-log", "true"); err != nil { + t.Errorf("error setting aws-sdk-debug-log: %v", err) + } + if err := f.Set("warn-on-invalid-tag", "true"); err != nil { + t.Errorf("error setting warn-on-invalid-tag: %v", err) + } + if err := f.Set("user-agent-extra", "extra-info"); err != nil { + t.Errorf("error setting user-agent-extra: %v", err) + } + if err := f.Set("batching", "true"); err != nil { + t.Errorf("error setting batching: %v", err) + } + if err := f.Set("modify-volume-request-handler-timeout", "1m"); err != nil { + t.Errorf("error setting modify-volume-request-handler-timeout: %v", err) + } + if err := f.Set("volume-attach-limit", "10"); err != nil { + t.Errorf("error setting volume-attach-limit: %v", err) + } + if err := f.Set("reserved-volume-attachments", "5"); err != nil { + t.Errorf("error setting reserved-volume-attachments: %v", err) + } + + if o.Endpoint != "custom-endpoint" { + t.Errorf("unexpected Endpoint: got %s, want custom-endpoint", o.Endpoint) + } + if o.HttpEndpoint != ":8080" { + t.Errorf("unexpected HttpEndpoint: got %s, want :8080", o.HttpEndpoint) + } + if !o.EnableOtelTracing { + t.Error("unexpected EnableOtelTracing: got false, want true") + } + if len(o.ExtraTags) != 2 || o.ExtraTags["key1"] != "value1" || o.ExtraTags["key2"] != "value2" { + t.Errorf("unexpected ExtraTags: got %v, want map[key1:value1 key2:value2]", o.ExtraTags) + } + if o.KubernetesClusterID != "cluster-123" { + t.Errorf("unexpected KubernetesClusterID: got %s, want cluster-123", o.KubernetesClusterID) + } + if !o.AwsSdkDebugLog { + t.Error("unexpected AwsSdkDebugLog: got false, want true") + } + if !o.WarnOnInvalidTag { + t.Error("unexpected WarnOnInvalidTag: got false, want true") + } + if o.UserAgentExtra != "extra-info" { + t.Errorf("unexpected UserAgentExtra: got %s, want extra-info", o.UserAgentExtra) + } + if !o.Batching { + t.Error("unexpected Batching: got false, want true") + } + if o.ModifyVolumeRequestHandlerTimeout != time.Minute { + t.Errorf("unexpected ModifyVolumeRequestHandlerTimeout: got %v, want 1m", o.ModifyVolumeRequestHandlerTimeout) + } + if o.VolumeAttachLimit != 10 { + t.Errorf("unexpected VolumeAttachLimit: got %d, want 10", o.VolumeAttachLimit) + } + if o.ReservedVolumeAttachments != 5 { + t.Errorf("unexpected ReservedVolumeAttachments: got %d, want 5", o.ReservedVolumeAttachments) + } +} + +func TestValidate(t *testing.T) { + tests := []struct { + name string + volumeAttachLimit int64 + reservedAttachments int + expectedErr bool + errMsg string + }{ + { + name: "both options not set", + volumeAttachLimit: -1, + reservedAttachments: -1, + expectedErr: false, + }, + { + name: "volumeAttachLimit set", + volumeAttachLimit: 10, + reservedAttachments: -1, + expectedErr: false, + }, + { + name: "reservedVolumeAttachments set", + volumeAttachLimit: -1, + reservedAttachments: 2, + expectedErr: false, + }, + { + name: "both options set", + volumeAttachLimit: 10, + reservedAttachments: 2, + expectedErr: true, + errMsg: "only one of --volume-attach-limit and --reserved-volume-attachments may be specified", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &Options{ + VolumeAttachLimit: tt.volumeAttachLimit, + ReservedVolumeAttachments: tt.reservedAttachments, + } + + err := o.Validate() + if (err != nil) != tt.expectedErr { + t.Errorf("Options.Validate() error = %v, wantErr %v", err, tt.expectedErr) + } + + if err != nil && err.Error() != tt.errMsg { + t.Errorf("Options.Validate() error message = %v, wantErrMsg %v", err.Error(), tt.errMsg) + } + }) + } +}