feature: init syslog functionality in pouchd #1500

fuweid · 2018-06-09T13:05:03Z

Ⅰ. Describe what this PR did

Add syslog driver into PouchContainer so that user can use more log driver, not just jsonfile.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Before add this feature, we have to do the following things:

allow user to specify the log-driver by CLI
add validation for the log options during create container

NOTE: In order to keep change scope, we can add validation into update/upgrade in the following PR if necessary.

After we do the above things, we can handle syslog like jsonfile. With syslog driver, user can write the log data in local syslog or send it to the remote syslog center.

In this PR, I introduce the third-party package srslog, which can handle the local and remote cases.

Ⅳ. Describe how to verify it

The user can use the following options to setup the syslog driver:

syslog-facility [priority]
syslog-address [for remote syslog center]
syslog-format [log format]
tag [allow user to log more container context]

only driver

➜  pouch git:(feature_syslog) pouch run --log-driver syslog busybox echo hello
hello

➜  tail -f /var/log/syslog
Jun 10 18:07:49 ubuntu-xenial kernel: [54778.963768] eth0: renamed from veth8ea9817
Jun 10 18:07:49 ubuntu-xenial kernel: [54778.974287] IPv6: ADDRCONF(NETDEV_CHANGE): veth571595e: link becomes ready
Jun 10 18:07:49 ubuntu-xenial kernel: [54778.974358] p0: port 3(veth571595e) entered forwarding state
Jun 10 18:07:49 ubuntu-xenial kernel: [54778.974368] p0: port 3(veth571595e) entered forwarding state
>>>>>> Jun 10 18:07:49 ubuntu-xenial 4d861e786eeb[14458]: hello
Jun 10 18:07:49 ubuntu-xenial kernel: [54779.175524] p0: port 3(veth571595e) entered disabled state
Jun 10 18:07:49 ubuntu-xenial kernel: [54779.175576] veth8ea9817: renamed from eth0
Jun 10 18:07:49 ubuntu-xenial kernel: [54779.218755] p0: port 3(veth571595e) entered disabled state
Jun 10 18:07:49 ubuntu-xenial kernel: [54779.221528] device veth571595e left promiscuous mode
Jun 10 18:07:49 ubuntu-xenial kernel: [54779.221532] p0: port 3(veth571595e) entered disabled state

driver with tag

➜  pouch git:(feature_syslog) pouch run --log-driver syslog \
--log-opt tag="id={{.ID}} - image-id={{.ImageID}} - {{.DaemonName}}" \
busybox echo hello
hello

➜  tail -f /var/log/syslog
....
Jun 10 18:09:21 ubuntu-xenial id=7cd48c5829da - image-id=8c811b4aec35 - pouchd[14458]: hello
....

driver with tag and different format

➜  pouch git:(feature_syslog) pouch run --log-driver syslog \
--log-opt tag="id={{.ID}} - image-id={{.ImageID}} - {{.DaemonName}}" \
--log-opt syslog-format=rfc5424micro \
busybox echo hello
hello

➜  tail -f /var/log/syslog
....
Jun 10 18:13:07 ubuntu-xenial 1 2018-06-10T18:13:07.220202+08:00 ubuntu-xenial id=9933ce8a4c83 - image-id=8c811b4aec35 - pouchd 14458 id=9933ce8a4c83 - image-id=8c811b4aec35 - pouchd - hello

Ⅴ. Special notes for reviews

I submitted several commits so that the reviewer can check commit one by one.
For example, first commit is to show the refactor small changed part. It can avoid to use big change to flood the reviewer. :)

After LGTM, we can use squash and change commit message before merge.

Known issues:

the srslog should provide the timeout option. Just in case that the remote log center set up and the client will hang.
for now, the containerio doesn't return error during initing the backend. I will send following PR to fix this.

codecov-io · 2018-06-09T13:31:07Z

Codecov Report

Merging #1500 into master will decrease coverage by 2.29%.
The diff coverage is 43.97%.

@@            Coverage Diff            @@
##           master    #1500     +/-   ##
=========================================
- Coverage   40.55%   38.25%   -2.3%     
=========================================
  Files         253      262      +9     
  Lines       16481    17874   +1393     
=========================================
+ Hits         6684     6838    +154     
- Misses       8941    10168   +1227     
- Partials      856      868     +12

Impacted Files	Coverage Δ
cli/common_flags.go	`0% <0%> (ø)`	⬆️
daemon/logger/info.go	`0% <0%> (ø)`
cli/container.go	`0% <0%> (ø)`	⬆️
daemon/logger/syslog/fmt.go	`0% <0%> (ø)`
daemon/mgr/container_logs.go	`84.21% <100%> (+9.56%)`	⬆️
apis/opts/log_options.go	`100% <100%> (ø)`
daemon/containerio/syslog.go	`18.75% <18.75%> (ø)`
daemon/logger/syslog/syslog.go	`25% <25%> (ø)`
daemon/containerio/options.go	`50.79% <28.57%> (-4.38%)`	⬇️
daemon/mgr/container.go	`34.71% <40.74%> (-14.58%)`	⬇️
... and 31 more

1. use logger.Info to contain container's information 2. add log driver validation in daemon 3. make the openContainerIO/attachContainerIO friendly Signed-off-by: Wei Fu <[email protected]>

Signed-off-by: Wei Fu <[email protected]>

1. add option in containerio 2. add syslog config validation in container mgr 3. add syslog option during init containerio Signed-off-by: Wei Fu <[email protected]>

Signed-off-by: Wei Fu <[email protected]>

allencloud · 2018-06-12T09:00:30Z

