Train: upload-puppet-modules fails to store data in swift when called multiple times

[Damien Ciabrini]

(Originally reported in https://bugzilla.redhat.com/show_bug.cgi?id=1808369)

In Train, one cannot re-run upload-puppet-modules once it's already stored some data in swift.

The first time puppet modules are uploaded in swift, a correct artifact override is correctly stored in swift, i.e.:

[stack@undercloud ~]$ cat /home/stack/.tripleo/environments/puppet-modules-url.yaml
# Heat environment to deploy artifacts via Swift Temp URL(s)
parameter_defaults:
    DeployArtifactURLs:
    - 'http://172.172.3.83:8080/v1/AUTH_2ba8739bd95040d38337e5ecbb68e620/overcloud-artifacts/puppet-modules.tar.gz?temp_url_sig=8790e72a801e8d8ad016b850c7fa0044f605f94f&temp_url_expires=1614329134

But when one re-runs upload-puppet-modules, tt will throw a warning and will complete with incorrect swift temp url.

(undercloud) [stack@director ~]$ upload-puppet-modules --directory puppet-modules

Creating tarball...
Tarball created.
/usr/bin/upload-swift-artifacts: line 132: [: {'Temp-Url-Key':: binary operator expected
Creating heat environment file: /home/stack/.tripleo/environments/puppet-modules-url.yaml
Uploading file to swift: /tmp/puppet-modules-Nm6O2gc/puppet-modules.tar.gz
+-----------------------+---------------------+----------------------------------+
| object | container | etag |
+-----------------------+---------------------+----------------------------------+
| puppet-modules.tar.gz | overcloud-artifacts | 66f9c13bb4ee7f8fb20421dbb2dd3231 |
+-----------------------+---------------------+----------------------------------+
Upload complete.
sha1: Option -hmac needs a value
sha1: Use -help for summary.

[Damien Ciabrini (dciabrin)]

Seems to happen only in Train, and to be fixed in Ussuri with Ife347f754ca9129580c092bb271bacdc032ae14b and Idf0dccf4cd7d040e803b2ed2b49b3c39919c5a60

[Herve Beraud (herveberaud)]

Hello,

Some comments to give more info about why we facing this issue.

These changes [1] introduced our python-openstackclient's script bug.

I tried to revert them and I successfully obtained the previous behavious:

```
$ openstack container show overcloud-artifacts -c properties -f value
Temp-Url-Key='81f83ae270a03be2faf4dfaeeb113653a42fff77'
$ openstack container show overcloud-artifacts -c properties -f json
{
  "properties": "Temp-Url-Key='81f83ae270a03be2faf4dfaeeb113653a42fff77'"
}
$ openstack container show overcloud-artifacts -c properties -f shell
properties="Temp-Url-Key='81f83ae270a03be2faf4dfaeeb113653a42fff77'"
```

I restored the previous behaviour and got the previous output by changing the code with:

```
216 class ShowContainer(command.ShowOne):
217 _description = _("Display container details")
218
219 def get_parser(self, prog_name):
220 parser = super(ShowContainer, self).get_parser(prog_name)
221 parser.add_argument(
222 'container',
223 metavar='<container>',
224 help=_('Container to display'),
225 )
226 return parser
227
228 def take_action(self, parsed_args):
229
230 data = self.app.client_manager.object_store.container_show(
231 container=parsed_args.container,
232 )
233 if 'properties' in data:
235 #data['properties'] = format_columns.DictColumn(data['properties'])
234 from osc_lib.cli import format_columns
236 data['properties'] = utils.format_dict(data.pop('properties'))
237 return zip(*sorted(six.iteritems(data)))

```

The rational behind these changes is to provide outputs at a machine-readable format when JSON/YAML formatter is specified, however
in your case we provide the `--format value` option in our CLI and it look like we always get a result at JSON format even when the `value` format is given.

These changes was introduced since the version 4.0.0 [3] of the python-openstackclient and only affect the branches master and train of this project.

```
$ git branch -a --contains c2630ae91a0e4bae662926a0e45d71c79683b584
  remotes/origin/master
  remotes/origin/stable/train
$ git log --oneline --no-merges 3.19.0..4.0.0 | grep c2630a
c2630ae9 Use cliff formattable columns in object storage commands
```

The format management was delegated to cliff since these changes [4][5].
Previous versions (before 4.0.0) used the `osc_lib.utils.format_dict` [6] which was called directly from the `containers` module of the python-openstackclient.
In recent version (after 4.0.0) this module is no longer directly called and is usage have been delegated to the human format managed by cliff [5].

I'm not an openstackclient expert but I suppose this format (value) should be managed by the human format provided through cliff now, which seems t...

[Herve Beraud (herveberaud)]

Also note that these changes was introduced in 2017 (old commit), but they was only released ~6 months ago [1]

[1] https://github.com/openstack/releases/commit/eec29aeb05268beb89d43d68d9ab6968ae9f24d8

[Alan Bishop (alan-bishop)]

I have this fixed on master, and will propose the patches for train in a few minutes...

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/712672

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/712672
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=81d9792c3dd4917eb9e520ea2d47e4c3fbd54617
Submitter: Zuul
Branch: stable/train

commit 81d9792c3dd4917eb9e520ea2d47e4c3fbd54617
Author: Alan Bishop <email address hidden>
Date: Wed Mar 4 14:01:55 2020 -0800

    Fix upload-swift-artifacts handling of json data

    The upload-swift-artifacts script needs to handle json encoding of
    the Temp-Url-Key property, and [1] was a step in the right direction.
    However, it contained two errors that this patch corrects:

    1. The property name is "Temp-Url-Key" (camel-case). Even if you specify
       "Temp-URL-Key" something will change "URL" to "Url". This patch uses
       the correct form when detecting whether the property exists.

    2. The logic for detecting whether the Temp-Url-Key property exists was
       inverted. The property doesn't exist when the jq output is "null".

    [1] Ife347f754ca9129580c092bb271bacdc032ae14b

    Closes-Bug: #1867135
    Change-Id: Idf0dccf4cd7d040e803b2ed2b49b3c39919c5a60
    (cherry picked from commit 29367cb96d9c7477cc5c0a8d90e0d3e2c57b8cba)

[Alan Bishop (alan-bishop)]

The train patch merged, but there hasn't been a release that includes it yet.