Rename sdc #153

khareRajshree · 2023-01-23T23:04:31Z

Description

Add support for Rename SDC operation from CSI-PowerFlex driver

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#402

Checklist:

I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
I have verified that new and existing unit tests pass locally with my changes
I have not allowed coverage numbers to degenerate
I have maintained at least 90% code coverage
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Unit testing (Refer GitHub workflow for more details)
Functional testing

Renaming SDC with name set as "prefix-hostname"

Renaming SDC with name set as "hostname"

JacobGros · 2023-01-23T23:23:00Z

seeing
systemID not found: 000000000001
in unit test failures. Can you make the corresponding changes to goscaleio and update go.mod to point to it for the checks?

khareRajshree · 2023-01-24T00:23:17Z

seeing systemID not found: 000000000001 in unit test failures. Can you make the corresponding changes to goscaleio and update go.mod to point to it for the checks?

updated the go.mod with the latest goscaleio commit.

JacobGros · 2023-01-24T21:09:46Z

I think case 3 should be dropped:
// case3: if IsSdcRenameEnabled=false and no prefix given then use the SDC ID for sdc name.
Reason:

If SdcRenameEnabled=false, customer probably doesn't want SDC to be named
Since Name = SDC ID, there will be no difference to customer, at least from GUI standpoint
Ask is to rename SDC, not name SDC, and if SDC name = null and we assign a name=ID to name, we're naming it.

Technically we're also naming SDC in cases 1 and 2, but in those cases, there is a visible difference to customer

khareRajshree · 2023-01-25T14:24:39Z

I think case 3 should be dropped: // case3: if IsSdcRenameEnabled=false and no prefix given then use the SDC ID for sdc name. Reason:

If SdcRenameEnabled=false, customer probably doesn't want SDC to be named

Since Name = SDC ID, there will be no difference to customer, at least from GUI standpoint

Ask is to rename SDC, not name SDC, and if SDC name = null and we assign a name=ID to name, we're naming it.

Technically we're also naming SDC in cases 1 and 2, but in those cases, there is a visible difference to customer

Addressed this, thanks.

bharathsreekanth · 2023-01-26T11:55:24Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

JacobGros · 2023-01-26T13:24:12Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

iirc: back when this was being planned, we wanted the driver behavior to match earlier versions by default

JacobGros · 2023-01-26T18:19:20Z

service/features/service.feature

@@ -942,3 +942,21 @@ Feature: VxFlex OS CSI interface
    When I call Probe
    And I call setQoSParameters with systemID "15dbbf5617523655" sdcID "d0f055a700000000" bandwidthLimit "10240" iopsLimit "10" volumeName "k8s-a031818af5" csiVolID "15dbbf5617523655-456ca4fc00000009" nodeID "9E56672F-2F4B-4A42-BFF4-88B6846FBFDA"
    Then the error contains "error setting QoS parameters"
+


These look good, but I think we should add some negative test cases

Negative test cases are added.

JacobGros · 2023-01-26T18:21:03Z

service/node.go

