Support host-based NFS replication by lukeatdell · Pull Request #466 · dell/csi-powerstore

lukeatdell · 2025-04-14T21:14:28Z

Description

These changes bring csi-powerstore host-based NFS volumes into scope for replication via csm-replication.
Main changes are in pkg/controller/replication.go.

Other changes include:

Refactoring the volume handle parsing function to return a single VolumeHandle struct with members defining the different parts of the volume handle.

GitHub Issues

List the GitHub issues impacted by this PR:

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

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 -- waiting on official feature name
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?

cert-csi certify for host-based nfs
cert-csi certify for native nfs
cert-csi certify for iscsi block
cert-csi certify for iscsi metro
replication e2e

storageClasses:
  - name: powerstore-iscsi
    minSize: 5Gi
    rawBlock: true
    expansion: true
    clone: true
    snapshot: true
    RWX: false
    RWOP: true
    ephemeral:
      driver: csi-powerstore.dellemc.com
      fstype: ext4
      volumeAttributes:
        arrayID: "<arrayid>"
        protocol: iSCSI
        size: 5Gi
  - name: powerstore-nfs
    minSize: 5Gi
    rawBlock: false
    expansion: true
    clone: true
    snapshot: true
    RWX: true
    RWOP: false
    ephemeral:
      driver: csi-powerstore.dellemc.com
      fstype: "nfs"
      volumeAttributes:
        arrayID: "<arrayid>"
        protocol: NFS
        size: 5Gi
        nasName: "<nas-name>"
  - name: powerstore-metro-ext4
    minSize: 5Gi
    rawBlock: true
    expansion: false
    clone: false
    snapshot: false
    RWX: false
    RWOP: true
  - name: powerstore-hbnfs
    minSize: 5Gi
    rawBlock: false
    expansion: true
    clone: false
    snapshot: true
    RWX: false
    RWOP: true
    ephemeral:
      driver: csi-powerstore.dellemc.com
      fstype: "nfs"
      volumeAttributes:
        arrayID: "<arrayid>"
        protocol: NFS
        size: 5Gi

- adding prefix detection and parsing to the parsing function had unintended side effects during testing. - updating method of prefix detection to isolate detection only during replication calls.

santhoshatdell · 2025-04-15T17:03:02Z

+		// host-based nfs volumes should have the "csi-nfs" parameter
+		// and a "nfs-" prefix in the volume ID that we need to remove
+		// for gopowerstore queries to succeed.
+		// Remove the prefix here and restore when building the response.


I see that we are adding the prefix to the remote volume only. Should the comment be updated?

The prefix is actually part of the volume handle for both local and remote.
When the request comes into the driver to CreateRemoteVolume() the local volume handle already contains the prefix, and we want to make sure the volume handle returned by this same function also adds the prefix, so the driver on the remote side knows it is a host-based nfs volume.

I'm open to suggestions on better wording to communicate the idea to the reader tho.

github-actions · 2025-04-15T18:55:11Z

Merging this branch will not change overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/dell/csi-powerstore/pkg/array	0.00% (ø)
github.com/dell/csi-powerstore/pkg/controller	0.00% (ø)
github.com/dell/csi-powerstore/pkg/node	0.00% (ø)

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/dell/csi-powerstore/pkg/array/array.go	0.00% (ø)	0	0	0
github.com/dell/csi-powerstore/pkg/controller/controller.go	0.00% (ø)	0	0	0
github.com/dell/csi-powerstore/pkg/controller/csi_extension_server.go	0.00% (ø)	0	0	0
github.com/dell/csi-powerstore/pkg/controller/replication.go	0.00% (ø)	0	0	0
github.com/dell/csi-powerstore/pkg/node/node.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.

Changed unit test files

github.com/dell/csi-powerstore/pkg/array/array_test.go
github.com/dell/csi-powerstore/pkg/controller/controller_test.go
github.com/dell/csi-powerstore/pkg/controller/replication_test.go

lukeatdell added 10 commits April 14, 2025 12:45

adding unit test to support hbnfs replication

89f047a

update parseVolumeID func and tests

b2957be

fixing expected returned prefix in parseVolumeID test

edf1d38

add func to get a prefix from a volume ID

be7a743

updating usage of parseVolumeID()

89ecf2d

add support for hbnfs replication

a0c0b05

updating debug logs and descriptions

cc2680d

renaming vars and struct members

93e957f

remove prefix from parsing and refactor.

8459ad2

- adding prefix detection and parsing to the parsing function had unintended side effects during testing. - updating method of prefix detection to isolate detection only during replication calls.

refactor prefix parsing func

5c44b94

lukeatdell marked this pull request as ready for review April 15, 2025 15:34

lukeatdell requested review from AkshaySainiDell, abhi16394, adarsh-dell, donatwork and santhoshatdell as code owners April 15, 2025 15:34

santhoshatdell reviewed Apr 15, 2025

View reviewed changes

falfaroc previously approved these changes Apr 15, 2025

View reviewed changes

PR comments: santhoshatdell

3176308

lukeatdell dismissed falfaroc’s stale review via 3176308 April 15, 2025 17:42

lukeatdell added 2 commits April 15, 2025 13:52

edit for clarity

00ca5c9

add check for csi-nfs parameter key's value

31c82f8

santhoshatdell approved these changes Apr 15, 2025

View reviewed changes

falfaroc approved these changes Apr 15, 2025

View reviewed changes

lukeatdell merged commit 22ed84c into main Apr 15, 2025

lukeatdell deleted the usr/lukeatdell/hbnfs-replication-support branch April 15, 2025 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support host-based NFS replication#466

Support host-based NFS replication#466
lukeatdell merged 13 commits into
mainfrom
usr/lukeatdell/hbnfs-replication-support

lukeatdell commented Apr 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

santhoshatdell Apr 15, 2025 •

edited

Loading

Uh oh!

lukeatdell Apr 15, 2025

Uh oh!

github-actions Bot commented Apr 15, 2025

Changed files (no unit tests)

Changed unit test files

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lukeatdell commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

GitHub Issues

Checklist:

How Has This Been Tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

santhoshatdell Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeatdell Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 15, 2025

Merging this branch will not change overall coverage

Changed files (no unit tests)

Changed unit test files

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukeatdell commented Apr 14, 2025 •

edited

Loading

santhoshatdell Apr 15, 2025 •

edited

Loading