-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
openshift-install: log debug output to file #689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm fine with this being on by default, it would be nice to have the option to turn this off.
/hold This isn't working as intended. |
/hold cancel |
Dangit |
cmd/openshift-install/log.go
Outdated
} | ||
|
||
func setupFileHook() (func(), error) { | ||
logfile, err := os.OpenFile(".openshift-install.log", os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0666) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to get big ;). Do we need to prompt folks to occasionally prune somewhere? That's one reason why putting this in the asset directory might be useful.
pkg/asset/store.go
Outdated
assetToStore = onDiskAsset | ||
source = onDiskSource | ||
// The asset is in the state file. The asset is sourced from state file. | ||
case foundInStateFile: | ||
logrus.Debugf("%sUsing %q loaded from state file", indent, asset.Name()) | ||
logrus.Tracef("%sUsing %q loaded from state file", indent, asset.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have enough trouble with asset contamination that I'd rather keep these at the debug level. Can you talk through why you're demoting them to trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize that was still a problem. I figured it was noise to the vast majority of users and felt like a deeper level of logging. I'll drop that commit.
Also add |
Put it in the asset directory :p |
@wking I moved that log file into the install directory. |
1.2.0 or newer is required in order to get the "trace" log level.
This changes the logging strategy so that it logs all output to a local file in addition to logging at the desired level to the console. Unfortunately, logrus doesn't have a facility for capturing all log entries (the hooks are only fired for entries which are at or below the specified level) so this disables the console output and sets up two hooks: one for the console and one for the local file.
it looks like we are already at Lines 432 to 438 in 30bb25a
We moved to the latest tag here d92442d |
/approve |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, crawford, wking 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:
Approvers can indicate their approval by writing |
/retest |
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439
06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689), the global logrus level has been TraceLevel. Decisions about logging to a particular target are now made when adding the hook for that target. This broke our Terraform debug approach, which had been based on the level for the global logger. Since 06c5fd3, logrus.GetLevel() == logrus.DebugLevel will never be true. With this commit, I've replaced that switch and now just push all Terraform stdout into the default logger at the debug level. A new LinePrinter provides a shim between Terraform's stdout stream and the line-based log calls. I've added a local buffer for stderr, because Run [1] doesn't collect it automatically like Output does [2]. LinePrinter.Lock protects us from parallel writes, because [3]: The slice is valid for use only until the next buffer modification (that is, only until the next call to a method like Read, Write, Reset, or Truncate). The docs aren't clear about whether that limitation applies to ReadString or not [4], but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice [5]. We're unlikely to actually have parallel writes for our Terraform use-case, but I'd rather be conservative now and not have to track down a race if someone decides to use LinePrinter for something concurrent later. The mutex is embedded at Alex's recommendation [6]. [1]: https://golang.org/pkg/os/exec/#Cmd.Run [2]: https://golang.org/pkg/os/exec/#Cmd.Output [3]: https://golang.org/pkg/bytes/#Buffer.Bytes [4]: https://golang.org/pkg/bytes/#Buffer.ReadString [5]: https://github.com/golang/go/blob/go1.11.2/src/bytes/buffer.go#L416-L439 [6]: openshift#730 (comment)
These were inconsistencies from 06c5fd3 (openshift-install: log debug output to file, 2018-11-15, openshift#689).
This makes it easier for folks who are less familiar with the debug logs to find the actual error message. For folks who *do* need access to the debug logs, we still have those in the recovered artifacts since openshift/installer@06c5fd36 (openshift-install: log debug output to file, 2018-11-15, openshift/installer#689). Generated with: $ sed -i 's/--log-level.debug //' $(git grep -l -- '--log-level.debug')
This changes the logging strategy so that it logs all output to a local
file in addition to logging at the desired level to the console.
Unfortunately, logrus doesn't have a facility for capturing all log
entries (the hooks are only fired for entries which are at or below the
specified level) so this disables the console output and sets up two
hooks: one for the console and one for the local file.