Accept non symbol values from settings

[Ladas]

Accept non symbol values from settings. With recent API change,
we are getting symbols transformed to strings.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1558031

[Ladas]

@miq-bot assign @agrare
@miq-bot add_label bug

[kbrock]

LGTM - would be nice if there were a trivial test in here

[agrare]

I have no problem with this, however we use symbols in a lot of places within settings though so I'm curious if there is a more general fix that can be done here /cc @abellotti

[Ladas]

@agrare right, so this fixes failing refreshes in several providers. If @abellotti has a fix that would not turn Symbol into the String, that might be the best. Since there are plenty Symbol values in settings, we are not sure what everything broke.

[bdunne]

I also started going down this path with another setting, but closed the PR after a discussion with @Fryguy where we came to the same conclusion: The API shouldn't be putting strings where we expect symbols.

[Ladas]

ok, https://bugzilla.redhat.com/show_bug.cgi?id=1558031 back to API.

@agrare or do you think we should still merge this? OpenShift refresh will remain broken until this is fixed and the settings values need to be put back to symbol. Closing for now ...

[cben]

I do feel using symbols vs strings in some values was an arbitrary choice we made that doesn't add any value (pun not intended), and we better stop doing that. Surprisingly, it seems we have very few of these!

$ outline-grep ' :[^ ]*[^ :] *$' log/last_settings.txt
:ems_refresh:
  :azure:
    :inventory_collections:
      :saver_strategy: :default
  :azure_network:
    :inventory_collections:
      :saver_strategy: :default
  :kubernetes:
    :inventory_collections:
      :saver_strategy: :batch
  :ec2:
    :inventory_collections:
      :saver_strategy: :batch
  :ec2_network:
    :inventory_collections:
      :saver_strategy: :batch
  :s3:
    :inventory_collections:
      :saver_strategy: :batch
  :ec2_ebs_storage:
    :inventory_collections:
      :saver_strategy: :batch
  :openshift:
    :inventory_collections:
      :saver_strategy: :batch
:workers:
  :worker_base:
    :queue_worker_base:
      :ems_metrics_collector_worker:
        :defaults:
          :poll_method: :escalate
      :ems_refresh_worker:
        :defaults:
          :poll_method: :normal
      :defaults:
        :dequeue_method: :drb
        :poll_method: :normal
      :ems_metrics_processor_worker:
        :poll_method: :escalate
    :defaults:
      :poll_method: :normal
:server:
  :worker_monitor:
    :kill_algorithm:
      :name: :used_swap_percent_gt_value
    :start_algorithm:
      :name: :used_swap_percent_lt_value

It might be cleaner to normalize to symbols where we read these Settings instead of deeper code using it, like here. I'm ok with both.

[cben]

What we really care about is not just symbols, it's whether it's safe to round-trip settings as JSON.

orig = Settings.to_hash
tripped = JSON.parse(orig.to_json, symbolize_names: true)

def diff(a, b, path = [])
  if a.class == Hash && b.class == Hash
    raise "#{path}: #{a.keys} != #{b.keys}" if a.keys != b.keys
    a.keys.each {|k| diff(a[k], b[k], path + [k])}
  else
    puts "#{path}: #{a.inspect} != #{b.inspect}" if a != b
  end
  nil
end

[29] pry(main)> diff(orig, tripped, [])
[:ems, :ems_azure, :api_versions, :availability_set]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :ip_address]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :load_balancer]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :managed_image]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :network_interface]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :network_security_group]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems, :ems_azure, :api_versions, :resource]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :resource_group]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :storage_account]: Sun, 01 Oct 2017 != "2017-10-01"
[:ems, :ems_azure, :api_versions, :storage_disk]: Thu, 30 Mar 2017 != "2017-03-30"
[:ems, :ems_azure, :api_versions, :template_deployment]: Tue, 01 Aug 2017 != "2017-08-01"
[:ems, :ems_azure, :api_versions, :virtual_machine]: Fri, 01 Dec 2017 != "2017-12-01"
[:ems, :ems_azure, :api_versions, :virtual_network]: Wed, 01 Nov 2017 != "2017-11-01"
[:ems_refresh, :azure, :inventory_collections, :saver_strategy]: :default != "default"
[:ems_refresh, :azure_network, :inventory_collections, :saver_strategy]: :default != "default"
[:ems_refresh, :kubernetes, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2_network, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :s3, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :ec2_ebs_storage, :inventory_collections, :saver_strategy]: :batch != "batch"
[:ems_refresh, :openshift, :inventory_collections, :saver_strategy]: :batch != "batch"
[:workers, :worker_base, :queue_worker_base, :ems_metrics_collector_worker, :defaults, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :queue_worker_base, :ems_refresh_worker, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :defaults, :dequeue_method]: :drb != "drb"
[:workers, :worker_base, :queue_worker_base, :defaults, :poll_method]: :normal != "normal"
[:workers, :worker_base, :queue_worker_base, :ems_metrics_processor_worker, :poll_method]: :escalate != "escalate"
[:workers, :worker_base, :defaults, :poll_method]: :normal != "normal"
[:server, :worker_monitor, :kill_algorithm, :name]: :used_swap_percent_gt_value != "used_swap_percent_gt_value"
[:server, :worker_monitor, :start_algorithm, :name]: :used_swap_percent_lt_value != "used_swap_percent_lt_value"

OK, so we also have some dates in azure, doesn't sound hard to take strings there.

Overall, way smaller problem than I feared, IMHO worth doing.

[Fryguy]

I have no problem with this, however we use symbols in a lot of places within settings though so I'm curious if there is a more general fix that can be done here /cc @abellotti

I agree with @cben in that the backend should be made to deal with values that are Strings, as we will likely remove symbols as possible values due to JSON roundtripping.

[Fryguy]

@Ladas I've reopened this.

[miq-bot]

Checked commit Ladas@2d04511 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[Ladas]

yeah, it makes sense not to require symbol

[Fryguy]

@Ladas Thinking about this, I think the code should be doing to_s instead of to_s. The reason is that they may put non-string values such as integers or nil (invalid, yes), and .to_sym on those will blow up.

[kbrock]

@Fryguy I'm thinking you meant this?

I think the code should be doing to_s instead of to_s

I think the code should be doing to_s instead of to_sym

---

remove symbols as possible values due to JSON roundtripping.

👍 I feel we should remove symbols from our public interface (JSON) and yaml files

[Ladas]

@Fryguy @kbrock I use the symbol settings elsewhere in the code. So I might just add .kind_of?(String) check, and that should solve your concerns?

Right now, I am making it blow up, if the setting is not recognized anyway. I could just log warning and fallback to default though.

[Fryguy]

I t's really a very edge case. Like, if someone accidentally typed 0. It can be fixed separately, as it's probably very unlikely.

The way this should be fixed is to introduced a validator method so the person making changes to the config can't even set the wrong values in the first place. It would be nice if providers could bring their own pluggable config validators.

[cben]

@simaishi Can we start backporting these PRs — this one has to go in first — to gaprindashvili?

I've reopened the BZ because full round-trip of settings via API still breaks some other settings, including a new case for regexps we're not sure how to solve (#17201 (comment), #18208 (comment)).
So the BZ can't move to POST soon, and is not yet cloned for backporting.
But meanwhile, people on gaprindashvili who attempt to read then write settings get major problems — broken refresh ("Unknown InventoryCollection saver strategy: :batch") and this PR should help a bit...

[simaishi]

@cben The BZ referenced in this PR isn't approved to be included in 5.9.z releases, so I'm not able to backport at this time...