-
Notifications
You must be signed in to change notification settings - Fork 274
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(k8s): flexible buildKit cache configuration with mode=max
support
#3239
Conversation
7d76995
to
70be70b
Compare
mode=max
support
cd7f616
to
f3c486d
Compare
631907e
to
60b8380
Compare
mode=max
supportmode=max
support
7507484
to
0010349
Compare
6cd248f
to
c40fa1d
Compare
The test failure seems to be unrelated to this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some comments and suggestions, but overall this is great and a very welcome improvement 👏
I'd love to see more documentation in the in-cluster building guide around this as well, as opposed to just documenting in the reference. A suggested good practice in the guide would make a lot of sense imo.
core/test/integ/src/plugins/kubernetes/container/build/buildkit.ts
Outdated
Show resolved
Hide resolved
// Detect gcr.io | ||
if (deploymentImageName.includes("gcr.io")) { | ||
return "inline" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be unnecessary, not sure, but we could use parseImageId
to narrow this detection to the hostname part specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with https://github.com/garden-io/garden/pull/3239/files#r978832787
the test for in-cluster registry will fail.... all the code looks very complex, I thought about adding port support but I'd be very afraid of breaking something else with that
diff --git a/core/src/plugins/kubernetes/container/build/buildkit.ts b/core/src/plugins/kubernetes/container/build/buildkit.ts
index 980b9378..37e418a9 100644
--- a/core/src/plugins/kubernetes/container/build/buildkit.ts
+++ b/core/src/plugins/kubernetes/container/build/buildkit.ts
@@ -37,6 +37,7 @@ import { getRunningDeploymentPod, millicpuToString, megabytesToString, usingInCl
import { PodRunner } from "../../run"
import { prepareSecrets } from "../../secrets"
import { ContainerModuleOutputs } from "../../../container/container"
+import { containerHelpers } from "../../../container/helpers"
export const buildkitImageName = "gardendev/buildkit:v0.9.3-1"
export const buildkitDeploymentName = "garden-buildkit"
@@ -322,13 +323,15 @@ export const getSupportedCacheMode = (
return cache.mode
}
+ const host = containerHelpers.parseImageId(deploymentImageName).host!
+
// Detect AWS ECR
- if (deploymentImageName.includes(".dkr.ecr.")) {
+ if (host.includes(".dkr.ecr.")) {
return "inline"
}
// Detect gcr.io
- if (deploymentImageName.includes("gcr.io")) {
+ if (host.includes("gcr.io")) {
return "inline"
}
Co-authored-by: Tim <tim@garden.io>
Co-authored-by: Tim <tim@garden.io>
Co-authored-by: Jon Edvald <edvald@gmail.com>
Co-authored-by: Jon Edvald <edvald@gmail.com>
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
`valid` is the equivalent of `allow(..).only()`
65df0bf
to
5a9ccce
Compare
@edvald thank you for the review! I tried to address all the comments, and when I didn't I tried to explain why. See you after vacation 👍 🍹 |
What this PR does / why we need it:
This is a result of a pairing session between @TimBeyer and @stefreak
Two main goals here:
We achieved them by introducing a
cache
key within theclusterBuildkit
config of the kubernetes provider.The default is the following configuration
(mode is implicitly
auto
by default)If you use gcr or aws ecr, this will effectively be equivalent to the current behaviour of garden, using an inline cache:
and if you use any other registry, the default mode auto is equivalent to:
more advanced configurations are now possible as well. example:
which will result in a better cache hit rate: if you are developing in a new feature branch, you will use the cache of the main branch for the first build, and export the cache to a feature branch specific tag. From then on, you will use the feature branch specific tag for consequent builds. This makes sure that other builds don't pollute the feature branch build cache, and your feature branch does not pollute the master branch build cache. The idea for this configuration originates from a discussion with @antoinelyset from Slite: #3219 (comment)
Furthermore, it is now possible to configure cache registries that deviate from the deploymentRegistry, like so:
Currently the only supported
type
isregistry
, but the way to articulate thecache
makes it possible to add other cache types as well in future pull requests.Which issue(s) this PR fixes:
Fixes #3219
Fixes #3110
Special notes for your reviewer:
joi
configuration. We couldn't get the defaults working without repeating them. Help appreciated