Skip to content

Fix Node Driver Crash Caused by Auto-Select Protocol#421

Merged
rishabhatdell merged 23 commits into
mainfrom
fix_1689_auto_transport_protocol
Mar 3, 2025
Merged

Fix Node Driver Crash Caused by Auto-Select Protocol#421
rishabhatdell merged 23 commits into
mainfrom
fix_1689_auto_transport_protocol

Conversation

@sakshi-garg1

@sakshi-garg1 sakshi-garg1 commented Jan 27, 2025 •
edited by rishabhatdell
Loading

Copy link
Copy Markdown
Contributor

Description

This PR addresses the issue of the node pod crashing when the X_CSI_TRANSPORT_PROTOCOL is empty in the powermax-array-config. The fix ensures that when the protocol is unspecified, the driver will prioritize available protocols in the following order:

  • NVMe/TCP
  • FC
  • iSCSI

The driver will automatically select the first available and working protocol from this list, establishing a connection based on these preferences.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
https://github.com/dell/csm/issues/1689
https://github.com/dell/csm/issues/1725

Checklist:

  • Have you run format,vet & lint checks against your submission?
  • Have you made sure that the code compiles?
  • Did you run the unit & integration tests successfully?
  • Have you maintained at least 90% code coverage?
  • Have you commented your code, particularly in hard-to-understand areas
  • Have you done corresponding changes to the documentation
  • Did you run tests in a real Kubernetes cluster?
  • 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

  • X_CSI_TRANSPORT_PROTOCOL empty with NVMe/TCP configured/ driver installed and running successfully + cert-csi
  • X_CSI_TRANSPORT_PROTOCOL empty with FC configured/ driver installed and running successfully + cert-csi
  • X_CSI_TRANSPORT_PROTOCOL empty with iSCSI configured/ driver installed and running successfully + cert-csi

@rishabhatdell rishabhatdell changed the title [WIP] fix_1689_auto_transport_protocol Fix Node Driver Crash Caused by Auto-Select Protocol Feb 6, 2025
@rishabhatdell rishabhatdell marked this pull request as ready for review February 7, 2025 12:36
@rishabhatdell rishabhatdell mentioned this pull request Feb 7, 2025
Closed
8 tasks
rishabhatdell
rishabhatdell previously approved these changes Feb 11, 2025
View reviewed changes
adarsh-dell
adarsh-dell reviewed Feb 11, 2025
View reviewed changes
Comment thread service/features/service.feature Outdated
adarsh-dell
adarsh-dell reviewed Feb 11, 2025
View reviewed changes
Comment thread service/features/service.feature
@adarsh-dell

adarsh-dell commented Feb 11, 2025

Copy link
Copy Markdown

@rishabhatdell @sakshi-garg1 , have we documented it properly in csm-doc that what is the priority order in case of Auto mode selection? If yes, please share the link of that section, would like to review that section to make sure that doc aligns with the code logic written here.
Thanks

@adarsh-dell

adarsh-dell commented Feb 11, 2025

Copy link
Copy Markdown

Please update the copyright year as well for all files that are being modified in this PR.

@rishabhatdell

rishabhatdell commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

@rishabhatdell @sakshi-garg1 , have we documented it properly in csm-doc that what is the priority order in case of Auto mode selection? If yes, please share the link of that section, would like to review that section to make sure that doc aligns with the code logic written here. Thanks

Added: dell/csm-docs#1450

adarsh-dell
adarsh-dell reviewed Feb 11, 2025
View reviewed changes
Comment thread service/service.go
suryagupta4
suryagupta4 previously approved these changes Feb 27, 2025
View reviewed changes
karthikk92
karthikk92 previously approved these changes Feb 27, 2025
View reviewed changes
adarsh-dell
adarsh-dell previously approved these changes Feb 27, 2025
View reviewed changes

@adarsh-dell adarsh-dell left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@suryagupta4 suryagupta4 dismissed stale reviews from adarsh-dell, karthikk92, and themself via 7c1b176 February 28, 2025 15:40
@santhoshatdell

santhoshatdell commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

@sakshi-garg1 @rishabhatdell : Multiple checks are failing now, after the UT PR merge. Please check and update this PR.

@github-actions

github-actions Bot commented Mar 3, 2025

Copy link
Copy Markdown
Contributor

Merging this branch will not change overall coverage

Impacted Packages Coverage Δ 🤖
github.com/dell/csi-powermax/service 0.00% (ø)
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/dell/csi-powermax/service/controller.go 0.00% (ø) 0 0 0
github.com/dell/csi-powermax/service/node.go 0.00% (ø) 0 0 0
github.com/dell/csi-powermax/service/service.go 0.00% (ø) 0 0 0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

@rishabhatdell rishabhatdell merged commit e855d72 into main Mar 3, 2025
@rishabhatdell rishabhatdell deleted the fix_1689_auto_transport_protocol branch March 3, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhi16394 abhi16394 Awaiting requested review from abhi16394

@bpjain2004 bpjain2004 Awaiting requested review from bpjain2004

@donatwork donatwork Awaiting requested review from donatwork

@jooseppi-luna jooseppi-luna Awaiting requested review from jooseppi-luna

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

5 more reviewers

@AkshaySainiDell AkshaySainiDell AkshaySainiDell approved these changes

@suryagupta4 suryagupta4 suryagupta4 approved these changes

@rishabhatdell rishabhatdell rishabhatdell approved these changes

@karthikk92 karthikk92 karthikk92 left review comments

@adarsh-dell adarsh-dell adarsh-dell left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sakshi-garg1 @adarsh-dell @rishabhatdell @santhoshatdell @karthikk92 @AkshaySainiDell @suryagupta4 @donatwork