[Federation] Create a script that dumps Federation pod logs after e2e test failures #43028

perotinus · 2017-03-13T22:30:09Z

This replaces the code that dumped the Federation pod logs to the console after each failed test.

NONE

k8s-ci-robot · 2017-03-13T22:30:09Z

Hi @perotinus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-03-13T22:30:15Z

This change is

madhusudancs · 2017-03-14T00:10:00Z

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

federation/cluster/log-dump.sh, line 24 at r1 (raw file):

set -o pipefail

readonly report_dir="${1:-_artifacts}"

By convention, we use all caps for global names. But I see why you are using lower cases here (log-dump.sh does this too). But can we use the convention in this script?

Same comment for variables below.

federation/cluster/log-dump.sh, line 25 at r1 (raw file):

readonly report_dir="${1:-_artifacts}"
readonly federation_namespace="${2:-federation-system}"

source federation/cluster/common.sh and use FEDERATION_NAMESPACE so that if we change it there everyone using it gets the new name. Also, I don't think you need to get this as a positional argument.

federation/cluster/log-dump.sh, line 28 at r1 (raw file):

readonly kube_system_namespace="${3:-kube-system}"

readonly apiserver_pod_containers="apiserver etcd"

Use an array. It is easier to read?

federation/cluster/log-dump.sh, line 29 at r1 (raw file):

readonly apiserver_pod_containers="apiserver etcd"
readonly dns_pod_containers="kubedns dnsmasq sidecar"

Same comment as above.

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

# Get the pods by their labels.
readonly federation_pod_names="$(kubectl get pods -l 'app=federated-cluster' --namespace=${federation_namespace} -o name | tr '\n' ' ')"

Is the translation really necessary?

federation/cluster/log-dump.sh, line 35 at r1 (raw file):

# Get the pods by their labels.
readonly federation_pod_names="$(kubectl get pods -l 'app=federated-cluster' --namespace=${federation_namespace} -o name | tr '\n' ' ')"
readonly dns_pod_names="$(kubectl get pods -l 'k8s-app=kube-dns' --namespace=${kube_system_namespace} -o name | tr '\n' ' ')"

Same comment.

federation/cluster/log-dump.sh, line 41 at r1 (raw file):

  # The API server pod has two containers
  if [[ "${pod_name}" == *apiserver* ]]; then
    for container in ${apiserver_pod_containers}; do

I think it is a good idea to extract this into a function and define $apiserver_pod_containers there.

federation/cluster/log-dump.sh, line 43 at r1 (raw file):

    for container in ${apiserver_pod_containers}; do
      kubectl logs "${pod_name}" -c "${container}" --namespace="${federation_namespace}" \
          >"${report_dir}/${pod_name#pod/}-${container}.log"

