Page MenuHomePhabricator

should we move $site global to a fact
Open, MediumPublic

Description

During a recent change i added a new custom type Wmflib::Site to validate site parameters and the question that came back was do we really need a new hardcoded list of sites. My response was

Personally i think it makes sense to have a datatype to validate the site (and going back to the previous discussion i actually prefer datacenteres as a name). however i take your point that we allready have this defined in multiple places. from my PoV (and without looking at all the use cases) i think we should:

  • move the site global parameter to a fact (and rename it datacenter)
  • have the values hardcoded here so we can validate parameters
  • create a function something like the following for when other classes profiles need to know the list of sites (this would also need to have the hard coded list)
function wmflib::get_datacenters(
  Enum['cache', 'primary', 'all'] $dc_type = 'all'
) {
  $datacenters {
    'eqiad' => 'primary',
    'codfw' => 'primary',
    'esams' => 'cache',
    'ulsfo' => 'cache',
    'eqsin' => 'cache',
   }
   if $dc_type = 'all' {
     $datacenters.keys
   else {
     $datacenters.filter |key, value| {
        $value = $dc_type
     }.keys
   }
}

Which also touch on another topic that cam up as to wether we should use the name datacenters over site. Site has in some versions of puppet been a reserved word so i would prefer to go with datacenteres. Also if we do this by adding a new fact then we could slowly migrate which should provide an easy pathforward

Event Timeline

jbond triaged this task as Medium priority.
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I like the idea of the datatype and having one list of sites we consider valid!

I made the case on the review for why sticking with site is important, reporting it here:

  • site is what we've been using in puppet since forever
  • site is what a bunch of things have 'hardcoded' (prometheus, netbox, possibly others)
  • shorter to type
  • no confusion on spelling datacenter vs datacentre
  • no "abbreviation temptation" (dc vs datacenter)

Thanks @fgiunchedi i had net to go over thoses comments and update, and from the comments it was agreeaded to use site which im happy)ish) with. for posperity io justwant to not that $site` has caused issues in the past as it use to be a reserved word however I don't think we will hit this again but wanted to mention it as a data point.

Something else to consider that was not mentioned in the discussion on gerrit. By creating a new name to store this data (e.g. $datacenbter, $location, $foobar), we could update the puppet code slowly i.e. introduce a new fact, test it compares well with the $::site global slowly (no too slowly) covert uses of $::site to $facts['...']. if we stick with using site then swapping the global variable for a fact would most likley have to be a big sweeping change doing everything in one step. Im not even sure what the precedence rules are when you have a global $site and a fact with the same name (or if its even supported) and try to access $::site