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

Dont append to slices in mergeStruct #377

Merged
merged 1 commit into from
Nov 18, 2015
Merged

Dont append to slices in mergeStruct #377

merged 1 commit into from
Nov 18, 2015

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Nov 18, 2015

@gotyaoi, I believe this is a change that you made for supporting directories. Can you explain why slices get appended but all other data structures get replaced here? If there isn't a strong reason to keep it I'd rather it just have a consistent behavior for all data types

I'm having an issue now where we're getting all slices doubled from the config to structs

@gotyaoi
Copy link
Contributor

gotyaoi commented Nov 18, 2015

Since slices could hold more than one value, my thought was that specifying all values if a particular part of the config were re-specified at a lower level would be undesirable. I would have done the same for maps, but they don't appear to be produced by our toml library, according to it's docs.

For example, with the disk plugin, start with a restrictive list of mountpoints, then expand it in the config directory.

Regarding the doubling, that appears to have been introduced here: b10b186#diff-2fe34e1b6f7f9aece0af74b1635502eeL181

It takes the plugin struct from the config, then passes it to ApplyPlugin, which is causing the doubling. The previous way, where the agent first created an empty plugin and then passed that to ApplyPlugin was to allow for some defaults to be specified by the agent, but overriden if necessary. If that's not necessary, then the plugin value retrieved by PluginsDeclared should already be enough, though that would break down if we needed to do things in ApplyPlugin in the future.

@sparrc
Copy link
Contributor Author

sparrc commented Nov 18, 2015

@gotyaoi I'm not sure I agree with having the ability to append to lists at lower levels of the config, that seems unintuitive to me. Can you explain what exactly the use-case there is?

@gotyaoi
Copy link
Contributor

gotyaoi commented Nov 18, 2015

Well, to expand on the example of the disk plugin, in the main config, you would have this section:

[disk]
mountpoints = ["/"]

then in the conf directory, you would have:

[disk]
mountpoints = ["/data"]

Which would get merged into:

[disk]
mountpoints = ["/", "/data"]

When you're working with a config management system, it can be nice to have this, as you have some common config that gets distributed to all nodes in the the main config file, then individual nodes or classes of nodes get assigned customizations as necessary. It's not absolutely necessary though. I think I was mostly inspired by the configuration system in fail2ban, which can be fairly complicated, so perhaps I was overthinking it.

@sparrc
Copy link
Contributor Author

sparrc commented Nov 18, 2015

OK, it makes sense, I'll have to think it over some more. Do you know if this is the only thing configuration-wise that relies on this behavior?

@gotyaoi
Copy link
Contributor

gotyaoi commented Nov 18, 2015

I don't believe that anything besides the tests rely on the appending to arrays behavior.

@sparrc sparrc force-pushed the double-lists-bug branch 3 times, most recently from 5a4a850 to 3e3f015 Compare November 18, 2015 17:29
@sparrc sparrc changed the title [WIP] Dont append to slices in mergeStruct Dont append to slices in mergeStruct Nov 18, 2015
@sparrc
Copy link
Contributor Author

sparrc commented Nov 18, 2015

@gotyaoi I think I would prefer to just have all types overwrite in the config directories, to me this seems more intuitive, and I think it's fairly easy to workaround with a config management system. You just need to have your config management have

[disk]
mountpoints = ["/", "/data"]

rather than just "/data". I know that this introduces some duplication of effort, but I still think that being able to say that configurations within directories always overwrite is worth it.

The 2nd thing I did was remove the call to ApplyPlugin(). I'm not 100% sure I understand what was happening here but from what I could tell, we already applied the plugin configuration when we call LoadPlugin, and we can just get the plugin config directly. If you know of something I might be missing there please let me know!

@gotyaoi
Copy link
Contributor

gotyaoi commented Nov 18, 2015

Fair enough.

The reasoning behind the ApplyPlugin, I believe, is to apply the plugin struct held by the config onto the plugin struct held by the agent. This would allow the agent to perform some default configuration of the struct, but allow that default configuration to be overruled by the config. You can see it more clearly in the agent's call to ApplyAgent: https://github.com/influxdb/telegraf/blob/master/agent.go#L73. The NewAgent function creates a default Agent struct, then calls ApplyAgent to fill in the details from the user supplied config. That's why prior to b10b186 LoadOutputs and LoadPlugins created a default output and a default plugin respectively and then passed those to ApplyPlugin/ApplyOutput. No default configuration was being done though, I believe.

@sparrc sparrc merged commit 21c4e70 into master Nov 18, 2015
@sparrc sparrc deleted the double-lists-bug branch November 18, 2015 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants