Save CLI log output to file included in support bundle #4289

cxbrowne1207 · 2022-12-05T21:43:49Z

Issue #, if available:

Description of changes:
This PR customizes the zap logger to log verbose logs to a file, and then include a copy of it in the support bundles when one is generated. This offers an easier way for the customer recover CLI run logs for debugging, when there are errors and interruptions of some kind.

Testing (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eks-distro-bot · 2022-12-05T21:43:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2022-12-05T21:53:30Z

Codecov Report

Merging #4289 (6bef564) into main (29a9135) will increase coverage by 0.09%.
The diff coverage is 90.24%.

@@            Coverage Diff             @@
##             main    #4289      +/-   ##
==========================================
+ Coverage   68.61%   68.70%   +0.09%     
==========================================
  Files         406      408       +2     
  Lines       33086    33310     +224     
==========================================
+ Hits        22701    22886     +185     
- Misses       8933     8969      +36     
- Partials     1452     1455       +3

Impacted Files	Coverage Δ
pkg/logger/zap.go	`85.48% <85.18%> (ø)`
pkg/diagnostics/collectors.go	`81.51% <100.00%> (+0.59%)`	⬆️
pkg/diagnostics/diagnostic_bundle.go	`77.85% <100.00%> (+0.54%)`	⬆️
pkg/logger/logger.go	`30.00% <100.00%> (ø)`
pkg/providers/nutanix/env.go	`80.00% <0.00%> (-1.25%)`	⬇️
pkg/networkutils/networkutils.go	`67.56% <0.00%> (-0.86%)`	⬇️
pkg/executables/govc.go	`63.94% <0.00%> (-0.19%)`	⬇️
internal/pkg/api/vsphere.go	`0.00% <0.00%> (ø)`
internal/pkg/api/vspheremachines.go	`0.00% <0.00%> (ø)`
pkg/api/v1alpha1/vspheremachineconfig_types.go	`57.14% <0.00%> (ø)`
... and 15 more

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

cxbrowne1207 · 2022-12-06T00:26:26Z

\hold

cxbrowne1207 · 2022-12-06T04:59:51Z

\unhold

g-gaston

Is there a requirement to store the logs in memory? Or is that just an implementation detail.
Do we need this log dump always or should be just do it selectively where we need it (CLI commands for example)?
How does this solve the interrupted SSH connection? It seems like we are only reading the in memory logs when the support bundle report is triggered. And that only happens if the CLI execution is maintained uninterrupted and only after a CLI error happens. If the SSH connection is dropped and the process is interrupted, the support bundle won't be created and there won't be any logs. TBH, the solution to this problem should be to use tmux or screen, SSH connections are out of the CLI's domain of responsibility, so any SSH associated issue should be solved with SSH tools. But you described that situation in your problem statement and I don't think this code solves that usecase.

Wouldn't this be simpler if we use config.OutputPaths?

pkg/diagnostics/collectors.go

pkg/logger/logcacher.go

pkg/logger/logger.go

danbudris · 2022-12-06T17:00:09Z

@g-gaston

How does this solve the interrupted SSH connection? It seems like we are only reading the in memory logs when the support bundle report is triggered. And that only happens if the CLI execution is maintained uninterrupted and only after a CLI error happens. If the SSH connection is dropped and the process is interrupted, the support bundle won't be created and there won't be any logs. TBH, the solution to this problem should be to use tmux or screen, SSH connections are out of the CLI's domain of responsibility, so any SSH associated issue should be solved with SSH tools. But you described that situation in your problem statement and I don't think this code solves that usecase.

Yeah from the conversation between @cxbrowne1207 and myself, the original impetus for this work was #1703; but the ticket over-states the importance of perserving the connection when SSH is used, and the real value is 'how do we get the CLI logs into the support bundle' rather than 'how do we maintain the session when closed'; I think that maybe we need to revisit the description, but the value of doing this as a way to get the CLI output into the support bundle stands IMO. Agree tho re: customer should be using tmux or w/e to maintain their sessions.

cxbrowne1207 · 2022-12-06T17:40:00Z

Is there a requirement to store the logs in memory? Or is that just an implementation detail.

Do we need this log dump always or should be just do it selectively where we need it (CLI commands for example)?

How does this solve the interrupted SSH connection? It seems like we are only reading the in memory logs when the support bundle report is triggered. And that only happens if the CLI execution is maintained uninterrupted and only after a CLI error happens. If the SSH connection is dropped and the process is interrupted, the support bundle won't be created and there won't be any logs. TBH, the solution to this problem should be to use tmux or screen, SSH connections are out of the CLI's domain of responsibility, so any SSH associated issue should be solved with SSH tools. But you described that situation in your problem statement and I don't think this code solves that usecase.

Wouldn't this be simpler if we use config.OutputPaths?

It's only an implementation detail, the goal is to have the logs to be ultimately stored in the support bundle it seems. As mentioned @danbudris mentioned, the original ticket was used for as a basis for the description, but there's a mismatch with the goal of saving the logs to the support bundle as it doesn't solve the SSH connection issue. So, the PR description is a mistake. Will update.

I guess the question is now whether or not the solution is too complex and if the same thing can be done with config.OutputPaths. It sounds like it may also be better than saving it in memory to address the cases where there is some kind of interruption before the support bundle phase. I'll look into that :)

cxbrowne1207 · 2022-12-06T21:23:54Z

@g-gaston @eks-distro-bot

Using OutputPath works well, but if we want to always log verbosely to the file like suggested on the ticket there are some caveats. We would have to customize the creation of the logger instead of using Zap.Config.Build in the InitZap method. So, our options are

Settle for logging to the file with whatever level is specified by the customer.
Customize the step to build a logger where zapcore.NewTee() is used to duplicate the logs, to the file and to the console.
We may be able to instantiate, a secondary logger that only logs to a file (may require some customization beyond Zap.Config.Build as well but would have to log twice every-time (its also separate from the original.

danbudris · 2022-12-06T21:52:47Z

Customize the step to build a logger where zapcore.NewTee() is used to duplicate the logs, to the file and to the console.

This seems reasonable to me; from looking at the docs and some quick googling this seems like the recommended approach.

g-gaston · 2022-12-06T22:27:13Z

For the record, there was quorum since I voted option 2 offline as well :)

pkg/logger/config.go

g-gaston · 2022-12-07T21:33:24Z

pkg/logger/config.go

+}
+
+// Build constructs a logger and returns a logger.
+func (cfg Config) Build() (*zap.Logger, error) {


This was exported mainly so that it could be accessed it from the tests.

pkg/logger/config.go

pkg/logger/zap.go

pkg/logger/logger.go

g-gaston · 2022-12-07T21:39:45Z

pkg/logger/logger.go

+	l              logr.Logger = logr.Discard()
+	once           sync.Once
+	outputFilename string


We should think about grouping these 3 (or at least the logger and output file) in a struct
Can be a follow up

pkg/diagnostics/collectors.go

pkg/diagnostics/diagnostic_bundle.go

pkg/logger/logger.go

pkg/logger/zap.go

pkg/diagnostics/collectors.go

g-gaston · 2022-12-08T20:28:59Z

cmd/eksctl-anywhere/cmd/root.go

@@ -32,7 +33,11 @@ func rootPersistentPreRun(cmd *cobra.Command, args []string) {
 }

 func initLogger() error {
-	if err := logger.InitZap(viper.GetInt("verbosity")); err != nil {
+	outputFilePath := fmt.Sprintf("./eksa-cli-%s.log", time.Now().Format("2006-01-02T15_04_05"))


should we look into deleting this file after a successful cli operation?

g-gaston · 2022-12-08T20:33:15Z

pkg/logger/zap.go

-		opt(&logr)
+
+	for _, name := range args.WithNames {
+		WithName(name)(&logr)


nit:

Suggested change

WithName(name)(&logr)

logr = logr.WithName(name)

I think it's a bit more readable? Unless I'm missing something

…on error

cxbrowne1207 · 2022-12-08T21:24:20Z

/approve

eks-distro-bot · 2022-12-08T21:24:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cxbrowne1207

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cxbrowne1207]

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

cxbrowne1207 · 2022-12-08T22:06:35Z

/unhold

eks-distro-bot added the do-not-merge/work-in-progress label Dec 5, 2022

eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold labels Dec 5, 2022

cxbrowne1207 marked this pull request as ready for review December 5, 2022 21:44

cxbrowne1207 force-pushed the save-cli-log-output branch from 6379b93 to 9dfc2b8 Compare December 5, 2022 23:56

cxbrowne1207 requested review from danbudris and g-gaston December 6, 2022 00:25

cxbrowne1207 changed the title ~~[WIP] Save CLI log output to file included in support bundle~~ Save CLI log output to file included in support bundle Dec 6, 2022

eks-distro-bot removed the do-not-merge/work-in-progress label Dec 6, 2022

cxbrowne1207 force-pushed the save-cli-log-output branch from f8402cb to 1e4143f Compare December 6, 2022 04:37

cxbrowne1207 force-pushed the save-cli-log-output branch from 1e4143f to 2a7c980 Compare December 6, 2022 05:00

g-gaston reviewed Dec 6, 2022

View reviewed changes

cxbrowne1207 force-pushed the save-cli-log-output branch from 2a7c980 to fb189b0 Compare December 7, 2022 04:10

cxbrowne1207 requested a review from g-gaston December 7, 2022 04:10

cxbrowne1207 force-pushed the save-cli-log-output branch from c90bf8e to 0ca4492 Compare December 7, 2022 06:42

g-gaston reviewed Dec 7, 2022

View reviewed changes

g-gaston reviewed Dec 8, 2022

View reviewed changes

save cli log output to file and store a copy in the support bundle up…

6bef564

…on error

cxbrowne1207 force-pushed the save-cli-log-output branch from b50f417 to 6bef564 Compare December 8, 2022 21:23

g-gaston approved these changes Dec 8, 2022

View reviewed changes

eks-distro-bot assigned g-gaston Dec 8, 2022

eks-distro-bot added the lgtm label Dec 8, 2022

eks-distro-bot added the approved label Dec 8, 2022

eks-distro-bot removed the do-not-merge/hold label Dec 8, 2022

eks-distro-bot merged commit edb31bc into aws:main Dec 8, 2022

cxbrowne1207 deleted the save-cli-log-output branch December 8, 2022 22:09

vivek-koppuru mentioned this pull request Dec 22, 2022

Save cli version and cluster yaml in diagnostic bundle #4497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save CLI log output to file included in support bundle #4289

Save CLI log output to file included in support bundle #4289

cxbrowne1207 commented Dec 5, 2022 •

edited

Loading

eks-distro-bot commented Dec 5, 2022

codecov bot commented Dec 5, 2022 •

edited

Loading

cxbrowne1207 commented Dec 6, 2022

cxbrowne1207 commented Dec 6, 2022

g-gaston left a comment

danbudris commented Dec 6, 2022 •

edited

Loading

cxbrowne1207 commented Dec 6, 2022 •

edited

Loading

cxbrowne1207 commented Dec 6, 2022 •

edited

Loading

danbudris commented Dec 6, 2022

g-gaston commented Dec 6, 2022

g-gaston Dec 7, 2022

cxbrowne1207 Dec 7, 2022

g-gaston Dec 7, 2022

cxbrowne1207 Dec 7, 2022

g-gaston Dec 8, 2022

g-gaston Dec 8, 2022

cxbrowne1207 commented Dec 8, 2022

eks-distro-bot commented Dec 8, 2022

cxbrowne1207 commented Dec 8, 2022

Save CLI log output to file included in support bundle #4289

Save CLI log output to file included in support bundle #4289

Conversation

cxbrowne1207 commented Dec 5, 2022 • edited Loading

eks-distro-bot commented Dec 5, 2022

codecov bot commented Dec 5, 2022 • edited Loading

Codecov Report

cxbrowne1207 commented Dec 6, 2022

cxbrowne1207 commented Dec 6, 2022

g-gaston left a comment

Choose a reason for hiding this comment

danbudris commented Dec 6, 2022 • edited Loading

cxbrowne1207 commented Dec 6, 2022 • edited Loading

cxbrowne1207 commented Dec 6, 2022 • edited Loading

danbudris commented Dec 6, 2022

g-gaston commented Dec 6, 2022

g-gaston Dec 7, 2022

Choose a reason for hiding this comment

cxbrowne1207 Dec 7, 2022

Choose a reason for hiding this comment

g-gaston Dec 7, 2022

Choose a reason for hiding this comment

cxbrowne1207 Dec 7, 2022

Choose a reason for hiding this comment

g-gaston Dec 8, 2022

Choose a reason for hiding this comment

g-gaston Dec 8, 2022

Choose a reason for hiding this comment

cxbrowne1207 commented Dec 8, 2022

eks-distro-bot commented Dec 8, 2022

cxbrowne1207 commented Dec 8, 2022

cxbrowne1207 commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading

danbudris commented Dec 6, 2022 •

edited

Loading

cxbrowne1207 commented Dec 6, 2022 •

edited

Loading

cxbrowne1207 commented Dec 6, 2022 •

edited

Loading