Skip to content

Commit 0997ceb

Browse files
nsricardorspjmurray
nsricardor
authored and
spjmurray
committed
Add parallelism to storageclass (#407)
In version 1.14.1 we introduced support for parallelism on filestorage api schema. While that change unlocked new capabilities, it also revealed a few practical limitations: * Parallelism aligns more naturally with the storage class rather than filestorage, since the storage class is intended to define protocols and performance characteristics. * The schema allows parallelism to be updated, but the remote driver does not currently support updates to a VIP pool. Even with future support, reducing parallelism would require safeguards because removing VIPs could disrupt existing connections and mounted volumes. * The parallelism parameter is not persisted, which means clients must always include it in requests; otherwise, updates can fail when generating the desired state for attached networks. * A VIP pool can be shared across multiple filestorage instances on the same network. The current schema allows different parallelism levels per filestorage on that network, which is not feasible in practice. To simplify the model and better reflect how the system behaves, this PR moves the parallelism setting from FileStorage to the FileStorageClass definition. The change includes backward compatibility handling, and the FileStorageClass parallelism defaults to the current behavior unless explicitly updated. If this direction looks good, we can later adjust the FileStorageClass parallelism to the desired value.
1 parent d3793b2 commit 0997ceb

File tree

9 files changed

+453
-314
lines changed

9 files changed

+453
-314
lines changed

‎charts/region/crds/region.unikorn-cloud.org_filestorageclasses.yaml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ spec:
4949
spec:
5050
description: FileStorageClassSpec defines the FileStorageClass.
5151
properties:
52+
parallelism:
53+
default: 4
54+
description: |-
55+
Parallelism defines the number of IP addresses that are assigned to the storage.
56+
More IP addresses, better performance. If the value specified overflows
57+
the available address range reserved on the network it will be capped
58+
at the maximum allowed value.
59+
type: integer
5260
protocols:
5361
description: Protocols specifies the storage protocols (e.g., NFSv3,
5462
NFSv4) supported by this class.

‎pkg/apis/unikorn/v1alpha1/types.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -943,6 +943,12 @@ type FileStorageClassSpec struct {
943943
Provisioner string `json:"provisioner"`
944944
// Protocols specifies the storage protocols (e.g., NFSv3, NFSv4) supported by this class.
945945
Protocols []Protocol `json:"protocols,omitempty"`
946+
// Parallelism defines the number of IP addresses that are assigned to the storage.
947+
// More IP addresses, better performance. If the value specified overflows
948+
// the available address range reserved on the network it will be capped
949+
// at the maximum allowed value.
950+
// +kubebuilder:default=4
951+
Parallelism *int `json:"parallelism,omitempty"`
946952
}
947953

948954
type FileStorageClassStatus struct{}

‎pkg/apis/unikorn/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/handler/storage/client.go‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ import (
5252
"sigs.k8s.io/controller-runtime/pkg/client"
5353
)
5454

55+
const (
56+
// DefaultParallelism is the default number of IP addresses assigned to storage.
57+
// This maintains legacy default behaviour for storage classes that do not specify a value.
58+
DefaultParallelism = 4
59+
)
60+
5561
type Driver interface {
5662
GetDetails(ctx context.Context, projectID string, fileStorageID string) (*types.FileStorageDetails, error)
5763
}
@@ -254,15 +260,15 @@ func (c *Client) Get(ctx context.Context, storageID string) (*openapi.StorageV2R
254260
return storage, nil
255261
}
256262

257-
func (c *Client) generateV2(ctx context.Context, organizationID, projectID, regionID string, request *openapi.StorageV2Update, storageClassID string) (*regionv1.FileStorage, error) {
263+
func (c *Client) generateV2(ctx context.Context, organizationID, projectID, regionID string, request *openapi.StorageV2Update, storageClass *openapi.StorageClassV2Read) (*regionv1.FileStorage, error) {
258264
networkClient := network.New(c.ClientArgs)
259265

260266
err := util.InjectUserPrincipal(ctx, organizationID, projectID)
261267
if err != nil {
262268
return nil, fmt.Errorf("%w: unable to set principal information", err)
263269
}
264270

265-
attachments, err := generateAttachmentList(ctx, networkClient, request)
271+
attachments, err := generateAttachmentList(ctx, networkClient, request, storageClass.Spec.Parallelism)
266272
if err != nil {
267273
return nil, err
268274
}
@@ -280,7 +286,7 @@ func (c *Client) generateV2(ctx context.Context, organizationID, projectID, regi
280286
NFS: &regionv1.NFS{
281287
RootSquash: checkRootSquash(request.Spec.StorageType.NFS),
282288
},
283-
StorageClassID: storageClassID,
289+
StorageClassID: storageClass.Metadata.Id,
284290
},
285291
}
286292

@@ -302,19 +308,11 @@ func checkRootSquash(nfs *openapi.NFSV2Spec) bool {
302308
return true
303309
}
304310

305-
func generateAttachmentList(ctx context.Context, networkClient *network.Client, in *openapi.StorageV2Update) ([]regionv1.Attachment, error) {
311+
func generateAttachmentList(ctx context.Context, networkClient *network.Client, in *openapi.StorageV2Update, parallelism int) ([]regionv1.Attachment, error) {
306312
if in == nil || in.Spec.Attachments == nil {
307313
return []regionv1.Attachment{}, nil
308314
}
309315

310-
// This is the default parallelism e.g. 4 IP addresses. This maintains the
311-
// legacy default behaviour.
312-
parallelism := 4
313-
314-
if in.Spec.StorageType.NFS != nil && in.Spec.StorageType.NFS.Parallelism != nil {
315-
parallelism = *in.Spec.StorageType.NFS.Parallelism
316-
}
317-
318316
networkIDs := in.Spec.Attachments.NetworkIds
319317
out := make([]regionv1.Attachment, len(networkIDs))
320318

@@ -432,7 +430,12 @@ func (c *Client) Update(ctx context.Context, storageID string, request *openapi.
432430
return nil, errors.OAuth2InvalidRequest("filestorage is being deleted")
433431
}
434432

435-
s := newUpdateSaga(c, current, request)
433+
sc, err := c.GetStorageClass(ctx, current.Spec.StorageClassID)
434+
if err != nil {
435+
return nil, err
436+
}
437+
438+
s := newUpdateSaga(c, current, request, sc)
436439
if err := saga.Run(ctx, s); err != nil {
437440
return nil, err
438441
}
@@ -528,8 +531,9 @@ func convertClass(in *regionv1.FileStorageClass) *openapi.StorageClassV2Read {
528531
return &openapi.StorageClassV2Read{
529532
Metadata: conversion.ResourceReadMetadata(in, nil),
530533
Spec: openapi.StorageClassV2Spec{
531-
RegionId: in.Labels[constants.RegionLabel],
532-
Protocols: convertProtocols(in.Spec.Protocols),
534+
RegionId: in.Labels[constants.RegionLabel],
535+
Protocols: convertProtocols(in.Spec.Protocols),
536+
Parallelism: ensureParallelism(in.Spec.Parallelism),
533537
},
534538
}
535539
}
@@ -544,6 +548,17 @@ func convertProtocols(in []regionv1.Protocol) []openapi.StorageClassProtocolType
544548
return out
545549
}
546550

551+
// ensureParallelism returns the given parallelism value if it is valid (non-nil and >= 1),
552+
// otherwise falls back to DefaultParallelism. This guarantees callers always receive a
553+
// usable, non-nil parallelism value.
554+
func ensureParallelism(in *int) int {
555+
if in == nil || *in < 1 {
556+
return DefaultParallelism
557+
}
558+
559+
return *in
560+
}
561+
547562
func quantityToSizeGiB(quantity resource.Quantity) int64 {
548563
return (quantity.Value() / (1 << 30))
549564
}

‎pkg/handler/storage/client_test.go‎

Lines changed: 83 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -161,9 +161,7 @@ func TestGenerateAttachmentList(t *testing.T) {
161161
NetworkIds: openapi.NetworkIDList{"net-1"},
162162
},
163163
StorageType: openapi.StorageTypeV2Spec{
164-
NFS: &openapi.NFSV2Spec{
165-
Parallelism: ptr.To(4),
166-
},
164+
NFS: &openapi.NFSV2Spec{},
167165
},
168166
},
169167
},
@@ -191,7 +189,7 @@ func TestGenerateAttachmentList(t *testing.T) {
191189
t.Run(tt.name, func(t *testing.T) {
192190
t.Parallel()
193191

194-
got, err := generateAttachmentList(ctx, netclient, tt.input)
192+
got, err := generateAttachmentList(ctx, netclient, tt.input, DefaultParallelism)
195193
require.NoError(t, err)
196194
require.Equal(t, tt.want, got)
197195
})
@@ -648,7 +646,8 @@ func TestConvertClass(t *testing.T) {
648646
ProvisioningStatus: corev1.ResourceProvisioningStatusProvisioned,
649647
},
650648
Spec: openapi.StorageClassV2Spec{
651-
Protocols: []openapi.StorageClassProtocolType{},
649+
Parallelism: DefaultParallelism,
650+
Protocols: []openapi.StorageClassProtocolType{},
652651
},
653652
},
654653
},
@@ -683,8 +682,9 @@ func TestConvertClass(t *testing.T) {
683682
Name: "sc-name",
684683
},
685684
Spec: openapi.StorageClassV2Spec{
686-
RegionId: "region-1",
687-
Protocols: []openapi.StorageClassProtocolType{openapi.StorageClassProtocolTypeNfsv3, openapi.StorageClassProtocolTypeNfsv4},
685+
RegionId: "region-1",
686+
Parallelism: DefaultParallelism,
687+
Protocols: []openapi.StorageClassProtocolType{openapi.StorageClassProtocolTypeNfsv3, openapi.StorageClassProtocolTypeNfsv4},
688688
},
689689
},
690690
},
@@ -700,6 +700,68 @@ func TestConvertClass(t *testing.T) {
700700
}
701701
}
702702

