Support compose global deploy mode to daemonset #999
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
What if the user use |
xianlubird
force-pushed
the
features/global-mode
branch
from
b33220c to
30c5ddb
Compare
|
@hangyan Nice catch ! Already change code, thanks. |
| opt.CreateD = false | ||
| opt.CreateDS = true | ||
| if opt.Controller != "" && opt.Controller != "daemonset" { | ||
| log.Fatal("Global deploy mode can only convert to daemonset") |
There was a problem hiding this comment.
I think fatal log is not a good idea. May be we should try to let user proceed anyway, wether use deployment( ignore deploy mode) or use ds(ignore flags). I think there should be some existing example to consult in the code.
There was a problem hiding this comment.
Thank you for review code on weekend. Already change code to use daemonset controller as default when global mode get in. @hangyan
xianlubird
force-pushed
the
features/global-mode
branch
from
30c5ddb to
3256551
Compare
| replica = 1 | ||
| opt.CreateD = false | ||
| opt.CreateDS = true | ||
| opt.Controller = "daemonset" |
There was a problem hiding this comment.
At warn message is needed if the user specific a --deployment flag.Also in my opinion, it's better to use deployment if the user provide a --deployment flag. Because if we ignore it, we can only create a daemonset if deployMode==global, there is no way to overwrite it. But on the other side, we can create daemonset(by default) and deployment(user provide deployment flag), I think this will give the user more choices.
There was a problem hiding this comment.
Sure, agree this opinion. Change code to that if user use --deployment flag, it will convert to Deployment and default to use StatefulSet. Also add test case to this situation. @hangyan
xianlubird
force-pushed
the
features/global-mode
branch
from
3256551 to
38984b3
Compare
| replica = 1 | ||
| if service.DeployMode == "global" && opt.Controller == "" { | ||
| opt.CreateD = false | ||
| opt.CreateDS = true |
There was a problem hiding this comment.
If the user specify --deployment flag and DeploMode==global, a warn message is needed
There was a problem hiding this comment.
@hangyan Done. Already add warn message
xianlubird
force-pushed
the
features/global-mode
branch
from
38984b3 to
84b419c
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Support compose global deploy mode to daemonset
@cdrage @hangyan Pls help to review if you are free. Thanks.