Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSI PowerFlex driver docs update - renameSDC and preapprovedGUID #455

Merged
merged 9 commits into from
Feb 16, 2023

Conversation

khareRajshree
Copy link
Contributor

@khareRajshree khareRajshree commented Feb 10, 2023

Description

Documentation update for CSI PowerFlex driver includes:

  • Updates for rename SDC feature.
  • Updates for Pre-approving SDC by GUID feature.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#402

Checklist:

  • Have you run a grammar and spell checks against your submission?
  • Have you tested the changes locally?
  • Have you tested whether the hyperlinks are working properly?
  • Did you add the examples wherever applicable?
  • Have you added high-resolution images?

Copy link

@rsedlock1958 rsedlock1958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed this. Several edits are required.

@khareRajshree
Copy link
Contributor Author

I have reviewed this. Several edits are required.

Hi @rsedlock1958, I have addressed the review comments.

rsedlock1958
rsedlock1958 previously approved these changes Feb 10, 2023
@JacobGros
Copy link
Contributor

please update the values.yaml section in install driver guide as well: https://github.com/dell/csm-docs/blob/main/content/docs/csidriver/installation/helm/powerflex.md#install-the-driver

## Pre-approving SDC by GUID

Starting in version 2.6, the CSI Driver for PowerFlex now supports renaming the SDCs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should say approving sdc by guid, renaming SDC was previous section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes done.

Copy link
Contributor

@JacobGros JacobGros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix typo and update installation section with the new values

@khareRajshree
Copy link
Contributor Author

Please fix typo and update installation section with the new values
Hi @JacobGros, I have addressed the comments, please re-review it once.

JacobGros
JacobGros previously approved these changes Feb 13, 2023
Copy link
Contributor

@JacobGros JacobGros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

VamsiSiddu-7
VamsiSiddu-7 previously approved these changes Feb 14, 2023
Copy link
Collaborator

@rajkumar-palani rajkumar-palani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please correct the review comments given.

Starting in version 2.6, the CSI Driver for PowerFlex now supports renaming of SDC. To use this feature, the node section of values.yaml should have renameSDC keys enabled with a prefix value.

If you want to rename the SDC, please make the following edits to your values file at `csi-vxflexos/helm/csi-vxflexos/values.yaml:`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide link to the values.yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CSI PowerFlex driver should be able to detect/read the SDC mode from the PowerFlex array and determine whether requests for SDC approval be made to the array prior to publishing a volume. This is specific to each SDC.

If you want to get the SDC pre-approved by GUID, please make the following edits to your values file at `csi-vxflexos/helm/csi-vxflexos/values.yaml:`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide link to the values.yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

