feat(executor): Minimize the number of Kubernetes API requests made by executors #4954

alexec · 2021-01-27T04:54:56Z

This PR minimizes the number of Kubernetes API requests made by the executor.

Today, start-up, the executors make 2 Kubernetes API requests immediately, get pod and watch pod. Tomorrow, a maximum of 1 request is now only made under two circumstances:

Using the k8sapi executor. Obviously.
When PNS has been unable to determine the container IDs due to a short-running main container.

I've added the logging of K8S requests to help diagnose regression in the future.

Executor
- Remove get pod for getting annotations on start-up (read from mounted annotation path)
- Remove watch pod on start-up completely
docker use docker ps to determine container IDs instead of API request
k8sapi business as usual
kubelet use localhost/pods API request to determine container IDs
pns introduce ARGO_CONTAINER_NAME to identify the container name for processes, works on long-running containers.

Testing Notes

System	Executor	Outcome
GCP	docker	PASS - fixed bug in killing sidecars
GCP	k8sapi	PASS
GCP	kubelet	PASS - reduced status hard loop
GCP	pns	PASS

workflow/executor/pns/pns.go

workflow/executor/k8sapi/client.go

workflow/executor/docker/docker.go

util/logs/workflow-logger.go

workflow/executor/k8sapi/k8sapi.go

workflow/executor/kubelet/kubelet.go

workflow/executor/pns/pns.go

test/e2e/smoke_test.go

Signed-off-by: Alex Collins <[email protected]>

.github/workflows/ci-build.yaml

test/e2e/fixtures/e2e_suite.go

test/e2e/functional_test.go

workflow/controller/workflowpod.go

workflow/executor/kubelet/client.go

workflow/util/util.go

workflow/executor/executor.go

Signed-off-by: Alex Collins <[email protected]>

alexec · 2021-02-02T01:10:10Z

Before:

hello-world-xlp8t wait time="2021-02-02T01:08:22.850Z" level=info msg="Watch pods 200"
hello-world-xlp8t wait time="2021-02-02T01:08:37.590Z" level=info msg="Get pods 200"
hello-world-xlp8t wait time="2021-02-02T01:08:37.598Z" level=info msg="Get pods 200"
hello-world-xlp8t wait time="2021-02-02T01:08:37.614Z" level=info msg="List log 200"

Signed-off-by: Alex Collins <[email protected]>

alexec · 2021-02-05T16:20:56Z

Good. The builds are failing as they should.

Signed-off-by: Alex Collins <[email protected]>

jessesuen · 2021-02-12T09:42:29Z

