Refactoring utility functions #1567
Conversation
luomiao
requested review from
venilnoronha,
msterin,
shuklanirdesh82 and
shivanshu21
luomiao
force-pushed
the
refactor
branch
2 times, most recently
from
c4575c8
to
1288e62
Compare
| @@ -86,7 +87,7 @@ func init() { | |||
| "conf_file": *configFile, | |||
| "using_conf_file_defaults": usingConfigFileDefaults, | |||
| }).Info("VMDK plugin tests started ") | |||
| log.SetFormatter(new(VmwareFormatter)) | |||
| log.SetFormatter(new(log_formatter.VmwareFormatter)) | |||
There was a problem hiding this comment.
We could remove this since it's already happening inside LogInit(..).
There was a problem hiding this comment.
Nice catch of this one. This looks like a duplicated call even before this PR. I will fix it by this chance.
| mountRoot = "/mnt/vmdk" // VMDK and photon volumes are mounted here | ||
| pluginSockDir = "/run/docker/plugins" // Unix sock for the plugin is maintained here | ||
| // Unix sock for the plugin is maintained here | ||
| pluginSockDir = "/run/docker/plugins" |
There was a problem hiding this comment.
nit: since we only have a single const, we could have it without the () block.
| // Load the configuration if one was provided. | ||
| c, err := config.Load(*configFile) | ||
| cfg, err := config.InitConfig(config.DefaultConfigPath, config.DefaultLogPath, | ||
| config.VSphereDriver, config.VSphereDriver) |
There was a problem hiding this comment.
Can we NOT pass any params here? InitConfig(..) should be able to pick these up easily, since they're also from within the config module.
We could also define DefaultDriver under config_linux.go and config_windows.go to provide a platform specific default driver name.
There was a problem hiding this comment.
We need to pass parameters when we add the new shared plugin.
For now we only have the vmdk plugin so it looks unnecessary.
But this is required when we have the new shared plugin in the following PRs.
It will become the following for vmdk plugin:
cfg, err := config.InitConfig(config.DefaultVMDKPluginConfigPath, config.DefaultVMDKPluginLogPath, config.VSphereDriver, config.VSphereDriver)
And for shared plugin:
cfg, err := config.InitConfig(config.DefaultSharedPluginConfigPath, config.DefaultSharedPluginLogPath, config.SharedDriver, config.SharedDriver)
There was a problem hiding this comment.
Okay. Then, should we use go build -tags ... in this case?
For example, we could create:
// config_shared_linux.go
// +build shared,linux
const (
DefaultConfigPath = ..
DefaultLogPath = ..
...
)
// config_vsphere_linux.go
// +build vsphere,linux
const (
DefaultConfigPath = ..
DefaultLogPath = ..
...
)
and similar ones for windows.
We could then let Go pick the correct set of consts during build time by invoking go build -tags shared ... and go build -tags vsphere .... This way, the config API will not have to deal with it's own defaults.
Thoughts?
There was a problem hiding this comment.
Let's keep it simple - we should understand what is going on by simply looking at the code. If I need to also parse the build system for figuring out what the code does, it becomes confusing.
| @@ -75,7 +76,7 @@ func init() { | |||
| flag.IntVar(¶llelVolumes, "parallel_volumes", 3, "Volumes per docker daemon for create/delete concurrent tests") | |||
| flag.IntVar(¶llelClones, "parallel_clones", 2, "Volumes per docker daemon for clone concurrent tests") | |||
| flag.Parse() | |||
| usingConfigFileDefaults := logInit(logLevel, logFile, configFile) | |||
| usingConfigFileDefaults := config.LogInit(logLevel, logFile, config.DefaultLogPath, configFile) | |||
There was a problem hiding this comment.
Similar to InitConfig(..), we could leave out the config.DefaultLogPath here.
There was a problem hiding this comment.
See my comments to the InitConfig.
This is similar to that one.
This is needed when we have the new shared plugin, which will have a different default log path.
luomiao
force-pushed
the
refactor
branch
2 times, most recently
from
d162681
to
0966910
Compare
client_plugin/utils/config/config.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| // PhotonDriver is a vmdk plugin driver for photon platform |
There was a problem hiding this comment.
nit: the existing comments kind of useless here. I'd say
// driver to handle single attach vmdk-based volumes in Photon Platform
...
// driver to handle single attach vmdk-based in vSphere/vCenter 6.0+
...
// driver to handle shared (multiple client) volumesThere was a problem hiding this comment.
It seems according to go check, it needs to be in the format of
// PhotonDriver is ....
PhotonDriver = "photon"
// VMDKDriver is ...
VMDKDriver = "vmdk"
I will update the following part accordingly.
There was a problem hiding this comment.
that's OK, my comment was more about content... and it's a nit anyways
client_plugin/utils/config/config.go
Outdated
| @@ -39,6 +56,8 @@ type Config struct { | |||
| Target string `json:",omitempty"` | |||
| Project string `json:",omitempty"` | |||
| Host string `json:",omitempty"` | |||
| Port int `json:",omitempty"` | |||
There was a problem hiding this comment.
please add a comment that this is superset and NOT all drivers need all fields
client_plugin/utils/config/config.go
Outdated
|
|
||
| // LogInit init log with passed logLevel (and get config from configFile if it's present) | ||
| // returns True if using defaults, False if using config file | ||
| func LogInit(logLevel *string, logFile *string, defaultLogFile string, configFile *string) bool { |
There was a problem hiding this comment.
generally passing multiple string params is error prone. IMO it's much better to define a struct with named params and pass &{name:value,...}.
| log.SetLevel(level) | ||
|
|
||
| if usingConfigDefaults { | ||
| log.Info("No config file found. Using defaults.") |
There was a problem hiding this comment.
nit: it would be nice to print (%v) the defaults
| } | ||
|
|
||
| // The windows plugin only supports the vsphere driver. | ||
| if runtime.GOOS == "windows" && c.Driver != defaultWindowsDriver { |
There was a problem hiding this comment.
I would made it a method in the driver, so you can call driver.check_config(). It's OK as is , but if/when we need to do another driver-specific decision here, I'd suggest we switch to driver methods
There was a problem hiding this comment.
Here do you mean the part from the check of
if runtime.GOOS == "windows" && c.Driver != defaultWindowsDriver?
I don't think this is a driver-specific code. This is the same for all drivers.
There was a problem hiding this comment.
you are right , this is different pivot - "does this platform support this driver?"
client_plugin/utils/config/config.go
Outdated
| "config": *configFile, | ||
| }).Info("Starting plugin ") | ||
|
|
||
| if c.Driver == PhotonDriver && err == nil { |
There was a problem hiding this comment.
ah, here is another decision. Is there a way to make these checks into a driver-specific method ?
There was a problem hiding this comment.
In the current code, drivers are initialized until a later point in main function:
if cfg.Driver == config.PhotonDriver {
driver = photon.NewVolumeDriver(cfg.Target, cfg.Project,
cfg.Host, config.MountRoot)
} else if cfg.Driver == config.VSphereDriver {
driver = vmdk.NewVolumeDriver(cfg.Port, cfg.UseMockEsx, config.MountRoot, cfg.Driver)
...
Is there a way to call into a driver-specific method before the driver is initialized?
Or is there a way that can avoid extra checks and avoid duplicating code at the same time?
There was a problem hiding this comment.
I think we can create driver instance earlier and then use the method on driver interface
There was a problem hiding this comment.
OK, let me add a switch for that and init the driver instance in main function and see how it looks like :)
|
PR is updated to avoid driver-specific checks in common path. |
Create and move new utility functions into utils folder:
main/log_formatter.go -> utils/log_formatter/log_formatter.go
main/main.go PluginServer main -> utils/plugin_server/plugin_server.go StartServer
main/main.go logInit main -> utils/config/config.go LogInit InitConfig
The new utility functions will be shared between different plugins.