Adopting Multiple CSI Driver improvements #309

jacobwolfaws · 2023-05-11T18:04:30Z

Is this a bug fix or adding new feature?
new feature + QOL changes

What is this PR about? / Why do we need it?
This PR is about bring the FSx for Lustre CSI Driver closer in parity with the EBS CSI Driver: https://github.com/kubernetes-sigs/aws-ebs-csi-driver to improve the repo's best practices and allow the start up taint change to be adopted within this driver as well. These changes include:

Adding inflight checks to Create and Delete RPC's (best practice) (Refactor inFlight key to add lock per volumeId aws-ebs-csi-driver#702 Make NodeStageVolume idempotent aws-ebs-csi-driver#163)
Adding feature gates to options (pre-emptively in preparation of json logging support) (Add support for JSON logging aws-ebs-csi-driver#1467)
Modularize Node Service and Controller Service (preparation for start up taint pr & best practice) (Refactor driver to modularize node service and controller service aws-ebs-csi-driver#257)
Increase MaxRetries (best practice Aws client config: increase MaxRetries aws-ebs-csi-driver#769)
Improve metadata collection (preparation for start up taint pr & best practice)
Implementing operating modes (preparation for start up taint pr & best practice) (Implement different driver modes and AWS Region override for controller service aws-ebs-csi-driver#438) Note: I changed the implementation slightly so that customers need to explicitly set mode using the mode flag
update go dependencies and remove vendor dir (best practice and part of Update k8s version #298)
Add json logging option (Add support for JSON logging aws-ebs-csi-driver#1467)
Specify driver mode in the values.yaml for easier customization and more clear default recommendation

Note: will update e2e dependencies in future PR

What testing is done?

Manual testing (built driver image and tested dynamic provisioning + mounting a file system)
- set json logging to confirms logged in json format properly
- controller set with different mode values
- confirmed that the driver image is built with the proper environment data
sanity tests and unit tests
e2e testing (in this PR)

k8s-ci-robot · 2023-05-11T18:04:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jacobwolfaws

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jacobwolfaws]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jacobwolfaws · 2023-05-11T19:02:14Z

/retest

Seems like there's a transient error with one of the sanity tests

hughdanliu · 2023-05-13T15:37:57Z

pkg/driver/internal/inflight.go

+
+const (
+	VolumeOperationAlreadyExistsErrorMsg   = "An operation with the given volume %s already exists"
+	SnapshotOperationAlreadyExistsErrorMsg = "An operation with the given snapshot %s already exists"


The FSx for Lustre CSI driver does not support snapshots yet, so this constant is unused. I think it would be best practice to remove this for now, especially because snapshots/backups may not be implemented for a while.

Good catch, fixed

hughdanliu · 2023-05-13T15:45:20Z

cmd/options/node_options_test.go

+	flag "github.com/spf13/pflag"
+)
+
+func TestNodeOptions(t *testing.T) {


It seems odd that you're adding this test for node options before node options is implemented. I'm assuming as-is these unit tests don't pass. Ideally this would be fixed, but doing so likely isn't the hassle given how these commits are stacked

It's just an empty test, so I believe it passes. Either way I agree, I don't think it makes sense to change this given how the commits are stacked

hughdanliu · 2023-05-13T15:47:00Z

cmd/options/server_options_test.go

+	flag "github.com/spf13/pflag"
+)
+
+func TestServerOptions(t *testing.T) {


Same here, unless I'm missing something I don't think these tests would pass. I may be mistaken though

Same response :)

hughdanliu · 2023-05-13T15:51:48Z

pkg/driver/driver.go

 	}
+
+	driver.controllerService = newControllerService(&driverOptions)


Is there a reason we set these here instead of in the driver declaration?

It's done like this to accommodate the different modes, the alternative would be to return a driver configured differently for each case. This is the format EBS uses + the one I prefer to that option

pkg/cloud/cloud.go

hughdanliu · 2023-05-13T16:08:41Z

pkg/cloud/metadata_k8s.go

+		return nil, fmt.Errorf("node providerID empty, cannot parse")
+	}
+
+	awsInstanceIDRegex := "s\\.i-[a-z0-9]+|i-[a-z0-9]+$"


nit: This regex retrieves the ec2 instance id of the node from the provider id, right? If so, I'm partial to specifying ec2InstanceId in the names of these variables. Do you have context into what would be required to provide support for the CSI driver in non-EKS Kubernetes environments?

hughdanliu · 2023-05-13T16:14:12Z

Makefile

@@ -12,13 +12,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-VERSION=v0.9.0
+VERSION?=v0.9.0


nit: Is there more context behind this change? Is there any world in which VERSION would be already set before making this declaration?

It just makes sense to move us into alignment with the EBS CSI Driver. I don't think it makes a difference for us, but for consistency's sake we should change it

charts/aws-fsx-csi-driver/templates/controller-deployment.yaml

hughdanliu · 2023-05-13T16:20:15Z

deploy/kubernetes/base/controller-deployment.yaml

@@ -34,10 +34,15 @@ spec:
          image: public.ecr.aws/fsx-csi-driver/aws-fsx-csi-driver:v0.9.0
          imagePullPolicy: IfNotPresent
          args:
+            # - --mode={all,controller,node} # specify the driver mode


Same idea as above, for my own understanding what is the default behavior when mode isn't specified? From what I can tell the switch statements are configured to throw an error. Why don't we specify the mode as controller here?

hughdanliu · 2023-05-13T16:32:18Z

.gitignore

@@ -6,3 +6,7 @@
 *.swp
 bin/
 *~
+
+# Vendor dir
+vendor/


smart, we should do this for the OpenZFS CSI driver too

cmd/options/node_options_test.go

torredil · 2023-05-16T14:38:42Z

pkg/driver/controller.go

@@ -71,10 +117,17 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		return nil, status.Error(codes.InvalidArgument, "Volume capabilities not provided")
 	}

-	if !d.isValidVolumeCapabilities(volCaps) {
+	if !isValidVolumeCapabilities(volCaps) {
 		return nil, status.Error(codes.InvalidArgument, "Volume capabilities not supported")


Suggest retrieving the access modes defined in the passed in volume capabilities and logging that.

Yup that probably makes sense to do, I'll add a TODO for a future PR

torredil · 2023-05-16T14:45:07Z

pkg/driver/driver.go

 )

-var (
-	volumeCaps = []csi.VolumeCapability_AccessMode{


Why is this removed?

I moved it into controller.go, similar to how EBS does it: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/pkg/driver/controller.go#L44

torredil · 2023-05-16T16:48:16Z

pkg/driver/constants.go

@@ -0,0 +1,6 @@
+package driver


This file only has one constant. Suggest moving in constants at your discretion, or removing it.

We plan to add the startup taint key to constants.go in a separate PR.
Example from EBS CSI Driver: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/pkg/driver/constants.go#L172-L175

Not opposed to removing constants.go, but I feel comfortable leaving it in right now

torredil · 2023-05-16T16:53:09Z

pkg/driver/controller.go

+		return nil, status.Error(codes.Aborted, msg)
+	}
+	defer d.inFlight.Delete(volName)
+
 	// create a new volume with idempotency
 	// idempotency is handled by `CreateFileSystem`
 	volumeParams := req.GetParameters()


nit: suggest using a switch statement here. Similar to:

for key, value := range req.GetParameters() { switch strings.ToLower(key) { case "fstype": klog.InfoS("\"fstype\" is deprecated, please use \"csi.storage.k8s.io/fstype\" instead") case VolumeTypeKey: volumeType = value

I think there's an opportunity to clean up code in the repo similar to this change, I'll add a TODO but think it's out of scope for this PR

+1 to Eddie's comment. We followed this implementation pattern for the OpenZFS CSI driver. I'd also recommend throwing an error on the create in the default cause to be consistent with the FSx API - if the user provides an invalid parameter in the storage class, they shouldn't expect to have a successful creation.

torredil · 2023-05-16T17:05:36Z

pkg/driver/controller.go

+		klog.V(5).InfoS("[Debug] Retrieving region from metadata service")
+		metadata, err := cloud.NewMetadataService(cloud.DefaultEC2MetadataClient, cloud.DefaultKubernetesAPIClient, region)
+		if err != nil {
+			klog.ErrorS(err, "Could not determine region from any metadata service. The region can be manually supplied via the AWS_REGION environment variable.")


missing klog.FlushAndExit(klog.ExitFlushTimeout, 1)

this is currently the same behavior as EBS: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/pkg/driver/controller.go#L88-L89 Should a similar change be made to the ebs csi driver?

panic also works here, I don't feel strongly about it.

I'm going to add a note about this for more research, it's possible we should just move all panics to this

torredil · 2023-05-16T17:06:44Z

pkg/driver/driver.go

+		csi.RegisterNodeServer(d.srv, d)
+	default:
+		return fmt.Errorf("unknown mode: %s", d.options.mode)
+	}

 	klog.Infof("Listening for connections on address: %#v", listener.Addr())


klog.Infof -> klog.InfoS

Reason: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions

I think this merits a separate commit/PR, because this change will need to be made in a lot of different places in the repo

torredil · 2023-05-16T17:07:03Z

pkg/driver/driver.go

@@ -104,3 +131,15 @@ func (d *Driver) Stop() {
 	klog.Infof("Stopping server")


klog.Infof -> klog.InfoS

khoang98 · 2023-05-16T20:40:29Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2023

k8s-ci-robot requested a review from khoang98 May 11, 2023 18:04

k8s-ci-robot requested a review from olemarkus May 11, 2023 18:04

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 11, 2023

hughdanliu reviewed May 13, 2023

View reviewed changes

Add inflight checks to Create and Delete RPCs

9b847ee

jacobwolfaws force-pushed the master branch 2 times, most recently from c70efba to 9a13bb1 Compare May 15, 2023 21:33

torredil reviewed May 16, 2023

View reviewed changes

cmd/options/node_options_test.go Outdated Show resolved Hide resolved

torredil reviewed May 16, 2023

View reviewed changes

jacobwolfaws added 7 commits May 16, 2023 18:14

Modularize Node Service and Controller Service

cddc4b6

Increase MaxRetries for AWS client config

5fdb604

Improve metadata collection

b53d4fa

Implement operating modes

e69536f

Update go dependencies and remove vendor dir

fd4e6c6

Enable json logging

e86f25e

Specify driver modes in values.yaml

770a2c6

jacobwolfaws force-pushed the master branch from 9a13bb1 to 770a2c6 Compare May 16, 2023 18:18

k8s-ci-robot assigned khoang98 May 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2023

k8s-ci-robot merged commit 20d5b32 into kubernetes-sigs:master May 16, 2023

jacobwolfaws mentioned this pull request Jun 20, 2023

Remove hostNetwork from helm and manifests #332

Merged

hughdanliu mentioned this pull request Jul 13, 2023

REQUEST: New membership for hughdanliu kubernetes/org#4327

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopting Multiple CSI Driver improvements #309

Adopting Multiple CSI Driver improvements #309

jacobwolfaws commented May 11, 2023 •

edited

Loading

k8s-ci-robot commented May 11, 2023

jacobwolfaws commented May 11, 2023 •

edited

Loading

hughdanliu May 13, 2023

jacobwolfaws May 15, 2023

hughdanliu May 13, 2023

jacobwolfaws May 15, 2023

hughdanliu May 13, 2023

jacobwolfaws May 15, 2023

hughdanliu May 13, 2023

jacobwolfaws May 15, 2023

hughdanliu May 13, 2023

hughdanliu May 13, 2023

jacobwolfaws May 15, 2023

hughdanliu May 13, 2023

hughdanliu May 13, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

hughdanliu May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

jacobwolfaws May 16, 2023

torredil May 16, 2023

khoang98 commented May 16, 2023

		}

		driver.controllerService = newControllerService(&driverOptions)

		@@ -104,3 +131,15 @@ func (d *Driver) Stop() {
		klog.Infof("Stopping server")

Adopting Multiple CSI Driver improvements #309

Adopting Multiple CSI Driver improvements #309

Conversation

jacobwolfaws commented May 11, 2023 • edited Loading

k8s-ci-robot commented May 11, 2023

jacobwolfaws commented May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khoang98 commented May 16, 2023

jacobwolfaws commented May 11, 2023 •

edited

Loading

jacobwolfaws commented May 11, 2023 •

edited

Loading