Refactor nas plugin and support accesspoint #914

tydra-wang · 2023-11-22T08:29:05Z

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-11-22T08:29:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2023-11-22T08:29:09Z

@tydra-wang: The label(s) kind/refactor cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind feature
/kind refactor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

huww98 · 2023-11-23T03:35:23Z

Maybe you can use #873 to implement v2 Auth

huww98 · 2023-11-23T12:41:46Z

Please see #874 for upgrade deps.

pkg/nas/accesspoint_controller.go

pkg/nas/cloud/nas_client_v2.go

pkg/nas/filesystem_controller.go

pkg/nas/internal/config.go

pkg/utils/auth.go

huww98 · 2024-03-08T09:15:35Z

Please rebase and get rid of the commit Revert "remove staging nas sdk", maybe combine it with remove staging nas sdk

feat(nas): NodePublish creates subpath if not exists nas: refactor subpath controllerserver nas: split filesystem controllerserver implements nas: split sharepath controllerserver implements refactor(nas): controller server chore: add staging and vendor refactor(nas): controller server fix(nas): init loop image file when NodePublishVolume upgrade client-go dependencies nas: patch deletion finalizer using ssa chore(nas): improve logs fix rebase fix(nas): set global query RegionId in v2 sdk client fix(nas): nas-vpc endpoint not set for some regions cleanup: fix deprecated function cleanup: rename pkg/nas/internal to pkg/nas/cloud refactor(nas): controller init refactor(nas): driver init refactor(nas): driver configs feat(nas): support mount accesspoint cleanup: constants fix(nas): use node name as node id feat(nas): support accesspoint provision

…ocessSuccess

mowangdk · 2024-03-11T09:39:33Z

pkg/nas/accesspoint_controller.go

+	}
+
+	var invalidParameters []string
+	parseParameter := func(key string, target any) {


elegant 👍

I would prefer to just switch on key. So that we can report errors on unknown keys. And we can get a compilation error when unknown value type is encountered (maybe due to future SDK update).

mowangdk · 2024-03-11T09:47:17Z

/lgtm

mowangdk · 2024-03-11T11:45:34Z

/approve

k8s-ci-robot · 2024-03-11T11:45:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mowangdk, tydra-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mowangdk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

huww98 · 2024-03-11T12:04:48Z

pkg/nas/accesspoint_controller.go

+	parseParameter("accesspointPermission", &req.Permission)
+	parseParameter("accesspointPosixUserId", &req.PosixUserId)
+	parseParameter("accesspointPosixGroupId", &req.PosixGroupId)
+	parseParameter("accesspointPosixSecondaryGroupIds", &req.PosixSecondaryGroupIds)


BTW, what is the expected format of this parameter? This looks strange, I would expect the type to be []int32, but it is *string.

huww98 · 2024-03-11T12:12:58Z

pkg/nas/cloud/nas_client_v2.go

+	if err == nil {
+		return false
+	}
+	sdkErr, ok := err.(*tea.SDKError)


prefer errors.As()

huww98 · 2024-03-11T12:19:23Z

pkg/nas/internal/config.go

+	}
+
+	// get csi-plugin configmap
+	cm, err := kubeClient.CoreV1().ConfigMaps(configMapNamespace).Get(context.Background(), configMapName, metav1.GetOptions{})


We can just read /etc/csi-plugin/config on node

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 22, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 22, 2023

tydra-wang changed the title ~~refactor nas controllerserver and utilize latest NAS CreateDir API~~ WIP: refactor nas controllerserver and utilize latest NAS CreateDir API Nov 22, 2023

k8s-ci-robot requested a review from mowangdk November 22, 2023 08:29

tydra-wang force-pushed the refactor-nas branch 6 times, most recently from 61fb40d to 95651d3 Compare November 24, 2023 07:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2023

tydra-wang force-pushed the refactor-nas branch from 1f576f9 to 2370fca Compare December 4, 2023 08:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2023

tydra-wang force-pushed the refactor-nas branch 2 times, most recently from c753c10 to 861d7e0 Compare December 7, 2023 03:58

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

tydra-wang force-pushed the refactor-nas branch 2 times, most recently from c6fa76c to ffd654b Compare February 29, 2024 06:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 29, 2024

