Project

General

Profile

Bug #11181

Better abstraction of ::local::node

Added by bertagaz over 1 year ago. Updated about 1 month ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Infrastructure
Target version:
Start date:
02/29/2016
Due date:
% Done:

30%

QA Check:
Dev Needed
Feature Branch:
puppet-lizard-manifests:bugfix/11181-abstract-local-node,puppet-tails:bugfix/11181-abstract-local-node
Type of work:
Sysadmin
Blueprint:
Easy:
No
Affected tool:

Description

When installing ecours.t.b.o (#8647), we had to move out some parts of our ::local::node declaration that were too tied to lizard's setup:

Part of it has been poorly handled into a ::tails::base:grub_setup class, but it's taking care of too many things at once: apparmor and the serial cmdline, which can be different from host to host.

Another part is the postfix canonical_sender setup which is tied to services we host on lizard.

We turned them into parameters to ::local::node one can deactivate, but it would be better if we abstracted that a bit more this class.


Related issues

Related to Tails - Feature #8647: Install an OS on the machine that will host the production monitoring setup Resolved 12/15/2015

History

#1 Updated by intrigeri over 1 year ago

  • Assignee set to bertagaz

I've done some initial cleanup, and moved more stuff to our tails::base class in order to help clarify what local::node is about. I'd like to keep this ticket about dealing with the regressions introduced when working on #8647, so I'll retitle it accordingly. We can create more precise tickets if we see other, more specific action we want to track, but that are not part of this task.

Part of it has been poorly handled into a ::tails::base:grub_setup class, but it's taking care of too many things at once: apparmor and the serial cmdline, which can be different from host to host.

The main regression I see here is that we have a tails::grub class that does quite some stuff, and is not configurable at all, so basically either what it does is OK for a given host and we can include that class, or we have to do things manually, and lose the benefits of having shared resources we can rely upon and rely to (e.g. Exec['update-grub']). I think this should be fixed by making configurable what needs to vary depending on the system:

  • AppArmor: I don't see why this should depend on the system; so far we've been enabling it everywhere; why exactly do we need exceptions now?
  • GRUB_CMDLINE_LINUX: indeed, at least the part that's about the serial console can vary, and more generally it's good to have a parameter that we can use to add kernel parameters as needed;
    • short-term, we could give the class a $console_kernel_parameters parameter (defaulting to what we generally want with libvirt, as done already in the class), and while we're at it a $additional_kernel_parameters parameter (empty by default), and then in Augeas[GRUB_CMDLINE_LINUX] we can concat these 2 variables to the AppArmor kernel parameters; I think it's the easiest good-enough way to clean up the mess, and that's all I'm expecting from this ticket in the GRUB area; and then, the $setup_grub parameter to local::node can be replaced by more meaningful ones, that will be passed through to tails::grub.
    • long-term, we could use https://github.com/hercules-team/augeasproviders_grub/ once it is fixed, or use grub2 2.00-10 (Jessie and onwards)'s new support for /etc/default/grub.d/*.cfg, that are read after /etc/default/grub => so we could drop snippets in there with content such as GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX apparmor=1 security=apparmor", etc., so we can deal with all situations just with tiny config files. But let's focus on the short-term for now.

Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.

I did some initial research on this one, and dumped my conclusions as a comment in classes.pp. tl;dr: this would not be specific to lizard at all, if we were doing things right. Anyway, I realize that revamping our MTA configuration is not what this ticket is about, so I guess we'll prefer a short-term workaround here again, so please just refactor things a bit so that their name makes sense:

  • Given that "local::" now means "common to all nodes managed by this set of manifests", apparently, in this context: rename local::postfix::canonical_senders to something that makes it clear it is about stuff hosted on lizard, such as lizard::postfix::canonical_senders.
  • Drop the $set_canonical_senders parameter: it's useless, instead we should guess dynamically if lizard::postfix::canonical_senders must be included (if $::hostname == lizard or $::fqdn =~ /\.lizard$/).

#2 Updated by intrigeri over 1 year ago

  • Related to Feature #8647: Install an OS on the machine that will host the production monitoring setup added

#3 Updated by bertagaz over 1 year ago

  • Target version set to Tails_2.5

#4 Updated by bertagaz over 1 year ago

  • Target version changed from Tails_2.5 to Tails_2.6

#5 Updated by anonym about 1 year ago

  • Target version changed from Tails_2.6 to Tails_2.7

#6 Updated by bertagaz about 1 year ago

  • Target version changed from Tails_2.7 to Tails_2.9.1

#7 Updated by anonym 10 months ago

  • Target version changed from Tails_2.9.1 to Tails 2.10

#8 Updated by anonym 9 months ago

  • Target version changed from Tails 2.10 to Tails_2.11

#9 Updated by bertagaz 8 months ago

  • Target version changed from Tails_2.11 to Tails_2.12

#10 Updated by bertagaz 8 months ago

  • Target version changed from Tails_2.12 to Tails_3.0

#11 Updated by bertagaz 5 months ago

  • Target version changed from Tails_3.0 to Tails_3.1

#12 Updated by bertagaz 4 months ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 30
  • QA Check set to Ready for QA
  • Feature Branch set to puppet-tails:bugfix/11181-abstract-local-node

intrigeri wrote:

  • AppArmor: I don't see why this should depend on the system; so far we've been enabling it everywhere; why exactly do we need exceptions now?

True.

  • GRUB_CMDLINE_LINUX: indeed, at least the part that's about the serial console can vary, and more generally it's good to have a parameter that we can use to add kernel parameters as needed;

I've pushed a branch implementing the short-term solution. I've also pushed a branch of the same name in the main lizard manifest Git repo, with 2 commits on top implementing the needed changes there. Please have a look if it is ready to be deployed.

Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.

  • Given that "local::" now means "common to all nodes managed by this set of manifests", apparently, in this context: rename local::postfix::canonical_senders to something that makes it clear it is about stuff hosted on lizard, such as lizard::postfix::canonical_senders.
  • Drop the $set_canonical_senders parameter: it's useless, instead we should guess dynamically if lizard::postfix::canonical_senders must be included (if $::hostname == lizard or $::fqdn =~ /\.lizard$/).

Pushed 502dd23 in the main lizard manifest Git repo, which does just that.

#13 Updated by intrigeri 4 months ago

I'll try to look at this, but I want to prioritize more pressing issues this month (my own Foundations Team work, and reviewing your work on the ISO build regressions brought by the new build setup). So I suspect I'll eventually have to postpone this to 3.2. It's been waiting for 16 months so it's not exactly urgent right now :)

