Proposal: unix-sockets support in Fluentd logging driver#26088
Proposal: unix-sockets support in Fluentd logging driver#26088thaJeztah merged 1 commit intomoby:masterfrom
Conversation
|
@justincormack |
|
ping @justincormack @tagomoris ptal |
|
The feature looks good to me. |
|
design LGTM, so moving to code review. I agree with @tagomoris that the variable name should be renamed |
|
ping @akirakoyasu can you update the PR to rename that variable? Let me know if you need suggestions |
6cb90f8 to
bef5e1f
Compare
|
@thaJeztah yes, I renamed the variable; |
|
Test failures are not related, looks like Docker Hub search has/had issues |
daemon/logger/fluentd/fluentd.go
Outdated
There was a problem hiding this comment.
Looks like we should do a refactor of this function at some point, because it's now getting too many output variables. Probably a lot cleaner to return a type
1a1a05d to
5ccd091
Compare
|
@thaJeztah |
|
@GordonTheTurtle I'm sorry, missing sign. :) |
daemon/logger/fluentd/fluentd.go
Outdated
There was a problem hiding this comment.
Would be helpful to use errors.Wrapf instead of fmt.Errorf here.
There was a problem hiding this comment.
thanks for comment. using it.
|
Minor comment, otherwise LGTM. |
|
also, @akirakoyasu can you squash your commits, so that there's a single commit in the PR? |
Signed-off-by: Akira Koyasu <mail@akirakoyasu.net> - add scheme to fluentd-address - define a new type `location` - use `errors.Wrapf`
5ccd091 to
cb176c8
Compare
|
@thaJeztah yes, squash done. |
|
code looks good. Do we need a followup change to https://github.com/docker/docker.github.io/blob/master/engine/admin/logging/fluentd.md ? Existing doc doesn't really discuss the host format or any restrictions. |
|
@MHBauer |
|
|
||
| host, port, err := net.SplitHostPort(address) | ||
| if err != nil { | ||
| if !strings.Contains(err.Error(), "missing port in address") { |
There was a problem hiding this comment.
How come we are checking the string on this error?
Worst case we can check for AddrError, but even so any error here would be an invalid address would it not?
There was a problem hiding this comment.
This code is for the default port.
I don't like this such as string check too, but an effort for parsing address by ourselves is not so suitable, I think.
There was a problem hiding this comment.
But why would it only be checking for missing port in address vs the whole thing being messed up?
To me, if we get an error here, shouldn't that always be returned?
| return nil, errors.Wrapf(err, "invalid fluentd-address %s", givenAddress) | ||
| } | ||
| return host, portnum, nil | ||
| return &location{ |
There was a problem hiding this comment.
Do we need this new type?
Return proto, host, port, err.
If proto == unix, then host == socket path
There was a problem hiding this comment.
I suggested to change, because the initial implementation was having 5 output variables #26088 (comment), but perhaps we can reduce that some other ways, agreed
There was a problem hiding this comment.
@akirakoyasu can you update with @cpuguy83's suggestion? Sorry for going back and forth
There was a problem hiding this comment.
We can deal with the extra type.
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
this will also need documentation changes to mention the supported options in https://docs.docker.com/engine/admin/logging/fluentd/#/fluentd-address
@akirakoyasu can you open a pull request in the vnext-engine branch in the documentation repository? The file for that part of the documentation can be found here; https://github.com/docker/docker.github.io/blob/vnext-engine/engine/admin/logging/fluentd.md
|
@thaJeztah |
|
Removing |
- What I did
add unix-sockets support in Fluentd logging driver
- How I did it
add handling scheme in parsing address
- How to verify it
cases below OK
- Description for the changelog
Fluentd logging driver supports unix sockets
- A picture of a cute animal (not mandatory but encouraged)
