Eliminates subscription-manager exec on every Puppet run #95

speer · 2019-08-01T08:11:21Z

Pull Request (PR) description

The idea behind this PR is to get rid of subscription-manager executions on every Puppet-run. If there are issues with subscription-manager, (ex. Satellite down or similar) these executions block all the Puppet runs on all connected systems. In those cases, the whole Puppet infrastructure becomes unusable, since during Puppet runs, all calls of "subscription-manager identity, subscription-manager list, subscription-manager repos" hang and lead to almost never finishing Puppet agent runs. There are basically no timeouts of some of these calls (neither server_timeout in rhsm.conf leads to improvements of the situation).
Also we found, that in such situations, the module had tried to run "auto attach" on all of our systems. This is probably because the onlyif condition of RHSM-subscribe triggered somehow...

Because of the reasons explained above, we found, this module should not rely on subscription-manager anymore for verifying the systems current state and use it only for registering systems, assigning/removing subscriptions and enabling/disabling repos.

With this PR, the module relies on entitlement certificates and the redhat.repo file, which are kept up to date periodically by rhsmcertd. Of course I agree, that there might occur race conditions, where these sources of truth could not always be as reliable as subscription-manager executions, but I find the trade-off justifiable, because of the issues explained above.

Partly, this problem has already been addressed by @kallies in #91 but this was limited to the executions of the rhsm facts only. So the current PR supersedes #91

In this PR I further removed the rhsm::repo type, which had already been replaced by rh_repo. I see no need of supporting two types that do the same, just differently.
Also I removed the pool parameter in init.pp, since I found that one should use rh_subscription for that.
Because of already touching the parameter list of init.pp, I also added the types contributed by @kallies in #86

Note, that this PR contains changes that break backward-compatibility.

This Pull Request (PR) fixes the following issues

Fixes #90
Fixes #92

vinzent

Good idea to prevent calling out to satellite if not necessary. Any larger Satellite installation I know has its problems.

vinzent · 2019-08-29T17:37:22Z

lib/puppet/provider/rh_subscription/redhat.rb

 Puppet::Type.type(:rh_subscription).provide(:redhat) do
  commands rhsm: '/usr/sbin/subscription-manager'
+  commands rct: '/usr/bin/rct'


for those that never heard of this command:

rct - Displays information (headers) about or size and statistics of a entitlement, product, or identity certificate used by Red Hat Subscription Manager.

vinzent · 2019-08-29T17:41:34Z

lib/puppet/provider/rh_subscription/redhat.rb

  mk_resource_methods

  def self.subscriptions
    subs = {}
-    rhsm('list', '--consumed').split("\n\n").each do |blk|


could you document the method? especially some return value example

vinzent · 2019-08-29T17:42:15Z

lib/puppet/provider/rh_subscription/redhat.rb

-        sub_data[element[1]] = element[2]
+      section = nil
+      cert_raw_data.split("\n").each do |line|
+        if line =~ %r{^([^:\s]+):$}


what are you doing here?

maybe some example of the output of rct would help.

Thank's for your feedback! Added some documentation, hope that explains it better :)

vinzent · 2019-08-29T17:50:13Z

manifests/init.pp

