Bug 1463278 - Leftover conf file in collectd
Leftover conf file in collectd
Status: CLOSED WONTFIX
Product: ovirt-engine-metrics
Classification: oVirt
Component: Generic (Show other bugs)
1.0.4.2
All All
unspecified Severity low
: ovirt-4.2.0
: ---
Assigned To: Shirly Radco
Lukas Svaty
:
Depends On:
Blocks: 1469119
  Show dependency treegraph
 
Reported: 2017-06-20 09:27 EDT by Lukas Svaty
Modified: 2017-08-23 04:55 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-07-18 04:45:01 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Metrics
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rule-engine: ovirt‑4.2+
lsvaty: testing_ack+


Attachments (Terms of Use)

  None (edit)
Description Lukas Svaty 2017-06-20 09:27:39 EDT
Description of problem:
Dont think we need /etc/collectd.d/postgresql.conf, thus can be removed as its obsolete by /etc/collectd.d/20-collectd_postgresql.conf 


Version-Release number of selected component (if applicable):
ovirt-engine-metrics-1.0.4.2-1.el7ev.noarch


How reproducible:
100%

Steps to Reproduce:
1. [root@ls-engine1 ~]# cat /etc/collectd.d/postgresql.conf 
LoadPlugin postgresql
2. [root@ls-engine1 ~]# cat /etc/collectd.d/20-collectd_postgresql.conf             
[root@ls-engine1 ~]# cat /etc/collectd.d/20-collectd_postgresql.conf 
LoadPlugin postgresql

<Plugin postgresql>
  <Query custom_deadlocks>
      Statement "SELECT deadlocks as num_deadlocks \
          FROM pg_stat_database \
          WHERE datname = $1;"

      Param database

      <Result>
          Type "pg_xact"
          InstancePrefix "num_deadlocks"
          ValuesFrom "num_deadlocks"
      </Result>
  </Query>

  <Database engine>
    Query custom_deadlocks
#    Query backends
#    Query transactions
#    Query queries
#    Query queries_by_table
#    Query query_plans
    Query table_states
#    Query query_plans_by_table
#    Query table_states_by_table
    Query disk_io
#    Query disk_io_by_table
    Query disk_usage
  </Database>
</Plugin>

Actual results:
Leftover from previous patches still included

Expected results:
/etc/collectd.d/postgresql.conf  should be removed
Comment 1 Shirly Radco 2017-06-20 09:35:45 EDT
Is the file there in new installation as well? or in upgrade only?
Comment 2 Lukas Svaty 2017-06-20 09:36:51 EDT
fresh installation, I dont think we would mind leftovers after upgrade as we are not cleaning preexisting conf files
Comment 3 Shirly Radco 2017-06-21 02:33:22 EDT
(In reply to Lukas Svaty from comment #2)
> fresh installation, I dont think we would mind leftovers after upgrade as we
> are not cleaning preexisting conf files

I don't see this file in the ovirt-engine-metrics repo.
We have 

/ansible/roles/ovirt_collectd/collectd_postgresql/templates/collectd_postgresql.conf.j2

that is deployed as /etc/collectd.d/20-collectd_postgresql.conf on the engine machine.

Both files should exists once we run the ansible script.

Can you please recheck this on a clean machine?
Comment 4 Lukas Svaty 2017-06-21 03:31:10 EDT
It seems to be added by package collectd-postgresql-5.7.1-4.el7.x86_64. Question is do we want to remove it on setting up our own conf file?
Comment 5 Shirly Radco 2017-06-21 04:31:00 EDT
I will add to this that collectd-virt plugin also adds libvirt.conf which we also don't need.

I would like to consider cleaning up all f the content of /etc/collectd.d/ before deploying the configuration files.

This will allow us to make sure there are no unneeded plugins that are loaded.

If a user would like to add additional plugins he should be instructed to include a separate directory in the main collectd.conf.

Same for fluentd.
Comment 6 Lukas Svaty 2017-06-21 04:35:52 EDT
In this case I would also add a note on top of Include "/etc/collectd.d"
in /etc/collectd.conf that this whole directory is under managment of ovirt and if desired to create new custom config directory
Comment 7 Yedidyah Bar David 2017-06-21 04:46:35 EDT
(In reply to Shirly Radco from comment #5)
> I will add to this that collectd-virt plugin also adds libvirt.conf which we
> also don't need.

But is this a problem?

> 
> I would like to consider cleaning up all f the content of /etc/collectd.d/
> before deploying the configuration files.

I'd rather not. This isn't common behavior.

> 
> This will allow us to make sure there are no unneeded plugins that are
> loaded.
> 
> If a user would like to add additional plugins he should be instructed to
> include a separate directory in the main collectd.conf.

And what if then the packagers of e.g. collectd-virt add their stuff there?
You are starting a virtual fight here. I do not think you want to.

> 
> Same for fluentd.
Comment 8 Shirly Radco 2017-07-18 03:56:11 EDT
Some collectd plugin packages add a file to config.d directory once they are installed, that loads the plugin.
This does not interfere, unless we decide not to use that plugin.
 
After discussion it was decided that we will not remove unneeded collectd packages and/or the files and will not touch the files created by the plugin.
This is to avoid deleting packages that were installed by the user.

In fresh installation the package will not be installed and will add documentation on what should be cleaned on upgrade.

Added a bug to collectd
https://github.com/collectd/collectd/issues/2368
Comment 9 Shirly Radco 2017-07-24 08:59:52 EDT
This seem to be a distributive packaging issue.
Who should we ask to change this behavior?
Comment 10 Sandro Bonazzola 2017-08-08 09:37:52 EDT
(In reply to Shirly Radco from comment #9)
> This seem to be a distributive packaging issue.

What does this mean?

> Who should we ask to change this behavior?

If you're asking who's packaging collectd for oVirt, it's me, mrunge@redhat.com and rmeggins@redhat.com for CentOS OpsTools SIG.
For Fedora Ruben Kerkhof <ruben@rubenkerkhof.com> is the maintainer.

But I've not understood what you want to change and why.
Comment 11 Shirly Radco 2017-08-09 04:45:02 EDT
(In reply to Sandro Bonazzola from comment #10)
> (In reply to Shirly Radco from comment #9)
> > This seem to be a distributive packaging issue.
> 
> What does this mean?

It means it is dependent on the packaging and not the project.
> 
> > Who should we ask to change this behavior?
> 
> If you're asking who's packaging collectd for oVirt, it's me,
> mrunge@redhat.com and rmeggins@redhat.com for CentOS OpsTools SIG.
> For Fedora Ruben Kerkhof <ruben@rubenkerkhof.com> is the maintainer.
> 
> But I've not understood what you want to change and why.

For some collectd plugins, when the package is installed, a load plugin file is added to /etc/collectd.d/.It load the file we no relation to our defined collectd configuration. So if we decide to disable the plugin in our conf file it will still be loaded by the pre-defined file.
Comment 12 Sandro Bonazzola 2017-08-23 04:50:08 EDT
(In reply to Shirly Radco from comment #11)
> (In reply to Sandro Bonazzola from comment #10)
> > (In reply to Shirly Radco from comment #9)
> > > This seem to be a distributive packaging issue.
> > 
> > What does this mean?
> 
> It means it is dependent on the packaging and not the project.
> > 
> > > Who should we ask to change this behavior?
> > 
> > If you're asking who's packaging collectd for oVirt, it's me,
> > mrunge@redhat.com and rmeggins@redhat.com for CentOS OpsTools SIG.
> > For Fedora Ruben Kerkhof <ruben@rubenkerkhof.com> is the maintainer.
> > 
> > But I've not understood what you want to change and why.
> 
> For some collectd plugins, when the package is installed, a load plugin file
> is added to /etc/collectd.d/.It load the file we no relation to our defined
> collectd configuration. So if we decide to disable the plugin in our conf
> file it will still be loaded by the pre-defined file.

I think having the default config enabling the plugin is expected behavior.
You should be able to override the default config with an higher priority config file. If it doesn't work, you'll have to file a bug on collectd to allow overriding of other conf files based on priority.
Comment 13 Yedidyah Bar David 2017-08-23 04:55:55 EDT
(In reply to Sandro Bonazzola from comment #12)
> I think having the default config enabling the plugin is expected behavior.
> You should be able to override the default config with an higher priority
> config file. If it doesn't work, you'll have to file a bug on collectd to
> allow overriding of other conf files based on priority.

There is a difference between "allowing overriding" (e.g. one file sets k=v1, a later file sets k=v2) and "unloading" a module. I do not think many services that allow loading DLLs at runtime/start time also allow "unloading" them.

I do not think this is a problem in itself. It might be a problem if there are settings that can only be set in the Load command and cannot be changed/overridden later. Not sure this is the case.

Note You need to log in before you can comment on or make changes to this bug.