fix(k8s): correctly resolve manifests when build is set
#4516
Conversation
This ended up being a bit of a refactor, since we already had a TODO in code around resolving manifests when using build dependencies, and that needed to be resolved to properly address the issue. `kubernetes` module/Deploy statuses now rely on a metadata ConfigMap, in principle akin to how Helm chart statuses are resolved. This avoids having to actually perform builds before working out the manifests that belong to a Deploy action. Along the way, avoided superfluous API calls when comparing resources and resolving their statuses.
|
Added a quick feature commit because I ran into it while working on this (no doubt quite welcome to those who have bumped into not being able to use globs in the |
There was a problem hiding this comment.
Extremely good work @edvald!
I like how this has the side effect of reducing the number of API calls.
This is a big change and we need to carefully see if we can live with interactions of this change together with the Automatic Environment Cleanup feature of Garden Cloud and similar work arounds that are being used in the wild (e.g. #3438)
| const api = await KubeApi.factory(log, ctx, k8sCtx.provider) | ||
|
|
||
| // FIXME: We're currently reading the manifests from the module source dir (instead of build dir) | ||
| // because the build may not have been staged. |
There was a problem hiding this comment.
What does this mean? Why has the build not been staged? 🤔
Trying to wrap my head around this
There was a problem hiding this comment.
Ah, to figure out wether or not the kubernetes deploy action is cached or not, and thus to figure out wether or not the build exec action needs to run, we need the manifest currently– now I start to understand.
We need to make sure that this new logic is being reflected in the "pause" logic of the cleanup runner in Garden Cloud (https://github.com/garden-io/garden-platform/blob/main/workflow-runner/src/runners/cleanup-runner/index.ts#L258) – if I understand this right, in the pause logic of the cleanup runner we now need to delete the config map, instead of overwriting the label?
There was a problem hiding this comment.
It would be good in the mid to long term if we standardise how the caching works across plugins, e.g. by using Garden Cloud as a backend and falling back to a local file backend if not connected to Cloud.
There was a problem hiding this comment.
Yeah that would be good to have helpers for, maybe something built-in on ctx, but figure that's out of scope here.
There was a problem hiding this comment.
Re: pausing environments, we are of course planning to make that more native+explicit (incl. a garden pause command) but I'd need to defer on how this might affect the current routine used in Cloud/Enterprise. @10ko do you see a problem upfront here?
Let's merge this soon, but it would be optimal if made a change in the Garden Cloud cleanup runner at least before releasing this, otherwise the pause feature will cease to be compatible with latest Garden. I will try to do that this week as I think it's not a big amount of work. |
|
What's the status on this one? Can we merge? (cc @stefreak) |
|
@edvald there are some merge conflicts. Please resolve those and let's merge this :) |
|
@edvald let's rebase this and merge |
# Conflicts: # core/src/plugins/exec/exec.ts # core/src/plugins/kubernetes/container/status.ts # core/src/plugins/kubernetes/kubernetes-type/common.ts # core/src/plugins/kubernetes/kubernetes-type/handlers.ts # core/src/plugins/kubernetes/status/status.ts # core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts # yarn.lock
# Conflicts: # core/src/plugins/openshift/deploy.ts
|
Hmm... Now there are some integration test failures in all VMs. I'll take a look. |
|
Fixed the test failures in fecb878. |
There was a problem hiding this comment.
Great work @vvagaytsev bringing this over the finish line! Thank you so much 🚀
|
@vvagaytsev @edvald @Walther @eysi09 did someone verify the AEC pause feature? Reading this comment sounds like there is definitely a change needed in Garden Cloud AEC |
The regression was introduced in #4516. Initial implementation of glob patterns in files paths could result into empty list of files. This fixes the behaviour by fail-fast existence checks for manifest files.
* fix(k8s): throw on missing k8s manifests files The regression was introduced in #4516. Initial implementation of glob patterns in files paths could result into empty list of files. This fixes the behaviour by fail-fast existence checks for manifest files. * test: add missing tests for `readManifest` Test coverage for `spec.files` of `Deploy` action of `kubernetes` type. * Add own Deployment manifest file, because builds are not executed in the test. * Add unit tests to document and cover the expected behaviour. Initial changes from c9efb47 did not have effect. The Deploy action `with-build-action` has never been called with `readManifests`.
This ended up being a bit of a refactor, since we already had a TODO in code around resolving manifests when using build dependencies, and that needed to be resolved to properly address the issue.
kubernetesmodule/Deploy statuses now rely on a metadata ConfigMap, in principle akin to how Helm chart statuses are resolved. This avoids having to actually perform builds before working out the manifests that belong to a Deploy action.Along the way, avoided superfluous API calls when comparing resources and resolving their statuses.
Closes #4505
Closes #4506
cc @stefreak