UPSTREAM: <carry>: with shard prefix support #96

p0lyn0mial · 2022-08-26T13:51:57Z

What type of PR is this?

What this PR does / why we need it:

This PR adds support for setting a shard prefix to the storage layer.
The prefix is used by the cache server.
The storage layer has to understand both the cluster and the shard prefixes
because the database can be shared between the kcp and the cache server.

Once a shard name is set, the storage layer will change the key under which a resource is stored, for example:

amber/root:org/default/secret
amber/root:org/secret

Where:

amber: is a shard name
root:org: is a cluster name
default: is a namespace name
secret: is a name of a resource

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

p0lyn0mial · 2022-08-26T13:54:13Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

@@ -238,15 +238,23 @@ const (
 // to resource directories enforcing namespace rules.
 func NoNamespaceKeyRootFunc(ctx context.Context, prefix string) string {
 	key := prefix
+	shard, err := genericapirequest.ValidShardFrom(ctx)


we need a better way of plugging a shard name into the storage layer.
something that the kcp and the cache server could customize upon initialization.

initially, I wanted to plug in different implementations but I realised the kcp and the cache server might share the same database instance.

stevekuznetsov · 2022-08-26T15:16:33Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go

+
+type Shard struct {
+	// Name is the name of the cluster.
+	Name string


Given that we (likely) have shard name requirements, and * will not be a valid shard name, is there any reason not just have type Shard string? Otherwise, what is the meaning of:

Shard{ Name: "*", Wildcard: false, }

or

Shard{ Name: "sapphire", Wildcard: true, }

?

a request like 'https://localhost:6443/shards/*/clusters/*/apis/apis.kcp.dev/v1alpha1/apiexports' will set

Shard{ Name: "", Wildcard: true }

a request like https://localhost:6443/shards/sapphire/clusters/system:sapphire/apis/apis.kcp.dev/v1alpha1/apiexports' will set

Shard{ Name: "sapphire", Wildcard: false }

Right, those are the happy paths. My question was about the others, which are (I think) not possible. In e.g. kcp-dev/kcp#1841 you have a New('*") value for wildcard... seems like this could also ?

I do use New(*) on the client side just to set the right URL path, then on the server side, it gets mapped to Wildcard: true, Name: "".

I think we need both fields. An empty Name:"", Wildcard: false indicates no shard was specified.

How is Name:"", Wildcard: false different from New("")?

My argument: the Wildcard field as you have written it is logically equivalent in every case to obj.Name == "*"

@stevekuznetsov I updated the type to type Shard string, now Shard("*") indicates wildcard and Shard("") indicates a request without a shard name specified.

I hope it is less confusing now.

p0lyn0mial · 2022-08-30T11:48:39Z

/assign @stevekuznetsov @sttts

ready for review, PTAL.

p0lyn0mial · 2022-08-30T12:03:17Z

proof PR kcp-dev/kcp#1851

sttts · 2022-08-31T08:38:13Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go

+	Name string
+
+	// If true the query applies to all shards
+	Wildcard bool


why this complex struct again? Why not "empty shard string = wildcard" or "* shard string = wildcard" ?

Note: we should do the same for the cluster.

we need both, I can replace with a string but then we need to compare to *

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go

sttts · 2022-08-31T08:43:53Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go

@@ -133,8 +134,12 @@ func (w *watcher) Watch(ctx context.Context, key string, rev int64, recursive bo
 	if err != nil {
 		return nil, err
 	}
+	shard, err := genericapirequest.ValidShardFrom(ctx)


what does this return if there is no shard in the context?

// ValidShardFrom returns the value of the shard key from the context. // An empty value is returned if there's no shard key, // or if the shard name is empty, and it wasn't a wildcard request.

sttts · 2022-08-31T08:46:29Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go

+// ValidShardFrom returns the value of the shard key from the context.
+// An empty value is returned if there's no shard key,
+// or if the shard name is empty, and it wasn't a wildcard request.
+func ValidShardFrom(ctx context.Context) (*Shard, error) {


why "Valid"? ShardFromOrEmpty would be more idiomatic

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go

sttts · 2022-08-31T11:49:00Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go

@@ -45,26 +51,31 @@ var (
 // adjustClusterNameIfWildcard determines the logical cluster name. If this is not a wildcard list/watch request,
 // the cluster name is returned unmodified. Otherwise, the cluster name is extracted from the key based on whether
 // it is a CR partial metadata request (prefix/identity/clusterName/remainder) or not (prefix/clusterName/remainder).
-func adjustClusterNameIfWildcard(cluster *genericapirequest.Cluster, crdRequest bool, keyPrefix, key string) logicalcluster.Name {
+func adjustClusterNameIfWildcard(shard genericapirequest.Shard, cluster *genericapirequest.Cluster, crdRequest bool, keyPrefix, key string) logicalcluster.Name {
 	if cluster.Name != logicalcluster.Wildcard {
 		return cluster.Name
 	}

 	keyWithoutPrefix := strings.TrimPrefix(key, keyPrefix)

 	var regex *regexp.Regexp


move this into the first if.

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go

stevekuznetsov · 2022-08-31T12:52:49Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go

+
+// ShardFrom returns the value of the shard key in the context, or an empty value if there is no shard key.
+func ShardFrom(ctx context.Context) Shard {
+	shard, ok := ctx.Value(shardContextKey).(Shard)


nit: this simplifies to

func ShardFrom(ctx context.Context) Shard { return ctx.Value(shardContextKey).(Shard) }

playground

it could, it will just panic if the value under the key cannot be cast to Shard

The shardContextKey is private to this package. The only way that it would panic is if someone wrote code in this package to add the wrong thing. /shrug

yes, I just don't like when the code panics but sure I can change it.

and it broke the kcp server

I0901 10:30:42.331404 38583 etcd3.go:306] Start monitoring storage db size metric for endpoint https://localhost:45743 with polling interval 30s panic: interface conversion: interface {} is nil, not request.Shard goroutine 1 [running]: k8s.io/apiserver/pkg/endpoints/request.ShardFrom(...) /go/pkg/mod/github.com/p0lyn0mial/kubernetes/staging/src/k8s.io/apiserver@v0.0.0-20220901092652-1cf194952b52/pkg/endpoints/request/context_shard.go:56

stevekuznetsov · 2022-08-31T12:55:52Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

 	cluster, err := genericapirequest.ValidClusterFrom(ctx)
 	if err != nil {
 		klog.Errorf("invalid context cluster value: %v", err)
 		return key
 	}
-	if cluster.Wildcard {


This is a bit hard to follow. Is it equivalent:

if !shard.Empty() { key += "/" + shard.String() } if !cluster.Wildcard { key += "/" + cluster.Name.String() } return key

it seems it is :)

stevekuznetsov · 2022-08-31T13:11:32Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go

@@ -45,26 +51,31 @@ var (
 // adjustClusterNameIfWildcard determines the logical cluster name. If this is not a wildcard list/watch request,
 // the cluster name is returned unmodified. Otherwise, the cluster name is extracted from the key based on whether
 // it is a CR partial metadata request (prefix/identity/clusterName/remainder) or not (prefix/clusterName/remainder).
-func adjustClusterNameIfWildcard(cluster *genericapirequest.Cluster, crdRequest bool, keyPrefix, key string) logicalcluster.Name {
+func adjustClusterNameIfWildcard(shard genericapirequest.Shard, cluster *genericapirequest.Cluster, crdRequest bool, keyPrefix, key string) logicalcluster.Name {


IMHO this doesn't really make sense to do with regex. What about something like:

func adjustClusterNameIfWildcard(shard genericapirequest.Shard, cluster *genericapirequest.Cluster, crdRequest bool, keyPrefix, key string) logicalcluster.Name { if cluster.Name != logicalcluster.Wildcard || !cluster.Wildcard { // TODO: fix this duplicity, as well return cluster.Name } keyWithoutPrefix := strings.TrimPrefix(key, keyPrefix) var extractCluster func([]string) logicalcluster.Name extractor := func(numParts, clusterPart int) func([]string) logicalcluster.Name { return func([]string) logicalcluster.Name { if len(parts) < numParts { klog.Warnf("shard=%s cluster=%s invalid key=%s had %d parts, not %d", shard, cluster, keyWIthoutPrefix, len(parts), numParts) return logicalcluster.Name{} } return logicalcluster.New(parts[clusterPart]) } } switch { case shard.Wildcard(): // expecting shardName/clusterName/<remainder> extractCluster = extractor(3,1) case cluster.PartialMetadataRequest && crdRequest: // expecting 2699f4d273d342adccdc8a32663408226ecf66de7d191113ed3d4dc9bccec2f2/root:org:ws/<remainder> // OR customresources/root:org:ws/<remainder> extractCluster = extractor(3,1) case cluster.Wildcard(): // expecting root:org:ws/<remainder> extractCluster = extractor(2,0) default: panic("bad programmer") } parts := strings.Split(keyWithoutPrefix, "/") return extractCluster(parts) }

uh, maybe :) It seems like what we have right now requires less code.
Maybe I could revisit this in the future and try to rewrite (refactor) it.

Less code how exactly? Half of what I wrote were comments ;) Existing approach with your diff is L29-L80=51LoC. Mine is 35LoC

I think using regex to strings.Split(key, "/") makes the current approach really hard to follow, especially as we grow more cases.

Is it about taste or a big improvement? Either way is not particularly pretty. But this func does not decide about maintainability of the code, does it? Would be fine with a minimal change. Would pretty much prefer a unit test for either implementation ;-)

@stevekuznetsov would it be okay to add unit tests and leave using the regexps for now?

btw: do we actually run k8s unit tests :) ?

I don't think it's taste, no, because using regex to split a string on / is really hard to understand - I think we all tripped over understanding this function when reviewing it, no? Indexing into a slice seems ... slightly more straightforward, as well as using switch/case to be exhaustive on options?

No, we do not yet run the tests.

@p0lyn0mial can you create an issue to track Steve's suggestion above? Would not like to block this PR on a refactoring that is kind of independent of this change because pre-existing.

keep the suggested code snipped, or even create the follow-up PR just after if it compiles.

done kcp-dev/kcp#1875

sttts · 2022-09-01T09:13:44Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch_test.go

+		clusterWildcard bool
+		shardWildcard   bool
+		prefix          string
+		builtInType     bool


can this be pure table test with inputs and outputs? Irritating there is no "expected*" in here. Just put genericapirequest.Cluster and genericapirequest.Shard in here, no need for abstraction.

sttts · 2022-09-01T09:36:36Z

/lgtm
/approve

This PR adds support for setting a shard prefix to the storage layer. The prefix is used by the cache server. The storage layer has to understand both the cluster and the shard prefixes because the database can be shared between the kcp and the cache server. Once a shard name is set, the storage layer will change the key under which a resource is stored, for example: amber/root:org/default/secret amber/root:org/secret Where: - amber: is a shard name - root:org: is a cluster name - default: is a namespace name - secret: is a name of a resource

p0lyn0mial commented Aug 26, 2022

View reviewed changes

p0lyn0mial mentioned this pull request Aug 26, 2022

cache-server: adds WithShardScope HTTP filter kcp-dev/kcp#1841

Merged

stevekuznetsov reviewed Aug 26, 2022

View reviewed changes

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch 3 times, most recently from e120a07 to 4147b2a Compare August 30, 2022 11:46

p0lyn0mial changed the title ~~WIP: shard prefix~~ UPSTREAM: <carry>: with shard prefix support Aug 30, 2022

p0lyn0mial assigned sttts and stevekuznetsov Aug 30, 2022

sttts reviewed Aug 31, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go Show resolved Hide resolved

sttts reviewed Aug 31, 2022

View reviewed changes

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch from 4147b2a to 9333285 Compare August 31, 2022 09:47

sttts reviewed Aug 31, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/request/context_shard.go Show resolved Hide resolved

sttts reviewed Aug 31, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go Show resolved Hide resolved

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch 2 times, most recently from ba40ff6 to 97e00f1 Compare August 31, 2022 10:41

sttts reviewed Aug 31, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_patch.go Show resolved Hide resolved

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch from 97e00f1 to 6986f91 Compare August 31, 2022 12:21

stevekuznetsov reviewed Aug 31, 2022

View reviewed changes

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch 2 times, most recently from a5ea09c to 4ac288e Compare September 1, 2022 09:07

sttts reviewed Sep 1, 2022

View reviewed changes

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch from 4ac288e to 1cf1949 Compare September 1, 2022 09:27

p0lyn0mial mentioned this pull request Sep 1, 2022

bump to the latest kube level kcp-dev/kcp#1851

Merged

p0lyn0mial force-pushed the kcp-1.24-shard-prefix branch from 1cf1949 to c8d74a9 Compare September 1, 2022 10:37

sttts approved these changes Sep 1, 2022

View reviewed changes

sttts merged commit dc9535a into kcp-dev:feature-logical-clusters-1.24 Sep 1, 2022

UPSTREAM: <carry>: with shard prefix support #96

UPSTREAM: <carry>: with shard prefix support #96

Conversation

p0lyn0mial commented Aug 26, 2022 • 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.:

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

p0lyn0mial commented Aug 30, 2022

p0lyn0mial commented Aug 30, 2022

sttts Aug 31, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

stevekuznetsov Aug 31, 2022 • 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

sttts commented Sep 1, 2022

p0lyn0mial commented Aug 26, 2022 •

edited

Loading

sttts Aug 31, 2022 •

edited

Loading

p0lyn0mial Aug 31, 2022 •

edited

Loading

stevekuznetsov Aug 31, 2022 •

edited

Loading