[WIP] Storageversion API for CRDs - update in just one place #1

richabanker · 2024-03-12T17:43:36Z

What type of PR is this?

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.:

roycaihw · 2024-03-19T16:33:12Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

 			err := apierrors.NewServiceUnavailable(err.Error())
 			responsewriters.ErrorNegotiated(err, Codecs, schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}, w, req)
 			return nil
 		}
 		return handlers.UpdateResource(storage, requestScope, r.admission)
 	case "patch":
-		if err := crdInfo.waitForStorageVersionUpdate(req.Context()); err != nil {
+		if err := crdInfo.waitForStorageVersionUpdate(req.Context(), crd, latestSVUpdateInfo); err != nil {


Would be nice if we can dedup the cases here. Any code change that needs to be duplicated into each case seems fragile long-term.

roycaihw · 2024-03-19T16:39:40Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+}
+
+func (r *crdHandler) updateLatestStorageVersionUpdateInfo(crdUID types.UID, svUpdateInfo *storageVersionUpdateInfo) {
+	r.latestStorageVersionUpdateInfo.Store(crdUID, svUpdateInfo)


IIUC we are only tracking the latest teardown. If there are multiple versions being torn down, do we block new CR writes until all the versions have been torn down?

So, we will be blocking CR writes on only the latest SV update to finish meaning we will wait for the latest teardown to finish.
If multiple CRD updates are received over some period of time, v1 -> v2 -> v3 and a CR request happens after v3 update was requested, CR would be blocked on v3 update to finish, v3 update would be blocked on v2 teardown to finish (provided that v2 update was processed fully) and v2 update(historically) was blocked on v1 teardown to finish (provided that v1 update was processed fully).

If multiple rapid CRD updates come in, and the map gets updated from v1 -> v3 directly, the new CR would be blocked on just v1 teardown to finish.

So I guess at any given time, because we only process the latest SV update, there can only be a single teardown in progress?

roycaihw · 2024-03-19T16:41:46Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+	// timeout is the time when the API server will unblock and allow CR
+	// write requests even if the storage version update hasn't been
+	// processed.
+	timeout time.Time


I thought we won't serve CR requests if the SV update fails. WDYT?

Oh yes that's what the code is doing, i.e. failing the CR request if timeout occurs. Updated the comment to reflect the same.

Signed-off-by: Andrew Sy Kim <[email protected]> Co-authored-by: Haowei Cai (Roy) <[email protected]>

jpbetz · 2024-03-20T00:36:27Z

staging/src/k8s.io/apiextensions-apiserver/pkg/storageversion/manager.go

+var (
+	// map from crd.UID -> storageVersionUpdateInfo to keep
+	// track of the latest storageversion update for a CRD.
+	latestStorageVersionUpdateInfo sync.Map
+)


Is there a reason this needs to be global and not in Manager?

made it a global in Manager in kubernetes#123999. Wonder what the preference is between a sync.Map global v/s lock + map fields in the Manager struct?

Thx. sync.Map is specialized and typically is a good choice if each key is only ever written once but read many times. It's a bit of judgement call on when exactly to use it.

Thanks! Updated the main PR to use a single sync.Map as a struct field for the manager.

jpbetz · 2024-03-20T00:46:03Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+		if err := waitForStorageVersionUpdate(req.Context(), r, crd, requestInfo, w, req); err != nil {
+			err := apierrors.NewServiceUnavailable(err.Error())
+			responsewriters.ErrorNegotiated(err, Codecs, schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}, w, req)
+			return nil
+		}


Can we do something to reduce the copy-paste verbosity of these checks?

Updated the main PR with a helper to make this more readable.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

jpbetz · 2024-03-20T00:58:27Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+// removeStorageLocked removes the cached storage with the given uid as key from the storage map. This function
 // updates r.customStorage with the cleaned-up storageMap and tears down the old storage.


document the returned channel?

Noted, thanks! Will incorporate this in the main PR

Updated in the main PR

jpbetz · 2024-03-20T01:08:47Z

staging/src/k8s.io/apiextensions-apiserver/pkg/storageversion/manager.go

+					close(errCh)
+				}
+				return
+			case <-time.After(1 * time.Minute):


Should this 1 * time.Minute also be defined as a constant like storageVersionUpdateTimeout is?

Yeah good point. Will incorporate this in the main PR

Done in the main PR

richabanker · 2024-03-20T01:17:54Z

Can we continue the review of these changes in the PR kubernetes#123999 instead of here? I'll close this PR since this was made against my fork of k/k. I'll incorporate the feedback in the aforementioned PR, and close this one to avoid confusion.

richabanker force-pushed the sv-single-update branch 4 times, most recently from 975751f to ecf65bf Compare March 13, 2024 03:38

richabanker changed the base branch from master to storage-version-api-beta March 14, 2024 20:37

richabanker force-pushed the sv-single-update branch 2 times, most recently from d7091f2 to a70533b Compare March 19, 2024 04:30

roycaihw reviewed Mar 19, 2024

View reviewed changes

richabanker force-pushed the sv-single-update branch 3 times, most recently from ff3ee7c to ab525a8 Compare March 19, 2024 20:18

add initial support for StorageVersion API for CRDs

d3f14ca

Signed-off-by: Andrew Sy Kim <[email protected]> Co-authored-by: Haowei Cai (Roy) <[email protected]>

richabanker force-pushed the sv-single-update branch 6 times, most recently from 5873fae to 16d7158 Compare March 19, 2024 23:13

add worker for SV updates

36c0eb1

richabanker force-pushed the sv-single-update branch from 16d7158 to 36c0eb1 Compare March 19, 2024 23:39

jpbetz reviewed Mar 20, 2024

View reviewed changes

richabanker closed this Mar 20, 2024

This was referenced Feb 19, 2025

test api review #11

Closed

test api review for an open PR against k/k #128372 that has a huge diff #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Storageversion API for CRDs - update in just one place #1

[WIP] Storageversion API for CRDs - update in just one place #1

richabanker commented Mar 12, 2024

roycaihw Mar 19, 2024

roycaihw Mar 19, 2024

richabanker Mar 19, 2024 •

edited

Loading

richabanker Mar 19, 2024

roycaihw Mar 19, 2024

richabanker Mar 19, 2024 •

edited

Loading

jpbetz Mar 20, 2024

richabanker Mar 20, 2024

jpbetz Mar 20, 2024

richabanker Mar 20, 2024

jpbetz Mar 20, 2024

richabanker Mar 20, 2024

richabanker Mar 20, 2024

jpbetz Mar 20, 2024

richabanker Mar 20, 2024

richabanker Mar 20, 2024

jpbetz Mar 20, 2024

richabanker Mar 20, 2024

richabanker Mar 20, 2024

richabanker commented Mar 20, 2024

		// removeStorageLocked removes the cached storage with the given uid as key from the storage map. This function
		// updates r.customStorage with the cleaned-up storageMap and tears down the old storage.

[WIP] Storageversion API for CRDs - update in just one place #1

[WIP] Storageversion API for CRDs - update in just one place #1

Conversation

richabanker commented Mar 12, 2024

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.:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richabanker Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richabanker Mar 19, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richabanker commented Mar 20, 2024

richabanker Mar 19, 2024 •

edited

Loading

richabanker Mar 19, 2024 •

edited

Loading