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

[sergelogvinov]

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:

Release note:

the flag node-service-no-os-client was deprecated

Manual tests:

• create/delete
• snapshot create/delete
• create from clone

[k8s-ci-robot]

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]

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]

/ok-to-test

[kayrus]

@sergelogvinov could you please rebase your PR?

[sergelogvinov]

@kayrus done.

Thank you!

[sergelogvinov]

I truly appreciate the opportunity to improve this code (plugin), and we have several suggestions and ideas regarding cmd/cinder-csi-plugin/main.go. However, I believe it would be more efficient to implement these changes in a separate PR, as we can do it in parallel with others PRs.

For now, I have removed any flags or changes related to this file. This PR is currently blocking me, and I am unable to move forward with my other PRs.

Thank you for your understanding!

[sergelogvinov]

@dulek, @jichenjc, @kayrus, @mdbooth Is there anything else I can do to help here?

[kayrus]

I have a couple of minor suggestions. The rest is LGTM.
Thank you for your patience.

[kayrus]

/lgtm

[kayrus]

@zetaab @dulek if you don't have objections, I'll approve this PR tomorrow

[kayrus]

I made yet another review round and have a couple of new question.

[kayrus]

Of the tempDir has child objects, the remove will fail. We need a recursive remove.

Suggested change

	// setup for test
	tempDir := os.TempDir()
	defer os.Remove(tempDir)
	volumePath := filepath.Join(tempDir, FakeTargetPath)
	err := os.MkdirAll(volumePath, 0750)
	if err != nil {
	t.Fatalf("Failed to set up volumepath: %v", err)
	}
	// setup for test
	tempDir := os.TempDir()
	volumePath := filepath.Join(tempDir, FakeTargetPath)
	err := os.MkdirAll(volumePath, 0750)
	if err != nil {
	t.Fatalf("Failed to set up volumepath: %v", err)
	}
	defer os.RemoveAll(tempDir)

[sergelogvinov]

Good catch. defer os.RemoveAll(tempDir) -> defer os.RemoveAll(volumePath)

os.TempDir() - returns /tmp folder, it does not create any folders, os.RemoveAll(tempDir) can remove /tmp during the tests...

[kayrus]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kayrus

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 [kayrus]

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