Now fully compatible with Docker

[obourdon]

You can now run this plugin within a Docker container

Note however that the container must have the privileged mode activated and /proc and /dev of host must be mounted as volumes in the container.

Example:

docker run --privileged -t -i -v /dev:/tmp/dev1 -v /proc:/tmp/proc1 
       -v $GOPATH/src/github.com/intelsdi-x/snap/build:/tmp/snap
       -v ~/SNAP-PLUGINS-4-MCP:/tmp/snap-plugins
        debian:jessie /bin/bash

[lmroz]

For functions that are callbacks/handlers (potentially executed in separate goroutines) it's safer to execute code modifying non-local variables in a mutex. Good pattern for such initialization function is to use initialized bool field of the object and when it's true skip init, in other case it should proceed regularly and make this field true at the end.

[lmroz]

I've added single comment about thread safety, but besides that everyting LGTM.
Great contribution 👍

[obourdon]

@lmroz thanks for catching this potential issue. I have added your suggestion in my latest commit "copying" it from cpu collector code

[lmroz]

@obourdon It's not exactly the solution I'd like to see (operations on regular variables are not atomic, unlikely in case of bool but it can actually hapen).

Here is example for other plugin that shows solution I'd expect:

type NovaPlugin struct {
    initialized      bool
    initializedMutex *sync.Mutex

    -------CUT-------
}

func (self *NovaPlugin) GetMetricTypes(cfg plugin.ConfigType) ([]plugin.MetricType, error) {
    err := self.init(cfg)

    if err != nil {
        return nil, err
    }
-------CUT-------
}

func (self *NovaPlugin) init(cfg interface{}) error {
    self.initializedMutex.Lock()
    defer self.initializedMutex.Unlock()

    if self.initialized {
        return nil
    }

    err := ReadConfig(cfg, &self.config)

    if err != nil {
        return fmt.Errorf("plugin initalization failed : [%v]", err)
    }

    -------CUT-------

    self.initialized = true

    return nil

}

func New() *NovaPlugin {
    self := new(NovaPlugin)
    self.initializedMutex = new(sync.Mutex)
    return self
}

It comes from nova plugin https://github.com/intelsdi-x/snap-plugin-collector-nova/blob/master/novaplugin/plugin.go

[jcooklin]

@lmroz, the emon plugin is private with no plans to open it. That said the
snippet you shared is a good one.

On Wed, Aug 17, 2016 at 7:37 AM Łukasz Mróz notifications@github.com
wrote:

@obourdon https://github.com/obourdon It's not exactly the solution I'd
like to see (operations on regular variables are not atomic, unlikely in
case of bool but it can actually hapen).

Here is example for other plugin that shows solution I'd expect:

// EmonPlugin implements plugin.Plugin interface
// use New() to instantiate (uses emon.Emon as backend)
type EmonPlugin struct {
instance emon.Emon

initialized bool
initMutex   *sync.Mutex

}

// init ensures that emon backend is initialized only once
// reads config from PluginMetricType or PluginConfigType
// if initialization failed panic is raised
func (self *EmonPlugin) init(cfg interface{}) {
self.initMutex.Lock()
defer self.initMutex.Unlock()

if self.initialized {
    return
}

cfgItems, err := config.GetConfigItems(cfg, []string{emon_path, emon_metrics_path, emon_counters_path, emon_loops, emon_duration})

if err != nil {
    panic(fmt.Errorf("plugin initalization failed : [%v]", err))
}

err = self.instance.Init(
    cfgItems[emon_path].(string),
    cfgItems[emon_metrics_path].(string),
    cfgItems[emon_counters_path].(string),
    cfgItems[emon_loops].(string),
    cfgItems[emon_duration].(string),
)

if err != nil {
    panic(fmt.Errorf("plugin initalization failed : [%v]", err))
}

metrics.CONSTANTS = self.instance.SystemConstants()

self.initialized = true

}

It comes from emon plugin
https://github.com/intelsdi-x/snap-plugin-collector-emon/blob/master/emon_plugin/plugin.go#L80

• this fragment itself is not 100% correct because it panics instead of
  returning error, but shows how to use mutex in case of initialization.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#18 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0q-DLCrEDgTt8rJeWvn5DuXN-ew_Y_ks5qgxy6gaJpZM4JXN73
.

[obourdon]

@lmroz many thanks for making this very precise. The question I now have is: "should this code be also implemented in all other plugins where the same kind of code is already implemented (CPU) as well as other pending PRs ???"

Thanks for your comments

[lmroz]

@jcooklin I realized it just after posting so changed snippet to another plugin (I hope it's not disclosing much) :/

@obourdon I think that code should be present in any plugin that does some kind of lazy initialization in callbacks/handlers (the only way possible to do initalization based on config).

[obourdon]

@lmroz thanks for the detailed snippet, very clear. Could you please review the new submission to see if it fits your expectations ?

[lmroz]

You should have that check done also under Lock. Having lock just during initialization prevents data corruption during initialization (like concurrent write to complex data structure like map), but does not prevent double initialization (that might lead to incorrect initialization, but hopefully not in this case). This line might be kept if also added in setProcDevPath. Then you'll have double checked lock pattern (https://en.wikipedia.org/wiki/Double-checked_locking ). Sorry for being that strict, but:

1. Even if now it's not causing problem it might do in future
2. Snippets from already existing plugins are copied to new ones, so we should use best known practice for such cases

[obourdon]

@lmroz no pb at all, fully agree that it is better to do it right at the soonest.
Current modification on its way for new review

[obourdon]

@lmroz I hope the latest code is more in sync with your expectations. The boolean check is now located within the lock code in accordance to your example here