Kustomize variation to deploy Online Boutique apps without the `grpc-health-probe` bin #1102

mathieu-benoit · 2022-09-27T14:49:25Z

Support native gRPC Healthcheck for livenessProbe and readinessProbe since GKE 1.24.
Partially implementing Switch to Kubernetes builtin grpc health checks #662 (following up on first experimental tests documented here [WIP] Use upstream Kubernetes gRPC Health Check feature #849)
- "Partially" because it won't be yet by default, not all GKE versions support that, just 1.24+. So it will be an optional Kustomize variation that users would be able to leverage if they want.
Size of the images got -3.9MB (virtual) / -10.9MB (on disk)
loadgenerator and frontend are not impacted by that (not gRPC apis/apps)
Adding 3 more Kustomize variations related to this one:
- container-images-registry (not needed, but similar to the one below)
- container-images-tag (not needed, but similar to the one below)
- container-images-tag-suffix (needed for this new Kustomize variation)
Review the setup of tokens to avoid issues with Kustomize and bash session
Update the release process (more precisely make-docker-images.sh) in order to build a second image next to the default/official one by adding a prefix -native-grpc-probes.
These -native-grpc-probes images are not built at every PR nor merge into main, they are only built during the release process.
Added those new Kustomize variations in the Smoke test in CI, with some sed examples too

Tests realized on a GKE 1.24 now available on rapid channel:

gcloud container clusters create tests \
    --zone=us-central1-b \
    --machine-type=e2-standard-2 \
    --num-nodes=4 \
    --release-channel=rapid

Build the containers for all the Online Boutique apps with the -native-grpc-probes image tag, you can leverage the script here.

Then, deploy this like:

cd kustomize/
SUFFIX=-native-grpc-probes
sed -i "s/CONTAINER_IMAGES_TAG_SUFFIX/$SUFFIX/g" components/container-images-tag-suffix/kustomization.yaml
kustomize edit add component components/container-images-tag-suffix
kustomize edit add component components/native-grpc-health-check
kubectl kustomize . | sed "s/$SUFFIX$SUFFIX/$SUFFIX/g" | kubectl apply -f

Note: for this variation, kubectl apply -k . won't work because there is a known issue currently in Kustomize where the tagSuffix is duplicated. The command lines above are a temporary workaround.

We won't make this the default configuration yet, GKE 1.24 is the default version for the rapid channel. We need to make the choice in the future when this configuration will be the default ones. GKE just got 1.24 in rapid channel in 2022-08-12 and is planned to have its end of life on 2023-09-30. So it's 1 year from now. Will it be a good time then, after 2023-09-30?

github-actions · 2022-09-29T13:15:51Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-09-29T14:18:04Z

🚲 PR staged at http://35.224.222.105

…stomize otherwise)

github-actions · 2022-09-29T21:28:28Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-09-30T15:21:26Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-09-30T15:42:23Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-09-30T18:27:48Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-09-30T20:05:33Z

🚲 PR staged at http://35.224.222.105

mathieu-benoit · 2022-09-30T22:35:14Z

Ready for your review, thanks!

github-actions · 2022-10-18T12:32:29Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-10-18T15:21:14Z

🚲 PR staged at http://35.224.222.105

hack/make-docker-images.sh

NimJay · 2022-10-21T14:49:33Z

kustomize/components/memorystore/kustomization.yaml

@@ -28,7 +28,7 @@ patchesStrategicMerge:
          - name: server
            env:
            - name: REDIS_ADDR
-              value: "{{REDIS_ADDR}}"
+              value: "REDIS_ADDR"


Question: Is there a reason we remove the curly braces?
Should we also fix:

sed -i "s/{{REDIS_ADDR}}/${REDIS_IP}:${REDIS_PORT}/g" components/memorystore/kustomization.yaml

inside kustomize/components/memorystore/README.md?

Maybe it was an accident?

Suggested change

value: "REDIS_ADDR"

value: "{{REDIS_ADDR}}"

Hey Nim, good question, actually I had that in the description: Review the setup of tokens to avoid issues with Kustomize and bash session. So that's on purpose otherwise we have issue in bash session when doing the sed part and/or when doing kustomize build . without these tokens replaced. Let me know if you need more info. But the tl,dr is we should have token just like this REDIS_ADDR not with any {} nor {{}}, etc.

Following this today, one possible unintended side effect is that the sed now matches the name and value. So I end up with:

- name: 10.34.24.75:6379 value: "10.34.24.75:6379"

Yep, that's why we document this like this: f60a67f. But yeah if someone is not doing the \" it could land to this error. Open for suggestions to improve this for the different files having tokens like this. PR about that are more than welcome ;)

Correct. I checked that I pulled latest and just reran the terraform deploy. I see the escape characters and the quote \" and it still seems to be replacing all of them.

To clarify, it worked in the sense that the application deployed beautifully, it just replaced all instances of REDIS_ADDR with 10.34.24.75:6379. (Including the instance at the top of the /microservices-demo/kustomize/components/memorystore/kustomization.yaml file.)

-# cartservice - replace REDIS_ADDR to target new Memorystore (redis) instance +# cartservice - replace 10.34.24.75:6379 to target new Memorystore (redis) instance - |- apiVersion: apps/v1 kind: Deployment @@ -27,8 +27,8 @@ patchesStrategicMerge: containers: - name: server env: - - name: REDIS_ADDR - value: "REDIS_ADDR" + - name: 10.34.24.75:6379 + value: "10.34.24.75:6379" # redis - remove the redis-cart Deployment - |- apiVersion: apps/v1

Gotcha, thanks. Do you mind reporting this as a dedicated issue please? We will need to tackle this to fix this broken Terraform deployment with Memorystore. Thanks!

I'll do that!

FYI: this will be fixed via #1202. Thanks!

Nice!

I also had a solution (all the backslashes!!) that I was going to PR today, but that is much cleaner.

NimJay · 2022-10-21T14:58:51Z

kustomize/components/container-images-registry/README.md

+From the `kustomize/` folder at the root level of this repository, execute this command:
+```
+REGISTRY=my-registry # Example: gcr.io/my-project/my-directory
+sed -i "s|CONTAINER_IMAGES_REGISTRY|${REGISTRY}|g" components/container-images-registry/kustomization.yaml


Rationale for my change:
I've changed "s/CONTAINER_IMAGES_REGISTRY/$REGISTRY/g" to "s|CONTAINER_IMAGES_REGISTRY|$REGISTRY|g" because the /s inside gcr.io/my-project/my-directory were breaking the sed command. Now we're no longer using regexes (we're using |s instead) to avoid /s from the ${REGISTRY} variable being interpreted as special characters.

kustomize/components/container-images-tag-suffix/README.md

NimJay · 2022-10-21T15:14:07Z

kustomize/components/memorystore/README.md

@@ -47,7 +47,7 @@ components:
 ```

 Update current Kustomize manifest to target this Memorystore (Redis) instance.
-```bash
+```sh
 REDIS_IP=$(gcloud redis instances describe redis-cart --region=${REGION} --format='get(host)')
 REDIS_PORT=$(gcloud redis instances describe redis-cart --region=${REGION} --format='get(port)')
 sed -i "s/{{REDIS_ADDR}}/${REDIS_IP}:${REDIS_PORT}/g" components/memorystore/kustomization.yaml


Suggestion: Should we update this sed command?

Suggested change

sed -i "s/{{REDIS_ADDR}}/${REDIS_IP}:${REDIS_PORT}/g" components/memorystore/kustomization.yaml

sed -i "s/REDIS_ADDR/${REDIS_IP}:${REDIS_PORT}/g" components/memorystore/kustomization.yaml

Regarding: #1102 (comment)

Oh yeah, maybe a merge issue, let me check that and update this.

kustomize/kustomization.yaml

