glance-manage db_export_metadefs should always require a directory option

[Travis Tripp]

Currently if you call glance-manage db_export_metadefs without any additional options, it will export all current metadefs into the glance conf dir (e.g. /etc/glance/metadefs/) without any warning. If the files already exist it will overwrite them. This is a potentially very dangerous operation that would delete all the existing files in that directory without warning and with no way of getting them back. A new user may just call it hoping to see the parameters and instead end up having all their files overwritten whether they want to or not.

I think that db_export_metadefs should always require a parameter of the output directory to save them to. Either that or export them to a safer location by default. Would like additional input from Glance team on this one.

Example:

$ glance-manage db_export_metadefs

2014-09-08 16:11:15.865 5029 DEBUG oslo.db.sqlalchemy.session [-] MySQL server mode set to STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION _init_events /usr/local/lib/python2.7/dist-packages/oslo/db/sqlalchemy/session.py:461

2014-09-08 16:11:15.900 5029 INFO glance.db.sqlalchemy.metadata [-] Namespace RandomNumberGenerator saved in /etc/glance/metadefs/RandomNumberGenerator.json

2014-09-08 16:11:15.910 5029 INFO glance.db.sqlalchemy.metadata [-] Namespace VirtualCPUTopology saved in /etc/glance/metadefs/VirtualCPUTopology.json

etc...

[Pawel Koniszewski (pawel-koniszewski)]

What about saving files to /tmp/metadefs instead of /etc/glance/metadefs? I would like to keep this parameter optional, but if we can't I'm fine with making it required.

[Bartosz Fic (bartosz-fic)]

I think it is good idea to save files in the temporary directory because default files in /etc/glance/metadefs cannot be overwritten. But we still have similar problem: after saving files in the temporary directory, all another exports will overwrite these files.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/124405

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/125034

[OpenStack Infra (hudson-openstack)]

Change abandoned by dominik dobruchowski (<email address hidden>) on branch: master
Review: https://review.openstack.org/125034
Reason: Fix improved.

[Ian Cordasco (icordasc)]

Is this really the best way forward? Would it not be better to transition to this being a required argument by issuing a warning when the files already exist and indicating they should pass a path to a directory in which to store the files?

[Pawel Koniszewski (pawel-koniszewski)]

Ian,

I have some worries about this solution:
1. In glance-manage shell we have no idea what is in database so we can't issue a warning here (why issue a warning when maybe not even one file will be overwritten?).
2. It is possible to issue a warning during export: get all namespaces from database and check provided directory. If at least one namespace matches then issue a warning and ask for directory. And ask for directory until there isn't any collision. But what if I want to overwrite these files? We would need to add another optional parameter, e.g. --overwrite to allow overwritting, or just overwrite if user didn't provide another directory (this is dirty).

I can see two more approaches:
1. Stick to optional parameter but at the beginning of export generate UUID and make it a prefix/suffix of each namespace. But again updating existing files will be harder.
2. Extend current solution - issue a warning if there are files inside directory (y - continue and overwrite, n - break the operation).

[Ian Cordasco (icordasc)]

Pawel,

There should be no interactivity after issuing a warning. I expect people are using this as part of cron jobs as well as manually. We /could/ detect if it's running from a user's shell but I would much rather avoid all of this complexity. To put it simply, we should warn users about the destructive action they're performing and then continue and overwrite it. I'm also not entirely convinced that this requires anything more than a warning about existing files that may be overwritten and better documentation, especially given that this would be a very sudden backwards incompatible change.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Glance Bot (<email address hidden>) on branch: master
Review: https://review.openstack.org/124405

[Alexander Bashmakov (abashmak)]

Planning to submit a patch following Ian's suggestion above (issuing a warning and proceeding with operation).

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/368231

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/368231
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=9968b0993fe1859b7accb6701f2fffcbed757bc0
Submitter: Jenkins
Branch: master

commit 9968b0993fe1859b7accb6701f2fffcbed757bc0
Author: Alexander Bashmakov <email address hidden>
Date: Fri Sep 9 13:44:36 2016 -0700

    Added overwrite warning for db_export_metadefs.

    The bug in [1] proposed to always require a directory option for
    glance-manage db_export_metadefs command. The rationale being that
    silently overwriting the default path /etc/glance/metadefs can
    be undesired. However, the feedback was that this command should
    stay silent in case it's called from cron jobs or automated tasks,
    and that it's better to simply log a warning and proceed, as well
    as update the documentation with a cautionary note. This patch
    addresses that request.

    [1] https://bugs.launchpad.net/glance/+bug/1367011

    Change-Id: Ie8cc4bb3769a6347fd25e2235a72c6358af70d42
    Closes-Bug: #1367011

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/glance 14.0.0.0b1 development milestone.