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

feat: add load image addon #151

Merged
merged 5 commits into from
Nov 18, 2021
Merged

feat: add load image addon #151

merged 5 commits into from
Nov 18, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Nov 15, 2021

This is one of two proposals to add image loading to KTF, broken out of #149. It is mutually exclusive with #152. Only one of the two should be merged.

This approach provides a plugin for loading images following the standard KTF plugin pattern. Building a cluster that includes this plugin will attempt to load an image and report a failure if that is not possible or if that is unsuccessful.

This addition lacks tests because KIND will not (contrary to my earlier expectations) load an image not already present on the host machine. We'd need to run docker pull or similar to ensure the requested image is available locally, and I'm loathe to screw around with that. My original test code is available at https://gist.github.com/rainest/079d07206913398acf65371703cecf0d

Pro/con evaluation

The argument against this approach is that we should not add cluster addons unless they are universally available on all clusters types.

I am in favor of this option:

  • I don't see an obvious reason to impose this restriction so long as we can provide feedback indicating that an addon is unavailable. While this case deals with cluster types, it seems reasonable that we'd extend the same paradigm to cluster versions (e.g. a GWAPI addon would not be available on 1.16, but would be available on {future version}). Addons are by definition not mandatory, and users can always simply not include them if they are unavailable on the desired cluster type.
  • Using the addon pattern allows us to establish a standard means of requesting an addon reporting availability that works regardless of your target cluster. We currently return an error in Deploy() for unavailability. Code that utilizes these is cluster agnostic: you always call WithAddons(<instance>), which always reports any failures when calling Build(). If you have multiple cluster types that support an addon set, swapping out the cluster type only requires calling a different package's New() function and passing the resulting Cluster downstream. If you go with feat: add load image KIND helper #152's approach, any test that requires a non-universal plugin must be written for the specific target cluster type.
  • In this specific case, loading images is not strictly specific to KIND. Although we don't yet provide a Minikube Cluster implementation, it supports an almost identical command. Loading images into GKE arguably isn't impossible, it's just much more complicated (you'd need to ensure GCR is enabled, push an image to it, and provide the new GCR image name to the user) and not something I needed for now.
  • Although I can't think of an example offhand, it seems reasonable that we may create addons that have partial support for some clusters, and the Build() -> Deploy() -> succeed or fail based on availability paradigm seems like it'd handle this easily.

We should look at more robust means of handling support indications if we think Deploy() failures are insufficient, but I think having a generic interface for attempting to load addons is far preferable to having separate paradigms for similar tasks (enabling functionality not exposed by the base Cluster functions), one of which is incapable of sharing both implementation and user-written test code.

@rainest rainest temporarily deployed to integration-tests November 15, 2021 23:31 Inactive
pkg/clusters/addons/loadimage/addon.go Outdated Show resolved Hide resolved
pkg/clusters/addons/loadimage/addon.go Outdated Show resolved Hide resolved
- Split loadimage source into components
- Convert cluster type handler into a switch
@rainest rainest marked this pull request as ready for review November 16, 2021 21:18
@rainest rainest requested a review from a team as a code owner November 16, 2021 21:18
@rainest rainest requested a review from mflendrich November 16, 2021 21:25
@rainest rainest temporarily deployed to integration-tests November 16, 2021 22:24 Inactive
@shaneutt
Copy link
Contributor

shaneutt commented Nov 17, 2021

thinking more about this over the morning it seems like having a cluster-deployed registry option could solve this problem reasonably well, and has the following benefits over this implementation:

  • would work on every type of cluster with a single implementation
  • makes the source of the image must less dependent on a local docker installation
  • is already an active feature request in A registry addon #147

Let me know your thoughts?

@rainest
Copy link
Contributor Author

rainest commented Nov 17, 2021

Re #151 (comment) tl;dr nah, use this still and add a push to GCR option if you want the same on GKE later. The experience for the test client is the same, and it's simpler and more standard.

Accessing images currently in the local Docker instance is the point here: if the Docker image was already published, you wouldn't need to load the image at all; you'd just provide the public URL of the image. This isn't a problem for the intended use case (you don't use this unless you have an image you haven't published), it's only a problem for running tests: in CI, you need to build the image before you can push it. Using an alternative private registry doesn't remove the requirement that you push images to the registry in advance.

Building and pushing is the responsibility of the test client, since you're (presumably) using this to test against an unpublished version of the application you're using KTF to test (KIC). I didn't set that up for Kong/kubernetes-ingress-controller#2005 since running the test locally against the development version seemed sufficient before those tests would normally run (after a tag push builds an image and a version with these changes is publicly available). I think having KIC CI build the image and run E2E against it would be useful, but it's something we don't strictly need immediately and the development time to add that also given that the goal here is to work around issues using the integration tests (no established paradigm of running differently-configured clusters during the same test run) or unit tests (nigh impossible here) for this.

The system mentioned in #147 is KIND's local registry option. That relies on the registry and cluster running on the same host, with Docker network assistance to expose one to the other. It wouldn't work in other contexts without a lot of hackery. Cluster-deployed registries that the cluster then pulls from aren't impossible, but there doesn't appear to be a well-established practice around doing it, just several blog articles that indicate it's doable. Given the amount of setup involved though, I'm not convinced that it's valuable--the end goal is the same, and for hosted clusters it's almost certainly going to be easier to just push to the standard hosted registry for that provider. This maybe goes back to the philosophical dispute we have over how this should be used, but IMO heterogeneous environments are a fact of life in IT and we should contend with that reality; you don't get everything working the same way universally unless you're in physics instead.

@rainest rainest temporarily deployed to integration-tests November 17, 2021 19:35 Inactive
@rainest rainest requested a review from shaneutt November 17, 2021 19:41
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

approving based on a desire to keep things unblocked, feel free to resolve any open comments as you see fit. I can accept the current implementation for now, despite wanting some different things.

pkg/clusters/addons/loadimage/addon.go Outdated Show resolved Hide resolved
@rainest rainest temporarily deployed to integration-tests November 18, 2021 19:33 Inactive
@rainest rainest temporarily deployed to integration-tests November 18, 2021 19:37 Inactive
@rainest rainest enabled auto-merge (squash) November 18, 2021 19:42
@rainest rainest merged commit 4c5baa4 into main Nov 18, 2021
@rainest rainest deleted the feat/loadimage-addon branch November 18, 2021 19:53
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.

3 participants