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: add systemd notify #1577

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

shaloulcy
Copy link
Contributor

For the case that systemctl start pouch, we need to make sure that the
pouchd has been started successfully. We cannot rely on the exit code
of systemctl only, we need to use NOTIFY_SOCKET to send the state to
the systemd.

Ⅰ. Describe what this PR did

For the case that systemctl start pouch, we need to make sure that the pouchd has been started successfully. We cannot rely on the exit code of systemctl only, we need to use NOTIFY_SOCKET to send the state to the systemd.

So when the pouchd has set up , it will send the state to systemd

Ⅱ. Does this pull request fix one issue?

fixes #1568

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Jun 22, 2018

Codecov Report

Merging #1577 into master will decrease coverage by 0.88%.
The diff coverage is 48.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
- Coverage   41.54%   40.65%   -0.89%     
==========================================
  Files         267      267              
  Lines       17375    17792     +417     
==========================================
+ Hits         7218     7233      +15     
- Misses       9264     9666     +402     
  Partials      893      893
Impacted Files Coverage Δ
cri/criservice.go 13.23% <15.38%> (-0.1%) ⬇️
apis/server/server.go 48.19% <50%> (+4.44%) ⬆️
daemon/daemon.go 57.38% <72.22%> (+1.13%) ⬆️
apis/server/exec_bridge.go 49.12% <0%> (-20.88%) ⬇️
daemon/mgr/container.go 39.77% <0%> (-10.85%) ⬇️
apis/server/container_bridge.go 73.04% <0%> (-9.7%) ⬇️
cli/update.go 0% <0%> (ø) ⬆️

For the case that systemctl start pouch, we need to make sure that the
pouchd has been started successfully. We cannot rely on the exit code
of systemctl only, we need to use NOTIFY_SOCKET to send the state to
the systemd.

Signed-off-by: Eric Li <[email protected]>
@shaloulcy shaloulcy requested a review from fuweid June 22, 2018 05:01
httpReady := <-httpReadyCh
criReady := <-criReadyCh

if httpReady && criReady {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cri manager may don't start if enable-cri=false is specified, is it ok in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok. If enable-cri=false is specified, we can read true from thehttpReadyCh

@pouchrobot
Copy link
Collaborator

ping @shaloulcy
We found that this PR is 44 commits, which is more than 10 commits, behind master.
Please rebase the branch against master and push it back again. Thanks a lot.

@fuweid
Copy link
Contributor

fuweid commented Jul 5, 2018

LGTM.

Since we don't have the adaptor to handle different servers, next step we can make it elegant.

@fuweid
Copy link
Contributor

fuweid commented Jul 5, 2018

But I need to confirm the version of dependency because it is different from https://github.com/alibaba/pouch/pull/1431/files

@fuweid fuweid changed the title feature: add systemd notify [WIP] feature: add systemd notify Jul 5, 2018
@fuweid fuweid changed the title [WIP] feature: add systemd notify feature: add systemd notify Jul 5, 2018
@fuweid
Copy link
Contributor

fuweid commented Jul 5, 2018

Confirm. We can merge it

@fuweid fuweid merged commit 26e93e1 into AliyunContainerService:master Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] add systemd notify functionality
5 participants