I would prefer this name to be ${report_dir}/federation/${pod_name#pod/}-${container}.log That way all federation logs are under a single heading on the logs page.

Example here: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-federation/2233/nodelog?junit=junit_runner.xml&wrap=on.

See how the logs are grouped.

federation/cluster/log-dump.sh, line 49 at r1 (raw file):

  kubectl logs "${pod_name}" --namespace="${federation_namespace}" \
      >"${report_dir}/${pod_name#pod/}.log"

Same comment as above.

federation/cluster/log-dump.sh, line 52 at r1 (raw file):

done

# Dump the DNS pod logs.

This has to be done per-cluster. It is unfortunate that we were only logging kube-dns in the host cluster in our framework.

But this is a good starting point. Could you add a TODO here saying this has to be logged per cluster? When you fix this in a future PR, you can fix this in the log-dump.sh script. That script logs per cluster anyway.

Comments from Reviewable

perotinus · 2017-03-14T01:13:45Z

Review status: 1 of 2 files reviewed at latest revision, 10 unresolved discussions.

federation/cluster/log-dump.sh, line 24 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

By convention, we use all caps for global names. But I see why you are using lower cases here (log-dump.sh does this too). But can we use the convention in this script?

Same comment for variables below.

Done.

federation/cluster/log-dump.sh, line 25 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

source federation/cluster/common.sh and use FEDERATION_NAMESPACE so that if we change it there everyone using it gets the new name. Also, I don't think you need to get this as a positional argument.

Done, I think. I believe that I had done this but had not repushed, so please let me know if there's something more to do here.

federation/cluster/log-dump.sh, line 28 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Use an array. It is easier to read?

Done.

federation/cluster/log-dump.sh, line 29 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Same comment as above.

Done.

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Is the translation really necessary?

I think so. Otherwise, the output has newlines, and it doesn't work with Bash for...in iteration.

federation/cluster/log-dump.sh, line 35 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Same comment.

See above. I believe this is necessary, or at least something to this effect.

federation/cluster/log-dump.sh, line 41 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I think it is a good idea to extract this into a function and define $apiserver_pod_containers there.

Done.

federation/cluster/log-dump.sh, line 43 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I would prefer this name to be ${report_dir}/federation/${pod_name#pod/}-${container}.log That way all federation logs are under a single heading on the logs page.

Example here: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-federation/2233/nodelog?junit=junit_runner.xml&wrap=on.

See how the logs are grouped.

Ah, OK. I was wondering if there was a more-standard way to group these. Thanks!

federation/cluster/log-dump.sh, line 49 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Same comment as above.

Done.

federation/cluster/log-dump.sh, line 52 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

This has to be done per-cluster. It is unfortunate that we were only logging kube-dns in the host cluster in our framework.

But this is a good starting point. Could you add a TODO here saying this has to be logged per cluster? When you fix this in a future PR, you can fix this in the log-dump.sh script. That script logs per cluster anyway.

Done. Do you mean fix in the non-federation log-dump.sh script? Does it not already dump the DNS pods? I suppose we could fix it here by using the E2E_ZONES flag (or perhaps another one if there's a more appropriate one).

Comments from Reviewable

madhusudancs · 2017-03-14T05:11:01Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

federation/cluster/log-dump.sh, line 25 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Done, I think. I believe that I had done this but had not repushed, so please let me know if there's something more to do here.

Looks like it is done now.

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

I think so. Otherwise, the output has newlines, and it doesn't work with Bash for...in iteration.

Are you sure about this? I think BASH only cares about whitespaces, not necessarily spaces.

for $e in $(ls -1); do
    echo $e
done

works for me.

If you are converting it into an array as you are doing now, it matters even little.

federation/cluster/log-dump.sh, line 52 at r1 (raw file):

Do you mean fix in the non-federation log-dump.sh script?

Yes.

Does it not already dump the DNS pods?

Not as far as I know. Here is an example: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-federation/2238/nodelog?junit=junit_runner.xml&wrap=on

I suppose we could fix it here by using the E2E_ZONES flag (or perhaps another one if there's a more appropriate one).

Sure, that works too.

federation/cluster/log-dump.sh, line 26 at r2 (raw file):

readonly REPORT_DIR="${1:-_artifacts}"

KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../..

We usually put KUBE_ROOT at the top. The very first thing after all the set -os

federation/cluster/log-dump.sh, line 27 at r2 (raw file):

KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../..
source "${KUBE_ROOT}/federation/cluster/common.sh"

It will be great if you add a comment listing for what you are sourcing this. Bash includes are too hard to track. Lack of good tooling makes the problem worse.

federation/cluster/log-dump.sh, line 31 at r2 (raw file):

# Dumps logs for all pods in a federation.
function dump_federation_pod_logs() {
  readonly federation_pod_names=($(kubectl get pods -l 'app=federated-cluster' --namespace=${FEDERATION_NAMESPACE} -o name | tr '\n' ' '))

local -r federation_pod_names is probably a better fit here.

federation/cluster/log-dump.sh, line 48 at r2 (raw file):

# - the name of the API server pod, with a pod/ prefix.
function dump_apiserver_pod_logs() {
  readonly apiserver_pod_containers=(apiserver etcd)

local -r may be?

Also, I was expecting the individual items in the array to be quoted. But this is fine since they are single word elements.

federation/cluster/log-dump.sh, line 75 at r2 (raw file):

echo "Dumping Federation and DNS pod logs to ${REPORT_DIR}"

OUTPUT_DIR="${REPORT_DIR}/federation"

We usually put all the global initialization at the top. Directory creation can happen here though.

Comments from Reviewable

madhusudancs · 2017-03-25T07:45:52Z

@k8s-bot ok to test

madhusudancs · 2017-03-25T07:46:30Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

madhusudancs · 2017-03-25T17:18:10Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

madhusudancs · 2017-04-05T22:30:26Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

perotinus · 2017-04-06T19:03:12Z

Thanks! PTAL when you get a chance.

I fixed the jenkins verification issue, but I'm not quite sure what's causing the other issue. It seems odd that it can't find the namespace, since I didn't actually modify anything that should affect the test setup. Can you ping the bot again?

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions.

federation/cluster/log-dump.sh, line 25 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Looks like it is done now.

Great!

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Are you sure about this? I think BASH only cares about whitespaces, not necessarily spaces.
for $e in $(ls -1); do
    echo $e
done
works for me.

If you are converting it into an array as you are doing now, it matters even little.

You're correct. Perhaps there are more issues with separators when you don't use an array? Bash is always a little bit like voodoo to me.

federation/cluster/log-dump.sh, line 26 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

We usually put KUBE_ROOT at the top. The very first thing after all the set -os

Done.

federation/cluster/log-dump.sh, line 27 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

It will be great if you add a comment listing for what you are sourcing this. Bash includes are too hard to track. Lack of good tooling makes the problem worse.

Done.

federation/cluster/log-dump.sh, line 31 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

local -r federation_pod_names is probably a better fit here.

Done.

federation/cluster/log-dump.sh, line 48 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

local -r may be?

Also, I was expecting the individual items in the array to be quoted. But this is fine since they are single word elements.

Done.

federation/cluster/log-dump.sh, line 75 at r2 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

We usually put all the global initialization at the top. Directory creation can happen here though.

Done.

Comments from Reviewable

madhusudancs · 2017-04-07T02:32:01Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

madhusudancs · 2017-04-07T06:22:18Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

perotinus · 2017-04-07T16:34:51Z

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

You're correct. Perhaps there are more issues with separators when you don't use an array? Bash is always a little bit like voodoo to me.

Resolving for Reviewable.

federation/cluster/log-dump.sh, line 31 at r2 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Done.

Resolving for Reviewable.

Comments from Reviewable

perotinus · 2017-04-07T16:36:02Z

@madhusudancs Can you LGTM this? Looks like the federation e2e tests finally passed. I'm not sure what the issue was before.

madhusudancs · 2017-04-07T17:34:12Z

Can you LGTM this?

Yeah, I was waiting for the tests to pass.

Looks like the federation e2e tests finally passed. I'm not sure what the issue was before.

Yeah, it had nothing to do with this PR. The test (read: infra) was broken.

madhusudancs · 2017-04-07T17:35:22Z

/lgtm
/approve

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

federation/cluster/log-dump.sh, line 34 at r1 (raw file):

Previously, perotinus (Jonathan MacMillan) wrote…

Resolving for Reviewable.

To me too :)

Comments from Reviewable

madhusudancs · 2017-04-07T21:28:29Z

@k8s-bot bazel test this

madhusudancs · 2017-04-07T21:40:22Z

@perotinus bazel failure looks like a legitimate failure. Can you please take a look?

…eration e2e tests

perotinus · 2017-04-07T21:57:49Z

@madhusudancs Thanks! Should be fixed now. Can you re-LGTM?

madhusudancs · 2017-04-07T22:10:25Z

/lgtm

k8s-github-robot · 2017-04-07T22:15:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, perotinus

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [madhusudancs]
~~test/e2e_federation/OWNERS~~ [madhusudancs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

madhusudancs · 2017-04-07T22:45:36Z

@k8s-bot gce e2e test this

madhusudancs · 2017-04-07T23:31:48Z

@k8s-bot gci gce e2e test this

k8s-github-robot · 2017-04-08T00:44:23Z

Automatic merge from submit-queue (batch tested with PRs 41775, 39678, 42629, 42524, 43028)

nikhiljindal · 2017-04-21T05:45:27Z

federation/cluster/log-dump.sh

+# Dumps logs for all pods in a federation.
+function dump_federation_pod_logs() {
+  local -r federation_pod_names=($(kubectl get pods -l 'app=federated-cluster' --namespace=${FEDERATION_NAMESPACE} -o name))
+  for pod_name in ${federation_pod_names[@]}; do


I see the following in logs:

./federation/cluster/log-dump.sh: line 34: federation_pod_names[@]: unbound variable

Example run: http://prow.k8s.io/log?pod=pull-kubernetes-federation-e2e-gce-1583

@perotinus could you please take a look?

@madhusudancs @nikhiljindal Fixed by #44881.

@perotinus thanks!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 13, 2017

k8s-github-robot assigned marun Mar 13, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 13, 2017

perotinus changed the title ~~[Federation] Create a script that dumps Federation pod logs after Federation e2e tests~~ [Federation] Create a script that dumps Federation pod logs after e2e test failures Mar 13, 2017

k8s-github-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 13, 2017

madhusudancs self-assigned this Mar 14, 2017

perotinus force-pushed the e2etestlogs branch 2 times, most recently from 327f31f to d610a22 Compare March 14, 2017 01:09

perotinus force-pushed the e2etestlogs branch from d610a22 to 6acda0d Compare April 6, 2017 19:02

perotinus force-pushed the e2etestlogs branch from 6acda0d to b3155af Compare April 6, 2017 19:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2017

[Federation] Create a script that dumps Federation pod logs after Fed…

12eb965

…eration e2e tests

perotinus force-pushed the e2etestlogs branch from b3155af to 12eb965 Compare April 7, 2017 21:51

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017

k8s-github-robot merged commit e7dfdd5 into kubernetes:master Apr 8, 2017

madhusudancs mentioned this pull request Apr 14, 2017

Gubernator logs for federation control plane components kubernetes/test-infra#1069

Closed

nikhiljindal reviewed Apr 21, 2017

View reviewed changes

perotinus deleted the e2etestlogs branch May 10, 2017 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Federation] Create a script that dumps Federation pod logs after e2e test failures #43028

[Federation] Create a script that dumps Federation pod logs after e2e test failures #43028

perotinus commented Mar 13, 2017 •

edited

k8s-ci-robot commented Mar 13, 2017

k8s-reviewable commented Mar 13, 2017

madhusudancs commented Mar 14, 2017

perotinus commented Mar 14, 2017

madhusudancs commented Mar 14, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Apr 5, 2017

perotinus commented Apr 6, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

k8s-github-robot commented Apr 8, 2017

nikhiljindal Apr 21, 2017

madhusudancs Apr 22, 2017

perotinus Apr 24, 2017

madhusudancs Apr 25, 2017

[Federation] Create a script that dumps Federation pod logs after e2e test failures #43028

[Federation] Create a script that dumps Federation pod logs after e2e test failures #43028

Conversation

perotinus commented Mar 13, 2017 • edited

k8s-ci-robot commented Mar 13, 2017

k8s-reviewable commented Mar 13, 2017

madhusudancs commented Mar 14, 2017

perotinus commented Mar 14, 2017

madhusudancs commented Mar 14, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Mar 25, 2017

madhusudancs commented Apr 5, 2017

perotinus commented Apr 6, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

perotinus commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

madhusudancs commented Apr 7, 2017

k8s-github-robot commented Apr 8, 2017

nikhiljindal Apr 21, 2017

Choose a reason for hiding this comment

madhusudancs Apr 22, 2017

Choose a reason for hiding this comment

perotinus Apr 24, 2017

Choose a reason for hiding this comment

madhusudancs Apr 25, 2017

Choose a reason for hiding this comment

perotinus commented Mar 13, 2017 •

edited