Bug 1486696 - 'hammer host update' removes existing host parameters
Summary: 'hammer host update' removes existing host parameters
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Satellite
Classification: Red Hat
Component: Hosts
Version: 6.3.0
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: 6.8.0
Assignee: Rahul Bajaj
QA Contact: Peter Ondrejka
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-08-30 11:25 UTC by Tomas Strachota
Modified: 2023-12-15 15:57 UTC (History)
14 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-10-27 12:57:21 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Foreman Issue Tracker 20725 0 High Closed 'hammer host update' removes existing host parameters 2021-01-19 15:27:55 UTC
Red Hat Product Errata RHSA-2020:4366 0 None None None 2020-10-27 12:57:47 UTC

Description Tomas Strachota 2017-08-30 11:25:38 UTC
<pre>
# rpm -q foreman
foreman-1.15.3-1.el7.noarch
# hammer --version
hammer (0.10.2)
 * hammer_cli_foreman (0.10.2)
 * hammer_cli_foreman_bootdisk (0.1.3)
 * hammer_cli_foreman_docker (unknown version)
 * hammer_cli_foreman_remote_execution (unknown version)
 * hammer_cli_foreman_tasks (unknown version)
 * hammer_cli_import (0.11.3)
 * hammer_cli_katello (0.10.0)
# hammer host info --name client1.local | grep param
# hammer host update --name client1.local --parameters param1=value1,param2=value2
Host updated
# hammer host info --name client1.local | grep param    param1 => value1
    param2 => value2
# hammer host update --name client1.local --parameters param3=value3
Host updated
# hammer host info --name client1.local | grep param
    param3 => value3
# hammer host update --name client1.local 
Host updated
# hammer host info --name client1.local | grep param
#
</pre>

Comment 1 Tomas Strachota 2017-08-30 11:25:44 UTC
Created from redmine issue http://projects.theforeman.org/issues/20725

Comment 3 Tomas Strachota 2017-08-30 11:32:01 UTC
The clone script didn't let me to modify the BZ text prior to entering to BZ, I'm adding some more info.

Reproducer steps:

1. Take existing host and update it's parameters via cli
    > hammer host update --id 7 --parameters param1=value1,param2=value2
    Host updated
2. Check detail of that host, it should print the parameters
    > hammer host info --id 7
    ...
    Parameters:
        param1 => value1
        param2 => value2
    All parameters:
        param1 => value1
        param2 => value2
    ...
3. Update the host again, this time without specifying any parameters
    > hammer host update --id 7 
    Host updated
4. Check its detail, the parameters are gone
    > hammer host info --id 7
    ...
    Parameters:               
    
    All parameters:
    
    ...

Comment 4 Jason Unovitch 2018-03-04 02:12:56 UTC
Hi, wanted to chime in and get on CC here.  I reported this as well a while back at https://groups.google.com/d/msg/foreman-users/fqKJ21YxDqw/Z_4OhZyYAAAJ and am just not getting around to the follow through.  Link in Ori as well from that post.

I observed this while testing 1.15.x in my work environment where I rely on both host parameters and scripted hammer commands to build so I had to stick with 1.14.  This is also still broken on 1.16.  I just checked this at home on a quick VM to see confirm.  You'll see the 'hammer host update ... --build true' blows away all the host level parameters.

Any idea what changed between 1.14 and 1.15+ that caused this?


[root@foreman ~]# rpm -q foreman tfm-rubygem-hammer_cli
foreman-1.16.0-1.el7.noarch
tfm-rubygem-hammer_cli-0.11.0-1.el7.noarch
[root@foreman ~]# sudo -u postgres psql foreman -c 'select * from parameters'could not change directory to "/root"
 id |         name          | value | reference_id |         created_at         |         updated_at         |  
    type       | priority | hidden_value 
----+-----------------------+-------+--------------+----------------------------+----------------------------+--
---------------+----------+--------------
  1 | test_global_parameter | True  |              | 2018-03-04 01:16:00.112994 | 2018-03-04 01:16:00.112994 | C
ommonParameter |        0 | f
  2 | test_group_paremeter  | True  |            1 | 2018-03-04 01:16:33.645192 | 2018-03-04 01:16:33.645192 | G
roupParameter  |       60 | f
  6 | test_host_parameter   | True  |            1 | 2018-03-04 01:58:18.824859 | 2018-03-04 01:58:18.824859 | H
ostParameter   |       70 | 
(3 rows)

[root@foreman ~]# !71
hammer host update --name foreman.j.unovitch.com --build true
Host updated
[root@foreman ~]# sudo -u postgres psql foreman -c 'select * from parameters'
could not change directory to "/root"
 id |         name          | value | reference_id |         created_at         |         updated_at         |  
    type       | priority | hidden_value 
----+-----------------------+-------+--------------+----------------------------+----------------------------+--
---------------+----------+--------------
  1 | test_global_parameter | True  |              | 2018-03-04 01:16:00.112994 | 2018-03-04 01:16:00.112994 | C
ommonParameter |        0 | f
  2 | test_group_paremeter  | True  |            1 | 2018-03-04 01:16:33.645192 | 2018-03-04 01:16:33.645192 | G
roupParameter  |       60 | f
(2 rows)

Comment 5 Jason Unovitch 2018-03-04 16:06:55 UTC
This issue is caused by the commit https://github.com/theforeman/foreman/pull/4224/commits/448cf059bff7c2eeccc8d46e483e1744ee41c69d

Temporarily commenting the "process_destroy" call in app/controllers/concerns/parameter_attributes.rb is sufficient to preserve variables but obviously breaks the intended functionality of the --parameters flag deleting old variables and setting new variables.  The --parameters behavior appears to be working as designed per bug 1274683 comment 9 (https://bugzilla.redhat.com/show_bug.cgi?id=1274683#c9) where "Using hammer host update --parameters does not update certain parameters but all parameters, meaning it should delete all the host_parameters and then creates only those you pass in the update. ... Please note if you are not updating all the parameters you will delete those you don't send."

However in the non --parameters flag it's now treated like an empty array and deleting all variables anyway.  The change in user facing behavior when not using --parameters feels wrong.  I'm not sure where the right place to fix this is.

Comment 8 Satellite Program 2018-09-05 10:05:17 UTC
Moving this bug to POST for triage into Satellite 6 since the upstream issue https://projects.theforeman.org/issues/20725 has been resolved.

Comment 9 Peter Ondrejka 2018-11-02 10:12:23 UTC
Checked in Satellite 6.5 snap 1, the case of empty update wiping out parameters has been fixed, though setting any parameter still deletes the preexisting ones. For example, I'm adding a custom param: 

~]# hammer host update --id 2 --parameters param1=value1
Host updated.
~]# hammer host info --id 2
...
Parameters:               
    param1 => value1
All parameters:           
    param1 => value1
    enable-puppet5 => true
    enable-epel => false

Then I decide to tweak enable-epel param:

~]# hammer host update --id 2 --parameters enable-epel=true
Host updated.
~]# hammer host info --id 2
Parameters:               
    enable-epel => true
All parameters:           
    enable-epel => true
    enable-puppet5 => true

The previous setting is wiped. I think most users would expect that the parameters they specify will get appended to existing ones. If we change the behavior to work like this, than we'll probably need also add a way to delete custom parameters.

Other thing I find strange is that apparently there is no comparison of supplied and existing non-custom params, for example having a host with enable-epel set to false:

~]# hammer host info --id 2
...
Parameters:               
All parameters:           
    enable-puppet5 => true
    enable-epel => false

...and setting it to the same value as it was

~]# hammer host update --id 2 --parameters enable-epel=false
Host updated.
~]# hammer host info --id 2 
Parameters:               
    enable-epel => false
All parameters:           
    enable-epel => false
    enable-puppet5 => true

Here, a custom param gets set unnecessarily, I'd expect that keys and values of supplied custom params would be checked against existing non-custom params.

Comment 13 Ivan Necas 2019-01-23 16:10:04 UTC
Peter: I would say this is expected behavior, one should use `host set-parameter` or `host delete-parameter` to update params without affecting the others. With that info, how would you suggest to continue with this BZ?

Comment 14 Peter Ondrejka 2019-01-25 17:20:31 UTC
It's good to hear there is a workaround. From the usability point of view it is quite tricky because "host update" can wipe out even the parameters set by "host set-parameter" + user needs to search logs to find out what was changed to recover from accidental wipeout.

I suggest at the minimum to add a note to hammer update help, something like:

--parameters PARAMS       Host parameters 
                          Comma-separated list of key=value. Note that any existing custom parameters will be replaced. To append parameters use 'hammer host set-parameter' instead.

I'd also consider adding some confirmation notice for parameter update, something like:

# hammer host update --id 1 --parameters FOO=bar
This will replace existing custom parameters. To append parameters use 'hammer host set-parameter' instead. Proceed? [y/n]

The message could be even more clever and appear only if there really are some existing params and it could say which are these ("You are about to wipe key=value,..."). Not sure if messages like this are being used elsewhere around hammer, I see a downside as any automated scripts would probably need to be updated with -y...

Third thing we could do is to ask documentation guys to explain host parameter updates, probably in the Hammer CLI guide. This would give support guys something to link to. I'll file the docs bug and reference it here.

Hope this helps.

Comment 15 Peter Ondrejka 2019-01-25 17:43:15 UTC
attaching the promised documentation bug https://bugzilla.redhat.com/show_bug.cgi?id=1669588

Comment 22 Bryan Kearney 2019-11-05 20:45:19 UTC
Upstream bug assigned to rabajaj

Comment 26 Peter Ondrejka 2020-06-10 12:22:50 UTC
Checked on Sat 6.8 snap 2, parameters don't get wiped out on host update unless stated explicitly with --parameters="". The fact that --parameters replaces the existing value is now stated in option help

Comment 29 errata-xmlrpc 2020-10-27 12:57:21 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 (Important: Satellite 6.8 release), and where to find the updated
files, follow the link below.

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

https://access.redhat.com/errata/RHSA-2020:4366


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