Skip to content
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

feature: lazy connect to syslog server #2068

Merged
merged 1 commit into from
Aug 10, 2018
Merged

feature: lazy connect to syslog server #2068

merged 1 commit into from
Aug 10, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Aug 9, 2018

Ⅰ. Describe what this PR did

The syslog driver will try to connect syslog server. However, the syslog
server might not start. We cannot guarantee the syslog server must
start before container.

For this case, we can postpone the connection initialize. syslog driver
only does validation during initialize. When caller try to write data,
it will connect to server.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Reimplement the syslog functionality to allow lazy connect

Ⅳ. Describe how to verify it

start a container

sudo pouch run --log-driver syslog --log-opt syslog-address=tcp://127.0.0.1:515 busybox sh -c "while true; do date; sleep 1; done"

wait for it and see the output on the shell.

$ sudo nc -l -p 515
<30>Aug  9 16:35:44 6abb5861fe82[18670]: Thu Aug  9 08:35:44 UTC 2018
<30>Aug  9 16:35:45 6abb5861fe82[18670]: Thu Aug  9 08:35:45 UTC 2018
<30>Aug  9 16:35:46 6abb5861fe82[18670]: Thu Aug  9 08:35:46 UTC 2018
<30>Aug  9 16:35:47 6abb5861fe82[18670]: Thu Aug  9 08:35:47 UTC 2018

open the nc to receive the message.

Ⅴ. Special notes for reviews

The syslog driver will try to connect syslog server. However, the syslog
server might not start. We cannot guarantee the syslog server must
start before container.

For this case, we can postpone the connection initialize. syslog driver
only does validation during initialize. When caller try to write data,
it will connect to server.

Signed-off-by: Wei Fu <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #2068 into master will increase coverage by 0.33%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2068      +/-   ##
==========================================
+ Coverage   63.93%   64.26%   +0.33%     
==========================================
  Files         206      208       +2     
  Lines       15762    15860      +98     
==========================================
+ Hits        10077    10192     +115     
+ Misses       4435     4406      -29     
- Partials     1250     1262      +12
Flag Coverage Δ
#criv1alpha1test 33.06% <0%> (-0.21%) ⬇️
#criv1alpha2test 33.69% <0%> (-0.27%) ⬇️
#integrationtest 38.54% <51.78%> (+0.65%) ⬆️
#unittest 24.13% <64.28%> (+0.36%) ⬆️
Impacted Files Coverage Δ
daemon/logger/syslog/conn.go 30% <30%> (ø)
daemon/logger/syslog/dialer.go 68.57% <68.57%> (ø)
daemon/logger/syslog/syslog.go 74.71% <87.71%> (+49.71%) ⬆️
ctrd/image.go 79.31% <0%> (-1.98%) ⬇️
daemon/mgr/container.go 55.31% <0%> (-0.22%) ⬇️
daemon/mgr/container_validation.go 33.04% <0%> (+2.6%) ⬆️
daemon/logger/syslog/validate.go 55.78% <0%> (+3.15%) ⬆️
daemon/mgr/container_logger.go 92.3% <0%> (+7.69%) ⬆️
... and 3 more

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 10, 2018
@allencloud allencloud merged commit 79eacbb into AliyunContainerService:master Aug 10, 2018
@fuweid fuweid deleted the feature_auto_connect_to_host_in_syslog_driver branch August 31, 2018 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/log kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants