Bug 887811 - a comprehensive glance-*.conf files should be included
Summary: a comprehensive glance-*.conf files should be included
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat OpenStack
Classification: Red Hat
Component: openstack-glance
Version: 1.0 (Essex)
Hardware: Unspecified
OS: Unspecified
high
medium
Target Milestone: snapshot3
: 2.1
Assignee: John Bresnahan
QA Contact: Attila Fazekas
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-12-17 11:58 UTC by Nikola Dipanov
Modified: 2016-04-27 01:23 UTC (History)
6 users (show)

Fixed In Version: openstack-glance-2012.2.3-1.el6ost
Doc Type: Bug Fix
Doc Text:
Clone Of: 887334
Environment:
Last Closed: 2013-03-05 18:30:04 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
Initial version of opensatck-glance packaging changes for review (8.93 KB, patch)
2013-02-01 17:40 UTC, Eoghan Glynn
pbrady: review+
Details | Diff
Reworked to limit distribution config to non-default values, with comment'd user-editable config under /etc/glance. (13.49 KB, patch)
2013-02-04 18:37 UTC, Eoghan Glynn
pbrady: review+
Details | Diff
Added simple mechanism for removing unsupported config. (13.78 KB, patch)
2013-02-05 16:07 UTC, Eoghan Glynn
no flags Details | Diff
Proposed final version of patch (19.36 KB, patch)
2013-02-08 15:35 UTC, Eoghan Glynn
no flags Details | Diff
Correct version this time (19.36 KB, patch)
2013-02-08 15:40 UTC, Eoghan Glynn
pbrady: review+
Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2013:0593 0 normal SHIPPED_LIVE Red Hat OpenStack 2.0 (Folsom) Preview bug fix and enhancement update 2013-03-05 23:28:55 UTC

Description Nikola Dipanov 2012-12-17 11:58:45 UTC
+++ This bug was initially created as a clone of Bug #887334 +++

Description of problem:

A .conf file with a comprehensive set of configuration options should be shipped.  Here is a good example to work from:

https://github.com/yocum137/nova.conf/blob/master/nova.conf

--- Additional comment from Dan Yocum on 2012-12-14 12:22:34 EST ---

This one looks like it's more up-to-date:

https://github.com/openstack/nova/blob/master/etc/nova/nova.conf.sample

Comment 2 Alan Pevec 2012-12-18 14:07:31 UTC
Unlike Nova, Glance RPM is not shipping own copies of glance-*.conf, it edits 
https://github.com/openstack/glance/blob/master/etc/glance-api.conf and
https://github.com/openstack/glance/blob/master/etc/glance-registry.conf using openstack-config --set so doc comments are preserved.

Comment 3 Eoghan Glynn 2012-12-20 16:56:29 UTC
We need to ensure that the openstack-glance RPM spec file tweaks the glance-{api|registry} config as necessary, sets sane defaults for RHOS where appropriate, and comments outs (as opposed to removing) any config options that we explicitly don't support.

We already have a pattern for this:

  https://github.com/fedora-openstack/openstack-glance/blob/master/openstack-glance.spec#L111

Comment 4 Mark McLoughlin 2013-01-16 16:49:27 UTC
See bug #887804 for what we're doing with Cinder. Glance is a little different, but the same ideas apply:

  1) Take any non-default values from our /etc/glance/glance-*.conf files and
     move those values into /usr/share/glance/glance-*.conf

     The systemd units and init scripts will need to be updated to do:

       --config-file=/usr/share/glance/glance-api-dist.conf --config-file=/etc/glance/glance-api.conf

     This means that users can start with an empty /etc/glance/glance-*.conf
     file and still be using the distribution defaults

     (Note: this shouldn't affect where the -paste.ini files are located given
      the logic in _get_paste_config_path(), but that's only because it looks
      at CONF.config_file[-1] to figure out it needs glance-api.conf. If it
      had looked at config_file[0] it would start looking for
      glance-api-dist.ini instead)

     This file should not be marked as %config(noreplace) since it is not
     intended to be user-editable

     One issue specific to glance is that the default config files contain
     values which aren't "distribution defaults" and are different from the
     defaults specified in the code. These values also need to go into the
     -dist.conf files. To avoid changing the effective defaults, we need

       verbose = True
       log_file = /var/log/glance/api.log
       filesystem_store_datadir = /var/lib/glance/images/
       scrubber_datadir = /var/lib/glance/scrubber
       image_cache_dir = /var/lib/glance/image-cache/

     also these differ from the code defaults, but it's unclear why:

       qpid_heartbeat = 5
       scrub_time = 43200

     sql_connection is the really obvious distribution default

2-4) Upstream's default config files are a fine start for /etc/glance/
     but we should comment out the values and make sure they match the
     effective defaults.

     This means the default user-editable config file contains nothing but
     comments explaining what options are available

  5) In the productized packages, we should strip the sample glance*.conf
     files to only include options we recommend using and perhaps improve the 
     descriptions.

  6) In our default /etc/glance/glance-*.conf we should also document the 
     [keystone_authtoken] section, but the keystone_authtoken defaults should
     live in /usr/share/glance/glance-*-dist.conf

Comment 5 Dan Yocum 2013-01-16 19:55:39 UTC
wrt qpid_heartbeat, see: https://bugs.launchpad.net/openstack-common/+bug/1050661

Derek and I are both using 60s on our clouds with good success.

Comment 6 Mark McLoughlin 2013-01-17 22:26:04 UTC
From apevec - "One thing to verify is Nova selinux policy: /etc/nova/nova.conf is etc_t while files under /usr/share/nova/ get usr_t"

Comment 7 Eoghan Glynn 2013-02-01 17:40:49 UTC
Created attachment 691642 [details]
Initial version of opensatck-glance packaging changes for review

This is a work-in-progress, but I'd like to get a snaity check on the approach adopted.

Comment 8 Eoghan Glynn 2013-02-04 18:37:48 UTC
Created attachment 692949 [details]
Reworked to limit distribution config to non-default values, with comment'd user-editable config under /etc/glance.

Updated patch for review.

Comment 9 Pádraig Brady 2013-02-05 12:00:01 UTC
Comment on attachment 692949 [details]
Reworked to limit distribution config to non-default values, with comment'd user-editable config under /etc/glance.

looks good. Thanks Eoghan.

Comment 10 Eoghan Glynn 2013-02-05 16:07:23 UTC
Created attachment 693472 [details]
Added simple mechanism for removing unsupported config.

Updated patch for review.

Comment 11 Eoghan Glynn 2013-02-08 15:35:53 UTC
Created attachment 695133 [details]
Proposed final version of patch

Patch now includes init/upstart files for glance scrubber and support for the glance cache utils, all picking up the distribution config first.

Comment 12 Eoghan Glynn 2013-02-08 15:40:56 UTC
Created attachment 695136 [details]
Correct version this time

Comment 13 Pádraig Brady 2013-02-08 18:34:36 UTC
John will include this for snap3

Comment 14 Pádraig Brady 2013-02-08 18:35:18 UTC
Comment on attachment 695136 [details]
Correct version this time

Well need to verify the non daemon binaries work

Comment 15 Pádraig Brady 2013-02-09 05:38:49 UTC
I've a query about these. Users will want to set these
so should they be in dist.conf?

openstack-config --set %{buildroot}%{_datadir}/glance/glance-$svc-dist.conf keystone_authtoken admin_tenant_name %%SERVICE_TENANT_NAME%%
openstack-config --set %{buildroot}%{_datadir}/glance/glance-$svc-dist.conf keystone_authtoken admin_user %SERVICE_USER%
openstack-config --set %{buildroot}%{_datadir}/glance/glance-$svc-dist.conf keystone_authtoken admin_password %SERVICE_PASSWORD%

Also bug #876763 is currently discussing documentation
for changing these variables, but referencing the orig locations.

Comment 16 Pádraig Brady 2013-02-10 03:58:27 UTC
To clarify my query in comment 15, there probably isn't an issue
putting these authtoken defaults in the -dist.conf but it
may be redundant given these are just the upstream defaults?
I also notice the double %% in %%SERVICE_TENANT_NAME%% which is inconsistent.

My query re docs referencing the orig locations was bogus,
as that is installation config as opposed to dist config,
and so should reference /etc/...

Comment 17 Pádraig Brady 2013-02-10 04:00:51 UTC
I notice the patch hacks come cache command line tools in the spec file.
That would be better as an explicit patch I think, but for better consistency with nova and cinder etc. I suggest to just use:
https://bugzilla.redhat.com/attachment.cgi?id=695652

Comment 18 Eoghan Glynn 2013-02-11 11:00:57 UTC
Thanks for the review, Pádraig.

WRT the keystone_authtoken settings, this was all pre-existing logic in the spec file that I simply moved around. I had assumed that the double "%%" was a deliberate choice to support two-stage substitution or some-such logic.

But if the double "%%" are now considered extraneous, I can easily remove those, and replace the calls to "openstack-config --set" by simply including these settings in the *-dist.conf files from the get-go.

Note the fact that these options are similarly set in the upstream glance-{api|registry}.conf files does not help us here, as the *-dist.conf files are created independently of those upstream versions.

Also, any authtoken settings that our tools (e.g. packstack) produce as a side effect of installation should go in the *-dict.conf files IMO, as the idea behind distribution config was IIUC to allow the services to still run successfully even if the user does something destructive like:

  $ rm -f /etc/glance/glance-*.conf 

WRT to the cache command-line tools, the idea of hacking the sys.argv was to minimize the scope of change to only the cache utils, as the api/registry/scrubber services all have the dist distribution config already identified to them via a different mechanism. The approach in https://bugzilla.redhat.com/attachment.cgi?id=695652 would be needlessly impactful in this case.

(Though of course if you object to the use of sed to make the change to the cache scripts, I could convert this this to a patch that we carry downstream only).

Comment 19 Eoghan Glynn 2013-02-12 10:05:57 UTC
One further datapoint on the sed versus patch approaches for adding the dist config directory to the search path for glance-cache config.

The sed-based approach would allow us to parameterize the directory paths, as opposed to hard-coding, e.g.:

 if (not("--config-file" in sys.argv or "--config-dir" in sys.argv)):\
     sys.argv.append("--config-file")\
-    sys.argv.append("/usr/share/glance/glance-cache-dist.conf")\
+    sys.argv.append("%{_datadir}/glance-cache-dist.conf")\
     sys.argv.append("--config-file")\
-    sys.argv.append("/etc/glance/glance-cache.conf")' bin/glance-cache-$role
+    sys.argv.append("%{_sysconfdir}/glance/glance-cache.conf")' bin/glance-cache-$role

Given that we're parameterizing these dirs everywhere in the spec file, it would seem unfortunate to hardcode in a patch.

Comment 20 John Bresnahan 2013-02-12 17:14:48 UTC
I applied this patch and pushed it for snapshot three on 2/11/13.  I used the patch referenced in comment 14.

Comment 22 Pádraig Brady 2013-02-27 22:00:11 UTC
There is still a minor issue with the config files in /etc/glance
I notice that the default values in glance-registry.conf and glance-api.conf
are not set. Specifically I noticed the sql_connection setting is
left at the upstream default rather than the distribution default.

Implications are that it's a bit confusing for users.
Also it will mean that openstack-db --init will not update
the password and so will only work with a default password.

The correct defaults can be maintained manually,
or preferably, updated by a script in the spec file
based on the -dist.conf contents.

Comment 24 errata-xmlrpc 2013-03-05 18:30:04 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2013-0593.html


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