[cinder-csi-plugin] ephemeral volume removal (#2602) #2640

sergelogvinov · 2024-09-03T15:38:20Z

What this PR does / why we need it:

Remove openstack credits from node plugin

Which issue this PR fixes(if applicable):

step 2 of #2599

Special notes for reviewers:

Add cli flags (they have same purpose as [BlockStorage] params from cloud-config).
It helps to avoid using the cloud-config on the nodes

search-order
rescan-on-resize
node-volume-attach-limit

# cinder-csi-plugin --help
...
      --node-volume-attach-limit int         The maximum number of volumes that can be attached to a node (default 256)
      --provide-controller-service           If set to true then the CSI driver does provide the controller service (default true)
      --provide-node-service                 If set to true then the CSI driver does provide the node service (default true)
      --rescan-on-resize                     If set to true then the CSI driver will rescan the device on volume resize
      --search-order string                  The search order for metadata service (default "configDrive,metadataService")

Release note:

If you override the default values for `rescan-on-resize` or `node-volume-attach-limit`, please switch to using CLI parameters in the DaemonSet pod arguments. Helm value path csi.plugin.extraArgs.

Manual tests:

create/delete
snapshot create/delete
create from clone

k8s-ci-robot · 2024-09-03T15:38:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kayrus for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2024-09-03T15:38:29Z

Hi @sergelogvinov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sergelogvinov · 2024-09-04T14:13:56Z

When you have a time, can you take a look on this changes @jichenjc @dulek

I think it's ready to go, but we need to wait for the official release first.
Thanks.

zetaab · 2024-09-04T16:28:15Z

/ok-to-test

kayrus · 2024-09-11T13:16:23Z

@sergelogvinov could you please rebase your PR?

sergelogvinov · 2024-09-11T13:35:06Z

@kayrus done.

Thank you!

sergelogvinov · 2024-09-11T15:08:52Z

/test pull-cloud-provider-openstack-check

jichenjc · 2024-09-12T03:39:40Z

/test pull-cloud-provider-openstack-check

sergelogvinov · 2024-09-12T11:43:02Z

I've changed the golangci-lint version to 1.61.0, and test pull-cloud-provider-openstack-check passed.

kayrus · 2024-09-17T15:35:47Z

@sergelogvinov I submitted a more agnostic PR that allows to run node service without credentials: #2655

k8s-ci-robot · 2024-09-17T23:41:17Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kayrus · 2024-09-20T07:58:05Z

cmd/cinder-csi-plugin/main.go


-		d.SetupNodeService(clouds[cloudNames[0]], mount, metadata, additionalTopologies)
+		d.SetupNodeService(mount, metadata, rescanOnResize, nodeVolumeAttachLimit, additionalTopologies)


It would be better to pass a struct instead of a number of vars. https://refactoring.guru/smells/long-parameter-list

switch to openstack.BlockStorageOpts{}

kayrus · 2024-09-20T07:59:02Z

cmd/cinder-csi-plugin/main.go

 		//Initialize mount
 		mount := mount.GetMountProvider()

+		//Backward compatibility, read [BlockStorage] parameters from cloud config
+		cfg, err := openstack.GetConfigFromFiles(cloudConfig)
+		if err == nil {


This pattern is a bit uncommon. I'd rather check for err != nil and handle an error.

kayrus · 2024-09-20T08:01:25Z

pkg/csi/cinder/driver.go

@@ -32,6 +32,12 @@ import (
 const (
 	driverName  = "cinder.csi.openstack.org"
 	topologyKey = "topology." + driverName + "/zone"
+
+	// MaxVolumesPerNode is the maximum number of volumes that can be attached to a node
+	MaxVolumesPerNode = 256


Where does this const come from?

It was hardcoded here

cloud-provider-openstack/pkg/csi/cinder/openstack/openstack.go

Line 144 in d069b1e

const defaultMaxVolAttachLimit int64 = 256

I think it makes sense to configure this value. OpenStack supports different hypervisors and each supports a different amount of attached volumes.

just in case, if the const/var doesn't need to be accessed from a different package, it's better to define its name starting with a lower case (non-exported). https://go.dev/tour/basics/3

Yep, --node-volume-attach-limit flag customize it

MaxVolumesPerNode -> maxVolumesPerNode 👍

Remove openstack credits from node plugin Signed-off-by: Serge Logvinov <serge.logvinov@sinextra.dev>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 3, 2024

k8s-ci-robot requested review from dulek and zetaab September 3, 2024 15:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 3, 2024

sergelogvinov force-pushed the no-ephemeral-2 branch 6 times, most recently from 5c01745 to 1912d64 Compare September 4, 2024 13:18

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 4, 2024

sergelogvinov force-pushed the no-ephemeral-2 branch from 1912d64 to 0747317 Compare September 4, 2024 14:10

sergelogvinov marked this pull request as ready for review September 4, 2024 14:10

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2024

k8s-ci-robot requested a review from jichenjc September 4, 2024 14:10

sergelogvinov force-pushed the no-ephemeral-2 branch from 0747317 to bb8a24a Compare September 11, 2024 13:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2024

sergelogvinov force-pushed the no-ephemeral-2 branch from bb8a24a to a002f62 Compare September 11, 2024 13:25

sergelogvinov force-pushed the no-ephemeral-2 branch 4 times, most recently from b9de18d to f154edb Compare September 12, 2024 11:29

kayrus mentioned this pull request Sep 13, 2024

[csi-cinder-plugin] Ephemeral Volume removal process #2599

Closed

sergelogvinov mentioned this pull request Sep 17, 2024

[cinder-csi-plugin] implement structured logging #2652

Draft

kayrus mentioned this pull request Sep 17, 2024

[cinder-csi]: allow node service to run without openstack client #2655

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2024

sergelogvinov force-pushed the no-ephemeral-2 branch from f154edb to d9e9d6f Compare September 18, 2024 10:33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

sergelogvinov force-pushed the no-ephemeral-2 branch 2 times, most recently from f3ca7fd to 8a2fa6c Compare September 20, 2024 07:49

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2024

kayrus reviewed Sep 20, 2024

View reviewed changes

sergelogvinov force-pushed the no-ephemeral-2 branch from 8a2fa6c to 6c66a39 Compare September 20, 2024 09:05

[cinder-csi-plugin] ephemeral volume removal (kubernetes#2602)

3a6fcc0

Remove openstack credits from node plugin Signed-off-by: Serge Logvinov <serge.logvinov@sinextra.dev>

sergelogvinov force-pushed the no-ephemeral-2 branch from 6c66a39 to 3a6fcc0 Compare September 20, 2024 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cinder-csi-plugin] ephemeral volume removal (#2602) #2640

[cinder-csi-plugin] ephemeral volume removal (#2602) #2640

sergelogvinov commented Sep 3, 2024 •

edited

Loading

k8s-ci-robot commented Sep 3, 2024

k8s-ci-robot commented Sep 3, 2024

sergelogvinov commented Sep 4, 2024

zetaab commented Sep 4, 2024

kayrus commented Sep 11, 2024

sergelogvinov commented Sep 11, 2024

sergelogvinov commented Sep 11, 2024

jichenjc commented Sep 12, 2024

sergelogvinov commented Sep 12, 2024

kayrus commented Sep 17, 2024

k8s-ci-robot commented Sep 17, 2024

kayrus Sep 20, 2024

sergelogvinov Sep 20, 2024

kayrus Sep 20, 2024

kayrus Sep 20, 2024

sergelogvinov Sep 20, 2024

kayrus Sep 20, 2024

kayrus Sep 20, 2024

sergelogvinov Sep 20, 2024

sergelogvinov Sep 20, 2024


		d.SetupNodeService(clouds[cloudNames[0]], mount, metadata, additionalTopologies)
		d.SetupNodeService(mount, metadata, rescanOnResize, nodeVolumeAttachLimit, additionalTopologies)

[cinder-csi-plugin] ephemeral volume removal (#2602) #2640

Are you sure you want to change the base?

[cinder-csi-plugin] ephemeral volume removal (#2602) #2640

Conversation

sergelogvinov commented Sep 3, 2024 • edited Loading

k8s-ci-robot commented Sep 3, 2024

k8s-ci-robot commented Sep 3, 2024

sergelogvinov commented Sep 4, 2024

zetaab commented Sep 4, 2024

kayrus commented Sep 11, 2024

sergelogvinov commented Sep 11, 2024

sergelogvinov commented Sep 11, 2024

jichenjc commented Sep 12, 2024

sergelogvinov commented Sep 12, 2024

kayrus commented Sep 17, 2024

k8s-ci-robot commented Sep 17, 2024

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

sergelogvinov commented Sep 3, 2024 •

edited

Loading