feature: deamon support --log-driver and --log-opt options #1647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PTAL @fuweid |
fuweid
reviewed
Jul 6, 2018
main.go
Outdated
| @@ -105,6 +107,10 @@ func setupFlags(cmd *cobra.Command) { | |||
| flagSet.BoolVar(&cfg.NetworkConfig.BridgeConfig.IPForward, "ipforward", true, "Enable ipforward") | |||
| flagSet.BoolVar(&cfg.NetworkConfig.BridgeConfig.UserlandProxy, "userland-proxy", false, "Enable userland proxy") | |||
|
|
|||
| // log config | |||
| flagSet.StringVar(&cfg.DefaultLogConfig.LogDriver, "log-driver", "", "Set default log driver") | |||
There was a problem hiding this comment.
I think we can set the log-driver to jsonfile, like mody does. WDTY?
There was a problem hiding this comment.
yep. If log-driver is empty, CI will fail.
main.go
Outdated
| @@ -19,6 +19,7 @@ import ( | |||
| "github.com/alibaba/pouch/storage/quota" | |||
| "github.com/alibaba/pouch/version" | |||
|
|
|||
| "github.com/alibaba/pouch/apis/opts" | |||
There was a problem hiding this comment.
please put this line to previous group.
fuweid
reviewed
Jul 6, 2018
daemon/mgr/container.go
Outdated
| LogDriver: types.LogConfigLogDriverJSONFile, | ||
| } | ||
| defaultConfig := mgr.Config.DefaultLogConfig | ||
| config.HostConfig.LogConfig = &defaultConfig |
There was a problem hiding this comment.
The request maybe come from swarm. If it does, the config.HostConfig.LogConfig is not nil but the value is empty. For this case, the config.HostConfig.LogConfig.LogDriver is empty and config.HostConfig.LogConfig. LogOpts is nil.
It isn't good practice in API design. However, we need to compatible with legacy system. Add other check that config.HostConfig.LogConfig is empty or not. WDTY?
fuweid
reviewed
Jul 6, 2018
daemon/mgr/container.go
Outdated
| } | ||
| // note: request from swarm is not standard. config.HostConfig.LogConfig is not nil, but its value is empty. | ||
| // We need to be compatible with this legacy system. | ||
| if config.HostConfig.LogConfig == nil || config.HostConfig.LogConfig.LogDriver == "" { |
There was a problem hiding this comment.
Could we use helper function to check the config.HostConfig.LogConfig is empty or not?
Like,
func isEmptyLogConfig(xx *types.LogConfig) bool {
if config.HostConfig.LogConfig == nil {
return true
}
return config.HostConfig.LogConfig.LogDriver == "" && len(config.HostConfig.LogConfig.LogOpts) == 0
}
WDTY?
main.go
Outdated
| @@ -105,6 +107,10 @@ func setupFlags(cmd *cobra.Command) { | |||
| flagSet.BoolVar(&cfg.NetworkConfig.BridgeConfig.IPForward, "ipforward", true, "Enable ipforward") | |||
| flagSet.BoolVar(&cfg.NetworkConfig.BridgeConfig.UserlandProxy, "userland-proxy", false, "Enable userland proxy") | |||
|
|
|||
| // log config | |||
| flagSet.StringVar(&cfg.DefaultLogConfig.LogDriver, "log-driver", "jsonfile", "Set default log driver") | |||
There was a problem hiding this comment.
s/"jsonfile"/types.LogConfigLogDriverJSONFile 😄
zhuangqh
force-pushed
the
feature_log_driver_option
branch
from
8ef0499 to
5670b9c
Compare
zhuangqh
force-pushed
the
feature_log_driver_option
branch
from
5670b9c to
72d3dc2
Compare
Codecov Report
@@ Coverage Diff @@
## master #1647 +/- ##
===========================================
- Coverage 41.45% 18.12% -23.34%
===========================================
Files 273 226 -47
Lines 17714 15033 -2681
===========================================
- Hits 7344 2725 -4619
- Misses 9461 12127 +2666
+ Partials 909 181 -728
|
zhuangqh
force-pushed
the
feature_log_driver_option
branch
from
72d3dc2 to
6839d63
Compare
fuweid
reviewed
Jul 9, 2018
daemon/mgr/container.go
Outdated
| @@ -414,6 +410,23 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty | |||
| }, nil | |||
| } | |||
|
|
|||
| // set manager log config to the container's log config if the container's log config is not specified. | |||
| func (mgr *ContainerManager) setDefaultLogConfig(logConfig *types.LogConfig) { | |||
There was a problem hiding this comment.
s/setDefaultLogConfig/setDefaultLogConfigIfMissing/g
daemon/mgr/container.go
Outdated
| @@ -414,6 +410,23 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty | |||
| }, nil | |||
| } | |||
|
|
|||
| // set manager log config to the container's log config if the container's log config is not specified. | |||
| func (mgr *ContainerManager) setDefaultLogConfig(logConfig *types.LogConfig) { | |||
| if logConfig == nil { | |||
There was a problem hiding this comment.
I don't think we can set the value to the config, since it is not pointer to pointer
zhuangqh
force-pushed
the
feature_log_driver_option
branch
3 times, most recently
from
ac009f4 to
ed72940
Compare
Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
zhuangqh
force-pushed
the
feature_log_driver_option
branch
from
ed72940 to
725825b
Compare
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: zhuangqh zhuangqhc@gmail.com
Ⅰ. Describe what this PR did
pouchd could start with --log-driver and --log-opt as the default configuration of log driver.
Ⅱ. Does this pull request fix one issue?
fixes #1646
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Due to the defect of go-swagger v0.12, the validation of these options are temporarily noneffective.