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 update daemon config function #1514

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

rudyfly
Copy link
Collaborator

@rudyfly rudyfly commented Jun 12, 2018

Ⅰ. Describe what this PR did

Currently, update daemon only supports with restful api, there is no cli command,
so need add it, in addition, it only supports update with pouchd is alive, so we need
update config file when pouchd is dead.
Pouch daemon config file is json format, it's hard to modify
with shell script, so add this function to change the config file easily.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

If pouchd is alive, it will change, daemon config if it supports,
and then change the config file. If pouchd is dead, it just update
the daemon config file. Currently, online update only support
image-proxy and label, more options will be supported at the
following few prs.

pouchd offline set:
You can specified the config file by set --config-file to update,
if pouchd is alive, you can set --offline=true to modify config file only,
more functions you can see by pouch updatedaemon --help

Ⅳ. Describe how to verify it

Online update

# pouch updatedaemon --image-proxy=127.0.0.1:5678

offline update

# pouch updatedaemon --offline=true --debug=true

Ⅴ. Special notes for reviews

Signed-off-by: Rudy Zhang [email protected]

@codecov-io
Copy link

codecov-io commented Jun 12, 2018

Codecov Report

Merging #1514 into master will decrease coverage by 0.1%.
The diff coverage is 11.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
- Coverage   41.25%   41.14%   -0.11%     
==========================================
  Files         255      257       +2     
  Lines       16837    16924      +87     
==========================================
+ Hits         6946     6964      +18     
- Misses       9024     9094      +70     
+ Partials      867      866       -1
Impacted Files Coverage Δ
client/client.go 66% <ø> (ø) ⬆️
cli/main.go 0% <0%> (ø) ⬆️
cli/update_daemon.go 0% <0%> (ø)
main.go 62.75% <100%> (+0.25%) ⬆️
network/mode/bridge/bridge.go 61.4% <100%> (+1.04%) ⬆️
client/daemon_update.go 100% <100%> (ø)
daemon/config/config.go 19.75% <16.66%> (+1.97%) ⬆️
apis/server/utils.go 59.52% <0%> (-4.77%) ⬇️
... and 3 more

"github.com/spf13/cobra"
)

// updateDescription is used to describe update command in detail and auto generate command doc.
Copy link
Contributor

Choose a reason for hiding this comment

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

%s/update/updatedaemon ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fine

@allencloud
Copy link
Collaborator

Could you tell us more about why we need this? @rudyfly

}

// write config to file
fd, err := os.OpenFile(udc.configFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
Copy link
Contributor

Choose a reason for hiding this comment

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

it will overwrite the origin config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, overwrite is right.

@rudyfly
Copy link
Collaborator Author

rudyfly commented Jun 13, 2018

@allencloud I have update the pr's description, please see Describe what this PR did

)

// updateDescription is used to describe updatedaemon command in detail and auto generate command doc.
var daemonUpdateDescription = "Update daemon's configurations, including debug level, image proxy, label, manager white list, " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

more doc needed here.

@Letty5411
Copy link
Contributor

Letty5411 commented Jun 13, 2018

Could add integration test in z_cli_daemon_test.go, for example verify online update label image proxy works.

@rudyfly rudyfly force-pushed the update-config branch 3 times, most recently from c97e20a to 92fbd46 Compare June 15, 2018 04:55
@rudyfly
Copy link
Collaborator Author

rudyfly commented Jun 15, 2018

@Letty5411 Add TestUpdateDaemonWithLabels in z_cli_daemon_test.go, PTAL


command.PouchRun("updatedaemon", "--label", "aaa=bbb").Assert(c, icmd.Success)

ret := command.PouchRun("info")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use: ret = RunWithSpecifiedDaemon(&cfg, "version")

Or it will send request to the default pouchd instead of the testing one.


defer cfg.KillDaemon()

command.PouchRun("updatedaemon", "--label", "aaa=bbb").Assert(c, icmd.Success)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use: ret = RunWithSpecifiedDaemon(&cfg, "updatedaemon", "--label", "aaa=bbb")

@Letty5411
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jun 15, 2018
Add update daemon config function. If pouchd is alive, it will change
daemon config if it supports, and then change the config file. If pouchd
is dead, it just update the daemon config file.

Pouchd dead:
You can specified the config file by set `--config-file` to update, more
functions you can see by `pouch updatedaemon --help`

Signed-off-by: Rudy Zhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants