✨Wait until APIExport virtual workspace URLs are ready

[astefanutti]

This PR follows up kcp-dev/kcp#2135 so that the manager is started, only after its APIExport virtual workspace URLs are ready.

[astefanutti]

/retest

[stevekuznetsov]

We need to do a GET or LIST first to see the current state (and short-circuit if the live read shows a URL), and use that resource version to start the watch. Also, likely best to have watch resumption on errors and re-list on compaction - which makes me wonder if we should instead start an informer.

[stevekuznetsov]

I think this check is likely superfluous and if it were to be true, the controller can do nothing except for panic and process exit, right?

[astefanutti]

This may be useful for the case where the APIExport name is not provided, as a way to pick the expected one. That's true the controller won't be able to to anything if the APIExport specified by name does not pass that check. So maybe add that check only when the name is not provided?

[stevekuznetsov]

If no name is specified, won't the watch with the metadata.name field selector return nothing?

[astefanutti]

The metadata.name field selector is only appended when the name is provided. We could skip the check as well in that case or fail-fast.

[astefanutti]

I've added the apiExportName == "" condition to the check.

[stevekuznetsov]

We should just make it required - why not?

[astefanutti]

I took it as an invariant that was already there, but making it required, possibly with a sensible default, would make sense and simplify things.

[astefanutti]

I've updated the api-export-name option to default to the controller APIExport name, so it avoids watching for multiple resources.

[astefanutti]

We need to do a GET or LIST first to see the current state (and short-circuit if the live read shows a URL), and use that resource version to start the watch. Also, likely best to have watch resumption on errors and re-list on compaction - which makes me wonder if we should instead start an informer.

Ah, I have assumed this is what the controller runtime cached client actually does by default for typed objects. Let me check.

[astefanutti]

We need to do a GET or LIST first to see the current state (and short-circuit if the live read shows a URL), and use that resource version to start the watch. Also, likely best to have watch resumption on errors and re-list on compaction - which makes me wonder if we should instead start an informer.

Ah, I have assumed this is what the controller runtime cached client actually does by default for typed objects. Let me check.

OK, I realise my assumption was wrong, for the obvious reason the split / caching client that relies on informers is that of the manager, which we aim to create here 🙃!

So, as it stands, that is with unset resourceVersion, the semantic for watch requests is "Get State and Start at Most Recent", which is starting at the most recent resource version, beginning with synthetic "Added" events of all resources instances that exist at that starting resource version. While it functionally works, it may be less efficient / scalable, than explicitly setting resourceVersion, as it requires a quorum read to be served. Yet that may be acceptable in that use case. More details can be found at https://kubernetes.io/docs/reference/using-api/api-concepts/#semantics-for-watch.

[stevekuznetsov]

I see - in any case, no reason that a closed watch stream while waiting should cause process exit, right?

[astefanutti]

I see - in any case, no reason that a closed watch stream while waiting should cause process exit, right?

I've added a retry logic in case the channel has been closed, so it resumes watching for APIExports if the previous watch stream timed out on idle, or just fails if the connection to the server cannot be re-established.

[openshift-ci]

@astefanutti: 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
ci/prow/e2e	1899c14	link	true	/test e2e

Full PR test history. Your PR dashboard.

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti, stevekuznetsov

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 [stevekuznetsov]

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