fix(k8s): handle AEC-paused resources properly #5122

vvagaytsev · 2023-09-21T11:49:14Z

What this PR does / why we need it:
A follow-up fix for #4846 to address @stefreak 's comment on the AEC.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
The commit-by-commit review might be easier :)

core/src/plugins/kubernetes/kubernetes-type/handlers.ts

twelvemo

Overall looks good, but i think we might be using the wrong annotation key.

core/src/plugins/kubernetes/kubernetes-type/handlers.ts

shumailxyz · 2023-09-21T12:35:05Z

e2e-run-actions is failing but i think it's a flake

twelvemo

Super quick turnaround!

core/src/plugins/kubernetes/kubernetes-type/handlers.ts

* refactor: extract named function for manifest post-processing * refactor: split manifest reading and post-processing * fix(k8s): correctly resolve manifests when `build` is set * fix(k8s): quickfix to avoid resetting `outdated` state to `ready` * test: fix tests * refactor(test): move helper to the upper-level scope It will be used by other test contexts. * refactor(test): introduce one more convenience helper * refactor(test): introduce and rename some local vars * refactor(test): use helpers to avoid code duplication * chore: re-arranged code Keep all helpers close to each other for simpler navigation. * chore(test): rename some tests and variables To avoid usage of the old glossary. * refactor(test): helper function to deploy in namespaces * DRY to avoid repetition and too complex local state * Less shared global state between different test contexts * Avoid dependencies between tests and reliance on the execution order * Ability to run individual tests locally * refactor(test): convert lambdas to functions * test: ensure all resources are deleted by `deleteKubernetesDeploy` A metadata `ConfigMap` describing what was last deployed must be deleted too. * test: restore initial module config state after each test To avoid unexpected pollution of the Garden instance's state. Multiple test can define temporary custom module config. Such config changes should not affect the other tests. * refactor(k8s): helper to compose k8s deploy status * refactor(k8s): return deploy status immediately on "missing" state * refactor: unwrap unnecessary else-block * chore: variable scoping * refactor: extract helper to check if deploy is outdated * refactor: move input args check into `getForwardablePorts` * chore: remove unnecessary code and comments Local mode checks were moved to `getForwardablePorts` in #5022. * chore: remove unnecessary try/catch Function `getForwardablePorts` does not call any potentially unsafe operations. It's not wrapped into try/catch in the other code places. * chore: remove unused local var * refactor: use SRP in status calculation Split individual resource status retrieval and overall state calculation. * refactor: simplified code Minimized mutability of the local vars. More straightforward and linear processing flow. * test: fix test setup to cover the original bug * chore: post-merge corrections * refactor: remove unnecessary function call The value has already been calculated as an immutable value. * chore: typos and wording Got rid of old-fashioned term "service". Replaced it with "deploy". --------- Co-authored-by: Jon Edvald <edvald@gmail.com>

This is a follow-up quickfix for #4846. It reads a specific annotation and checks if it has a special value set by Cloud. This needs to be changed to reply on a more generic and reliable way of k8s resource comparison.

To avoid repetitive creation of the same immutable string.

…n key

stefreak

Great work! 👍

I think we need to still make sure that deploy -f always deploys even when the deploy is ready already; But that can be done in a separate PR.

shumailxyz · 2023-09-25T10:26:04Z

@vvagaytsev Some of the tests are failing. I wonder if those are due to the manifest hash check you added.

vvagaytsev · 2023-09-25T10:29:28Z

@shumailxyz thanks! I see, that's because of the annotation change. The tests share the same in-cluster resource. After the new test is executed, I'll ensure that everything is reverted back to the normal state.

@stefreak thanks! Yes, let's address the --force flag in a separate PR/

Because `Regex` is a stateful object.

stefreak · 2023-09-25T11:12:57Z

core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts

