✨ Partition reconciliation #2469

fgiloux · 2022-12-09T09:31:17Z

Summary

This PR implements a reconciliation mechanism so that the endpoints listed in the status of an APIExportEndpointSlice match the filtering provided by the specified partition.

Design document

Related issue(s)

Follows #2432
Fixes #2333

fgiloux · 2022-12-09T09:33:50Z

/hold

this PR is based on #2432 and will be rebased once it has merged

fgiloux · 2023-01-07T21:26:13Z

/retest

fgiloux · 2023-01-10T11:56:07Z

/unhold

p0lyn0mial · 2023-01-10T13:55:08Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

@@ -224,6 +254,32 @@ func (c *controller) enqueueAllAPIExportEndpointSlices(shard interface{}) {
 	}
 }

+// enqueuePartition maps a Partition to APIExportEndpointSlices for enqueuing.
+func (c *controller) enqueuePartition(obj interface{}) {
+	key, err := kcpcache.DeletionHandlingMetaClusterNamespaceKeyFunc(obj)


why not to use an obj to get slices?

As per the previous comment. This is only used for enqueuing.

yes, it seems we could make use of the object to find what we need

are partition names unique across workspace?

I don't think so, does it mean we always have to find a local partition for the given slice?

would it make sense to list endpoints for the given cluster and check which ones reference a particular name instead?

does it mean we always have to find a local partition for the given slice?

yes that is the intended design, partition and APIExportEndpointSlices being in the same workspace. There can however be multiple of them in different workspaces for covering different regions for instance.
Otherwise we would need to make them global and at this point in time I don't see the need for it.

would it make sense to list endpoints for the given cluster and check which ones reference a particular name instead?

there is max one endpoint per shard, the Partition resource allows to specify shards for which endpoints get added to the APIExportEndpointSlice.

p0lyn0mial · 2023-01-10T13:58:23Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

+		getPartition: func(clusterName logicalcluster.Name, name string) (*topologyv1alpha1.Partition, error) {
+			return partitionClusterInformer.Lister().Cluster(clusterName).Get(name)
+		},
+		getAPIExportEndpointSlicesByPartition: func(key string) ([]interface{}, error) {


why not to change the signature of this method to return strong types?

also the signature could accept partition name not a key

This would mean looping one more time, although I expect the loop to be small. This is however only used for enqueuing and not exposed to the reconcile (there is a separate struct).

p0lyn0mial · 2023-01-10T14:01:28Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

+		},
+	)
+
+	if err := apiExportEndpointSliceClusterInformer.Informer().AddIndexers(cache.Indexers{


use indexers.AddIfNotPresentOrDie

Yes I will do that

p0lyn0mial · 2023-01-10T14:03:30Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

-		listShards: func() ([]*corev1alpha1.Shard, error) {
-			return shardClusterInformer.Lister().List(labels.Everything())
+		listShards: func(selector labels.Selector) ([]*corev1alpha1.Shard, error) {
+			return shardClusterInformer.Lister().List(selector)
 		},
 		getAPIExportEndpointSlice: func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIExportEndpointSlice, error) {
 			return apiExportEndpointSliceClusterInformer.Lister().Cluster(clusterName).Get(name)
 		},
 		getAPIExport: func(path logicalcluster.Path, name string) (*apisv1alpha1.APIExport, error) {
 			return indexers.ByPathAndName[*apisv1alpha1.APIExport](apisv1alpha1.Resource("apiexports"), apiExportClusterInformer.Informer().GetIndexer(), path, name)


did we make it local ? (doesn't use the global lister)

it is the other way around, only looking at the cache server. This was discussed and done in the previous PR (APIExportEndpointSlice reconciliation)

should we change the name of the variable ?

my strategy so far has been to use local and global prefix when both are used and not to have any prefix otherwise. I thought it was the consensus. It seems it is not the case (anymore?). I will add the global prefix to shardClusterInformer and apiExportClusterInformer.

p0lyn0mial · 2023-01-10T14:04:45Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

+	}
+
+	logger := logging.WithObject(logging.WithReconciler(klog.Background(), ControllerName), obj.(*topologyv1alpha1.Partition))
+	logging.WithQueueKey(logger, key).V(2).Info("queuing APIExportEndpointSlices because Partition changed")


too verbose, make it V(4)

it is not consistent across the other controllers, sometimes V(2) is used, not more often V(4). I can use V(4) here if you want

p0lyn0mial · 2023-01-10T14:08:47Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

@@ -64,35 +68,103 @@ func (r *endpointsReconciler) reconcile(ctx context.Context, apiExportEndpointSl
 	}
 	apiExport, err := r.getAPIExport(apiExportPath, apiExportEndpointSlice.Spec.APIExport.Name)
 	if err != nil {
+		reason := apisv1alpha1.InternalErrorReason


is this an internal error so that the conditions stay stable?

how will we know the error?

It is an internal error not to leak any information to the user(discussed in the previous PR).
The error details are available in the logs for a privileged user.

an error could be used for troubleshooting

hence in the logs

p0lyn0mial · 2023-01-10T14:11:54Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

+		)
+		conditions.MarkFalse(
+			apiExportEndpointSlice,
+			apisv1alpha1.APIExportEndpointSliceURLsReady,


so a single transient error can flip this condition false, should we do better?

If it is an error, which is recoverable the condition is indeed set to false because the reconciliation was not successful but it is queued again. If it is transient it flips back to true when the condition has gone.
The alternative is to set it as "unknown"

set to unknown

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

p0lyn0mial · 2023-01-10T14:22:47Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

+			apisv1alpha1.APIExportEndpointSliceURLsReady,
+			apisv1alpha1.ErrorGeneratingURLsReason,
+			conditionsv1alpha1.ConditionSeverityError,
+			"error listing shards",


how will we known the err?

logs for privileged users.

p0lyn0mial · 2023-01-10T14:23:57Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go


-	if err = r.updateEndpoints(ctx, apiExportEndpointSlice, apiExport); err != nil {
+	if err = r.updateEndpoints(ctx, apiExportEndpointSlice, apiExport, shards); err != nil {


or pass the selector :)

p0lyn0mial · 2023-01-11T12:31:54Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

+					apisv1alpha1.PartitionValid,
+					apisv1alpha1.InternalErrorReason,
+					conditionsv1alpha1.ConditionSeverityError,
+					err.Error(),


didn't you want to hide errors?

No. I wanted to avoid leaking information and it does not. APIExportEndpointSlice and Partitions are cluster scoped resources in the same workspace.

i am confused how does it differ from https://github.com/kcp-dev/kcp/pull/2469/files#diff-a028d37be8294707fd6fdcb63d85f41486832f61eb70c14605fc5fc712958f48R94 ?

An APIExport from a different workspace may be referenced in the APIExportEndpointSlice. It is not the case for a Partition (only considering the same workspace). By creating an APIExportEndpointSlice (or an APIBinding) a user may get additional information on the APIExport and other workspaces if the error was returned. This scenario does not happen for Partition.

I was thinking that an invalid request - a one that would use an incorrect APIExport would be rejected by https://github.com/kcp-dev/kcp/pull/2560/files#r1064359708

p0lyn0mial · 2023-01-11T12:32:04Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

+				apisv1alpha1.PartitionValid,
+				apisv1alpha1.PartitionInvalidReferenceReason,
+				conditionsv1alpha1.ConditionSeverityError,
+				err.Error(),


didn't you want to hide errors?

sttts · 2023-01-11T12:51:38Z

pkg/apis/apis/v1alpha1/types_apiexportendpointslice.go

+	// Partition reference is invalid.
+	PartitionInvalidReferenceReason = "PartitionInvalidReference"
+	// PartitionNotFoundReason is a reason for the PartitionValid condition that the referenced Partition is not found.
+	PartitionNotFoundReason = "PartitionNotFound"


this is not used, is it? (not a blocker to merge)

true. I will remove it

sttts · 2023-01-11T12:57:20Z

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

+					apisv1alpha1.PartitionValid,
+					apisv1alpha1.PartitionInvalidReferenceReason,
+					conditionsv1alpha1.ConditionSeverityError,
+					err.Error(),


I don't like exposing this error. Rule of thumb: we must not give a hint about existence of a workspace ever. Here, this is not that clear. (follow-up is fine)

sttts · 2023-01-11T12:58:39Z

/lgtm
/approve

openshift-ci · 2023-01-11T13:02:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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~~ [sttts]
~~config/crds/OWNERS~~ [sttts]
~~pkg/apis/OWNERS~~ [sttts]

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

fgiloux · 2023-01-11T14:13:42Z

/retest

fgiloux · 2023-01-11T14:54:30Z

/retest

fgiloux · 2023-01-11T19:47:30Z

/retest

fgiloux · 2023-01-12T06:31:46Z

@sttts rebased and repushed. Checks are now successful but lgtm label has been removed.

…rtition Signed-off-by: Frederic Giloux <fgiloux@redhat.com>

sttts · 2023-01-12T12:15:44Z

/lgtm

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2022

openshift-ci bot requested review from davidfestal and s-urbaniak December 9, 2022 09:31

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 9, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2022

fgiloux force-pushed the partition-reconciliation branch 3 times, most recently from dc435b3 to 731507d Compare December 20, 2022 07:51

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

fgiloux mentioned this pull request Dec 20, 2022

✨ Partitionset reconciliation #2513

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2022

fgiloux force-pushed the partition-reconciliation branch from 731507d to 1b84a53 Compare January 6, 2023 12:51

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023

fgiloux force-pushed the partition-reconciliation branch from 1b84a53 to a91c0b3 Compare January 7, 2023 19:26

fgiloux force-pushed the partition-reconciliation branch 2 times, most recently from 2d4ab7b to ba989f8 Compare January 10, 2023 08:28

fgiloux requested review from sttts and p0lyn0mial January 10, 2023 08:30

fgiloux force-pushed the partition-reconciliation branch 2 times, most recently from 7fc8039 to 33a632d Compare January 10, 2023 11:26

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023

p0lyn0mial reviewed Jan 10, 2023

View reviewed changes

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Jan 10, 2023

View reviewed changes

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go Show resolved Hide resolved

p0lyn0mial reviewed Jan 10, 2023

View reviewed changes

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Jan 10, 2023

View reviewed changes

fgiloux force-pushed the partition-reconciliation branch 3 times, most recently from 6090da8 to 5b6195a Compare January 11, 2023 12:27

p0lyn0mial reviewed Jan 11, 2023

View reviewed changes

sttts reviewed Jan 11, 2023

View reviewed changes

openshift-ci bot assigned sttts Jan 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023

fgiloux force-pushed the partition-reconciliation branch from 5b6195a to e341969 Compare January 11, 2023 21:07

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023

fgiloux mentioned this pull request Jan 12, 2023

🌱 End-to-end tests for APIExportEndpointSlice #2608

Merged

Amend apiexportendpointslice reconciliation to filter endpoints by pa…

d224162

…rtition Signed-off-by: Frederic Giloux <fgiloux@redhat.com>

fgiloux force-pushed the partition-reconciliation branch from e341969 to d224162 Compare January 12, 2023 10:03

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023

openshift-merge-robot merged commit 929f7a6 into kcp-dev:main Jan 12, 2023


		if err = r.updateEndpoints(ctx, apiExportEndpointSlice, apiExport); err != nil {
		if err = r.updateEndpoints(ctx, apiExportEndpointSlice, apiExport, shards); err != nil {

✨ Partition reconciliation #2469

✨ Partition reconciliation #2469

Conversation

fgiloux commented Dec 9, 2022

Summary

Related issue(s)

fgiloux commented Dec 9, 2022

fgiloux commented Jan 7, 2023

fgiloux commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Jan 10, 2023 • 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

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 Jan 11, 2023 • 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

sttts commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

fgiloux commented Jan 11, 2023

fgiloux commented Jan 11, 2023

fgiloux commented Jan 11, 2023

fgiloux commented Jan 12, 2023

sttts commented Jan 12, 2023

p0lyn0mial Jan 10, 2023 •

edited

Loading

p0lyn0mial Jan 11, 2023 •

edited

Loading