Skip to content

Unity CSI driver incorrectly labelling nodes for NFS protocol#321

Merged
samihan-dell merged 16 commits into
mainfrom
usr/samihan-dell/nfs-label-check
Jul 7, 2025
Merged

Unity CSI driver incorrectly labelling nodes for NFS protocol#321
samihan-dell merged 16 commits into
mainfrom
usr/samihan-dell/nfs-label-check

Conversation

@samihan-dell

@samihan-dell samihan-dell commented Jun 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Add nfs labels for topology keys only when the corresponding NFS servers are enabled on the array.
ref PR: dell/gounity#94

GitHub Issues

List the GitHub issues impacted by this PR:

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

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?

  • Installed Driver with 1 NFS Enabled Array and nfs label applies as expected and able to provision volumes.
  • Installed Driver with 1 NFS Disabled Array and nfs label not applies as expected and able to provision volumes.
  • Installed Driver with Multi-Array (Array1: NFS Disabled & Array2: NFS Enabled):
    • NFS label not added for Array1 to csinode.
    • NFS label added for Array2 to csinode.
    • Provisioning failed for NFS volumes for Array1.
    • Able to provision for NFS volumes for Array2.
  • Upgrade test from 2.14 to latest driver
    • Provisioning failed for NFS volumes.
    • Provisioning worked for non-NFS volumes.
    • Tested against non-NFS volumes created before the upgrade:
      • Validated volume can be re-mounted to the same node as well as a different node.
      • Validated Snapshot:
        • Write data to the volume.
        • Validated that a snapshot can be taken.
        • Validated volume creation from snapshot.
      • Validated that volume cloning is possible.
        • Validated that the cloned volume can be mounted.

@samihan-dell samihan-dell mentioned this pull request Jul 1, 2025
Merged
12 tasks
@samihan-dell samihan-dell marked this pull request as ready for review July 1, 2025 11:29
AkshaySainiDell
AkshaySainiDell reviewed Jul 2, 2025
View reviewed changes
Comment thread service/node.go Outdated
Comment thread service/node_test.go
@samihan-dell samihan-dell changed the base branch from main to usr/samihan-dell/package-name-change July 5, 2025 18:07
@samihan-dell samihan-dell changed the base branch from usr/samihan-dell/package-name-change to main July 5, 2025 18:08
@samihan-dell samihan-dell marked this pull request as draft July 5, 2025 18:09
@github-actions

github-actions Bot commented Jul 6, 2025

Copy link
Copy Markdown
Contributor

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/dell/csi-unity/service 86.29% (+0.03%) 👍
github.com/dell/csi-unity/service/csiutils 88.41% (ø)
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/dell/csi-unity/service/controller.go 87.14% (ø) 1050 915 135
github.com/dell/csi-unity/service/csi_extension_server.go 96.22% (ø) 291 280 11
github.com/dell/csi-unity/service/csiutils/csiutils.go 88.41% (ø) 207 183 24
github.com/dell/csi-unity/service/node.go 79.88% (+0.17%) 964 (+8) 770 (+8) 194 👍
github.com/dell/csi-unity/service/service.go 92.21% (ø) 398 367 31
github.com/dell/csi-unity/service/validator.go 98.11% (ø) 159 156 3

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.

Changed unit test files

  • github.com/dell/csi-unity/service/controller_test.go
  • github.com/dell/csi-unity/service/csi_extension_test.go
  • github.com/dell/csi-unity/service/csiutils/csiutils_test.go
  • github.com/dell/csi-unity/service/node_test.go
  • github.com/dell/csi-unity/service/validator_test.go

@samihan-dell samihan-dell marked this pull request as ready for review July 6, 2025 13:21
@samihan-dell samihan-dell merged commit d4c8967 into main Jul 7, 2025
6 checks passed
@samihan-dell samihan-dell deleted the usr/samihan-dell/nfs-label-check branch July 7, 2025 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atye atye Awaiting requested review from atye

@ChristianAtDell ChristianAtDell Awaiting requested review from ChristianAtDell

@bandak2 bandak2 Awaiting requested review from bandak2

@Deepak-Ghivari Deepak-Ghivari Awaiting requested review from Deepak-Ghivari

@EvgenyUglov EvgenyUglov Awaiting requested review from EvgenyUglov

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

@francis-nijay francis-nijay Awaiting requested review from francis-nijay

@panigs7 panigs7 Awaiting requested review from panigs7

@rajendraindukuri rajendraindukuri Awaiting requested review from rajendraindukuri

@santhoshatdell santhoshatdell Awaiting requested review from santhoshatdell

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

2 more reviewers

@karthikk92 karthikk92 karthikk92 approved these changes

@AkshaySainiDell AkshaySainiDell AkshaySainiDell approved these changes

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.

3 participants

@samihan-dell @karthikk92 @AkshaySainiDell