Introduce Puppetmaster Parameter In kick.py

[matze]

In order to ensure script kick.py operates with the correct hosts.yaml when invoked, it must become extended by a new parameter (-m) that allows for specifying the master host to fetch the contents from, e.g.:

user=mathias; master=example.com
./kick.py -u $user -m $master ...

The idea is that if a master has been specified, the remote source (ssh://$user@$master/etc/puppet/infrastructure/modules/private/hiera/hosts.yaml) is used in favor of the local one (file://./modules/private/hiera/hosts.yaml).

Note that in order for this to remain as simple as possible, it requires compliance with our implicit file-path conventions and, of course, requires the $user account to be present on both the master server and the host to provision. This, however, is not different to the current kick.py version.

[trev]

In order to do anything meaningful, kick.py always needs to know what the puppet master is (to update the repository there). The corresponding host name is currently hardcoded, and I don't think it makes much sense to make it configurable given that the hosts themselves also rely on that particular host name. At the very least, it should default to puppetmaster.adblockplus.org so that we don't have to type that on the command line every time.

Finally, I don't think that running kick.py with the local hosts.yaml file makes any sense whatsoever - that was a temporary hack, it should be removed altogether.

[matze]

Frankly, I have disagree. In my opinion, the script should use the local hiera.yaml when no master has been given, so one can use it in e.g. a local test setup with both master and slave.
But it shall not be used as "alternative" to the production version, of course.

The intention is quite simple - I am trying to make both kick.py and run.py something of actual use in anyenvironment. Which then also allows for testing those scripts without prior modification or additional risks to our production environment.

By the way; Felix and I discussed changing the development host names to something like *.local (e.g. filter1.adblockplus.local) when porting each setup to Hiera. This allows for convenient, static entries in a developers /etc/hosts, whilst heavily decreasing chances for confusion.

[fhd]

I agree that this parameter makes sense.

But I believe the default behaviour is going to be a footgun either way: Wladimir (and I) expect kick.py to provision production nodes, based on the production hiera.yaml. Matze has use cases we don't: Using the local hiera.yaml and provisioning development nodes. Seems like whatever default we're going to go for, there will be accidents.

So how about just making this a required parameter? (We might have to think of another value or parameter for using the local hiera.yaml.)

[trev]

Ok, I see. However, I'd rather reduce the inconvenience here. There are really only two scenarios from what I can see: remote (with puppetmaster.adblockplus.org and production hosts.yaml) and local (with 10.8.0.99 and local hosts.yaml from private-stub). How about --local (-l) and --remote (-r) command line flags, with one being required?

[matze]

I like the --local and --remote idea. But I still dislike hard-coding our puppet master server anywhere in the repository. It's a production host, and one of our goals is to remove any production-specific entity from the infrastructure repo.

In order to avoid the inconvenience of manually specifying the --remote FQDN every time, I'll introduce a $PUPPET_MASTER_FQDN environment variable that, if present, overwrites the default puppetmaster. This way, one can either configure the local environment or just configure ~/.ssh/config accordingly, e.g.:

Host puppetmaster
    HostName puppetmaster.example.com

Note that this would also allow for specifying the puppetmaster.local later (the Vagrant box), and thus testing the entire mechanism.

[fhd]

Replying to matze:

I like the --local and --remote idea. But I still dislike hard-coding our puppet master server anywhere in the repository. It's a production host, and one of our goals is to remove any production-specific entity from the infrastructure repo.

Note that this is not premature generalisation BTW, we will need to use a different master for our on-site infrastructure.

In order to avoid the inconvenience of manually specifying the --remote FQDN every time, I'll introduce a $PUPPET_MASTER_FQDN environment variable that, if present, overwrites the default puppetmaster.

So to sum up:

1. -l will use the local hiera.yaml file.
2. -r will use puppetmaster or, if set, $PUPPET_MASTER_FQDN

I can live with that, but I think using puppetmaster as a default is too cryptic/footgun-y. Would be easier to just require that environment variable to be set.

I don't mind too much either way, but personally I'd prefer to always specify the puppet master, simpler and less error prone IMHO. Creating an alias is about as much effort as setting an environment variable.

[matze]

The [first patch-set] has been published for review. Note that this is part one of a trilogy, where the first patch ports the option handling from getopt to argparse, which allows for easier re-using of the parameters and thus easier handling in this case.

The second patch-set will introduce the new parameters, but they will only change which hosts.yaml file is considered. After this stage, the run.py and kick.py scripts can be used in regular fashion again.

The third and final patch-set will update the behavior so that -l uses vagrant(1) instead of ssh(1).

[matze]

The [second patch set] has been published for review. Note that the third will be done in another ticket, because the context and urgency is not quite the same there..

[matze]

Done:

• ​https://hg.adblockplus.org/infrastructure/rev/69a1c7b648f9
• ​https://hg.adblockplus.org/infrastructure/rev/f465f14e23f1

Related:

• ​https://hg.adblockplus.org/infrastructure/rev/9d538f307875
• ​https://hg.adblockplus.org/infrastructure/rev/900a53e93d93