#14 Updated by intrigeri 3 months ago

  • Target version changed from Tails_3.1 to Tails_3.2

#15 Updated by intrigeri 2 months ago

  • Feature Branch changed from puppet-tails:bugfix/11181-abstract-local-node to puppet-lizard-manifests:bugfix/11181-abstract-local-node,puppet-tails:bugfix/11181-abstract-local-node

(Apparently there are changes to review there as well.)

#16 Updated by intrigeri 2 months ago

  • Description updated (diff)

#17 Updated by intrigeri 2 months ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed
  • GRUB_CMDLINE_LINUX: indeed, at least the part that's about the serial console can vary, and more generally it's good to have a parameter that we can use to add kernel parameters as needed;

I've pushed a branch implementing the short-term solution. I've also pushed a branch of the same name in the main lizard manifest Git repo, with 2 commits on top implementing the needed changes there. Please have a look if it is ready to be deployed.

Regarding commit 90b10dd6819438518c8fd2fcbfd6cf5505f2415c in puppet-lizard-manifests:

  • Please use Hiera to pass parameters (and the you can revert back to include ::local::node): the usual rationale for separating code from config applies here.
  • For the buse.riseup.net node, you passed console=tty0 via additional_kernel_parameters. Any reason not to pass it through console_kernel_parameters?

Regarding commit 06644a7fe31cbc8a7796a248381ea970927ea892 in puppet-lizard-manifests: I don't understand what these new params are for. They don't seem to be used anywhere. I don't understand how this whole thing can work: I don't see where we're passing to tails::grub the parameters we want. Did I miss something? Perhaps just revert this change and pass them with Hiera?

I doubt $kernel_parameters = "${kernel_parameters} elevator=noop" can work: IIRC one cannot reassign a value to an already set variable in Puppet, since order does not matter and thus state can't change.

The default value of $terminal and $console_kernel_parameters feels wrong: it means we'll have no usable console on any new bare metal host we set up, unless it has something useful on ttyS0. I suggest using "serial console" for the terminal (so both work) and always passing console=tty0 in addition to whatever serial console we want (e.g. that's what we currently do on buse).

Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.

[...]

Pushed 502dd23 in the main lizard manifest Git repo, which does just that.

Looks good!

#18 Updated by bertagaz about 1 month ago

  • Target version changed from Tails_3.2 to Tails_3.3

Also available in: Atom PDF