703+
func TestGetStorageClassParallelism(t *testing.T) {
704+
t.Parallel()
705+
706+
tests := []struct {
707+
name string
708+
parallelism *int
709+
wantParallelism int
710+
}{
711+
{
712+
name: "explicit parallelism value",
713+
parallelism: ptr.To(8),
714+
wantParallelism: 8,
715+
},
716+
{
717+
name: "nil parallelism defaults to DefaultParallelism",
718+
parallelism: nil,
719+
wantParallelism: DefaultParallelism,
720+
},
721+
{
722+
name: "zero parallelism defaults to DefaultParallelism",
723+
parallelism: ptr.To(0),
724+
wantParallelism: DefaultParallelism,
725+
},
726+
}
727+
728+
for _, tt := range tests {
729+
t.Run(tt.name, func(t *testing.T) {
730+
t.Parallel()
731+
732+
storageClass := &regionv1.FileStorageClass{
733+
ObjectMeta: metav1.ObjectMeta{
734+
Name: "sc-1",
735+
Namespace: testNamespace,
736+
Labels: map[string]string{
737+
constants.RegionLabel: "region-1",
738+
},
739+
},
740+
Spec: regionv1.FileStorageClassSpec{
741+
Protocols: []regionv1.Protocol{regionv1.NFSv3},
742+
Parallelism: tt.parallelism,
743+
},
744+
}
745+
746+
k8sClient := newFakeClient(t, storageClass)
747+
748+
c := &Client{
749+
ClientArgs: common.ClientArgs{
750+
Client: k8sClient,
751+
Namespace: testNamespace,
752+
},
753+
}
754+
755+
ctx := newContextWithPermissions(t.Context())
756+
757+
result, err := c.GetStorageClass(ctx, "sc-1")
758+
require.NoError(t, err)
759+
require.NotNil(t, result)
760+
require.Equal(t, tt.wantParallelism, result.Spec.Parallelism)
761+
})
762+
}
763+
}
764+
703765
func TestConvertProtocols(t *testing.T) {
704766
t.Parallel()
705767

@@ -786,7 +848,7 @@ func TestGenerateV2(t *testing.T) {
786848

787849
client, ctx := newClientAndContext(t, k8s, &identityauth.Info{Userinfo: &identityopenapi.Userinfo{Sub: "user-1"}}, &principal.Principal{Actor: "actor@example.com"})
788850

789-
got, err := client.generateV2(ctx, tt.input.organizationID, tt.input.projectID, tt.input.regionID, tt.input.request, tt.input.storageClassID)
851+
got, err := client.generateV2(ctx, tt.input.organizationID, tt.input.projectID, tt.input.regionID, tt.input.request, tt.input.storageClass)
790852

791853
require.NoError(t, err)
792854
require.NotNil(t, got)
@@ -900,7 +962,7 @@ func TestGenerateV2Validations(t *testing.T) {
900962

901963
c, ctx := newClientAndContext(t, client, tt.authorization, tt.principal)
902964

903-
got, err := c.generateV2(ctx, tt.input.organizationID, tt.input.projectID, tt.input.regionID, tt.input.request, tt.input.storageClassID)
965+
got, err := c.generateV2(ctx, tt.input.organizationID, tt.input.projectID, tt.input.regionID, tt.input.request, tt.input.storageClass)
904966

905967
require.Nil(t, got)
906968
require.Error(t, err)
@@ -912,7 +974,7 @@ type generateV2Input struct {
912974
organizationID string
913975
projectID string
914976
regionID string
915-
storageClassID string
977+
storageClass *openapi.StorageClassV2Read
916978
request *openapi.StorageV2Update
917979
}
918980

@@ -925,7 +987,17 @@ func newDefaultGenerateV2Input() *generateV2Input {
925987
organizationID: "org-1",
926988
projectID: "proj-1",
927989
regionID: "reg-1",
928-
storageClassID: "sc-1",
990+
storageClass: &openapi.StorageClassV2Read{
991+
Metadata: corev1.ResourceReadMetadata{
992+
Id: "sc-1",
993+
Name: "sc-1",
994+
HealthStatus: corev1.ResourceHealthStatusHealthy,
995+
ProvisioningStatus: corev1.ResourceProvisioningStatusProvisioned,
996+
},
997+
Spec: openapi.StorageClassV2Spec{
998+
Parallelism: DefaultParallelism,
999+
},
1000+
},
9291001
request: &openapi.StorageV2Update{
9301002
Metadata: corev1.ResourceWriteMetadata{
9311003
Name: "test-filestorage",

0 commit comments

Comments
 (0)