```
The renameSDC section is going to be used by the Node Service, it has two keys enabled and prefix:
* `enabled`: Boolean variable that specifies if the renaming for SDC is to be carried out or not. Is true then the driver will perform the rename operation. By default, its value will be false.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct the typo from Is True to If True

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

> NOTE: Currently, the CSI-PowerFlex driver only supports GUID for the restricted SDC mode.

If SDC approval is denied, then provisioning of the volume should not be attempted and the appropriate error message is reported in the logs/events so the user is informed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change "the" to "an" appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@khareRajshree
Copy link
Contributor Author

please correct the review comments given.

Hi @rajkumar-palani, have addressed the review comments.

## Rename SDC

Starting in version 2.6, the CSI Driver for PowerFlex now supports renaming of SDC. To use this feature, the node section of values.yaml should have renameSDC keys enabled with a prefix value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: CSI driver for PowerFlex will support renaming of SDCs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

## Pre-approving SDC by GUID

Starting in version 2.6, the CSI Driver for PowerFlex now supports pre-approving SDC by GUID.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: CSI Driver for PowerFlex will support pre-approving SDC by GUID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


Starting in version 2.6, the CSI Driver for PowerFlex now supports pre-approving SDC by GUID.
CSI PowerFlex driver should be able to detect/read the SDC mode from the PowerFlex array and determine whether requests for SDC approval be made to the array prior to publishing a volume. This is specific to each SDC.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid "should", gives the impression that it is best try. I would rephrase as "CSI PowerFlex driver will detect the SDC mode set on the PowerFlex array and will request SDC approval from the array prior to publishing a volume"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CSI PowerFlex driver should be able to detect/read the SDC mode from the PowerFlex array and determine whether requests for SDC approval be made to the array prior to publishing a volume. This is specific to each SDC.

If you want to get the SDC pre-approved by GUID, please make the following edits to your [values.yaml](https://github.com/dell/csi-powerflex/blob/main/helm/csi-vxflexos/values.yaml) file:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to : To request SDC approval for GUID, make the following edits...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

> NOTE: Currently, the CSI-PowerFlex driver only supports GUID for the restricted SDC mode.

If SDC approval is denied, then provisioning of the volume should not be attempted and an appropriate error message is reported in the logs/events so the user is informed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would avoid "should" again. If SDC approval is denied, then provisioning of the volume will not be attempted and an appropriate ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


If you want to rename the SDC, please make the following edits to your [values.yaml](https://github.com/dell/csi-powerflex/blob/main/helm/csi-vxflexos/values.yaml) file:
To request renaming of SDC, make the following edits to [values.yaml](https://github.com/dell/csi-powerflex/blob/main/helm/csi-vxflexos/values.yaml) file:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, minor comment : To enable renaming of SDC, make the following ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -665,9 +665,9 @@ Once the volume gets created, the ControllerPublishVolume will set the QoS limit

## Rename SDC

Starting in version 2.6, the CSI Driver for PowerFlex now supports renaming of SDC. To use this feature, the node section of values.yaml should have renameSDC keys enabled with a prefix value.
Starting in version 2.6, the CSI driver for PowerFlex will support renaming of SDCs. To use this feature, the node section of values.yaml should have renameSDC keys enabled with a prefix value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting with version 2.6, the CSI driver for PowerFlex ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* `prefix`: string variable that is used to set the prefix for SDC name.

Now based on these two keys, there are certain scenarios on which the driver is going to perform the rename SDC operation:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove "Now". Based on these two keys, the driver will perform the rename SDC operation as described below:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change it to "Based on these two keys, the driver will perform the rename SDC operation as described below", does it make sense?

Copy link
Collaborator

@rajkumar-palani rajkumar-palani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@bharathsreekanth bharathsreekanth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry Rajshree, I was looking at this again and made some comments. They are minor but I thought these will make the docs look crisper. I am approving the PR, but do consider the comments and see if it makes sense.

> NOTE: Currently, the CSI-PowerFlex driver only supports GUID for the restricted SDC mode.

If SDC approval is denied, then provisioning of the volume will not be attempted and an appropriate error message is reported in the logs/events so the user is informed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment, you can remove "so the user is informed" as purpose of logs/events is information to user. I will leave it up to you if you want to change it or leave as is.

## Rename SDC

Starting with version 2.6, the CSI driver for PowerFlex will support renaming of SDCs. To use this feature, the node section of values.yaml should have renameSDC keys enabled with a prefix value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the last sentence? you are mentioning how to use the feature to rename SDC in the line below? upto you, if you want to change it.

prefix: "sdc-test"
```
The renameSDC section is going to be used by the Node Service, it has two keys enabled and prefix:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to mention Node Service in the documentation? That is internal to our implementation right?
To enable renameSDC following keys are used.

```
The renameSDC section is going to be used by the Node Service, it has two keys enabled and prefix:
* `enabled`: Boolean variable that specifies if the renaming for SDC is to be carried out or not. If true then the driver will perform the rename operation. By default, its value will be false.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rephrase as "Boolean variable that specified is renaming of SDC is desired". If true, then the driver will perform the rename operation. By default, the value is set to false.

@khareRajshree khareRajshree merged commit dd97eff into release-1.6.0 Feb 16, 2023
rajkumar-palani pushed a commit that referenced this pull request Feb 27, 2023
* PowerFlex driver doc updated for renameSDC and preapprovedGUID features
rajkumar-palani pushed a commit that referenced this pull request Mar 9, 2023
* PowerFlex driver doc updated for renameSDC and preapprovedGUID features
@shanmydell shanmydell deleted the pflex-renameSDC-preapprovedGUID branch July 20, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants