Refactor: reliability, retries, reduced k8s apiserver load #14

artem-zinnatullin · 2020-07-23T18:42:09Z

This PR combines all the changes I had to make to the plugin to make it stable in our CI environment:

AWS EKS cluster
k8s apiserver was failing under the load created by frequent kubectl invocations made by the plugin/jobs: Unable to connect to the server: net/http: TLS handshake timeout / Unable to connect to the server: dial tcp x.x.x.x:443: i/o timeout
~10k CI jobs per day, peaking at 3-4k per hour

It is an evolution of the original PR #8 which I closed and promised to open a new one.

This PR:

Wraps every kubectl invocation in a while true loop with interval and timeout being configurable, this allows surviving k8s apiserver downtimes or say network issues while also being fast in a normal scenario
Unifies configuration values while allowing them to be overridden via environment variables
Changes log streaming to first wait for some logs to appear and only then stream the log, previously we often were getting logs printed either multiple times or never
Reduces number of kubectl invocations to reduce load on k8s apiserver
Adds option to skip job cleanup logic if it was successful for users relying on spec.ttlSecondsAfterFinished or lwolf/kube-cleanup-operator
Adds option to set spec.ttlSecondsAfterFinished
Adds while true loop with configurable interval and timeout to wait for completion of log streaming racing with job status check
Fixes Jobs fail if branch names are longer than 63 characters #5 when a job annotation like branch name is longer than 63 symbols (limitation in k8s)
Removes commit message from job annotation, it causes unrecoverable job metadata overflow if it's long
Propagates BUILDKITE_BUILD_ID to the Job environment
Propagates actual Pod exit code to Buildkite if Job fails, very useful for Buildkite retries based on exit code or other exit code analysis

I know it sounds like a lot of changes but without them the plugin didn't perform well in our environment. I tried to make the code as clean, readable and safe as my Bash knowledge allows.

Hope this brings more users to the plugin and a better experience for them!

…user value

coderanger · 2020-09-08T23:03:55Z

set -x will only echo the running commands, it's not very verbose. And stdout/err desync is fairly common depending on your underlying container logging driver :) Docker's default one collects the two independently and in pipes so they are basically batched. The env var change is required and correct, the input format looks like KEY="VALUE" as confirmed by the Buildkite docs. It might even need more work depending on the formatting used for values that actually contain ", I haven't tested that yet.

… to YAML

artem-zinnatullin · 2020-10-07T01:17:49Z

lib/job.jsonnet

@@ -69,6 +71,7 @@ function(jobName, agentEnv={}, stepEnvFile='', patchFunc=identity) patchFunc({
    BUILDKITE_PLUGIN_K8S_RESOURCES_REQUEST_MEMORY: '',
    BUILDKITE_PLUGIN_K8S_RESOURCES_LIMIT_MEMORY: '',
    BUILDKITE_PLUGIN_K8S_WORKDIR: std.join('/', [env.BUILDKITE_BUILD_PATH, buildSubPath]),
+    BUILDKITE_PLUGIN_K8S_JOB_TTL_SECONDS_AFTER_FINISHED: '86400',


I'm really bad at Jsonnet, I've declared default value in YAML, but not sure how to keep this declaration in .jsonnet 😬

This looks correct! We have these as default values then we override them with any values provided by the pipeline https://github.com/EmbarkStudios/k8s-buildkite-plugin/pull/14/files#diff-d8106e0da5023a3b7eb0401c475f88b2R75

artem-zinnatullin · 2020-10-07T01:19:33Z

plugin.yml

@@ -63,6 +63,18 @@ configuration:
      type: string
    use-agent-node-affinity:
      type: boolean
+    print-resulting-job-spec:
+      type: boolean
+      default: false


Interestingly, I couldn't find use of default in any Buildkite plugin https://github.com/topics/buildkite-plugin, but JSON Schema that Buildkite uses seems to allow it https://json-schema.org/understanding-json-schema/reference/generic.html#annotations

That's interesting, does buildkite actually set a default value from the yaml on behalf of the plugin or does it allow it solely for documentation purposes?

artem-zinnatullin · 2020-10-07T01:24:39Z

@iffyio I think I've resolved all comments you've left up until now :)

Btw, fyi I've also added Pod exit code propagation (updated PR description), very useful addition in my opinion 🙃

iffyio · 2020-10-08T18:39:11Z

@iffyio I think I've resolved all comments you've left up until now :)

