Opened 4 years ago

Closed 3 years ago

#3638 closed change (fixed)

Refactor hiera('servers') into Puppet class adblockplus::host

Reported by: matze Assignee: matze
Priority: P2 Milestone:
Module: Infrastructure Keywords:
Cc: fred, fhd Blocked By: #3574
Blocking: #6, #1462, #1568, #3305, #3576, #3770 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29338046/
https://codereview.adblockplus.org/29338054/
https://codereview.adblockplus.org/29340915/
https://codereview.adblockplus.org/29340979/
https://codereview.adblockplus.org/29340973/
https://codereview.adblockplus.org/29367240/

Description (last modified by matze)

The current approach to feed Puppet information about our servers has proven to be quite impractical. Introduced together with the migration to our YAML-based, combined Hiera/ENC solution last year, the use of the hiera("servers") hash setup in base.yaml is quite repetitive, see i.e.:

In addition, this approach does not align well with the Puppet language, especially considering the limitations of the 2.7* version family, but that is an issue for another day.

Background

While managing these in a named hash, available as e.g. hiera("servers"), seems to be a simple way to access the desired information, the common approach using create_resources() to process the returned hash (one of the few tools available in Puppet 2.7) shows various disadvantages:

  • Each use requires an individual Puppet type to be created, and passed on to the create_resources() function, along with the $servers hash. The code and documentation overhead is relatively large, though the results are usually quite practical.
  • The Puppet type must recognize every parameter, even those which are irrelevant to its domain, otherwise the resource creation will result in a Puppet error.
  • The hash key or $title of any hash item has a special meaning as a Puppet namevar, but at the same type any "name" key has no effect when create_resources() is invoked. Instead, the default behavior using $title is applied, which leads to inconsistent results with other approaches (see below).
  • The $dns parameter is optional, by default accumulated by $name and some $domain or $authority value. This is never documented, but implemented in various manifests, templates, and scripts (see below).
  • The $domain or $authority value is not always taken from the same source.

There are other code fragments examining the servers hash, i.e.:

  • Puppet ERB templates
  • Possible hacks with inline templates
  • Ruby functions (not used yet)

For now, the most problematic, most often occurring use-case is a setup where host M needs to recognize each host C[x] of a list of clients C[0..n], like in the examples above, or any other master-slave relationship. More advanced examples include setups where any client C must recognize properties of the master, i.e. the TLS certificate of M (like Puppet itself, or #6 Bacula).

Todo

The improvement with the most effect for now seems to be the introduction of a conventional (meta-) resource any other code fragment may access. This allows for both keeping the current schema for now, whilst establishing a single official resource, featuring commonly accumulated properties:

  • Introduce a Puppet type adblockplus::host aligned with base::explicit_host_record()
  • Introduce parameter $adblockplus::hosts to setup hosts based on hiera_hash() and create_resources()
  • Migrate either of the above workarounds to access the newly created resources instead
  • Remove obsolete code fragments

Finally, we should refactor our puppet_node_classifier.rb script to not read the server information from hosts.yaml, but instead require each host to be present as an individual hosts/$HOSTNAME.yaml file. In practice most hosts require individual configuration anyway, i.e. for persistent encryption keys and monitoring quota, and maintaining host information would become more intuitive.

Status

This issue is considered rather urgent because a lot other issues would benefit from it, i.e.:


Namely, each of the above would require some of the aforementioned workarounds or other hacks to get implemented. In the same turn, each of these code fragments would require re-implementation after this ticket has been addressed.

Change History (20)

comment:1 Changed 4 years ago by matze

  • Blocking 6, 3574, 3576 added
  • Cc fred added

comment:2 Changed 4 years ago by matze

  • Description modified (diff)
  • Ready set

comment:3 Changed 4 years ago by matze

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

The first patch-set includes the introduction of both class adblockplus::host and parameter $adblockplus::hosts.

comment:4 Changed 4 years ago by matze

  • Blocking 3669 added

comment:5 Changed 3 years ago by abpbot

  • Blocked By 3574 added
  • Blocking 3574 removed

comment:6 Changed 3 years ago by matze

  • Blocking 1462, 1568, 3305 added
  • Cc fhd added
  • Review URL(s) modified (diff)

The former patch-set turned out to be impractical during the implementation of follow-up tickets. Based on the new experience I've created a new version, including examples on how to tackle the most re-occurring tasks, each of which has been proven to work well in further development.

Luckily, the first patch-set has never been pushed. The new one is in review now.

comment:7 Changed 3 years ago by matze

  • Review URL(s) modified (diff)

There is now another patch-set for review, based on the new adblockplus::host implementation. I believe it to be a very good example on how the new approach tackles some if the issues discussed in the ticket description above.

comment:8 Changed 3 years ago by matze

  • Blocking 3770 added

comment:9 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/infrastructure/rev/739d1ea52ea4

comment:10 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/infrastructure/rev/b4550bec8247

comment:11 Changed 3 years ago by matze

  • Blocking 3669 removed

comment:12 Changed 3 years ago by matze

  • Review URL(s) modified (diff)

The last remaining direct invocation of hiera('servers') outside of class base is in the sitescripts.ini template for the stats-master. We can easily migrate the template magic using the newly introduced adblockplus::host::$role mechanism and conditional sitescripts::configfragment definitions, but there are a few other prerequisites which we need to address before:

  • Migrate the node definition from the statsserver.pp manifest into a Hiera role
  • Migrate the node definitions from the downloadserver.pp manifest into a Hiera role
  • Migrate the node definition from the updateserver.pp manifest into a Hiera role
  • Finally fix the global Host[] and Sshkey[] realization introduced by accident

The first and last points are covered by the recent patch-set. Note that the latter requires the removal of the resulting configuration fragments in production after it has landed, which could become quite some effort if not done in automated fashion (more later).

Last edited 3 years ago by matze (previous) (diff)

comment:13 Changed 3 years ago by matze

  • Review URL(s) modified (diff)

Two more patch-sets, for aforementioned migration of downloadserver.pp and updateserver.pp into Hiera, respectively.

comment:14 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/infrastructure/rev/e492c647d6ca

comment:15 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/infrastructure/rev/c7f0974d6bb5

comment:16 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/infrastructure/rev/011568177058

comment:17 Changed 3 years ago by matze

Last reference is in modules/statsmaster/templates/sitescripts.ini.erb. We probably need to translate the iteration through the hiera('...') hash there into a sitescripts::configfragment defined by adblockplus::host::statsmaster.

comment:18 Changed 3 years ago by matze

  • Review URL(s) modified (diff)

comment:19 Changed 3 years ago by abpbot

comment:20 Changed 3 years ago by matze

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.