From 4a1bf295bc943800a364d8b0b7b8f96ef8904496 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Fri, 19 Jun 2026 22:10:31 -0700 Subject: [PATCH] [manila-csi-plugin] Wait for access rule to become active after granting Manila's GrantAccess API is asynchronous. The access rule starts in "queued_to_apply" state and transitions through "applying" before reaching "active". Both the NFS and CephFS share adapters were proceeding to mount immediately after granting access, which could fail if the backend had not yet applied the rule. Add a shared waitForAccessRuleActive helper that polls until the access rule reaches "active" state. On "error" state or timeout, the helper revokes the failed rule so the next reconcile retries cleanly. Both adapters now call this helper after GrantAccess. For CephFS, this replaces the previous wait loop that only checked for AccessKey population without verifying the rule state itself. Signed-off-by: Goutham Pacha Ravi --- pkg/csi/manila/manilaclient/client.go | 4 + pkg/csi/manila/manilaclient/interface.go | 1 + pkg/csi/manila/shareadapters/accessrights.go | 94 +++++++++ .../manila/shareadapters/accessrights_test.go | 194 ++++++++++++++++++ pkg/csi/manila/shareadapters/cephfs.go | 33 +-- pkg/csi/manila/shareadapters/nfs.go | 6 +- tests/sanity/manila/fakemanilaclient.go | 15 ++ 7 files changed, 316 insertions(+), 31 deletions(-) create mode 100644 pkg/csi/manila/shareadapters/accessrights.go create mode 100644 pkg/csi/manila/shareadapters/accessrights_test.go diff --git a/pkg/csi/manila/manilaclient/client.go b/pkg/csi/manila/manilaclient/client.go index 24f0d7fbfa..20e8695d38 100644 --- a/pkg/csi/manila/manilaclient/client.go +++ b/pkg/csi/manila/manilaclient/client.go @@ -82,6 +82,10 @@ func (c Client) GrantAccess(ctx context.Context, shareID string, opts shares.Gra return shares.GrantAccess(ctx, c.c, shareID, opts).Extract() } +func (c Client) RevokeAccess(ctx context.Context, shareID string, accessID string) error { + return shares.RevokeAccess(ctx, c.c, shareID, shares.RevokeAccessOpts{AccessID: accessID}).ExtractErr() +} + func (c Client) GetSnapshotByID(ctx context.Context, snapID string) (*snapshots.Snapshot, error) { return snapshots.Get(ctx, c.c, snapID).Extract() } diff --git a/pkg/csi/manila/manilaclient/interface.go b/pkg/csi/manila/manilaclient/interface.go index a0ec8c7aa6..bcd9d2ed6d 100644 --- a/pkg/csi/manila/manilaclient/interface.go +++ b/pkg/csi/manila/manilaclient/interface.go @@ -42,6 +42,7 @@ type Interface interface { GetAccessRights(ctx context.Context, shareID string) ([]shares.AccessRight, error) GrantAccess(ctx context.Context, shareID string, opts shares.GrantAccessOptsBuilder) (*shares.AccessRight, error) + RevokeAccess(ctx context.Context, shareID string, accessID string) error GetSnapshotByID(ctx context.Context, snapID string) (*snapshots.Snapshot, error) GetSnapshotByName(ctx context.Context, snapName string) (*snapshots.Snapshot, error) diff --git a/pkg/csi/manila/shareadapters/accessrights.go b/pkg/csi/manila/shareadapters/accessrights.go new file mode 100644 index 0000000000..403bcd74a9 --- /dev/null +++ b/pkg/csi/manila/shareadapters/accessrights.go @@ -0,0 +1,94 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shareadapters + +import ( + "context" + "fmt" + "time" + + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" + "k8s.io/klog/v2" +) + +const ( + accessRuleStateActive = "active" + accessRuleStateError = "error" + accessRuleStateQueuedApply = "queued_to_apply" + accessRuleStateApplying = "applying" + + waitForAccessRuleTimeout = 3 + waitForAccessRuleRetries = 10 +) + +func waitForAccessRuleActive(ctx context.Context, manilaClient manilaclient.Interface, shareID, accessID string) (*shares.AccessRight, error) { + var ( + backoff = wait.Backoff{ + Duration: time.Second * waitForAccessRuleTimeout, + Factor: 1.2, + Steps: waitForAccessRuleRetries, + } + result *shares.AccessRight + ) + + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + rights, err := manilaClient.GetAccessRights(ctx, shareID) + if err != nil { + return false, fmt.Errorf("failed to get access rights for share %s: %v", shareID, err) + } + + for i := range rights { + if rights[i].ID != accessID { + continue + } + + switch rights[i].State { + case accessRuleStateActive: + result = &rights[i] + return true, nil + case accessRuleStateError: + revokeErr := manilaClient.RevokeAccess(ctx, shareID, accessID) + if revokeErr != nil { + klog.Errorf("failed to revoke errored access rule %s for share %s: %v", accessID, shareID, revokeErr) + } + return false, fmt.Errorf("access rule %s for share %s is in error state", accessID, shareID) + case accessRuleStateQueuedApply, accessRuleStateApplying: + klog.V(4).Infof("access rule %s for share %s is in state %s, retrying...", accessID, shareID, rights[i].State) + return false, nil + default: + return false, fmt.Errorf("access rule %s for share %s is in unexpected state %s", accessID, shareID, rights[i].State) + } + } + + return false, fmt.Errorf("access rule %s not found for share %s", accessID, shareID) + }) + + if err != nil { + if wait.Interrupted(err) { + revokeErr := manilaClient.RevokeAccess(ctx, shareID, accessID) + if revokeErr != nil { + klog.Errorf("failed to revoke timed-out access rule %s for share %s: %v", accessID, shareID, revokeErr) + } + return nil, fmt.Errorf("timed out waiting for access rule %s for share %s to become active", accessID, shareID) + } + return nil, err + } + + return result, nil +} diff --git a/pkg/csi/manila/shareadapters/accessrights_test.go b/pkg/csi/manila/shareadapters/accessrights_test.go new file mode 100644 index 0000000000..ee046f5e26 --- /dev/null +++ b/pkg/csi/manila/shareadapters/accessrights_test.go @@ -0,0 +1,194 @@ +/* +Copyright 2026 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shareadapters + +import ( + "context" + "strings" + "testing" + + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/messages" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/sharetypes" + "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/snapshots" + "k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient" +) + +var _ manilaclient.Interface = &mockManilaClient{} + +type mockManilaClient struct { + callCount int + accessRights [][]shares.AccessRight + revokeCalled bool +} + +func (m *mockManilaClient) GetMicroversion() string { return "" } +func (m *mockManilaClient) SetMicroversion(_ string) {} +func (m *mockManilaClient) GetShareByID(_ context.Context, _ string) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareByName(_ context.Context, _ string) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) CreateShare(_ context.Context, _ shares.CreateOptsBuilder) (*shares.Share, error) { + return nil, nil +} +func (m *mockManilaClient) DeleteShare(_ context.Context, _ string) error { return nil } +func (m *mockManilaClient) ExtendShare(_ context.Context, _ string, _ shares.ExtendOptsBuilder) error { + return nil +} +func (m *mockManilaClient) GetExportLocations(_ context.Context, _ string) ([]shares.ExportLocation, error) { + return nil, nil +} +func (m *mockManilaClient) SetShareMetadata(_ context.Context, _ string, _ shares.SetMetadataOptsBuilder) (map[string]string, error) { + return nil, nil +} +func (m *mockManilaClient) GrantAccess(_ context.Context, _ string, _ shares.GrantAccessOptsBuilder) (*shares.AccessRight, error) { + return nil, nil +} +func (m *mockManilaClient) GetSnapshotByID(_ context.Context, _ string) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) GetSnapshotByName(_ context.Context, _ string) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) CreateSnapshot(_ context.Context, _ snapshots.CreateOptsBuilder) (*snapshots.Snapshot, error) { + return nil, nil +} +func (m *mockManilaClient) DeleteSnapshot(_ context.Context, _ string) error { return nil } +func (m *mockManilaClient) GetExtraSpecs(_ context.Context, _ string) (sharetypes.ExtraSpecs, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareTypes(_ context.Context) ([]sharetypes.ShareType, error) { + return nil, nil +} +func (m *mockManilaClient) GetShareTypeIDFromName(_ context.Context, _ string) (string, error) { + return "", nil +} +func (m *mockManilaClient) GetUserMessages(_ context.Context, _ messages.ListOptsBuilder) ([]messages.Message, error) { + return nil, nil +} + +func (m *mockManilaClient) GetAccessRights(_ context.Context, _ string) ([]shares.AccessRight, error) { + idx := m.callCount + if idx >= len(m.accessRights) { + idx = len(m.accessRights) - 1 + } + m.callCount++ + return m.accessRights[idx], nil +} + +func (m *mockManilaClient) RevokeAccess(_ context.Context, _ string, _ string) error { + m.revokeCalled = true + return nil +} + +func TestWaitForAccessRuleAlreadyActive(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "active", AccessKey: "key123"}, + }, + }, + } + + result, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.ID != "ar-1" { + t.Errorf("expected ID ar-1, got %s", result.ID) + } + if result.AccessKey != "key123" { + t.Errorf("expected AccessKey key123, got %s", result.AccessKey) + } + if mock.callCount != 1 { + t.Errorf("expected 1 call, got %d", mock.callCount) + } +} + +func TestWaitForAccessRuleTransientToActive(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "queued_to_apply"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "applying"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "active", AccessKey: "key456"}, + }, + }, + } + + result, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.State != "active" { + t.Errorf("expected state active, got %s", result.State) + } + if result.AccessKey != "key456" { + t.Errorf("expected AccessKey key456, got %s", result.AccessKey) + } + if mock.callCount != 3 { + t.Errorf("expected 3 calls, got %d", mock.callCount) + } +} + +func TestWaitForAccessRuleErrorState(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-1", ShareID: "share-1", State: "queued_to_apply"}, + }, + { + {ID: "ar-1", ShareID: "share-1", State: "error"}, + }, + }, + } + + _, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "error state") { + t.Errorf("expected 'error state' in error message, got: %s", err.Error()) + } + if !mock.revokeCalled { + t.Error("expected RevokeAccess to be called") + } +} + +func TestWaitForAccessRuleNotFound(t *testing.T) { + mock := &mockManilaClient{ + accessRights: [][]shares.AccessRight{ + { + {ID: "ar-other", ShareID: "share-1", State: "active"}, + }, + }, + } + + _, err := waitForAccessRuleActive(context.Background(), mock, "share-1", "ar-1") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "not found") { + t.Errorf("expected 'not found' in error message, got: %s", err.Error()) + } +} diff --git a/pkg/csi/manila/shareadapters/cephfs.go b/pkg/csi/manila/shareadapters/cephfs.go index 4d3dcfe40b..5d9d9f1215 100644 --- a/pkg/csi/manila/shareadapters/cephfs.go +++ b/pkg/csi/manila/shareadapters/cephfs.go @@ -20,11 +20,9 @@ import ( "context" "fmt" "strings" - "time" "github.com/gophercloud/gophercloud/v2" "github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares" - "k8s.io/apimachinery/pkg/util/wait" manilautil "k8s.io/cloud-provider-openstack/pkg/csi/manila/util" "k8s.io/cloud-provider-openstack/pkg/util" "k8s.io/klog/v2" @@ -74,37 +72,12 @@ func (Cephfs) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([] if err != nil { return nil, fmt.Errorf("failed to grant access right: %v", err) } - if result.AccessKey == "" { - // Wait till a ceph key is assigned to the access right - backoff := wait.Backoff{ - Duration: time.Second * 5, - Factor: 1.2, - Steps: 10, - } - wait_err := wait.ExponentialBackoff(backoff, func() (bool, error) { - rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID) - if err != nil { - return false, fmt.Errorf("error get access rights for share %s: %v", args.Share.ID, err) - } - if len(rights) == 0 { - return false, fmt.Errorf("cannot find the access right we've just created") - } - for _, r := range rights { - if r.AccessTo == accessRightClient && r.AccessKey != "" { - accessRight = &r - return true, nil - } - } - klog.V(4).Infof("Access key for %s is not set yet, retrying...", accessRightClient) - return false, nil - }) - if wait_err != nil { - return nil, fmt.Errorf("timed out while attempting to get access rights for share %s: %v", args.Share.ID, err) - } + accessRight, err = waitForAccessRuleActive(ctx, args.ManilaClient, args.Share.ID, result.ID) + if err != nil { + return nil, err } } return []shares.AccessRight{*accessRight}, nil - } func (Cephfs) BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error) { diff --git a/pkg/csi/manila/shareadapters/nfs.go b/pkg/csi/manila/shareadapters/nfs.go index c74672e77e..a92f25f580 100644 --- a/pkg/csi/manila/shareadapters/nfs.go +++ b/pkg/csi/manila/shareadapters/nfs.go @@ -65,7 +65,11 @@ func (NFS) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([]sha if err != nil { return nil, fmt.Errorf("failed to grant access right: %v", err) } - rights = append(rights, *right) + activeRight, err := waitForAccessRuleActive(ctx, args.ManilaClient, args.Share.ID, right.ID) + if err != nil { + return nil, err + } + rights = append(rights, *activeRight) } } diff --git a/tests/sanity/manila/fakemanilaclient.go b/tests/sanity/manila/fakemanilaclient.go index a1531485bb..ad37920e09 100644 --- a/tests/sanity/manila/fakemanilaclient.go +++ b/tests/sanity/manila/fakemanilaclient.go @@ -216,12 +216,27 @@ func (c fakeManilaClient) GrantAccess(_ context.Context, shareID string, opts sh accessRight.ID = intToStr(fakeAccessRightID) accessRight.ShareID = shareID + accessRight.State = "active" fakeAccessRights[fakeAccessRightID] = accessRight fakeAccessRightID++ return accessRight, nil } +func (c fakeManilaClient) RevokeAccess(_ context.Context, shareID string, accessID string) error { + if !shareExists(shareID) { + return gophercloud.ErrResourceNotFound{} + } + + id := strToInt(accessID) + if _, ok := fakeAccessRights[id]; !ok { + return gophercloud.ErrResourceNotFound{} + } + + delete(fakeAccessRights, id) + return nil +} + func (c fakeManilaClient) GetSnapshotByID(_ context.Context, snapID string) (*snapshots.Snapshot, error) { s, ok := fakeSnapshots[strToInt(snapID)] if !ok {