apis/opts/log_options.go

+// convertKVStringsToMap converts ["key=value"] into {"key":"value"}
+//
+// TODO(fuwei): make it common in the opts.ParseXXX().
+func convertKVStringsToMap(values []string) (map[string]string, error) {


How about encapsulating the function into pkg?
Since I think this is a common function there.

make senses.

YaoZengzeng · 2018-06-12T10:14:08Z

daemon/logger/syslog/validate.go

+	// ErrInvalidSyslogFormat represents the invalid format.
+	ErrInvalidSyslogFormat = errors.New("invalid syslog format")
+
+	// ErrFailedToLoadX509KeyPair is to used to indicate that it's failed to load x590 key pair.


load x509 key pair? 😄

haha find a typo! good catch.

YaoZengzeng · 2018-06-12T10:51:48Z

daemon/mgr/container_logger.go

+	"github.com/sirupsen/logrus"
+)
+
+func optionsForContainerio(c *Container) []func(*containerio.Option) {


This function is only used for log options and in the file container_logger.go

Maybe the name logOptionsForContainerio is more appropriate?

make senses.

YaoZengzeng · 2018-06-12T10:53:16Z

daemon/logger/syslog/syslog.go

+
+// Close closes the Syslog.
+func (s *Syslog) Close() error {
+	return s.writer.Close()


Is it OK to close Syslog multiple times?

In my opinion, I don't think we should care the multiple closes. Even if the srslog supports the case.

We should close the syslog once. If we closes it multiple times, we should reconsider that the logic is ok. And that we will introduce sync.Lock things if we support the case. Is it worth? 😄

Yes, I totally agree that the backend should be closed only once.

But now pouchd will include all the backends in both Stdout and Stderr. when being closed, both of them will close all the backends. And this is unreasonable.

So my PR #1388 fixes this problem. The close of Stdout and Stderr only close the corresponding ring buffer. Only when the IO is closed, all the backends will be closed 😄

@YaoZengzeng we need to drain the ringbuffer, right? When we close the IO, we need to wait the data has been drained. 😄

Yes, it is exactly what #1388 do. 😄

1. s/optionsForContainerio/logOptionsForContainerio/g 2. make the convertKVStringsToMap into pkg/utils 3. fix typo Signed-off-by: Wei Fu <[email protected]>

YaoZengzeng · 2018-06-19T06:47:47Z

LGTM.

pouchrobot added areas/log kind/feature size/L labels Jun 9, 2018

refactor: allow container mgr to use custom log driver

fbe9222

1. use logger.Info to contain container's information 2. add log driver validation in daemon 3. make the openContainerIO/attachContainerIO friendly Signed-off-by: Wei Fu <[email protected]>

pouchrobot added size/XL and removed size/L labels Jun 9, 2018

Wei Fu added 2 commits June 9, 2018 22:45

feature: allow cli to use custom log driver and opts

39f8a4d

Signed-off-by: Wei Fu <[email protected]>

feature: add github.com/RackSec/srslog dependence

bb7318b

Signed-off-by: Wei Fu <[email protected]>

pouchrobot added size/XXL and removed size/XL labels Jun 10, 2018

Wei Fu added 3 commits June 10, 2018 16:40

feature: add syslog driver and add some fields in logger.Info

9e2bbf4

Signed-off-by: Wei Fu <[email protected]>

feature: allow user to use syslog log driver

26f3405

1. add option in containerio 2. add syslog config validation in container mgr 3. add syslog option during init containerio Signed-off-by: Wei Fu <[email protected]>

feature: limit the logs API which only can be used for jsonfile

cf0e2e7

Signed-off-by: Wei Fu <[email protected]>

fuweid changed the title ~~[WIP] feature: init syslog functionality in pouchd~~ feature: init syslog functionality in pouchd Jun 10, 2018

fuweid requested review from allencloud and YaoZengzeng June 10, 2018 10:18

allencloud reviewed Jun 12, 2018

View reviewed changes

YaoZengzeng reviewed Jun 12, 2018

View reviewed changes

refactor: change for the reviewer

5d9dc50

1. s/optionsForContainerio/logOptionsForContainerio/g 2. make the convertKVStringsToMap into pkg/utils 3. fix typo Signed-off-by: Wei Fu <[email protected]>

allencloud merged commit b077f48 into AliyunContainerService:master Jun 19, 2018

fuweid deleted the feature_syslog branch August 3, 2018 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: init syslog functionality in pouchd #1500

feature: init syslog functionality in pouchd #1500

fuweid commented Jun 9, 2018 •

edited

Loading

codecov-io commented Jun 9, 2018 •

edited

Loading

allencloud Jun 12, 2018

fuweid Jun 12, 2018

YaoZengzeng Jun 12, 2018

fuweid Jun 12, 2018

YaoZengzeng Jun 12, 2018

fuweid Jun 12, 2018

YaoZengzeng Jun 12, 2018

fuweid Jun 12, 2018 •

edited

Loading

YaoZengzeng Jun 12, 2018

fuweid Jun 12, 2018

YaoZengzeng Jun 12, 2018

YaoZengzeng commented Jun 19, 2018

feature: init syslog functionality in pouchd #1500

feature: init syslog functionality in pouchd #1500

Conversation

fuweid commented Jun 9, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

only driver

driver with tag

driver with tag and different format

Ⅴ. Special notes for reviews

codecov-io commented Jun 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaoZengzeng commented Jun 19, 2018

fuweid commented Jun 9, 2018 •

edited

Loading

codecov-io commented Jun 9, 2018 •

edited

Loading

fuweid Jun 12, 2018 •

edited

Loading