+	// fetch hostname
+	hostName, ok := os.LookupEnv("HOSTNAME")
+	if !ok {
+		Log.Infof("%s not set\n", "HOSTNAME")


I don't think this block should be hit, but if it is, should we exit here?
If there's no hostname and no prefix, SDC name will = ""

Yes, We should not hit this case. Now I have made the changes to return an error if the HOSTNAME is not. We wont hit this case at all, But added it for safe side.

bharathsreekanth · 2023-01-26T18:21:24Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

iirc: back when this was being planned, we wanted the driver behavior to match earlier versions by default

I understand, but I was thinking if we can improve SDC names by renaming them to be more legible as opposed to what we had earlier, would that have any negative impact? would that cause any volumes mapped via the SDC to fail when users upgrade to newer version of the driver.

JacobGros · 2023-01-26T18:26:39Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

iirc: back when this was being planned, we wanted the driver behavior to match earlier versions by default

I understand, but I was thinking if we can improve SDC names by renaming them to be more legible as opposed to what we had earlier, would that have any negative impact? would that cause any volumes mapped via the SDC to fail when users upgrade to newer version of the driver.

What if a customer already named their SDCs based on their own convention and then updated driver? Their old SDC names will get replaced and they will probably be confused as to why that happened. I think in general, we should not alter anything on their pflex array unless they request it via enable value.

JacobGros · 2023-01-26T18:29:10Z

Looks good, outside of some quick comments, my two main questions are:

Are these changes coming to CSM-Operator and CSI-Operator as well?
Is there a doc PR to review?
Thanks

bharathsreekanth · 2023-01-26T18:52:32Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

iirc: back when this was being planned, we wanted the driver behavior to match earlier versions by default

I understand, but I was thinking if we can improve SDC names by renaming them to be more legible as opposed to what we had earlier, would that have any negative impact? would that cause any volumes mapped via the SDC to fail when users upgrade to newer version of the driver.

What if a customer already named their SDCs based on their own convention and then updated driver? Their old SDC names will get replaced and they will probably be confused as to why that happened. I think in general, we should not alter anything on their pflex array unless they request it via enable value.

If customers already renamed, I guess this might be a concern. Thanks for explaining.

khareRajshree · 2023-01-26T19:57:17Z

Is there a downside to renaming SDC always and add only a SDCPrefix if a customer specifies a prefix?

iirc: back when this was being planned, we wanted the driver behavior to match earlier versions by default

I understand, but I was thinking if we can improve SDC names by renaming them to be more legible as opposed to what we had earlier, would that have any negative impact? would that cause any volumes mapped via the SDC to fail when users upgrade to newer version of the driver.

What if a customer already named their SDCs based on their own convention and then updated driver? Their old SDC names will get replaced and they will probably be confused as to why that happened. I think in general, we should not alter anything on their pflex array unless they request it via enable value.

If customers already renamed, I guess this might be a concern. Thanks for explaining.

If customer upgraded the driver from 2.5 to 2.6 suppose, then in that case rename would not happen on its own unless they have enabled it from values.yaml, so whatever was the previous name it would still stay the same for them.

VamsiSiddu-7 · 2023-02-01T10:51:29Z

unit-test-results-rename-sdc.txt

bharathsreekanth · 2023-02-01T12:52:23Z

service/node.go

+		sdcID := sdc.Sdc.ID
+
+		var newName string
+		if len(opts.SdcPrefix) > 0 {


you can simplify code here if you would like (nothing wrong with what you have).
newName = hostName
if (sdcPrefix > 0) newName = opts.SdcPrefix + "_" + newName.
Basically remove the else clause. Not major, I am fine with as is.

JacobGros

LGTM

khareRajshree requested review from abhi16394, bharathsreekanth, JacobGros, mjsdell and jooseppi-luna as code owners January 23, 2023 23:04

JacobGros reviewed Jan 26, 2023

View reviewed changes

bharathsreekanth previously approved these changes Jan 26, 2023

View reviewed changes

khareRajshree added 11 commits January 31, 2023 02:17

added support for renaming SDC

f4203c7

node.go updated

92c2e79

node.go updated

3b08004

unit tests added

3ece0b8

unit tests updated

ff84122

node.go updated

cdbebaa

node.go updated

fc03fd4

node.go updated

ba6e8a0

node.go updated

9f4c548

UTs updated

4ca7f2b

node.go updated

fda4b8a

UTs updated

96c1b13

VamsiSiddu-7 dismissed bharathsreekanth’s stale review via 96c1b13 January 31, 2023 07:33

VamsiSiddu-7 force-pushed the rename-sdc branch from 9a35a52 to 96c1b13 Compare January 31, 2023 07:33

modified renameSdc unit-tests and hostName logic.

f8b081a

VamsiSiddu-7 requested review from bharathsreekanth and JacobGros February 1, 2023 09:31

bharathsreekanth previously approved these changes Feb 1, 2023

View reviewed changes

added the missing comment in values.yaml for renamesdc.

5885386

VamsiSiddu-7 dismissed bharathsreekanth’s stale review via 5885386 February 1, 2023 13:09

bharathsreekanth approved these changes Feb 1, 2023

View reviewed changes

JacobGros approved these changes Feb 1, 2023

View reviewed changes

VamsiSiddu-7 merged commit 499e2ec into main Feb 1, 2023

JacobGros mentioned this pull request Mar 28, 2023

[FEATURE]: User friendly name to the PowerFlex dell/csm#181

Closed

anandrajak1 deleted the rename-sdc branch October 17, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename sdc #153

Rename sdc #153

khareRajshree commented Jan 23, 2023 •

edited

Loading

JacobGros commented Jan 23, 2023

khareRajshree commented Jan 24, 2023

JacobGros commented Jan 24, 2023 •

edited

Loading

khareRajshree commented Jan 25, 2023

bharathsreekanth commented Jan 26, 2023

JacobGros commented Jan 26, 2023

JacobGros Jan 26, 2023

VamsiSiddu-7 Feb 1, 2023

JacobGros Jan 26, 2023

VamsiSiddu-7 Feb 1, 2023

bharathsreekanth commented Jan 26, 2023

JacobGros commented Jan 26, 2023

JacobGros commented Jan 26, 2023

bharathsreekanth commented Jan 26, 2023

khareRajshree commented Jan 26, 2023

VamsiSiddu-7 commented Feb 1, 2023

bharathsreekanth Feb 1, 2023

JacobGros left a comment

Rename sdc #153

Rename sdc #153

Conversation

khareRajshree commented Jan 23, 2023 • edited Loading

Description

GitHub Issues

Checklist:

How Has This Been Tested?

JacobGros commented Jan 23, 2023

khareRajshree commented Jan 24, 2023

JacobGros commented Jan 24, 2023 • edited Loading

khareRajshree commented Jan 25, 2023

bharathsreekanth commented Jan 26, 2023

JacobGros commented Jan 26, 2023

JacobGros Jan 26, 2023

Choose a reason for hiding this comment

VamsiSiddu-7 Feb 1, 2023

Choose a reason for hiding this comment

JacobGros Jan 26, 2023

Choose a reason for hiding this comment

VamsiSiddu-7 Feb 1, 2023

Choose a reason for hiding this comment

bharathsreekanth commented Jan 26, 2023

JacobGros commented Jan 26, 2023

JacobGros commented Jan 26, 2023

bharathsreekanth commented Jan 26, 2023

khareRajshree commented Jan 26, 2023

VamsiSiddu-7 commented Feb 1, 2023

bharathsreekanth Feb 1, 2023

Choose a reason for hiding this comment

JacobGros left a comment

Choose a reason for hiding this comment

khareRajshree commented Jan 23, 2023 •

edited

Loading

JacobGros commented Jan 24, 2023 •

edited

Loading