-  $package_ensure                    = 'latest',
-  Array[String[1]] $enabled_repo_ids = [],
-) {
+  Optional[String[1]]    $rh_user               = undef,


Just FYI: back in the days we explicitly didn't use data types potentially stay compatible with the old puppet provided by Redhat with Satellite 6.

As the type/provider code does not work with Puppet 3 Agents, this is fine for Satellite >= 6.4 (cornercase: Puppet 4/5 Server and Puppet 3 Agent)

bastelfreak · 2019-08-31T10:09:49Z

manifests/init.pp

-  Array[String[1]] $enabled_repo_ids = [],
-) {
+  Optional[String[1]]    $rh_user               = undef,
+  Optional[String[1]]    $rh_password           = undef,


stricter datatypes \o/

kallies · 2019-09-10T13:28:05Z

lib/puppet/provider/rh_repo/redhat.rb

-      end
-      repos[repo_data['Repo ID']] = {
-        enabled: repo_data['Enabled']
+    repo_file = Puppet::Util::IniConfig::PhysicalFile.new('/etc/yum.repos.d/redhat.repo')


This will improve reliability and performce!

lib/puppet/provider/rh_subscription/redhat.rb

kallies · 2019-10-10T07:22:54Z

@bastelfreak @ekohl does this PR still need feedback?

ekohl · 2019-10-10T07:42:10Z

I hadn't looked at it before. For the future, splitting the data types and other cleanups into a separate commit makes reviewing easier. You don't have to split it now. I must admit I don't know enough about subscription manager to properly review this though :(

kallies

(pdk) Rubocop convention (comma after the last item of a multiline hash)

kallies · 2019-10-16T07:45:43Z

lib/puppet/provider/rh_subscription/redhat.rb

-        serial: sub_data['Serial']
+      subs[sub_data['Certificate']['Pool ID']] = {
+        name: sub_data['Order']['Name'],
+        serial: sub_data['Certificate']['Serial']


Rubocop mentions the missing comma at the end of the line

If I add the comma, TravisCI build fails...

Offenses:

lib/puppet/provider/rh_subscription/redhat.rb:108:50: C: Style/TrailingCommaInLiteral: Avoid comma after the last item of a hash. (https://github.com/bbatsov/ruby-style-guide#no-trailing-array-commas)

serial: sub_data['Certificate']['Serial'], ^

lib/puppet/provider/rh_subscription/redhat.rb:119:30: C: Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call. (https://github.com/bbatsov/ruby-style-guide#no-trailing-params-comma)

serial: data[:serial],

kallies · 2019-10-16T07:45:57Z

lib/puppet/provider/rh_subscription/redhat.rb

  def self.instances
    subscriptions.map do |pool, data|
      new(
        ensure: :present,
        name:   pool,
-        active: active?(data[:active]),
        serial: data[:serial]


Rubocop mentions the missing comma at the end of the line

If I add the comma, TravisCI build fails...

Offenses:

lib/puppet/provider/rh_subscription/redhat.rb:108:50: C: Style/TrailingCommaInLiteral: Avoid comma after the last item of a hash. (https://github.com/bbatsov/ruby-style-guide#no-trailing-array-commas)

serial: sub_data['Certificate']['Serial'], ^

lib/puppet/provider/rh_subscription/redhat.rb:119:30: C: Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call. (https://github.com/bbatsov/ruby-style-guide#no-trailing-params-comma)

serial: data[:serial],

Yes, in Rubocop we have different rules. Personally I'd prefer trailing commas here as well but let's not change rubocop rules as part of this PR.

@kallies

Note, that this commit contains changes that break backward-compatibility. Additionally: * this gets rid of `rhsm::repo` which had already been replaced by `rh_repo`. There is no need of supporting two types that do the same, just differently. * types have been added to the `rhsm` class (contrib @kallies). * the `pool` parameter has been removed, since one should use `rh_subscription` for that. * service `rhsmcertd` is ensured to be enabled and running. This service is responsible for a periodical cert renewal and auto-attach (see rhsm.conf). * exec 'sm yum clean all' has beed removed. * `RHSM-subscribe` has been removed, since auto-attach happens already periodical via rhsmcertd. Attachment of pool subscriptions happens via `rh_subscription`.

igalic

i did a quick review of this code, and i like it a lot.
i didn't realize we had an internal IniFile parser in Utils!

What can we do to get this merged as quickly as possible?

speer force-pushed the rewrite branch 2 times, most recently from aa1e1e6 to b0b0517 Compare August 1, 2019 09:07

rcalixte mentioned this pull request Aug 2, 2019

Add Sensitive to RHSM exec and update metadata #96

Merged

juniorsysadmin added backwards-incompatible needs-feedback Further information is requested labels Aug 3, 2019

vinzent approved these changes Aug 29, 2019

View reviewed changes

speer force-pushed the rewrite branch from b0b0517 to ecc24ea Compare August 30, 2019 09:14

bastelfreak reviewed Aug 31, 2019

View reviewed changes

kallies mentioned this pull request Sep 10, 2019

Read enabled repositories from inifile #91

Closed

kallies reviewed Sep 10, 2019

View reviewed changes

lib/puppet/provider/rh_subscription/redhat.rb Outdated Show resolved Hide resolved

speer force-pushed the rewrite branch from ecc24ea to 6064a59 Compare September 10, 2019 13:43

kallies mentioned this pull request Sep 11, 2019

Add defaults and make server_timeout and inotify configurable #86

Merged

kallies approved these changes Oct 16, 2019

View reviewed changes

speer force-pushed the rewrite branch from 6064a59 to 70e8843 Compare October 16, 2019 08:30

speer force-pushed the rewrite branch from 70e8843 to 2c280ca Compare October 16, 2019 08:35

igalic reviewed Oct 16, 2019

View reviewed changes

vinzent merged commit ee78ef0 into voxpupuli:master Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminates subscription-manager exec on every Puppet run #95

Eliminates subscription-manager exec on every Puppet run #95

speer commented Aug 1, 2019

vinzent left a comment

vinzent Aug 29, 2019

vinzent Aug 29, 2019

vinzent Aug 29, 2019

vinzent Aug 29, 2019

speer Aug 30, 2019

vinzent Aug 29, 2019

kallies Sep 11, 2019

bastelfreak Aug 31, 2019

kallies Sep 10, 2019

kallies commented Oct 10, 2019

ekohl commented Oct 10, 2019

kallies left a comment

kallies Oct 16, 2019

speer Oct 16, 2019

kallies Oct 16, 2019

speer Oct 16, 2019

ekohl Oct 16, 2019

igalic left a comment

Eliminates subscription-manager exec on every Puppet run #95

Eliminates subscription-manager exec on every Puppet run #95

Conversation

speer commented Aug 1, 2019

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

vinzent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kallies commented Oct 10, 2019

ekohl commented Oct 10, 2019

kallies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment