✨ Partitionset reconciliation #2513

fgiloux · 2022-12-20T12:34:18Z

Summary

This PR implements a reconciliation mechanism so that shard partitions can be automatically created based on specified dimensions and selector.

Design document

Related issue(s)

Follows #2469
Fixes #2334

fgiloux · 2023-01-13T20:34:44Z

/retest

p0lyn0mial

I did a first pass, for now only over the API. Let me know what you think.

p0lyn0mial · 2023-01-16T16:39:56Z

pkg/apis/topology/v1alpha1/types_partitionset.go

+	// the MatchLabels generated for the Partition according to the dimensions in the PartitionSet.
+	// This does not limit the filtering as any MatchLabel can also be expressed as
+	// a MatchExpression.
+	MatchExpressions []metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty"`


I think that a standard selector can be matchLabels or/and matchExpression and I think that both can be set at the same time. Not sure if I understand why we cannot use a standard label.

Also could we rename it to shardSelector/shardMatchExpression ?

as per the comment " MatchExpressions are used here instead of Selector to avoid possible collisions with the MatchLabels generated for the Partition".
PartitionSets are used to generate Partitions. Partitions have a LabelSelector. LabelSelectors have two parts:

MatchLabels

MatchExpressions

MatchLabels of Partitions are generated from the Dimension field of PartitionSet. MatchExpressions of Partitions are copied from MatchExpressions of PartitionSet. Keeping this separation provides a clearer UX. It would be more difficult for users to understand the generated Partitions from a PartitionSet if MatchLabels could be a mix of the Dimension generation and MatchLabels inherited from the PartitionSet. As per the comment there is no trade-off in terms of capabilities: any MatchLabels can be expressed by MatchExpressions.

as per the comment " MatchExpressions are used here instead of Selector to avoid possible collisions with the MatchLabels generated for the Partition".

if we have a collision, does it mean we can achieve the same by providing a selector on a Partition resource :) ?

If I understand your point correctly PartitionSet is just a convenient way to create Partitions. Service Providers are free to create Partitions the way it best suits them: using PartitionSet or directly creating the resource. If they do it manually they can set Selectors: MatchLabels and MatchExpressions in Partitions in a similar way to what is generated by the controller.

p0lyn0mial · 2023-01-16T16:40:54Z

pkg/reconciler/topology/partitioning_test.go

@@ -0,0 +1,125 @@
+/*
+Copyright 2022 The KCP Authors.


nit: 2023 (applies to other places as well)

The PR and commit were created in 2022

p0lyn0mial · 2023-01-16T21:25:31Z

pkg/apis/topology/v1alpha1/types_partitionset.go

+	// the MatchLabels generated for the Partition according to the dimensions in the PartitionSet.
+	// This does not limit the filtering as any MatchLabel can also be expressed as
+	// a MatchExpression.
+	MatchExpressions []metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty"`


Would it make sense to make PartitionSetSpec.Dimensions and/or PartitionSetSpec.matchExpressions/PartitionSetSpec.selector immutable ?

I think this could be less disruptive. A SP could create a new PartitionSet wait for a Partition to be created, switch an APIExport to the new Partition and then delete the old PartitionSet

As I see it this is not the likely flow. This does not take in considerations that Partitions need to be colocated with the APIExportEndpointSlices that consume them. Here is how I see the flow:

A service provider update a PartitionSet

The set of Partitions bound to the PartitionSet is updated accordingly in the PartitionSet workspace

Each of this Partition is copied into the workspace where the intended APIExportEndpointSlice is located. Each APIExportEndpointSlice is intended for an operator/controller manager instance. It makes URLs for a set of shards identified by the Partition available to it.

The service provider can choose at any convenient time to patch the APIExportEndpointSlices with the new Partitions
There is no modification of the service and no disruption until the APIExportEndpointSlices are patched.

The steps above could be automated but it is another discussion.

I'm not sure if I read the code correctly but it seems that we are deleting old partitions potentially being used by APIExportEndpointSlice which seems to be disruptive.

xref:

kcp/pkg/reconciler/topology/partitionset_reconcile.go

Line 123 in c991948

if err := c.deletePartition(ctx, logicalcluster.From(oldPartition).Path(), oldPartition.Name); err != nil && !apierrors.IsNotFound(err) {

Yes we do delete partitions but only in the workspace where the PartitionSet has been created. Partitions used by APIExportEndpointSlices should be copied into the workspaces where the APIExportEndpointSlices are located to suit the placement decision: regional, cloud proximity. Theses copied Partitions should not get modified or deleted by the reconciliation of the PartitionSet.

p0lyn0mial · 2023-01-18T07:46:53Z

pkg/reconciler/topology/resources.go

+func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequirement, matchLabels map[string]string) *topologyv1alpha1.Partition {
+	return &topologyv1alpha1.Partition{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: name + "-",


It appears to me that providers will have to manually introspect generated Partitions to make use of them because names appear random and there is no explanation of the generated selector, am I right?

Wondering if we could/should improve this?

Names are generated as Partitions are. We could create deterministic names by hashing the Partition spec but I am not sure it would be reasonable. An annotation may be better suited to store the hash. This would be stable as the PartitionSet reconciliation only does Partition create/delete, no update.
That said I am not sure it would be a significant improvement:

Shard topology should be stable, e.g. you may add new shards in a region time to time but adding a new region is a rare event that would need action from Service Providers for them to cover it. It would also mean just a few additional Partitions to be taken care of. Deletion is not much different.

PartitionSet change is driven by Service Provider and it means that they want to change the distribution of their service deployments. In that case they can check the deltas between the deployed Partitions and the newly generated one, even if a random name is used for Partitions. Some tooling on the client side may help with that.

There may be bits in the workflow that can be improved and I am open to proposals. My current thinking is that this could get better addressed when we develop some client side tooling that automates service providers deployments and topology changes, possibly taking a PartitionSet and an APIExport as parameter. I am a bit wary with having this automation on the server side because:

that could drive automated changes that introduce big disruption and cost increases by service providers, so that it would need to be gated, hence may not bring much gain compared to a client-side approach.

I am not sure that cross-shard communication will always be possible.

fgiloux · 2023-01-23T08:56:59Z

/retest

sttts · 2023-01-23T09:54:21Z

pkg/reconciler/topology/resources.go

@@ -22,12 +22,20 @@ import (
 	topologyv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/topology/v1alpha1"
 )

+const dimensionsLabelKeyBase = "partition.kcp.io/"


partition.topology.kcp.io/

fgiloux · 2023-01-25T21:47:57Z

@sttts @p0lyn0mial I have implemented the changes we discussed:

using the dimension values for Partition names (still using name generator taking care of overflows)
removed the labels with the dimensions (as the information is now made visible by the name)
using a selector in PartitionSet rather than matchExpressions only (some collision check was needed)

fgiloux · 2023-01-25T21:48:30Z

/retest

p0lyn0mial · 2023-01-30T17:45:10Z

pkg/apis/topology/v1alpha1/types_partitionset.go

-	// selector (optional) is a label selector that filters shard targets.
-	Selector *metav1.LabelSelector `json:"selector,omitempty"`
+	// ShardSelector (optional) specifies filtering for shard targets.
+	ShardSelector *metav1.LabelSelector `json:"shardSelector,omitempty"`


I like the name :)

p0lyn0mial · 2023-01-30T18:23:11Z

pkg/server/server.go

@@ -453,6 +453,12 @@ func (s *Server) Run(ctx context.Context) error {
 		}
 	}

+	if s.Options.Controllers.EnableAll || enabled.Has("partition") {


partitionset ?

I would be ok with changing to partitionset. I used partition as we don't have any other controller specific to Partitions. Partitions alone don't get reconciled. It is only in the context of an APIExportEndpointSlice that the controller takes the referenced Partition in consideration for populating the endpoints. The same mechanism could also be leveraged in other places in the future.

we should honestly remove all these ifs. We don't use them anywhere.

ack. Outside of the scope of this PR

p0lyn0mial · 2023-01-30T19:13:24Z

pkg/reconciler/topology/partitionset_controller.go

+		return
+	}
+
+	logger := logging.WithObject(logging.WithReconciler(klog.Background(), ControllerName), obj.(*topologyv1alpha1.Partition))


move after line 218 so that we are sure the obj is obj.(*topologyv1alpha1.Partition)

p0lyn0mial · 2023-01-30T19:18:31Z

pkg/reconciler/topology/partitionset_indexes.go

+	for _, ownerRef := range partition.OwnerReferences {
+		if ownerRef.Kind == "PartitionSet" {
+			path := logicalcluster.From(partition).Path()
+			if !ok {


p0lyn0mial · 2023-01-30T19:40:05Z

pkg/reconciler/topology/partitionset_controller.go

+	if !ok {
+		return false
+	}
+	if !reflect.DeepEqual(oldShard.Labels, newShard.Labels) {


return reflect.DeepEqual(oldShard.Labels, newShard.Labels)

return !reflect.DeepEqual(oldShard.Labels, newShard.Labels)
done

p0lyn0mial · 2023-01-30T19:57:35Z

pkg/reconciler/topology/partitionset_reconcile.go

+}
+
+func (c *controller) deletePartitions(ctx context.Context, partitionSet *topologyv1alpha1.PartitionSet) error {
+	partitionSet.Status.Count = 0


why is this modification required?

oh, it tracks the number of associated partitions.
should we then decrease this number by the number of successful calls to deletePartition ?

deletePartitions (mind the s at the end) is only call in case of an issue with the selector (lines 55 and 83) the count should then be set back to 0. In case of regular deletion because of a change in shards deletePartitionwithout s is called (lines 147 and 182). In case of no error with the selector the number of endpoints is already captured in the size of matchLabelsMap, no need for additional counting.
I will move partitionSet.Status.Count = 0one level up as it is not clear from the name of the function that it does that as well.

p0lyn0mial · 2023-01-30T19:59:33Z

pkg/reconciler/topology/partitionset_controller.go

+		getPartitionSet: func(clusterName logicalcluster.Name, name string) (*topologyv1alpha1.PartitionSet, error) {
+			return partitionSetClusterInformer.Lister().Cluster(clusterName).Get(name)
+		},
+		getPartitionsByPartitionSet: func(key string) ([]*topologyv1alpha1.Partition, error) {


could we change the signature of this method to accept an object instead of the key?
we wouldn't have to calculate the key on each invocation.

yes I can change the signature of the method but still we will need to calculate the key in the method to use the indexer, won't we?

p0lyn0mial · 2023-01-30T20:03:29Z

pkg/reconciler/topology/partitionset_reconcile.go

+	}
+	for _, partition := range existingPartitions {
+		if err := c.deletePartition(ctx, logicalcluster.From(partition).Path(), partition.Name); err != nil && !apierrors.IsNotFound(err) {
+			return err


since this is a network call, we could be more greedy and try all existingPartitions and collect all errors, wdyt?

I am not sure how you would do that. I thought of DeleteCollection with FieldSelector. FieldSelectoris however limited to certain fields and the owner reference is not among them.

just loop over existingPartitions, try to deletePartition, collect any errors and return at the end :) ?

ok so it makes no difference in terms of network calls, does it?

It does, we try all existingPartitions even if we failed to delete a partition. Hopefully on the next iteration we will have less work to do. Does it make sense?

p0lyn0mial · 2023-01-30T20:13:27Z

pkg/reconciler/topology/partitionset_reconcile.go

+			topologyv1alpha1.PartitionsReady,
+			topologyv1alpha1.ErrorGeneratingPartitionsReason,
+			conditionsv1alpha1.ConditionSeverityError,
+			"error listing Shards",


shouldn't we log the err too?

The error is returned by the reconcile function, hence logged by the controller

p0lyn0mial · 2023-01-30T20:31:12Z

pkg/reconciler/topology/partitionset_reconcile.go

+		labelSelector = labels.Everything()
+	} else {
+		var err error
+		labelSelector, err = metav1.LabelSelectorAsSelector(partitionSet.Spec.ShardSelector)


since an invalid selector is expensive (deletePartitions) would it make sense to move validation to an admission plugin and simply return an error (without deletion) here?

see above. We can discuss it on slack

p0lyn0mial · 2023-01-31T07:58:15Z

pkg/reconciler/topology/partitionset_reconcile.go

+	}
+
+	var matchLabelsMap map[string]map[string]string
+	if partitionSet.Spec.ShardSelector != nil {


it looks like we could get rid of this if statement since the partition function can already handle nil shardSelectorLabels

I cannot reference MatchLabels on a nil pointer

p0lyn0mial · 2023-01-31T07:59:04Z

pkg/reconciler/topology/partitionset_reconcile.go

+
+// partition populates shard label selectors according to dimensions.
+// It only keeps selectors that have at least one match.
+func partition(shards []*corev1alpha1.Shard, dimensions []string, shardSelectorLabels map[string]string) (matchLabelsMap map[string]map[string]string) {


would it make sense to rename to partitionByDimensionsAndLabels?

I don't see the need for now. There is currently a single way of partitioning. Labelscould also be ambiguous in this context. Dimensionsare a way to slice and dice by Labels. Labelsare also used for filtering, defining the boundaries of the piece to slice and dice. What I mean is that Dimensionsand Labelsare not of the same nature for the partitioning.

p0lyn0mial · 2023-01-31T08:05:18Z

pkg/reconciler/topology/partitionset_reconcile.go

+		for _, label := range labels {
+			labelValue, ok := shard.Labels[label]
+			if !ok {
+				break


what does it mean?
what if we didn't find let's say the second label out of four? (len(key) > 0 will apply)

true. Adding a boolean to check the match

p0lyn0mial · 2023-01-31T08:08:53Z

pkg/reconciler/topology/partitionset_reconcile.go

+					conditionsv1alpha1.ConditionSeverityError,
+					"old partition could not get deleted",
+				)
+				return err


again, since this is a network call, we could be more greedy and try all oldPartitions, wdyt?

I am not sure how you would do that here. There is no single selector for the Partitions to delete.

p0lyn0mial · 2023-01-31T08:14:52Z

pkg/reconciler/topology/partitionset_reconcile.go

+			if !ok {
+				break
+			}
+			key = key + "+" + label + "=" + labelValue


i'm not sure if i understand why the key has to collect all labels with their values, couldn't we calculate some hash from the labels and values ?

This is a kind of hash. We could use a hash function for it. I am not sure that the processing time would be shorter and it was nice to have something readable by debugging.

if we won't reduce the size of the key it might grow indefinitely, no?

The keys are only used as a transient and quick way of comparing the desired state of Partitions with the existing ones. They are not stored.

p0lyn0mial · 2023-01-31T08:18:27Z

pkg/reconciler/topology/partitionset_reconcile.go

+					conditionsv1alpha1.ConditionSeverityError,
+					"partition could not get created",
+				)
+				return err


since this is a network call, we could be more greedy and try to create all partitions

I am not sure how you would do that

sttts · 2023-02-07T08:25:58Z

pkg/reconciler/topology/partitionset/partitionset_controller.go

+				c.enqueueAllPartitionSets(obj)
+			},
+			UpdateFunc: func(oldObj, newObj interface{}) {
+				if filterShardEvent(oldObj, newObj) {


explain in a comment what the filtering is and why it makes sense

sttts · 2023-02-07T08:26:28Z

pkg/reconciler/topology/partitionset/partitionset_controller.go

+
+	indexers.AddIfNotPresentOrDie(partitionClusterInformer.Informer().GetIndexer(), cache.Indexers{
+		indexPartitionsByPartitionSet: indexPartitionsByPartitionSetFunc,
+	})


nit: indexes first, event handler after that.

sttts · 2023-02-07T08:26:49Z

pkg/reconciler/topology/partitionset/partitionset_controller.go

+type controller struct {
+	queue workqueue.RateLimitingInterface
+
+	// kcpClusterClient is cluster aware and used to communicate with kcp API servers


no need for the comment. This is convention.

pkg/reconciler/topology/partitionset/partitionset_controller.go

sttts · 2023-02-07T08:33:32Z