workflow/controller/workflowpod.go

 	return *ctr
 }

 func (woc *wfOperationCtx) newWaitContainer(tmpl *wfv1.Template) (*apiv1.Container, error) {
 	ctr := woc.newExecContainer(common.WaitContainerName, tmpl)
-	ctr.Command = []string{"argoexec", "wait"}
+	ctr.Command = []string{"argoexec", "wait", "--loglevel", getExecutorLogLevel()}


jessesuen · 2021-02-12T09:46:25Z

workflow/executor/pns/pns.go

+func (p *PNSExecutor) Wait(ctx context.Context, containerNames, sidecarNames []string) error {
+
+	allContainerNames := append(containerNames, sidecarNames...)
+	go p.pollRootProcesses(ctx, allContainerNames)


I believe this happens sufficiently early along in the process to remove the need for WaitInit().

jessesuen · 2021-02-12T09:51:32Z

workflow/controller/operator.go

-				continue
+				log.Infof("Ignoring %d exit code of container '%s'", t.ExitCode, ctr.Name)
+			} else {
+				return wfv1.NodeFailed, msg


this code is much cleaner now

jessesuen · 2021-02-12T10:00:07Z

workflow/executor/docker/docker.go

+				// https://github.com/kubernetes/kubernetes/blob/ca6bdba014f0a98efe0e0dd4e15f57d1c121d6c9/pkg/kubelet/dockertools/labels.go#L37
+				"--filter=label=io.kubernetes.pod.namespace="+d.namespace,
+				"--filter=label=io.kubernetes.pod.name="+d.podName,


jessesuen · 2021-02-12T10:13:32Z

workflow/executor/pns/pns.go

-func (p *PNSExecutor) pollRootProcesses(timeout time.Duration) {
-	log.Warnf("Polling root processes (%v)", timeout)
-	deadline := time.Now().Add(timeout)
+func (p *PNSExecutor) pollRootProcesses(ctx context.Context, containerNames []string) {


The previous implementation limited our hard tight-looped polling to 1 minute, because it was quite aggressive. How do we limit this in the new implementation?

Good point. I’ve removed this by accident and need to add it back.

this is fixed now, I find it is easier to do timeouts using context.WithTimeout because you usually have a context available

Signed-off-by: Alex Collins <[email protected]>

alexec · 2021-02-12T17:18:33Z

@jessesuen I've addressed your comment.

Signed-off-by: Alex Collins <[email protected]>

alexec mentioned this pull request Jan 27, 2021

feat: Allow to specify resource version for executor #4941

Closed

1 task

alexec commented Jan 27, 2021

View reviewed changes

workflow/executor/pns/pns.go Show resolved Hide resolved

terrytangyuan reviewed Jan 27, 2021

View reviewed changes

workflow/executor/k8sapi/client.go Show resolved Hide resolved

alexec changed the title ~~feat(executor): Remove need for watch~~ feat(executor): Reduce usage of watch when waiting for container start Jan 27, 2021

alexec commented Jan 28, 2021

View reviewed changes

workflow/executor/docker/docker.go Outdated Show resolved Hide resolved

alexec commented Jan 28, 2021

View reviewed changes

util/logs/workflow-logger.go Outdated Show resolved Hide resolved

terrytangyuan reviewed Jan 28, 2021

View reviewed changes

workflow/executor/k8sapi/k8sapi.go Outdated Show resolved Hide resolved

workflow/executor/kubelet/kubelet.go Outdated Show resolved Hide resolved

workflow/executor/pns/pns.go Outdated Show resolved Hide resolved

alexec commented Jan 28, 2021

View reviewed changes

test/e2e/smoke_test.go Outdated Show resolved Hide resolved

alexec mentioned this pull request Jan 28, 2021

Experiencing issues with the PNS executor #1256

Closed

alexec force-pushed the now branch 3 times, most recently from fa9ede2 to 3bafd3e Compare January 31, 2021 17:26

feat(executor): Remove need for watch

f84fb1a

Signed-off-by: Alex Collins <[email protected]>

alexec force-pushed the now branch from 3bafd3e to f84fb1a Compare January 31, 2021 17:26

alexec changed the title ~~feat(executor): Reduce usage of watch when waiting for container start~~ feat(executor): Reduce the number of Kubernetes API requests make by executors Jan 31, 2021

alexec requested review from jessesuen and dtaniwaki January 31, 2021 17:37

alexec changed the title ~~feat(executor): Reduce the number of Kubernetes API requests make by executors~~ feat(executor): Minimize the number of Kubernetes API requests made by executors Jan 31, 2021

alexec commented Jan 31, 2021

View reviewed changes

alexec added 8 commits January 31, 2021 09:56

feat(executor): Remove need for watch

9a6a1de

Signed-off-by: Alex Collins <[email protected]>

test: More tests

64862b4

Signed-off-by: Alex Collins <[email protected]>

test: More tests

c04090d

Signed-off-by: Alex Collins <[email protected]>

Merge branch 'more-tests' into now

e614705

Signed-off-by: Alex Collins <[email protected]>

test: More tests

146a599

Signed-off-by: Alex Collins <[email protected]>

Merge branch 'more-tests' into now

964db59

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

b7785e0

Signed-off-by: Alex Collins <[email protected]>

Merge branch 'master' into now

4e8736f

Signed-off-by: Alex Collins <[email protected]>

alexec linked an issue Feb 2, 2021 that may be closed by this pull request

Experiencing issues with the PNS executor #1256

Closed

feat(executor): Remove need for watch

e1ea2e9

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

5cb9570

Signed-off-by: Alex Collins <[email protected]>

alexec added 15 commits February 5, 2021 13:55

Merge branch 'master' into now

c214f8e

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

1da3726

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

ec5246e

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

6a56fe2

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

4b62841

Signed-off-by: Alex Collins <[email protected]>

Merge branch 'master' into now

ea6f832

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

a5ae663

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

42d86ac

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

fefed4c

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

f1ac596

Signed-off-by: Alex Collins <[email protected]>

ignore test

560347d

Signed-off-by: Alex Collins <[email protected]>

ignore test

394a721

Signed-off-by: Alex Collins <[email protected]>

Merge branch 'master' into now

94a778b

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

2c87588

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

ca47612

Signed-off-by: Alex Collins <[email protected]>

alexec force-pushed the now branch from d857e63 to ca47612 Compare February 10, 2021 22:25

Merge branch 'master' into now

dbb20c4

Signed-off-by: Alex Collins <[email protected]>

jessesuen reviewed Feb 12, 2021

View reviewed changes

alexec added 2 commits February 12, 2021 09:11

Merge branch 'master' into now

b6edcc8

Signed-off-by: Alex Collins <[email protected]>

feat(executor): Remove need for watch

a98f414

Signed-off-by: Alex Collins <[email protected]>

jessesuen approved these changes Feb 12, 2021

View reviewed changes

Merge branch 'master' into now

48d86c3

Signed-off-by: Alex Collins <[email protected]>

alexec merged commit 2ff4db1 into argoproj:master Feb 12, 2021

alexec deleted the now branch February 12, 2021 20:03

simster7 mentioned this pull request Feb 16, 2021

v2.12.9 cherry-pick #5119

Closed

33 tasks

simster7 mentioned this pull request Feb 23, 2021

Cherry-pick for v2.12.10 #5179

Closed

34 tasks

simster7 mentioned this pull request Mar 8, 2021

v2.12.10 cherry-pick #5326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(executor): Minimize the number of Kubernetes API requests made by executors #4954

feat(executor): Minimize the number of Kubernetes API requests made by executors #4954

alexec commented Jan 27, 2021 •

edited

Loading

alexec commented Feb 2, 2021

alexec commented Feb 5, 2021

jessesuen Feb 12, 2021

jessesuen Feb 12, 2021

jessesuen Feb 12, 2021

jessesuen Feb 12, 2021

jessesuen Feb 12, 2021

alexec Feb 12, 2021

alexec Feb 12, 2021 •

edited

Loading

alexec commented Feb 12, 2021

feat(executor): Minimize the number of Kubernetes API requests made by executors #4954

feat(executor): Minimize the number of Kubernetes API requests made by executors #4954

Conversation

alexec commented Jan 27, 2021 • edited Loading

Testing Notes

alexec commented Feb 2, 2021

alexec commented Feb 5, 2021

jessesuen Feb 12, 2021

Choose a reason for hiding this comment

jessesuen Feb 12, 2021

Choose a reason for hiding this comment

jessesuen Feb 12, 2021

Choose a reason for hiding this comment

jessesuen Feb 12, 2021

Choose a reason for hiding this comment

jessesuen Feb 12, 2021

Choose a reason for hiding this comment

alexec Feb 12, 2021

Choose a reason for hiding this comment

alexec Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

alexec commented Feb 12, 2021

alexec commented Jan 27, 2021 •

edited

Loading

alexec Feb 12, 2021 •

edited

Loading