shared block support - libopenstorage extensions #2470

sulakshm · 2024-08-26T07:18:00Z

What this PR does / why we need it:
Explain the PR and why it is needed.

Shared raw block extensions of libopenstorage - basic and necessary infra extensions only carved out of poc effort.
full poc here: https://github.com/pure-px/porx/pull/13673

Which issue(s) this PR fixes (optional)

PWX# https://purestorage.atlassian.net/browse/PWX-38767

Testing Notes
Add testing output or passing unit test output here.

Special notes for your reviewer:
Add any notes for the reviewer here.

lns@lsundararajan:~/srcs/src/github.com/libopenstorage/openstorage$ make docker-proto
tools/sdkver/sdkver.go:28:2: no required module provides package github.com/libopenstorage/openstorage/api: go.mod file not found in current directory or any parent directory; see 'go help modules'
docker pull quay.io/openstorage/osd-proto:pre-gomodules
pre-gomodules: Pulling from openstorage/osd-proto
0be2a68855d7: Pull complete
7411b9c958a5: Pull complete
a2897e1575f2: Pull complete
ab80474990d3: Pull complete
9279a3a30137: Pull complete
a1dd8e24fe6c: Pull complete
6446d66e18c2: Pull complete
f2fa2ceb3165: Pull complete
580d82cf7036: Pull complete
Digest: sha256:76b09a1a0231f9cf78f22eecd243d7ddb2bcc464e0cea9493a2d08dfe534ef14
Status: Downloaded newer image for quay.io/openstorage/osd-proto:pre-gomodules
quay.io/openstorage/osd-proto:pre-gomodules
docker run \
        --privileged --rm \
        -v /home/lns/srcs/src/github.com/libopenstorage/openstorage:/go/src/github.com/libopenstorage/openstorage \
        -e "GOPATH=/go" \
        -e "DOCKER_PROTO=yes" \
        -e "PATH=/bin:/usr/bin:/usr/local/bin:/go/bin:/usr/local/go/bin" \
        quay.io/openstorage/osd-proto:pre-gomodules \
                make proto mockgen
>>> Generating protobuf definitions from api/api.proto
protoc -I /go/src/github.com/libopenstorage/openstorage \
        -I /usr/local/include \
        -I /go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --go_out=plugins=grpc:. \
        /go/src/github.com/libopenstorage/openstorage/api/api.proto
protoc -I /go/src/github.com/libopenstorage/openstorage \
        -I /usr/local/include \
        -I /go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --grpc-gateway_out=logtostderr=true:. \
        /go/src/github.com/libopenstorage/openstorage/api/api.proto
protoc -I /go/src/github.com/libopenstorage/openstorage \
        -I /usr/local/include \
        -I /go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --swagger_out=logtostderr=true:/go/src/github.com/libopenstorage/openstorage/api/server/sdk \
        /go/src/github.com/libopenstorage/openstorage/api/api.proto
>>> Upgrading swagger 2.0 to openapi 3.0
mv api/server/sdk/api/api.swagger.json api/server/sdk/api/20api.swagger.json
swagger2openapi api/server/sdk/api/20api.swagger.json -o api/server/sdk/api/api.swagger.json
rm -f api/server/sdk/api/20api.swagger.json
>>> Generating grpc protobuf definitions from pkg/flexvolume/flexvolume.proto
protoc -I/usr/local/include -I/go/src/github.com/libopenstorage/openstorage -I/go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis --go_out=plugins=grpc:. /go/src/github.com/libopenstorage/openstorage/pkg/flexvolume/flexvolume.proto
protoc -I/usr/local/include -I/go/src/github.com/libopenstorage/openstorage -I/go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis --grpc-gateway_out=logtostderr=true:. /go/src/github.com/libopenstorage/openstorage/pkg/flexvolume/flexvolume.proto
>>> Generating protobuf definitions from pkg/jsonpb/testing/testing.proto
protoc -I /go/src/github.com/libopenstorage/openstorage /go/src/github.com/libopenstorage/openstorage/pkg/jsonpb/testing/testing.proto --go_out=plugins=grpc:.
>>> Updating SDK versions
go run tools/sdkver/sdkver.go --swagger api/server/sdk/api/api.swagger.json
0.101.54
go get github.com/golang/mock/gomock
go get github.com/golang/mock/mockgen || echo "ignoring go get build error"
package io/fs: unrecognized import path "io/fs" (import path does not begin with hostname)
ignoring go get build error
cd /go/src/github.com/golang/mock/mockgen && git checkout v1.2.0 && go install github.com/golang/mock/mockgen
Note: checking out 'v1.2.0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 51421b9 mockgen: use Controller.Helper() in generated mocks
mockgen -destination=api/mock/mock_storagepool.go -package=mock github.com/libopenstorage/openstorage/api OpenStoragePoolServer,OpenStoragePoolClient
mockgen -destination=api/mock/mock_cluster.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageClusterServer,OpenStorageClusterClient
mockgen -destination=api/mock/mock_node.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageNodeServer,OpenStorageNodeClient
mockgen -destination=api/mock/mock_diags.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageDiagsServer,OpenStorageDiagsClient
mockgen -destination=api/mock/mock_volume.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageVolumeServer,OpenStorageVolumeClient
mockgen -destination=api/mock/mock_watch.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageWatchServer,OpenStorageWatchClient,OpenStorageWatch_WatchClient,OpenStorageWatch_WatchServer
mockgen -destination=api/mock/mock_bucket.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageBucketServer,OpenStorageBucketClient
mockgen -destination=cluster/mock/cluster.mock.go -package=mock github.com/libopenstorage/openstorage/cluster Cluster
mockgen -destination=api/mock/mock_fstrim.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemTrimServer,OpenStorageFilesystemTrimClient
mockgen -destination=api/mock/mock_fscheck.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemCheckServer,OpenStorageFilesystemCheckClient
mockgen -destination=api/mock/mock_defrag.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemDefragServer,OpenStorageFilesystemDefragClient
mockgen -destination=api/server/mock/mock_schedops_k8s.go -package=mock github.com/portworx/sched-ops/k8s/core Ops
mockgen -destination=volume/drivers/mock/driver.mock.go -package=mock github.com/libopenstorage/openstorage/volume VolumeDriver
mockgen -destination=bucket/drivers/mock/bucket_driver.mock.go -package=mock github.com/libopenstorage/openstorage/bucket BucketDriver
lns@lsundararajan:~/srcs/src/github.com/libopenstorage/openstorage$

