pkg/lineprinter: Add a wrapper for Write -> Print #730
Conversation
openshift-ci-robot
added
size/L
wking
force-pushed
the
terraform-to-logger
branch
from
15b0784
to
92308f3
Compare
|
Whats the reasoning for impl |
wking
force-pushed
the
terraform-to-logger
branch
from
92308f3
to
debabb6
Compare
I wanted to be able to ensure any final subprocess output was logged, even if the subprocess died mid-line without having printed a trailing newline. A |
hmm 😕 , it is preventing the installer from dying mid-priniting not subprocess right? |
It's not keeping anything from dying. It's ensuring that, if the Terraform subprocess happens to die mid-line, that we still get that partial line in the logs. There's nothing we can do about the installer dying mid-line (collecting that would be up to whoever was calling the installer). |
|
The e2e-aws errors included: which would be orthogonal to this change. You can see that the Terraform logs are being captured again. /retest |
There is trailing |
wking
force-pushed
the
terraform-to-logger
branch
from
debabb6
to
c4c3c63
Compare
wking
force-pushed
the
terraform-to-logger
branch
from
c4c3c63
to
aa7162a
Compare
|
No newlines now. /retest |
| @@ -0,0 +1,27 @@ | |||
| package lineprinter | |||
There was a problem hiding this comment.
lineprinters is splitting on \n it should be the one trimming it. Why add something new?
There was a problem hiding this comment.
lineprinters is splitting on
\nit should be the one trimming it. Why add something new?
If you hook LinePrinter up to fmt.Print, you do want the trailing newlines:
$ cat test.go
package main
import (
"fmt"
"github.com/openshift/installer/pkg/lineprinter"
)
func main() {
linePrinter := &lineprinter.LinePrinter{
Print: func (args ...interface{}) {
fmt.Print(args...)
},
}
linePrinter.Write([]byte("1\n2\n3\n4"))
linePrinter.Close()
}
$ go run test.go
1
2
3
4There was a problem hiding this comment.
lineprinters is splitting on
\nit should be the one trimming it. Why add something new?If you hook
LinePrinterup tofmt.Print, you do want the trailing newlines:$ cat test.go package main import ( "fmt" "github.com/openshift/installer/pkg/lineprinter" ) func main() { linePrinter := &lineprinter.LinePrinter{ Print: func (args ...interface{}) { fmt.Print(args...) }, } linePrinter.Write([]byte("1\n2\n3\n4")) linePrinter.Close() } $ go run test.go 1 2 3 4
This example makes it seems like lineprinter is not named correctly then.
There was a problem hiding this comment.
This example makes it seems like lineprinter is not named correctly then.
It reads in a stream and calls the wrapped print on each line. What would you call it instead?
wking
force-pushed
the
terraform-to-logger
branch
2 times, most recently
from
77f0380
to
d94db67
Compare
|
Dunno what these failures are about, looks like they are maybe all artifact-upload failures? /retest |
pkg/lineprinter/lineprinter.go
Outdated
| // any remaining partial line. | ||
| type LinePrinter struct { | ||
| buf bytes.Buffer | ||
| lock sync.Mutex |
There was a problem hiding this comment.
Mutexes are usually embedded so you can "lock" the struct directly.
|
This looks good to me. Sorry for breaking it in the first place and writing that conditional before that! |
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)
wking
force-pushed
the
terraform-to-logger
branch
from
d94db67
to
2dd9230
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
1 similar comment
|
/retest |
|
/retest |
|
e2e-aws has lots of errors like: But #724 went through recently, so maybe if we try again... 🤞 /retest |
|
/retest |
|
/retest See if openshift/release#2269 helped. |
$ oc project ci-op-s2mqkj8s
$ oc logs e2e-aws -c test
...
fail [k8s.io/kubernetes/test/e2e/scheduling/resource_quota.go:129]: Expected error:
<*errors.errorString | 0xc420091570>: {
s: "timed out waiting for the condition",
}
timed out waiting for the condition
not to have occurred
failed: (1m35s) "[sig-scheduling] ResourceQuota should create a ResourceQuota and capture the life of a secret. [Suite:openshift/conformance/parallel] [Suite:k8s] [Suite:openshift/smoke-4]"
started: (1/235/325) "[k8s.io] Pods should be updated [NodeConformance] [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s] [Suite:openshift/smoke-4]"
...That's |
|
/retest |
1 similar comment
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
06c5fd3 (#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.DebugLevelwill 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
LinePrinterprovides a shim between Terraform's stdout stream and the line-based log calls.I've added a local buffer for stderr, because
Rundoesn't collect it automatically likeOutputdoes.LinePrinter.Lockprotects us from parallel writes, because:The docs aren't clear about whether that limitation applies to
ReadStringor not, but the backing implementation has no internal lock to protect from changes made before the internal reference is copied into a new slice. 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.