Bug 887811
Summary: | a comprehensive glance-*.conf files should be included | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | Red Hat OpenStack | Reporter: | Nikola Dipanov <ndipanov> | ||||||||||||
Component: | openstack-glance | Assignee: | John Bresnahan <jbresnah> | ||||||||||||
Status: | CLOSED ERRATA | QA Contact: | Attila Fazekas <afazekas> | ||||||||||||
Severity: | medium | Docs Contact: | |||||||||||||
Priority: | high | ||||||||||||||
Version: | 1.0 (Essex) | CC: | abaron, dyocum, eglynn, jbresnah, markmc, pbrady | ||||||||||||
Target Milestone: | snapshot3 | Keywords: | Triaged | ||||||||||||
Target Release: | 2.1 | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Whiteboard: | |||||||||||||||
Fixed In Version: | openstack-glance-2012.2.3-1.el6ost | Doc Type: | Bug Fix | ||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||
Clone Of: | 887334 | Environment: | |||||||||||||
Last Closed: | 2013-03-05 18:30:04 UTC | Type: | Bug | ||||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||||
Documentation: | --- | CRM: | |||||||||||||
Verified Versions: | Category: | --- | |||||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||||
Embargoed: | |||||||||||||||
Attachments: |
|
Description
Nikola Dipanov
2012-12-17 11:58:45 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. 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 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 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. 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" 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.
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 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.
Created attachment 693472 [details]
Added simple mechanism for removing unsupported config.
Updated patch for review.
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.
Created attachment 695136 [details]
Correct version this time
John will include this for snap3 Comment on attachment 695136 [details]
Correct version this time
Well need to verify the non daemon binaries work
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. 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/... 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 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). 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. I applied this patch and pushed it for snapshot three on 2/11/13. I used the patch referenced in comment 14. 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. 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 |