✨ make APIExportEndpointSlice consumer aware #3256

mjudeikis · 2025-01-18T20:53:28Z

Summary

This part makes APIExportEndpointSlices consumers aware. Meaning it will add or remove URLs if consumers are present or goes away.

In this PR:

Current APIExportEndpointSlice reconcile is split into 2 - status and url

status - status controller checks if API export is valid, partitions are set right, and it can be queried. Most of these objects operate in a single-shard context, so this reconciliation works only in a single-shard context. CAVITE: Based on the partition setup - the shards involved will be different. Hence we store the final computed selector into status for the URL reconciler to pick up.

url - URL reconcile operates cross-shards setup. It takes APIExportEndpointSlice from the cache and local shard and observes APIBindings and APIExports and, based on consumers, will update URLs.

In addition, it uses not a commuter but Server-Side Apply patch to update only its own field, which is owned by shard.

If we want to remove the selector from status - it would require an overload single reconciliation quite a lot. And filter when we can update statuses and when not. Overall, I tried, and the code readability was very poor.

Related issue(s)

Fixes # #3221

Release Notes

make APIExportEndpointSlices consumer aware

kcp-ci-bot · 2025-01-18T20:53:33Z

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

kcp-ci-bot · 2025-01-24T15:29:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mjudeikis. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_controller.go

pkg/reconciler/apis/apiexportendpointslice/apiexportendpointslice_reconcile.go

mjudeikis · 2025-01-30T15:38:54Z

/retest

pkg/indexers/apiexport.go

sttts · 2025-01-31T08:02:33Z

pkg/indexers/apiexport.go

+
+	path := logicalcluster.NewPath(apiExportEndpointSlice.Spec.APIExport.Path)
+	if path.Empty() {
+		path = logicalcluster.From(apiExportEndpointSlice).Path()


don't we need both ways to address it?

Depends on how you look it up later in the code.

Not sure I understand this comment.
I don't :D

Now we only index exports as <lcluster>:export or <path>:export. Not both.

mjudeikis · 2025-02-01T12:53:33Z

/retest

sttts · 2025-02-05T18:02:47Z

sdk/apis/apis/v1alpha1/types_apiexportendpointslice.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2022 The KCP Authors.
+Copyright 2025 The KCP Authors.


nit: we usually don't update when just changing files.

sttts · 2025-02-05T18:04:56Z

sdk/apis/apis/v1alpha1/types_apiexportendpointslice.go

@@ -93,10 +102,16 @@ type APIExportEndpoint struct {
 }

 func (in *APIExportEndpointSlice) GetConditions() conditionsv1alpha1.Conditions {
+	if in == nil {
+		return nil
+	}


Not sure we want this. It encourages brittle code.

sttts · 2025-02-05T18:07:18Z

sdk/apis/apis/v1alpha1/types_apiexportendpointslice.go

+	// EndpointSliceReadyForURLs is a condition for APIExportEndpointSlice that reflects the readiness of the slice to
+	// provide URLs. It is set to True when the slice is ready to provide URLs which will be lifecycled based on existing consumers in the different
+	// shards.
+	APIExportEndpointSliceReadyForURLs conditionsv1alpha1.ConditionType = "EndpointReadyForURLs"


ReadyForEndpoints.

Actually not clear why we need this. When is it not ready?

Its more like handover/signal to secondary url controller. I used this a trigger. We could treat all conditions ready as signal too and just drop this all togheter.

Was thinking about the label selector. It must be known, right?

sttts · 2025-02-05T18:09:52Z

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_reconcile.go

+	}
+
+	s := apiExportEndpointSlice.Status.ShardSelector
+	if s == "" { // should never happen.


is this what the condition above is guarding?

In theory yes, but I wanted to avoid cases where somebody patched status and now it might give false positive.
And I think now that maybe this be omitempty pointer... Need to check if empty selector is valid case.

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_reconcile.go

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller_test.go

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller.go

sttts · 2025-02-05T18:26:56Z

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller.go

+
+	_, _ = globalShardClusterInformer.Informer().AddEventHandler(events.WithoutSyncs(cache.ResourceEventHandlerFuncs{
+		AddFunc: func(obj interface{}) {
+			c.enqueueAllAPIExportEndpointSlices(obj)


Why do we care about other shards?

I was thinking about a case where a change in shards could impact APIExportSlices flow. But wrong assumptions

sttts · 2025-02-05T18:29:43Z

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller.go

+}
+
+// enqueueAPIExportEndpointSlicesForAPIExport enqueues APIExportEndpointSlices referencing a specific APIExport.
+func (c *controller) enqueueAPIExportEndpointSlicesForAPIExport(obj interface{}) {


why do we care about reconciling when an export changes? Won't APIBindings eventually change and that is enough for us to be triggered?

It's one of those wrong assumptions, I think. WIll try

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller.go

sttts · 2025-02-05T18:32:26Z

pkg/reconciler/apis/apiexportendpointsliceurls/apiexportendpointsliceurls_controller.go

+		return
+	}
+
+	{ // local to shard


why double code?

readability. It isolates variables and makes it very clear to read two blocks. Its all about reading code and reducing mental overhead when reading the code.
// local to shard
// from cache

mjudeikis · 2025-02-09T12:53:35Z

/retest

Signed-off-by: Mangirdas Judeikis <[email protected]> On-behalf-of: @SAP [email protected]

mjudeikis · 2025-02-09T16:24:27Z

/retest

kcp-ci-bot · 2025-02-09T16:33:39Z

@mjudeikis: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-sharded	`5546531`	link	true	`/test pull-kcp-test-e2e-sharded`

Full PR test history

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. I understand the commands that are listed here.

kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Jan 18, 2025

kcp-ci-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 18, 2025

mjudeikis marked this pull request as ready for review January 18, 2025 20:57

kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2025

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from 4bd9adf to f1b4a98 Compare January 19, 2025 20:54

kcp-ci-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2025

mjudeikis marked this pull request as draft January 19, 2025 20:54

kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2025

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from f1b4a98 to 4f51231 Compare January 24, 2025 15:29

kcp-ci-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2025

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from 4f51231 to 84ba80d Compare January 24, 2025 15:31

mjudeikis changed the title ~~✨ APIExport noop without APIBinding~~ POC ✨ APIExport with aware consumers Jan 24, 2025

sttts reviewed Jan 24, 2025

View reviewed changes

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

sttts reviewed Jan 24, 2025

View reviewed changes

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

sttts reviewed Jan 24, 2025

View reviewed changes

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

sttts reviewed Jan 24, 2025

View reviewed changes

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

sttts reviewed Jan 24, 2025

View reviewed changes

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

sttts reviewed Jan 24, 2025

View reviewed changes

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

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from 84ba80d to 4371b8f Compare January 26, 2025 20:14

kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2025

mjudeikis commented Jan 31, 2025

View reviewed changes

pkg/indexers/apiexport.go Show resolved Hide resolved

mjudeikis force-pushed the mjudeikis/apiexport.ep branch 2 times, most recently from e6ed986 to 9c6093a Compare January 31, 2025 06:48

embik self-requested a review January 31, 2025 07:57

sttts reviewed Jan 31, 2025

View reviewed changes

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from 96c29f1 to 705e3bb Compare January 31, 2025 17:17

mjudeikis force-pushed the mjudeikis/apiexport.ep branch from 705e3bb to cda4f26 Compare February 2, 2025 10:30

embik linked an issue Feb 5, 2025 that may be closed by this pull request

feature: APIExports VirtualWorkspace without consumer enhancement #3221

Open

1 task