tydra-wang force-pushed the refactor-nas branch from ffd654b to d0af129 Compare February 29, 2024 06:26

tydra-wang changed the title ~~WIP: refactor nas controllerserver and utilize latest NAS CreateDir API~~ WIP: refactor nas plugin and support accesspoint Feb 29, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024

tydra-wang force-pushed the refactor-nas branch from 5739c02 to d1680b9 Compare March 6, 2024 03:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024

tydra-wang force-pushed the refactor-nas branch from 096cf61 to 8dba9ed Compare March 8, 2024 03:24

mowangdk reviewed Mar 8, 2024

View reviewed changes

pkg/nas/accesspoint_controller.go Show resolved Hide resolved

pkg/nas/cloud/nas_client_v2.go Show resolved Hide resolved

pkg/nas/filesystem_controller.go Outdated Show resolved Hide resolved

pkg/nas/internal/config.go Show resolved Hide resolved

huww98 reviewed Mar 8, 2024

View reviewed changes

pkg/utils/auth.go Show resolved Hide resolved

tydra-wang force-pushed the refactor-nas branch from 8dba9ed to 6e48bc9 Compare March 11, 2024 03:13

tydra-wang added 9 commits March 11, 2024 17:17

fix: managedtoken expiration

125b043

nas: do not use server-side-apply when patch

1997976

nas: add ENABLE_NAS_SUBPATH_FINALIZER env variable config

4d8d78c

fix rebase

fd66ad1

chore: add comments

7c55610

feat: bump alinas-utils rpm version

0cf914d

fix(nas): accesspoint provision

474659e

fix(nas): use sync.Map for pvcMountTargetMap/pvcFileSystemIDMap/pvcPr…

c5d950a

…ocessSuccess

tydra-wang force-pushed the refactor-nas branch from 3571c50 to c5d950a Compare March 11, 2024 09:18

tydra-wang marked this pull request as ready for review March 11, 2024 09:21

k8s-ci-robot requested a review from mowangdk March 11, 2024 09:22

mowangdk reviewed Mar 11, 2024

View reviewed changes

k8s-ci-robot assigned mowangdk Mar 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2024

tydra-wang changed the title ~~WIP: refactor nas plugin and support accesspoint~~ Refactor nas plugin and support accesspoint Mar 11, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2024

k8s-ci-robot merged commit 94b0e8a into kubernetes-sigs:master Mar 11, 2024
6 checks passed

huww98 reviewed Mar 11, 2024

View reviewed changes

pkg/nas/cloud/nas_client_v2.go

if err == nil {

return false

}

sdkErr, ok := err.(*tea.SDKError)

Copy link

Contributor

huww98 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer errors.As()

huww98 reviewed Mar 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor nas plugin and support accesspoint #914

Refactor nas plugin and support accesspoint #914

tydra-wang commented Nov 22, 2023 •

edited

Loading

k8s-ci-robot commented Nov 22, 2023

k8s-ci-robot commented Nov 22, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

huww98 commented Nov 23, 2023

huww98 commented Nov 23, 2023

huww98 commented Mar 8, 2024

mowangdk Mar 11, 2024

huww98 Mar 11, 2024

mowangdk commented Mar 11, 2024

mowangdk commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

huww98 Mar 11, 2024

huww98 Mar 11, 2024

huww98 Mar 11, 2024

Refactor nas plugin and support accesspoint #914

Refactor nas plugin and support accesspoint #914

Conversation

tydra-wang commented Nov 22, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Nov 22, 2023

k8s-ci-robot commented Nov 22, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

huww98 commented Nov 23, 2023

huww98 commented Nov 23, 2023

huww98 commented Mar 8, 2024

mowangdk Mar 11, 2024

Choose a reason for hiding this comment

huww98 Mar 11, 2024

Choose a reason for hiding this comment

mowangdk commented Mar 11, 2024

mowangdk commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

huww98 Mar 11, 2024

Choose a reason for hiding this comment

huww98 Mar 11, 2024

Choose a reason for hiding this comment

huww98 Mar 11, 2024

Choose a reason for hiding this comment

tydra-wang commented Nov 22, 2023 •

edited

Loading