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.

Sign up for GitHub

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

Jump to bottom

Add support for node specific MDM configuration #369

Merged
merged 8 commits into from
Dec 10, 2024
Merged

Add support for node specific MDM configuration #369

merged 8 commits into from
Dec 10, 2024

Conversation

donatwork
Copy link
Contributor

@donatwork donatwork commented Dec 6, 2024 •
edited
Loading

Description

Support configuring the SDC driver on a per node basis based on the configured zone. Added a pre-init mode for the driver based on the value of X_CSI_MODE and will run a method to read the secret and node labels and then save an MDM list based on the node zones. The default behavior is also preserved if no zones and labels are configured.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1612

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

  • Regression testing on new cluster
  • Test functionality with new version of Operator that sets the pre-init mod.

@donatwork donatwork changed the title DRAFT: Add support for node specific MDM configuration Add support for node specific MDM configuration Dec 9, 2024
@donatwork donatwork marked this pull request as ready for review December 9, 2024 12:30
@donatwork donatwork force-pushed the usr/donatwork/1413-mdm-info branch from 87a7a90 to 165178e Compare December 9, 2024 12:53
JacobGros
JacobGros requested changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@JacobGros JacobGros left a comment •
edited
Loading

Choose a reason for hiding this comment

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

one small change needed in a few places:
MDM list w/ multiple clusters isn't comma separated.
Each cluster is separated with a "&"
So returning an MDM string for 2 clusters shouldn't look like this:

"192.168.1.1,192.168.1.2,192.168.2.1,192.168.2.2",

It would need to look like

"192.168.1.1,192.168.1.2&192.168.2.1,192.168.2.2",

@donatwork
Copy link
Contributor Author

one small change needed in a few places: MDM list w/ multiple clusters isn't comma separated. Each cluster is separated with a "&" So returning an MDM string for 2 clusters shouldn't look like this:

"192.168.1.1,192.168.1.2,192.168.2.1,192.168.2.2",

It would need to look like

"192.168.1.1,192.168.1.2&192.168.2.1,192.168.2.2",

Implemented change.

@donatwork donatwork requested a review from JacobGros December 9, 2024 15:03
JacobGros
JacobGros previously approved these changes Dec 9, 2024
View reviewed changes
shaynafinocchiaro
shaynafinocchiaro reviewed Dec 9, 2024
View reviewed changes
service/preinit.go
func (s *DefaultArrayConfigurationProvider) GetArrayConfiguration() ([]*ArrayConnectionData, error) {
arrayConfig, err := getArrayConfig(nil)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an error message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented.

service/preinit.go Outdated
Log.Infof("Zone key detected, will configure MDMs for this node, key: %s", labelKey)
nodeLabels, err := s.GetNodeLabels(context.Background())
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an error message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented.

tdawe
tdawe reviewed Dec 9, 2024
View reviewed changes
main.go Outdated Show resolved Hide resolved
@donatwork
Address PR comments:
9eca8c2 
 * Log errors and return new wrapped error struct.
 * When the node label is missing or empty configure an empty MDM list.
@donatwork
Copy link
Contributor Author

Based on feedback I changed behavior so that missing node labels will configure an empty MDM list

JacobGros
JacobGros previously approved these changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@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

atye
atye reviewed Dec 9, 2024
View reviewed changes
service/preinit.go Outdated Show resolved Hide resolved
@donatwork donatwork requested review from JacobGros and atye December 9, 2024 22:21
atye
atye previously approved these changes Dec 9, 2024
View reviewed changes
JacobGros
JacobGros requested changes Dec 9, 2024
View reviewed changes
service/preinit.go Show resolved Hide resolved
@donatwork donatwork requested a review from JacobGros December 10, 2024 11:43
JacobGros
JacobGros approved these changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@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

@donatwork donatwork merged commit fef7388 into main Dec 10, 2024
6 checks passed
@donatwork donatwork deleted the usr/donatwork/1413-mdm-info branch December 10, 2024 15:44
@atye atye mentioned this pull request Dec 10, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobGros JacobGros JacobGros approved these changes

@tdawe tdawe tdawe approved these changes

@atye atye atye left review comments

@daniyaliqbal2024 daniyaliqbal2024 Awaiting requested review from daniyaliqbal2024

@abhi16394 abhi16394 Awaiting requested review from abhi16394 abhi16394 is a code owner

@adarsh-dell adarsh-dell Awaiting requested review from adarsh-dell adarsh-dell is a code owner

@AkshaySainiDell AkshaySainiDell Awaiting requested review from AkshaySainiDell AkshaySainiDell is a code owner

@bpjain2004 bpjain2004 Awaiting requested review from bpjain2004 bpjain2004 is a code owner

@santhoshatdell santhoshatdell Awaiting requested review from santhoshatdell

@lukeatdell lukeatdell Awaiting requested review from lukeatdell

@bharathsreekanth bharathsreekanth Awaiting requested review from bharathsreekanth

@ChristianAtDell ChristianAtDell Awaiting requested review from ChristianAtDell

@shaynafinocchiaro shaynafinocchiaro Awaiting requested review from shaynafinocchiaro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@donatwork @tdawe @atye @JacobGros @shaynafinocchiaro @ChristianAtDell