Convert dumpDiag to use `kind export logs` #591

astoycos · 2023-03-21T15:52:40Z

Today in KTF we manually step through all the pods/objects in the cluster to get debug information. KIND now supports simply running kind export logs which does a better job of giving us a cluster debug overview. Switch the DumpDiagnostics function to use this command for cluster log dumping instead.

Intentionally leave the logic which allows each addon to write their own specific cleanup functionality.

CLAassistant · 2023-03-21T15:52:46Z

All committers have signed the CLA.

astoycos · 2023-03-21T15:53:09Z

testing before moving out of draft state

pkg/clusters/diagnostics.go

codecov-commenter · 2023-03-21T16:55:31Z

Codecov Report

Patch coverage has no change and project coverage change: -47.94 ⚠️

Comparison is base (5e019ce) 57.89% compared to head (ef75273) 9.95%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #591       +/-   ##
==========================================
- Coverage   57.89%   9.95%   -47.94%     
==========================================
  Files          43      43               
  Lines        3451    3455        +4     
==========================================
- Hits         1998     344     -1654     
- Misses       1189    3056     +1867     
+ Partials      264      55      -209

Flag	Coverage Δ
integration-test	`9.95% <0.00%> (-47.94%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/clusters/diagnostics.go	`0.00% <0.00%> (-41.99%)`	⬇️
pkg/clusters/types/kind/cluster.go	`44.87% <0.00%> (-30.13%)`	⬇️
pkg/clusters/types/kind/utils.go	`24.75% <0.00%> (-21.92%)`	⬇️

... and 35 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

astoycos · 2023-03-21T17:09:58Z

Example dump from a KIND cluster can be seen in artifacts here: https://github.com/redhat-et/bpfd/actions/runs/4481401852

czeslavo · 2023-03-22T11:46:14Z

From the functional perspective that looks good to me now. 🎉

The integration tests seem to be failing because of missing access to secrets, we need to figure out a way around it to make the PR green.

In the meantime, we will also need a changelog entry for the changes. Bonus points from me for an integration test for Kind implementation that would ensure the dump functionality indeed works as expected if you have time for it. :)

czeslavo · 2023-03-22T13:16:18Z

Rebased onto main after making the workflow skip tests that require secrets: #597

astoycos · 2023-03-22T13:54:28Z

Thanks @czeslavo! I tested against a KIND cluster as mentioned above, but don't have a GKE cluster to test against :(

czeslavo · 2023-03-22T14:04:16Z

Thanks @czeslavo! I tested against a KIND cluster as mentioned above, but don't have a GKE cluster to test against :(

Understood, let's skip GKE for now. What I meant was adding an automated integration test to our test suite under test/integration for Kind. You could take some inspiration from test/integration/kind_cluster_test.go and kind_version_test.go. I'd imagine the test could go as follows:

Create a Kind cluster.
Call its DumpDiagnostics method.
Ensure that the returned directory exists and is not empty.

You can run an individual integration test using TEST_RUN=YourTestName make test.integration.

astoycos · 2023-03-22T14:06:28Z

+1 sorry I just saw your previous comment, I'll try and get to that Int test today!

shaneutt · 2023-04-12T16:17:42Z

Just checking in on this one, any help needed?

astoycos · 2023-04-12T16:36:23Z

Yep sorry for the snail pace I started this locally just need to finish it up...

rainest

I did a test run with this on https://github.com/Kong/kubernetes-ingress-controller/actions/runs/4694072692?pr=3886 and it looks the describe info and resource dumps are getting lost with this change.

diagnostics-integration-tests-enterprise-postgres.zip has logs in the new KIND logs format, but no kubectl_get_all.yaml or kubectl_describe_all.txt. Am I missing somewhere else they got moved, or are those indeed getting lost somehow?

randmonkey · 2023-04-19T07:40:47Z

I did a test run with this on https://github.com/Kong/kubernetes-ingress-controller/actions/runs/4694072692?pr=3886 and it looks the describe info and resource dumps are getting lost with this change.

diagnostics-integration-tests-enterprise-postgres.zip has logs in the new KIND logs format, but no kubectl_get_all.yaml or kubectl_describe_all.txt. Am I missing somewhere else they got moved, or are those indeed getting lost somehow?

I think the dump of kubectl get all and kubectl describe all are removed.

astoycos · 2023-04-20T16:09:58Z

Updated to add back kubectl-decribe-all and kubectl-get-all for ALL diagnostic runs, Also added an integration test and updated the existing addon one.

Just need to make sure TestKindDiagnosticDump and TestKongAddonDiagnostics passes correctly in GH actions.

shaneutt · 2023-04-21T07:31:26Z

Looks like we need to rebase for latest main but then otherwise tests are passing @astoycos 👍

pkg/clusters/diagnostics.go

test/integration/kind_diagnostics_test.go

Today in KTF we manually step through all the pods/objects in the cluster to get debug information. KIND now supports simply running `kind export logs` which does a better job of giving us a cluster debug overview. Switch the DumpDiagnostics function to use this command for cluster log dumping instead. Intentionally leave the logic which allows each addon to write their own specific cleanup functionality. Also continue to export the `kubectl_describe_all.txt` and `kubectl_get_all.yaml` files for all cluster types. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

Add new integration test for using `kind export logs` in DumpDiagnostics. Update old addon test to use the new filepath for pod logs. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

Add godoc comment to DumpAllDescribeAll function. use t.cleanup instead of defer. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

pmalek

Nothing major from my side, just 2 nits to consider

pkg/clusters/diagnostics.go

czeslavo

🚀

Standardize function signatures for all cluster diagnostics functions. Standardize name of diagnostic output directory from a mixture of "output" and "outDir" to just "outdir". Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

pmalek

👍

astoycos requested review from a team and shaneutt as code owners March 21, 2023 15:52

astoycos marked this pull request as draft March 21, 2023 15:52

astoycos temporarily deployed to gcloud March 21, 2023 15:59 — with GitHub Actions Inactive

czeslavo reviewed Mar 21, 2023

View reviewed changes

pkg/clusters/diagnostics.go Outdated Show resolved Hide resolved

astoycos force-pushed the fix-dump branch from 5d8a38a to a1fe67f Compare March 21, 2023 16:17

astoycos temporarily deployed to gcloud March 21, 2023 16:19 — with GitHub Actions Inactive

astoycos force-pushed the fix-dump branch from a1fe67f to fede95e Compare March 21, 2023 16:26

czeslavo reviewed Mar 21, 2023

View reviewed changes

pkg/clusters/diagnostics.go Outdated Show resolved Hide resolved

astoycos temporarily deployed to gcloud March 21, 2023 17:57 — with GitHub Actions Inactive

astoycos force-pushed the fix-dump branch from fede95e to 1d8f167 Compare March 21, 2023 19:06

astoycos temporarily deployed to gcloud March 21, 2023 19:18 — with GitHub Actions Inactive

czeslavo force-pushed the fix-dump branch from 1d8f167 to 9861d53 Compare March 22, 2023 13:15

czeslavo temporarily deployed to gcloud March 22, 2023 13:15 — with GitHub Actions Inactive

astoycos marked this pull request as ready for review March 22, 2023 13:53

shaneutt mentioned this pull request Mar 22, 2023

Allow running integration and E2E tests in tests workflow for external contributors #596

Open

2 tasks

rainest reviewed Apr 13, 2023

View reviewed changes

randmonkey mentioned this pull request Apr 17, 2023

test: add timeout to context to trigger cleanup on wait for env ready Kong/kubernetes-ingress-controller#3894

Merged

astoycos force-pushed the fix-dump branch from f908ad3 to 5064924 Compare April 20, 2023 16:06

astoycos temporarily deployed to gcloud April 21, 2023 02:29 — with GitHub Actions Inactive

randmonkey temporarily deployed to gcloud April 21, 2023 09:27 — with GitHub Actions Inactive

randmonkey previously approved these changes Apr 21, 2023

View reviewed changes

pmalek reviewed Apr 21, 2023

View reviewed changes

pkg/clusters/diagnostics.go Outdated Show resolved Hide resolved

test/integration/kind_diagnostics_test.go Outdated Show resolved Hide resolved

astoycos added 3 commits April 24, 2023 14:46

Add new Int test and update old one

53c6909

Add new integration test for using `kind export logs` in DumpDiagnostics. Update old addon test to use the new filepath for pod logs. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

Respond to review

3e1671c

Add godoc comment to DumpAllDescribeAll function. use t.cleanup instead of defer. Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

astoycos dismissed randmonkey’s stale review via 563dc36 April 24, 2023 18:54

astoycos force-pushed the fix-dump branch from 563dc36 to 3e1671c Compare April 24, 2023 18:55

shaneutt requested review from randmonkey, czeslavo, pmalek and rainest April 24, 2023 19:11

astoycos temporarily deployed to gcloud April 24, 2023 19:11 — with GitHub Actions Inactive

randmonkey previously approved these changes Apr 25, 2023

View reviewed changes

pmalek reviewed Apr 25, 2023

View reviewed changes

pkg/clusters/diagnostics.go Outdated Show resolved Hide resolved

pkg/clusters/diagnostics.go Outdated Show resolved Hide resolved

czeslavo previously approved these changes Apr 25, 2023

View reviewed changes

Standardize function Signature

ef75273

Standardize function signatures for all cluster diagnostics functions. Standardize name of diagnostic output directory from a mixture of "output" and "outDir" to just "outdir". Signed-off-by: Andrew Stoycos <astoycos@redhat.com>

astoycos dismissed stale reviews from czeslavo and randmonkey via d66cb8b April 25, 2023 13:17

astoycos force-pushed the fix-dump branch from d66cb8b to ef75273 Compare April 25, 2023 13:19

shaneutt requested review from pmalek and czeslavo April 25, 2023 14:29

astoycos temporarily deployed to gcloud April 25, 2023 14:29 — with GitHub Actions Inactive

pmalek approved these changes Apr 25, 2023

View reviewed changes

shaneutt enabled auto-merge (squash) April 25, 2023 14:32

shaneutt merged commit ba60f3a into Kong:main Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert dumpDiag to use `kind export logs` #591

Convert dumpDiag to use `kind export logs` #591

astoycos commented Mar 21, 2023

CLAassistant commented Mar 21, 2023 •

edited

Loading

astoycos commented Mar 21, 2023

codecov-commenter commented Mar 21, 2023 •

edited

Loading

astoycos commented Mar 21, 2023

czeslavo commented Mar 22, 2023

czeslavo commented Mar 22, 2023

astoycos commented Mar 22, 2023

czeslavo commented Mar 22, 2023

astoycos commented Mar 22, 2023

shaneutt commented Apr 12, 2023

astoycos commented Apr 12, 2023

rainest left a comment

randmonkey commented Apr 19, 2023 •

edited

Loading

astoycos commented Apr 20, 2023 •

edited

Loading

shaneutt commented Apr 21, 2023

pmalek left a comment •

edited

Loading

czeslavo left a comment

pmalek left a comment

Convert dumpDiag to use kind export logs #591

Convert dumpDiag to use kind export logs #591

Conversation

astoycos commented Mar 21, 2023

CLAassistant commented Mar 21, 2023 • edited Loading

astoycos commented Mar 21, 2023

codecov-commenter commented Mar 21, 2023 • edited Loading

Codecov Report

astoycos commented Mar 21, 2023

czeslavo commented Mar 22, 2023

czeslavo commented Mar 22, 2023

astoycos commented Mar 22, 2023

czeslavo commented Mar 22, 2023

astoycos commented Mar 22, 2023

shaneutt commented Apr 12, 2023

astoycos commented Apr 12, 2023

rainest left a comment

Choose a reason for hiding this comment

randmonkey commented Apr 19, 2023 • edited Loading

astoycos commented Apr 20, 2023 • edited Loading

shaneutt commented Apr 21, 2023

pmalek left a comment • edited Loading

Choose a reason for hiding this comment

czeslavo left a comment

Choose a reason for hiding this comment

pmalek left a comment

Choose a reason for hiding this comment

Convert dumpDiag to use `kind export logs` #591

Convert dumpDiag to use `kind export logs` #591

CLAassistant commented Mar 21, 2023 •

edited

Loading

codecov-commenter commented Mar 21, 2023 •

edited

Loading

randmonkey commented Apr 19, 2023 •

edited

Loading

astoycos commented Apr 20, 2023 •

edited

Loading

pmalek left a comment •

edited

Loading