pnookala-px · 2024-10-04T02:55:41Z

pkg/options/options.go

+	// OptionsMigrateVolume control volume migration of shared raw block devices in kubevirt env for VMs
+	// - Attach
+	// This option indicates the next active coordinator node - machineID is expected
+	OptionsMigrateVolume = "VOLUME_MIGRATE_TO_MACHINE"
 )


does the above change need to be part of this PR?

the above change allows VM migration to be tested through CLI.
The entire shared block and VM migration features are developed and tested together.

pnookala-px · 2024-10-04T03:01:13Z

api/spec/spec_handler.go

@@ -894,6 +907,9 @@ func (d *specHandler) SpecOptsFromString(
 	if ok, fsFormatOptions := d.getVal(SpecFsFormatOptionsRegex, str); ok {
 		opts[api.SpecFsFormatOptions] = fsFormatOptions
 	}
+	if ok, shared := d.getVal(sharedBlockRegex, str); ok {
+		opts[api.SpecSharedBlock] = shared
+	}


There is spec_handler_test.go, can we add unit tests there as well?

pnookala-px

Changes look good. Just add a unit test case in spec-handelr_test.go and the migration option how it is relevant to current PR, is not very clear. I possible separate out adding of spec and migration related code into two different PR.s

sangleganesh

overall looks good but I have left some comments for you to consider.

sangleganesh · 2024-10-08T23:38:19Z

api/api.proto

+  uint32 source_node = 3; // source migration internal node id
+  string source_machine = 4; // source migration external machine id


do we need both (here and below ?) Ideally you should use only machine_id

This structure is used only for testing migration workflow.
source_machine is the external cli interface (pxctl host attach --migrate <machine id> vol)
source_node is the conversion to internal px-storage node id.
they are setup part of the spec, so grpc can push them along to px-storage. grpc does not have access to methods to do conversion.

sangleganesh · 2024-10-08T23:38:43Z

api/api.proto

+  MULTI_NODE_SINGLE_WRITER = 4; // read many write one volume support
+  MULTI_NODE_MULTI_WRITER = 5; // unrestricted rwx
+  SINGLE_NODE_SINGLE_WRITER = 6; // read write vol, from a single node
+  SINGLE_NODE_MULTI_WRITER = 7; // px default volume support


remove "px"

sangleganesh · 2024-10-08T23:39:42Z

api/api.proto

@@ -706,6 +732,8 @@ message VolumeSpec {
  bool near_sync = 51;
  // NearSyncReplicationStrategy is replication strategy for near sync volumes
  NearSyncReplicationStrategy near_sync_replication_strategy = 52;
+  // shared_block is true if this volume can be concurrently accessed by multiple nodes
+  bool shared_block = 54;


should we just add access mode enum member instead of another bool ? (shared, sharedv4, shared_block ?)

+1 on enum. Today RWO raw block is dictated through FSType field set to None instead of an explicit field for access. We could use the AccessMode to dictate RWO/RWX/ROX etc and another enum called volumeMode to dictate Block/File

Lots of this code is carried over for legacy reasons. I will convert shared_block to an enum, but will not touch other pieces of code that want to use this field.

adityadani · 2024-10-09T00:20:36Z

api/api.proto

+  // vm migration support
+  Migration migration = 33;
+  // ExportedOn tracks attached volumes on multiple nodes for RWX and ROX volumes
+  repeated string exported_on = 34;


Between attached_on and exported_on which field takes precedence?

px is still single coordinator logic.
attached_on is primary active coordinator.
exported_on is all encompassing inclusive of passive coordinators.

most of the code and logic should rely on existing attached_on.
only those logic that needs to work for srb should look at exported_on

adityadani · 2024-10-09T00:21:37Z

api/spec/spec_handler.go

@@ -106,6 +106,7 @@ var (
 	haRegex                       = regexp.MustCompile(api.SpecHaLevel + "=([0-9]+),?")
 	cosRegex                      = regexp.MustCompile(api.SpecPriority + "=([A-Za-z]+),?")
 	sharedRegex                   = regexp.MustCompile(api.SpecShared + "=([A-Za-z]+),?")
+	sharedBlockRegex              = regexp.MustCompile(api.SpecSharedBlock + "=([A-Za-z]+),?")


I would avoid defining anything here in spec handler since that results into a new storage class parameter which can be totally avoided by relying solely on PVC's accessMode & volumeMode fields.

I carried forward existing framework. csi spec does indeed carry right recommendations. i will focus on getting this field properly initialized without special storageclass needs.

Signed-off-by: lsundararajan <[email protected]>

sulakshm requested a review from pnookala-px August 26, 2024 07:18

sulakshm force-pushed the ln/release-9.9 branch 2 times, most recently from e86325d to 25d6405 Compare August 27, 2024 07:02

sulakshm changed the title ~~shared raw block support - libopenstorage extensions~~ shared block support - libopenstorage extensions Aug 27, 2024

sulakshm force-pushed the ln/release-9.9 branch 6 times, most recently from f521d58 to eea8ca2 Compare August 30, 2024 03:58

sulakshm force-pushed the ln/release-9.9 branch from eea8ca2 to ca857ac Compare October 1, 2024 19:27

pnookala-px reviewed Oct 4, 2024

View reviewed changes

sangleganesh approved these changes Oct 8, 2024

View reviewed changes

adityadani reviewed Oct 9, 2024

View reviewed changes

lsundararajan added 3 commits October 9, 2024 12:39

PWX-38767: shared block support - libopenstorage extensions

e3d3397

Signed-off-by: lsundararajan <[email protected]>

ut for shared block options

e982e34

Signed-off-by: lsundararajan <[email protected]>

review feedback

c8abd44

Signed-off-by: lsundararajan <[email protected]>

sulakshm force-pushed the ln/release-9.9 branch from 5eb123b to c8abd44 Compare October 9, 2024 12:43

lsundararajan added 6 commits October 9, 2024 13:12

review feedback

8818b2e

Signed-off-by: lsundararajan <[email protected]>

compile fixup

7c0a5ff

Signed-off-by: lsundararajan <[email protected]>

compile fixup

91ece4b

Signed-off-by: lsundararajan <[email protected]>

cleanup

95dd911

Signed-off-by: lsundararajan <[email protected]>

fix compile

01e7df4

Signed-off-by: lsundararajan <[email protected]>

compile fix

60432ac

Signed-off-by: lsundararajan <[email protected]>

ppadalia closed this Nov 14, 2024

ppadalia reopened this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shared block support - libopenstorage extensions #2470

shared block support - libopenstorage extensions #2470

sulakshm commented Aug 26, 2024 •

edited

Loading

pnookala-px Oct 4, 2024

sulakshm Oct 7, 2024

pnookala-px Oct 4, 2024

sulakshm Oct 7, 2024

pnookala-px left a comment

sangleganesh left a comment

sangleganesh Oct 8, 2024

sulakshm Oct 9, 2024 •

edited

Loading

sangleganesh Oct 8, 2024

sangleganesh Oct 8, 2024

adityadani Oct 9, 2024 •

edited

Loading

sulakshm Oct 9, 2024

adityadani Oct 9, 2024

sulakshm Oct 9, 2024

adityadani Oct 9, 2024

sulakshm Oct 9, 2024

		uint32 source_node = 3; // source migration internal node id
		string source_machine = 4; // source migration external machine id

shared block support - libopenstorage extensions #2470

Are you sure you want to change the base?

shared block support - libopenstorage extensions #2470

Conversation

sulakshm commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnookala-px left a comment

Choose a reason for hiding this comment

sangleganesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sulakshm Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityadani Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sulakshm commented Aug 26, 2024 •

edited

Loading

sulakshm Oct 9, 2024 •

edited

Loading

adityadani Oct 9, 2024 •

edited

Loading