Skip to content

collect logs when ci-entrypoint.sh is used to run e2e tests#2043

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
marosset:windows-logs-ci-entrypoint
Feb 7, 2022
Merged

collect logs when ci-entrypoint.sh is used to run e2e tests#2043
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
marosset:windows-logs-ci-entrypoint

Conversation

@marosset
Copy link
Copy Markdown
Contributor

@marosset marosset commented Feb 2, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:
Gets pod logs for Windows nodes when cluster is provisioned with ci-entrypoint.sh

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1864

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Adding support for ci-entrypoint.sh to collect logs for Windows nodes

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/azure Issues or PRs related to azure provider labels Feb 2, 2022
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 2, 2022
@marosset marosset force-pushed the windows-logs-ci-entrypoint branch 20 times, most recently from 2094ead to 714bf14 Compare February 3, 2022 22:46
@marosset marosset changed the title WIP: Windows logs ci entrypoint collect logs when ci-entrypoint.sh is used to run e2e tests Feb 3, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2022
@marosset marosset force-pushed the windows-logs-ci-entrypoint branch from 714bf14 to 7748acd Compare February 3, 2022 22:57
@marosset
Copy link
Copy Markdown
Contributor Author

marosset commented Feb 3, 2022

/assign @jsturtevant @CecileRobertMichon

Comment thread hack/log/log-dump.sh
echo "Deploying log-dump-daemonset-windows"
"${KUBECTL}" apply -f "${REPO_ROOT}/hack/log/log-dump-daemonset-windows.yaml"
echo "Waiting for log-dump-daemonset-windows"
"${KUBECTL}" wait pod -l app=log-dump-node-windows --for=condition=Ready --timeout=5m
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone know if this would cause issues if there aren't any windows nodes / the DS doesn't schedule any pods?
I'm assuming not but will try and test this out...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine since total number of pods will be 0. Did you try it out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this out and the kubectl command returned with exitcode 1 (and error: no matches resources found).

when I ran kubectl get pod -l app=log-dump-node-windows -ojsonpath='{.items[*].metadata.name}' like on the next line that returns success and no output.

Should we let the wait command fail and exit out of the function or should I add || true there and just the for loop no-op?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check to see if TEST_WINDOWS envvar was set and only call dump_workload_cluster_logs_windows if it is

@marosset
Copy link
Copy Markdown
Contributor Author

marosset commented Feb 4, 2022

/retest

Copy link
Copy Markdown
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you had fun with slashes between windows and linux 😄

Comment thread hack/log/log-dump.sh Outdated
Comment thread hack/log/log-dump.sh
echo "Deploying log-dump-daemonset-windows"
"${KUBECTL}" apply -f "${REPO_ROOT}/hack/log/log-dump-daemonset-windows.yaml"
echo "Waiting for log-dump-daemonset-windows"
"${KUBECTL}" wait pod -l app=log-dump-node-windows --for=condition=Ready --timeout=5m
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine since total number of pods will be 0. Did you try it out?

@marosset
Copy link
Copy Markdown
Contributor Author

marosset commented Feb 4, 2022

Looks like you had fun with slashes between windows and linux 😄

I also had fun with the carriage return windows adds in addition to the newline character (which I forgot about)

@marosset marosset force-pushed the windows-logs-ci-entrypoint branch from 7748acd to 15704ac Compare February 4, 2022 23:25
@jsturtevant
Copy link
Copy Markdown
Contributor

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2022
@mboersma
Copy link
Copy Markdown
Contributor

mboersma commented Feb 7, 2022

This is great and looks good to me.

I noticed that the default node dump log is 0 bytes. Is that expected?

@marosset
Copy link
Copy Markdown
Contributor Author

marosset commented Feb 7, 2022

This is great and looks good to me.

I noticed that the default node dump log is 0 bytes. Is that expected?

Yes, that container runs the pause image which doesn't log anything to stdout/stderr.
This script only collects logs for linux containers running in kube-system namespace but I would like to logs for all containers for Windows.

Copy link
Copy Markdown
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit a16ce1c into kubernetes-sigs:main Feb 7, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Feb 7, 2022
@marosset marosset deleted the windows-logs-ci-entrypoint branch February 8, 2022 00:18
@CecileRobertMichon CecileRobertMichon modified the milestones: v1.1, v1.2 Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for ci-entrypoint to collect Windows logs

5 participants