+      } finally {
+        // Redeploy the resource to restore the original value of `k8sManifestHashAnnotationKey` annotation,
+        // because the remote resource state is used by the other tests.
+        await kubernetesDeploy(deployParams)


I understand if that's a bigger change and you want to do it separately; But would it be better to run kubernetesDeploy in a beforeEach in a group for all the tests that rely on it being deployed?

seeing try {} finally {} in a test is strange when mocha provides before and after primitives for these kinds of things

That requires more changes and re-grouping of the tests. We can do it later if we have more tests that modify remote resources that should be re-deployed on the test completion.

I added a dedicated config for this test case in 75c1b24.
Now we don't need to restore the remote resource state used by the other tests.

To avoid unwanted modifications of the remote resources used by the other tests.

stefreak

👍

core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts

shumailxyz

Awesome 💯

Just a typo that i found.

Co-authored-by: Shumail Mohyuddin <shumailmohyuddin@gmail.com>

vvagaytsev requested review from shumailxyz and twelvemo September 21, 2023 11:49

vvagaytsev changed the title ~~Fix/k8s action type aec~~ @vvagaytsev fix(k8s): handle AEC-paused resources properly Sep 21, 2023

vvagaytsev changed the title ~~@vvagaytsev fix(k8s): handle AEC-paused resources properly~~ fix(k8s): handle AEC-paused resources properly Sep 21, 2023

shumailxyz reviewed Sep 21, 2023

View reviewed changes

core/src/plugins/kubernetes/kubernetes-type/handlers.ts Outdated Show resolved Hide resolved

vvagaytsev requested a review from shumailxyz September 21, 2023 12:05

twelvemo requested changes Sep 21, 2023

View reviewed changes

core/src/plugins/kubernetes/kubernetes-type/handlers.ts Show resolved Hide resolved

twelvemo self-requested a review September 21, 2023 12:33

shumailxyz previously approved these changes Sep 21, 2023

View reviewed changes

twelvemo previously approved these changes Sep 21, 2023

View reviewed changes

vvagaytsev added this pull request to the merge queue Sep 21, 2023

stefreak removed this pull request from the merge queue due to a manual request Sep 21, 2023

stefreak reviewed Sep 21, 2023

View reviewed changes

core/src/plugins/kubernetes/kubernetes-type/handlers.ts Outdated Show resolved Hide resolved

vvagaytsev marked this pull request as draft September 21, 2023 14:49

stefreak mentioned this pull request Sep 21, 2023

garden deploy: deployments are not being scaled up to 1 replica #3438

Closed

shumailxyz self-requested a review September 22, 2023 08:24

vvagaytsev dismissed stale reviews from twelvemo and shumailxyz via b483afc September 25, 2023 09:33

vvagaytsev force-pushed the fix/k8s-action-type-aec branch 2 times, most recently from b483afc to 4569244 Compare September 25, 2023 09:35

vvagaytsev added 8 commits September 25, 2023 12:01

fix(k8s): handle AEC-paused resources properly

f41fc05

This is a follow-up quickfix for #4846. It reads a specific annotation and checks if it has a special value set by Cloud. This needs to be changed to reply on a more generic and reliable way of k8s resource comparison.

refactor(k8s): explicit typing

c8e416f

refactor(k8s): extract local variable

6d18ae1

To avoid repetitive creation of the same immutable string.

chore: add TODO comment

c1be672

refactor(k8s): introduce named constant for manifest-hash annotatio…

058a04d

…n key

refactor(k8s): use helper to construct the annotation key

7e4e55e

chore: helper function to check sha256 validity

aeece69

refactor: move hash-helper function to a dedicated module

163beb3

vvagaytsev force-pushed the fix/k8s-action-type-aec branch from e15a682 to f2f2704 Compare September 25, 2023 10:02

vvagaytsev marked this pull request as ready for review September 25, 2023 10:04

vvagaytsev requested review from stefreak and twelvemo September 25, 2023 10:04

stefreak previously approved these changes Sep 25, 2023

View reviewed changes

vvagaytsev added 2 commits September 25, 2023 12:58

test: fix integration test to restore the valid cluster state

fbfb2b6

fix: re-create Regex object before every test call

0d145d5

Because `Regex` is a stateful object.

vvagaytsev dismissed stefreak’s stale review via 0d145d5 September 25, 2023 11:00

stefreak reviewed Sep 25, 2023

View reviewed changes

test: use separate module config in integ test

75c1b24

To avoid unwanted modifications of the remote resources used by the other tests.

vvagaytsev requested a review from stefreak September 25, 2023 11:50

vvagaytsev enabled auto-merge September 25, 2023 13:39

stefreak previously approved these changes Sep 25, 2023

View reviewed changes

vvagaytsev added this pull request to the merge queue Sep 25, 2023

shumailxyz reviewed Sep 25, 2023

View reviewed changes

core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts Outdated Show resolved Hide resolved

shumailxyz previously approved these changes Sep 25, 2023

View reviewed changes

vvagaytsev removed this pull request from the merge queue due to a manual request Sep 25, 2023

vvagaytsev dismissed stale reviews from shumailxyz and stefreak via 07d32c6 September 25, 2023 14:33

chore: typo fix

07d32c6

Co-authored-by: Shumail Mohyuddin <shumailmohyuddin@gmail.com>

vvagaytsev enabled auto-merge September 25, 2023 14:33

vvagaytsev requested review from shumailxyz and stefreak September 25, 2023 14:33

shumailxyz approved these changes Sep 25, 2023

View reviewed changes

vvagaytsev added this pull request to the merge queue Sep 25, 2023

Merged via the queue into main with commit ed87cfd Sep 25, 2023
42 checks passed

vvagaytsev deleted the fix/k8s-action-type-aec branch September 25, 2023 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(k8s): handle AEC-paused resources properly #5122

fix(k8s): handle AEC-paused resources properly #5122

vvagaytsev commented Sep 21, 2023 •

edited

twelvemo left a comment

shumailxyz commented Sep 21, 2023

twelvemo left a comment

stefreak left a comment

shumailxyz commented Sep 25, 2023

vvagaytsev commented Sep 25, 2023

stefreak Sep 25, 2023

stefreak Sep 25, 2023

vvagaytsev Sep 25, 2023

vvagaytsev Sep 25, 2023

stefreak left a comment

shumailxyz left a comment

fix(k8s): handle AEC-paused resources properly #5122

fix(k8s): handle AEC-paused resources properly #5122

Conversation

vvagaytsev commented Sep 21, 2023 • edited

twelvemo left a comment

Choose a reason for hiding this comment

shumailxyz commented Sep 21, 2023

twelvemo left a comment

Choose a reason for hiding this comment

stefreak left a comment

Choose a reason for hiding this comment

shumailxyz commented Sep 25, 2023

vvagaytsev commented Sep 25, 2023

stefreak Sep 25, 2023

Choose a reason for hiding this comment

stefreak Sep 25, 2023

Choose a reason for hiding this comment

vvagaytsev Sep 25, 2023

Choose a reason for hiding this comment

vvagaytsev Sep 25, 2023

Choose a reason for hiding this comment

stefreak left a comment

Choose a reason for hiding this comment

shumailxyz left a comment

Choose a reason for hiding this comment

vvagaytsev commented Sep 21, 2023 •

edited