Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

liveupdate: use ImageMap for Tilt-built image matching #5677

Merged
merged 6 commits into from
Apr 11, 2022

Conversation

milas
Copy link
Contributor

@milas milas commented Apr 7, 2022

There's three supported Live Update K8s selector configurations:
(1) Tilt-built image (e.g. from docker_build or custom_build)
(2) External image (e.g. file_sync_only extension + nginx image)
(3) Container name (e.g. some k8s_custom_deploy configurations)

For (1) and (2), we match the container image against the image
selector in the v1alpha1.LiveUpdateKubernetesSelector. As a result,
this meant that for (1), the selector reference was the fully-qualified
"from-cluster" reference, i.e. using local registry.

For example, if the image is referred to in the Tiltfile as follows:

docker_build('gcr.io/foo/bar', ...)

Then the Tilt-built image might be something like
localhost:51245/gcr_io_foo_bar:tilt-123.

For (2), the image reference will be used as-is since it's not built
by Tilt.

While not relevant here, for completeness, for (3), the container name
is a direct comparison.

The v1alpha1.LiveUpdateKubernetesSelector is populated via a call
to ImageTarget::InferLiveUpdateProperties(). As a result, this needs
the registry information so that it could determine the proper image
name to use for the selector for (1).

This is problematic because the local registry initialization is no
longer guaranteed at startup - it's part of Cluster reconciliation
once the client connection is successfully established.

By adding a new option to the selector to pass a reference to an
ImageMap, we can grab the fully-qualified reference via
.Status.ImageFromCluster.

@milas milas requested review from nicks, landism and lizzthabet April 7, 2022 20:14
@milas milas self-assigned this Apr 7, 2022
@milas
Copy link
Contributor Author

milas commented Apr 7, 2022

This PR is super chonky as-is because there's codegen/reconciler stub/apiserver-to-state dispatch that was needed for the final solution to come together (which I wasn't 100% sure was going to work before doing it 😅)

I wanted to get a sanity check on the approach, and then I'll split it up into smaller PRs for review

@milas milas force-pushed the milas/lu-image-selector branch 2 times, most recently from afd10d7 to ef521e2 Compare April 7, 2022 20:25
There's three supported Live Update K8s selector configurations:
 (1) Tilt-built image (e.g. from `docker_build` or `custom_build`)
 (2) External image (e.g. `file_sync_only` extension + `nginx` image)
 (3) Container name (e.g. some `k8s_custom_deploy` configurations)

For (1) and (2), we match the container image against the image
selector in the `v1alpha1.LiveUpdateKubernetesSelector`. As a result,
this meant that for (1), the selector reference was the fully-qualified
"from-cluster" reference, i.e. using local registry.

For example, if the image is referred to in the Tiltfile as follows:
```
docker_build('gcr.io/foo/bar', ...)
```
Then the Tilt-built image might be something like
`localhost:51245/gcr_io_foo_bar:tilt-123`.

For (2), the image reference will be used as-is since it's not built
by Tilt.

While not relevant here, for completeness, for (3), the container name
is a direct comparison.

The `v1alpha1.LiveUpdateKubernetesSelector` is populated via a call
to `ImageTarget::InferLiveUpdateProperties()`. As a result, this needs
the registry information so that it could determine the proper image
name to use for the selector for (1).

This is problematic because the local registry initialization is no
longer guaranteed at startup - it's part of `Cluster` reconciliation
once the client connection is successfully established.

By adding a new option to the selector to pass a reference to an
`ImageMap`, we can grab the fully-qualified reference via
`.Status.ImageFromCluster`.
@milas milas force-pushed the milas/lu-image-selector branch from ef521e2 to fea304c Compare April 7, 2022 20:39
Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

internal/controllers/core/liveupdate/reconciler.go Outdated Show resolved Hide resolved
@milas milas merged commit 8d918d9 into master Apr 11, 2022
@milas milas deleted the milas/lu-image-selector branch April 11, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants