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

CSM Operator Installation - Added CR fields for PowerMax and PowerFlex #1458

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ChristianAtDell
Copy link
Contributor

Description

This PR adds documentation to support the changes made in dell/csm-operator#905 .

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1762

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?

image

image

Copy link

github-actions bot commented Feb 27, 2025

Test Results

77 tests  ±0   77 ✅ ±0   3s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 11202fc. ± Comparison against base commit 78c299a.

♻️ This comment has been updated with latest results.

alikdell
alikdell previously approved these changes Feb 27, 2025
@ChristianAtDell ChristianAtDell changed the title added fields to appropriate files CSM Operator Installation - Added CR fields for PowerMax and PowerFlex Feb 27, 2025
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.

No edits required

@@ -157,6 +157,7 @@ run `/opt/emc/scaleio/sdc/bin/drv_cfg --add_mdm --ip 10.xx.xx.xx.xx,10.xx.xx.xx`
| X_CSI_DEBUG | To enable debug mode | No | true |
| X_CSI_ALLOW_RWO_MULTI_POD_ACCESS | Setting allowRWOMultiPodAccess to "true" will allow multiple pods on the same node to access the same RWO volume. This behavior conflicts with the CSI specification version 1.3. NodePublishVolume description that requires an error to be returned in this case. However, some other CSI drivers support this behavior and some customers desire this behavior. Customers use this option at their own risk. | No | false |
| INTERFACE_NAMES | A mapping of node names to interface names. Only necessary when SDC is disabled. | No | none |
| CSI_LOG_LEVEL | Controls the log level for messages printed to the driver logs. Allowable values per logrus are [INFO, DEBUG, TRACE, WARN, ERROR, FATAL, and PANIC](https://github.com/sirupsen/logrus/blob/master/logrus.go#L25). | No | INFO |
Copy link
Contributor

@atye atye Feb 27, 2025

Choose a reason for hiding this comment

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

What's the likelihood of https://github.com/sirupsen/logrus/blob/master/logrus.go#L25 becoming a dead or incorrect link in the future?

Maybe it's simpler to not mention logrus at all; just allowable values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Virtually nil. https://github.com/sirupsen/logrus/commits/master/logrus.go It has not been updated in 6 years.

I thought about that, too; in a few places that we have CSI_LOG_LEVEL elsewhere we only mention INFO and DEBUG as allowable values, and I'm pretty confident that's not true because it all goes to logrus which accepts the wider selection. I will adjust it if someone has a strong opinion to only include the allowable values, and even trim it down to the two values we mention elsewhere (even if that is not technically correct).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be consistent in our messaging across all the references to CSI_LOG_LEVEL.

santhoshatdell
santhoshatdell previously approved these changes Feb 28, 2025
@@ -160,6 +160,7 @@ Example:
| X_CSI_VSPHERE_PORTGROUP | Existing portGroup that driver will use for vSphere | Yes | "" |
| X_CSI_VSPHERE_HOSTNAME | Existing host(initiator group)/host group(cascaded initiator group) that driver will use for vSphere | Yes | "" |
| X_CSI_VCenter_HOST | URL/endpoint of the vCenter where all the ESX are present | Yes | "" |
| X_CSI_POWERMAX_DEBUG | Enable/Disable gopowermax library-level debugging. | No | false |

Choose a reason for hiding this comment

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

Do we need to have this also added for other binding layers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this comment with Aron:

gopowermax is the one being added with this PR, so we should be covered.

gopowerscale has:
https://github.com/dell/gopowerscale/blob/ef35bfd90f23593d67077d191328562ec17fc696/api/api.go#L58
but that isn't supported in operator at present.

goscaleio has the http flags, but like gopowerscale, they aren't currently supported in operator.

gopowerstore has:
https://github.com/dell/gopowerstore/blob/171b9c405d0d17e5cdaa2734c80f1637a4c01b5c/api/api.go#L138
and that is supported in operator: https://github.com/dell/csm-operator/blob/04fd860891c3f1ec640d24fbeaa273c18963aa95[…]/operatorconfig/driverconfig/powerstore/v2.13.0/controller.yaml

We don't currently expose that value in our Operator sample, nor do we call it out in csm-docs, but it is possible to add it like I did for the two flags already mentioned in this docs PR. Fairly trivial change in both repos, just add it to the docs tables and add a value to the samples in operator.

...

Per feedback, I am converting this PR to draft and working with my team to increase the scope of this defect and add granular control of library debug flags to other libraries.

@@ -28,7 +28,7 @@ To deploy the Operator, follow the instructions available [here](../../../operat

2. **Create `secret` file:**.

a. Create a file called `secret.yaml` or pick a [sample](https://github.com/dell/csi-powerscale/blob/main/samples/secret/secret.yaml) that has Powerscale array connection details:
a. Create a file called `secret.yaml` or pick a [sample](https://github.com/dell/csi-powerscale/blob/main/samples/secret/secret.yaml) that has Powerscale array connection details:
Copy link

Choose a reason for hiding this comment

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

It should probably be "PowerScale", not "Powerscale".

Comment on lines +161 to +162
| GOSCALEIO_DEBUG | Enable/Disable goscaleio library-level debugging. | No | false |
| GOSCALEIO_SHOWHTTP | Enable/Disable goscaleio library-level REST request logging. Enabling will also **enable** GOSCALEIO_DEBUG regardless of GOSCALEIO_DEBUG setting. | No | false |
Copy link

Choose a reason for hiding this comment

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

For gocsi you use "library level" and here you use "library-level". These descriptions should be consistent.

@@ -160,6 +160,8 @@ Example:
| X_CSI_VSPHERE_PORTGROUP | Existing portGroup that driver will use for vSphere | Yes | "" |
| X_CSI_VSPHERE_HOSTNAME | Existing host(initiator group)/host group(cascaded initiator group) that driver will use for vSphere | Yes | "" |
| X_CSI_VCenter_HOST | URL/endpoint of the vCenter where all the ESX are present | Yes | "" |
| X_CSI_DEBUG | Enable/disable gocsi library level debugging. | No | false |
Copy link

Choose a reason for hiding this comment

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

Inconsistency with "library-level" in other places.

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.

9 participants