github-actions · 2022-10-21T15:31:30Z

🚲 PR staged at http://35.224.222.105

…//github.com/GoogleCloudPlatform/microservices-demo into mathieu-benoit/without-grpc-health-probe-bin

github-actions · 2022-10-21T15:32:57Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-10-21T15:34:43Z

🚲 PR staged at http://35.224.222.105

github-actions · 2022-10-21T15:37:15Z

🚲 PR staged at http://35.224.222.105

NimJay

Thanks again, Mathieu, for proactively figuring (and implementing) this out! 👏
Everything looks good.

I have tested all the Kustomize building.
I've committed some minor fixes/edits to avoid back-and-forth.

But I have not deployed the new -native-grpc-probes to a GKE cluster, but I trust that they work.

Feel free to merge whenever.

…health-probe` bin (GoogleCloudPlatform#1102) * without-grpc-health-probe-bin * fix docker build for cartservice * kustomize - add container-image-registry * kustomize - add container-image-tag * kustomize - add container-image-tag-suffix * fix container-images-registry * fix container-images-tag * complete native-grpc-health-check Kustomize variation * Review tokens in Kustomize files: $() instead of {{}} (issues with Kustomize otherwise) * docker build -t "${image}-native-grpc" * Bonify the custom test in Kustomize CI * Review token to avoid issue in bash with sed: @() instead of $() * Fix token issues with Kustomize * More documentation for the new Kustomize variations * fix docs * update docs * fix link in doc * Fix container-images-registry/README.md * Update container-images-registry/README.md * Update container-images-tag-suffix/README.md * Update container-images-tag-suffix/README.md * Update native-grpc-health-check/README.md * yaml|bash|output in docs * Remove {{ }} around REDIS_ADDR * fix "REDIS_ADDR" token Co-authored-by: minherz <minherz@users.noreply.github.com> Co-authored-by: Nim Jayawardena <nimjay@google.com>

without-grpc-health-probe-bin

f3e6ef7

mathieu-benoit requested a review from a team as a code owner September 27, 2022 14:49

mathieu-benoit marked this pull request as draft September 27, 2022 14:49

mathieu-benoit and others added 8 commits September 27, 2022 10:49

Merge branch 'main' into mathieu-benoit/without-grpc-health-probe-bin

b1e0835

Merge branch 'main' into mathieu-benoit/without-grpc-health-probe-bin

b53a97b

fix docker build for cartservice

f458993

kustomize - add container-image-registry

8d1bd03

kustomize - add container-image-tag

9c31b94

kustomize - add container-image-tag-suffix

921a04c

fix container-images-registry

1f4d261

fix container-images-tag

c146e00

mathieu-benoit changed the title ~~without-grpc-health-probe-bin~~ Kustomize variation to deploy Online Boutique apps without the grpc-health-probe bin Sep 29, 2022

mathieu-benoit mentioned this pull request Sep 29, 2022

[WIP] Use upstream Kubernetes gRPC Health Check feature #849

Closed

4 tasks

complete native-grpc-health-check Kustomize variation

e44a138

Review tokens in Kustomize files: $() instead of {{}} (issues with Ku…

bd38492

…stomize otherwise)

mathieu-benoit and others added 2 commits September 30, 2022 11:10

Merge branch 'main' into mathieu-benoit/without-grpc-health-probe-bin

72a2bfb

docker build -t "${image}-native-grpc"

5ed543a

Mathieu Benoit added 2 commits September 30, 2022 14:24

Bonify the custom test in Kustomize CI

0ca4a52

Review token to avoid issue in bash with sed: @() instead of $()

77a38a0

Fix token issues with Kustomize

403eb0b

More documentation for the new Kustomize variations

7c5acf7

mathieu-benoit marked this pull request as ready for review September 30, 2022 22:34

fix link in doc

7b3096d

Merge branch 'main' into mathieu-benoit/without-grpc-health-probe-bin

3e0e2c3