Btw, fyi I've also added Pod exit code propagation (updated PR description), very useful addition in my opinion

Thank you! I'll take another look at this soon.

keith · 2020-10-09T16:49:26Z

hooks/command

+  sleep "$log_complete_retry_interval_sec"
+done
+
+status=""


Suggested change

status=""

status=1

we see issues where somehow we end up exiting with this default value:

/buildkite/plugins/github-com-artem-zinnatullin-k8s-buildkite-plugin-git-65192202c691578f25d42d04d57fb4b12d5b49c6/hooks/command: line 149: exit: null: numeric argument required

so having this more conservative exit code is a bit nicer

The null is likely coming from setting the status to the output of jq e.g here https://github.com/EmbarkStudios/k8s-buildkite-plugin/pull/14/files#diff-7a7c160f9b73cfeaf4d8b93aee769fc1R188

@artem-zinnatullin I think we'd want to only set status if jq finds a match? one way would be to use enable exit code with jq -e so that it exits with non-zero if no match is found and then, only set the status if the exit code is 0.

jq -e sounds good, annoying that you have to do set +e; jq -e; set -e; or write to a file to like if jq -e > result to handle both error code and output, but what can you do I guess 😅

iffyio

This looks great! Left a few comments, most of which should be minor fixes I think

README.md

iffyio · 2020-10-12T17:59:20Z

README.md

+If set to `true` plugin cleans up finished k8s job.
+Default value: `true`.
+
+If you have TTL controller or https://github.com/lwolf/kube-cleanup-operator running, it is highly recommended to set the value to `false` to reduce load on k8s api servers.


Similar suggestion to the job-cleanup-via-plugin option

iffyio · 2020-10-12T19:08:00Z

hooks/command

+  while true
+  do
+    set +e
+    pod_json=$(kubectl get pod "$pod_name" -o json)
+    init_container_status="$(echo "$pod_json" | jq ".status.initContainerStatuses[0].state.terminated.exitCode")"
+    if [[ -n "$init_container_status" && "$init_container_status" != "0" ]]; then
+      echo "Warning: init container failed with exit code $init_container_status, this usually indicates plugin misconfiguration or infrastructure failure"
+      status="$init_container_status"
+    else
+      status="$(echo "$pod_json" | jq ".status.containerStatuses[0].state.terminated.exitCode")"
+    fi
+    set -e
+    if [[ -n "$status" ]]; then
+      break
+    else
+      sleep "$job_status_retry_interval_sec"
+      if [[ $timeout -gt 0 ]]; then
+        (( counter -= job_status_retry_interval_sec ))
+      fi
+    fi
+  done


Awesome work checking init containers too 🙌

hooks/command

iffyio · 2020-10-12T19:23:19Z

hooks/command

+  status="0"
+else
+  while true
+  do


Let's also print a message that says we're checking init containers status

iffyio · 2020-10-12T19:29:16Z

hooks/command

-  status=0
+  status="0"
+else
+  while true


This looks like we'll keep looping if we keep failing to get the init container status? Let's break out of the loop after a hardcoded number of attempts or timeout instead?

iffyio · 2020-10-12T19:41:18Z

.gitignore

@@ -0,0 +1 @@
+.idea/


Let's not include a gitignore in the PR since its possible to have git exclude these files locally https://stackoverflow.com/questions/1753070/how-do-i-configure-git-to-ignore-some-files-locally

Co-authored-by: Ifeanyi Ubah <[email protected]>

iffyio · 2021-04-07T06:45:37Z

This landed in a8edf37 Thanks a lot @artem-zinnatullin I'll look to create a release soon

artem-zinnatullin · 2021-04-07T06:52:08Z

Glad to see that, sorry I left it unfinished, got personal stuff in the way

…

On Wed, Apr 7, 2021 at 9:45 AM Ifeanyi Ubah ***@***.***> wrote: Closed #14 <#14> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHMDXAITLUSS2E37RLZTXLTHP5R5ANCNFSM4PGAEHFA> .

artem-zinnatullin added 30 commits May 21, 2020 02:53

Fix logs duplication

64d0054

Run kubectl logs --follow in a loop, it actually fails sometimes…

df92276

Redirect err to /dev/null, add error example to comment

2142fe6

improve comments

bb3fdf9

Add timeout to the check

e628984

do not tail

7cf0ad5

Add check for log snapshot content

e86fe72

Add debug

1a8f2e6

fix contents check

a7e7b62

print debug

391faaa

print more debug

27af568

print more debug

b55eaf8

Remove timeout

2f581ba

add set -x, dont use file

df7444b

set +e

54583a9

Remove -xg

81e99da

Add race fix

75cbe9c

Remove empty print, move out local var

97ae246

Increase log loop interval to avoid k8s API endpoint overload, allow …

724410e

…user value

Retry all kubectl commands

c4735dc

Add retries to pre-exit

5f03990

Add ttlSecondsAfterFinished of 1 day, same as cleanup

0ba78db

rely only on ttlSecondsAfterFinished

d63009d

Reduce TTL to ease load on apiserver

7328036

print env vars in pre-exit, catch exit code

5f9f582

also print cmd args in pre-exit

8fe3ce5

Skip job cleanup if command successful, saves 3 kubectl calls

d6b7b52

Reduce ttl to 600

df4880b

Add job ttl property with old value as default

2104237

Parse ttl as int

3bc537a

artem-zinnatullin added 13 commits September 15, 2020 14:33

WIP propagate exit code

259cd3b

Wrap in loop

6519220

Inline log_loop_snapshot_limit_bytes

32b9e28

Use one convention for ENV var names

e8e9cfc

Add docs for new env variables

8603721

Return old jobs cleanup back

dffecae

Check job_apply_exit_code

5daa8a1

Dont sleep if jobstatus has been resolved

941e86b

Cleanup finished jobs by default

2085c88

Document BUILDKITE_PLUGIN_K8S_JOB_TTL_SECONDS_AFTER_FINISHED

1cef288

Move some of the properties to YAML

f89209b

Add jobs-cleanup-via-plugin and job-cleanup-after-finished-via-plugin…

071bb47

… to YAML

Unify retry interval and timeout env var names

b5a6969

artem-zinnatullin commented Oct 7, 2020

View reviewed changes

Update README with new env var names

3d1589a

keith reviewed Oct 9, 2020

View reviewed changes

artem-zinnatullin added 4 commits October 9, 2020 16:10

Use init container exit code if it failed

f46dac5

Add warning about init container failure

d948c35

Print bootstrap container logs

cb2f53e

fix func name

09dd224

iffyio reviewed Oct 12, 2020

View reviewed changes

Apply suggestions from code review

f7a867d

Co-authored-by: Ifeanyi Ubah <[email protected]>

iffyio closed this Apr 7, 2021

artem-zinnatullin mentioned this pull request Feb 9, 2023

No way to disable git mirror #55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: reliability, retries, reduced k8s apiserver load #14

Refactor: reliability, retries, reduced k8s apiserver load #14

artem-zinnatullin commented Jul 23, 2020 •

edited

Loading

coderanger commented Sep 8, 2020 •

edited

Loading

artem-zinnatullin Oct 7, 2020

iffyio Oct 12, 2020

artem-zinnatullin Oct 7, 2020

iffyio Oct 12, 2020

artem-zinnatullin commented Oct 7, 2020

iffyio commented Oct 8, 2020

keith Oct 9, 2020

iffyio Oct 12, 2020

artem-zinnatullin Oct 13, 2020

iffyio left a comment

iffyio Oct 12, 2020

iffyio Oct 12, 2020

iffyio Oct 12, 2020

iffyio Oct 12, 2020

iffyio Oct 12, 2020

iffyio commented Apr 7, 2021

artem-zinnatullin commented Apr 7, 2021 via email

		@@ -0,0 +1 @@
		.idea/

Refactor: reliability, retries, reduced k8s apiserver load #14

Refactor: reliability, retries, reduced k8s apiserver load #14

Conversation

artem-zinnatullin commented Jul 23, 2020 • edited Loading

coderanger commented Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin commented Oct 7, 2020

iffyio commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio commented Apr 7, 2021

artem-zinnatullin commented Apr 7, 2021 via email

artem-zinnatullin commented Jul 23, 2020 •

edited

Loading

coderanger commented Sep 8, 2020 •

edited

Loading