disable-chef-client isn't preserved over reboots

In production#2570 (comment 401672637) we saw that disabling chef-client with disable-chef-client wasn't preserved over a reboot - which unexpectedly brought the DB back into rotation with a high replication delay.

We need to figure out why it isn't preserved over reboots (the node was in an unhealthy state, so maybe it just failed to write changes to disk in this one case and works in general - need to test) and fix it.

added SRE:On-call corrective action oncall teamReliability workflow-infraTriage labels

marked this issue as related to production#2570 (closed)

mentioned in issue production#2570 (closed)

added severity2 + 1 deleted label and removed teamReliability label

mentioned in issue production#2656 (closed)

mentioned in issue #11235 (moved)

mentioned in issue #11317 (moved)

mentioned in issue #11379 (moved)

mentioned in issue #11440 (moved)

mentioned in issue #11496 (moved)

mentioned in issue #11549 (moved)

mentioned in issue #11596 (moved)

mentioned in issue #11681 (moved)

It's the bootstrap script; it reinstalls chef explicitly (https://ops.gitlab.net/gitlab-com/gl-infra/terraform-modules/google/bootstrap/-/blob/master/scripts/bootstrap-v9.sh#L96) which will re-create the chef-client binary that got moved out of the way by the disabler.

The simplest direct approach is to make the bootstrap script aware of the disabler (check for the disabled binary in /usr/bin/chef-client-alias-when-disabled perhaps?) and not install in that case. However, the shutdown script also de-registers the node from chef so rebooting the node would remove it from chef's database until we get around to re-enabling chef properly. That may be acceptable; these situations tend to be edge-case scenarios where, after the reboot, we'll be performing manual actions with a goal to either re-enable chef and the node in due course, or destroy it permanently. There's a small risk we could end up with orphaned nodes, but that could happen now for a number of other similar reasons so it's probably not a huge problem.

@hphilipps @cmiskell Are we not running the chef-client as a systemd service? If the file is present at boot, we can use ConditionPathExists with the negation, to not start that service, if the file exists.

@T4cC0re

   Loaded: loaded (/etc/systemd/system/chef-client.service; enabled; vendor preset: enabled)

So yes, it does run as systemd service, but that still doesn't prevent the bootstrap script from manually installing and then invoking chef-client at startup.

@cmiskell I think the deregister dance we do on shutdown is problematic in this case. If we did not have that, we could simply omit the install after the initial bootstrap. But the node not being registered in chef if the client was disabled and the node being shutdown makes this a tough nut to crack.

I am still not sure why we de-register automatically anyway. We're not auto-scaling, and when deleting a node, we should just make this a scripted step. The auto-deregister is nice but also does not work all the time (had it fail a couple of times already), so we need to look and clean manually still. Removing that would allow us to at least easily implement a proper way of persisting a disabled chef-client across reboots.

Especially, because those nodes can turn into time-bombs. All it takes for chef to be disabled because of something, and then GCP detects a hypervisor failure and reboots the machine on another host. We had those regularly in the past, although admittedly they have become a bit rarer. Uncontrolled chef-changes like these can cause problems that might be very hard to debug.

mentioned in issue #11749 (moved)

mentioned in issue #11808 (moved)

mentioned in issue #11872 (moved)

mentioned in issue #11985 (moved)

mentioned in issue #12041 (moved)

mentioned in issue #12104 (moved)

mentioned in issue #12155 (moved)

mentioned in issue #12212 (moved)

mentioned in issue #12227 (moved)

mentioned in issue #12255 (moved)

mentioned in issue #12323 (moved)

mentioned in issue #12382 (moved)

mentioned in issue #12564 (moved)

mentioned in issue #12628 (moved)

mentioned in issue #12673 (moved)

mentioned in issue #12730 (moved)

mentioned in issue #12861 (moved)

mentioned in issue #12919 (moved)

mentioned in issue #13030 (moved)

mentioned in issue #13100 (moved)

mentioned in issue #13163 (moved)

mentioned in issue #13221 (moved)

mentioned in issue #13286 (moved)

mentioned in issue #13479 (moved)

mentioned in issue #13569 (moved)

mentioned in issue #13633 (moved)

Changing team label to teamReliability per https://gitlab.com/gitlab-com/gl-infra/infrastructure/-/issues/13662

added teamReliability label and removed 1 deleted label

mentioned in issue #13686 (moved)

mentioned in issue #13733 (moved)

mentioned in issue #13782 (moved)

mentioned in issue #13824 (moved)

mentioned in issue #13883 (moved)

mentioned in issue #13942 (moved)

mentioned in issue #14022 (moved)

mentioned in issue #14077 (moved)

mentioned in issue #14181 (moved)

mentioned in issue #14228 (moved)

This is potentially a big scope issue. We're going to time box to investigate the feasibility. This issue will be complete when the determination is made how feasible and how much work this will involve. If feasible with a reasonable effort, we will create a new issue to capture that work.

added workflow-infraReady label and removed workflow-infraTriage label

added 1 deleted label

assigned to @cmcfarland

added priority2 label

Q: did we make a change to not have Patroni start on boot or from Chef.

It does seem that postgresql is actively stopped during Chef runs, and patroni is set to never start up.

@hphilipps or @nnelson Could you help confirm this? And maybe speak to if this helps solve the root problem this issue is trying to solve.

@cmcfarland It is true that chef-client is stopping the default Ubuntu postgresql service (because postgres needs to be managed by Patroni) and that chef also is not setting the patroni service to be enabled.

But chef is restarting the Patroni service if it needs to change patroni.yml and to prevent this we disabled chef-client (as we manually set the noloadbalance tag in patroni.yml to take it out of rotation to reboot it). So it was an unpleasant surprise that chef-client was enabled again after the reboot.

Speaking generally: disabling chef-client is not really save, as any (intended or unintended) reboot will enable it again.

@hphilipps Thank you for helping me better understand this. I am going to brainstorm some ideas below to fix this.

mentioned in issue #14263 (moved)

mentioned in issue #14323 (moved)

Brainstorm

The narrowest change I can think of is what was mentioned above, essentially.

1. Update the teardown script to detect the shim and take no chef action because of it.
2. Update the bootstrap script to detect the shim and take no chef action because of it.
3. Update the scripts on patroni nodes and test them.

This would mean that a disabled chef node could be restarted with little impact aside from the following:

1. Since a restarted/stopped node is still in the chef inventory, there may be alerts about it sent to EOC.
2. A deleted node after having chef disabled would not be purged from the inventory and chef client and nodes would need to be deleted by hand after such an event.

The only alternative I can imagine is to have the chef disable shim create a lockfile that the patroni recipe can detect after a reboot and decide not to start postgresql when that lockfile is noticed.

1. A deleted node after having chef disabled would not be purged from the inventory and chef client and nodes would need to be deleted by hand after such an event.

As a long-tail follow-up, we should watch for "stale" chef clients that haven't checked in for longer than X days, and send an alert to prompt cleanup/investigation.

@craig What do you think of a chef-repo scheduled job that just notifies of nodes that have not converged in a set amount of time. I considered a prometheus check for this, but if a node is provisioned, but never runs, it never shows up in our metrics.

@cmcfarland I like it. Simple, straightforward, and with some breadcrumbs in runbooks and the project README, should be fairly discoverable.

My first take on this last little piece. It doesn't have to be great, but it just needs to run and error out, basically.

https://gitlab.com/gitlab-com/gl-infra/chef-repo/-/merge_requests/1061

I am going to make a new issue to track this detection work. https://gitlab.com/gitlab-com/gl-infra/infrastructure/-/issues/14791

added workflow-infraIn Progress label and removed workflow-infraReady label

mentioned in issue production#5715 (closed)

marked this issue as related to production#5715 (closed)

mentioned in issue #14399 (moved)

My most recent attempt to test this had good results for the tear-down script, but the startup script had some bash issues. I will be attempting the change issue to test in staging again. production#5715 (closed)

The tests worked. Making a list of modules to update:

• generic-sv-with-group
• generic-stor
• generic-stor-with-group
• generic-stor-redis
• monitoring-with-count
• generic-sv-sidekiq - Module may be depricated
• pubsubbeat - Module may be depricated

This change is currently only applied to a single fleet of console in gstg. But we need to test more fleets before widening our deploy of this change. The testing would involve updating the module and performing a single, chef-enabled reboot of a node from the fleet. Which fleets should we test with?

Fleets to test with:

• generic-stor: file-hdd?
• generic-stor-with-group: patroni?
• generic-stor-redis: redis-cache (this is the only option)
• monitoring-with-count: prometheus?
• generic-sv-sidekiq: (none exist)
• pubsubbeat: (none exist)

• generic-stor: file-hdd?

"thanos-compact" may be another option.

Should we pick the one that is lower impact? Or more likely to be used in production? In this case, a gitaly node in GSTG is about as impactful on a reboot test as thanos-compact might be. I suppose we would need to conduct the reboot when a deploy is not occurring, though.

Oh you are right, a Gitaly reboot in gstg is not going to be impactful as long as a deployment is not happening. We should do gitaly then, since it's the most critical between the two, and will give us more confidence for applying the change in gprd.

I'll make an MR with these changes and try to come up with a reboot change issue.

Change issue: production#5807 (closed) MR: https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/merge_requests/3091

mentioned in issue #14445 (moved)

mentioned in issue production#5807 (closed)

mentioned in epic &562 (closed)

The job for GSTG failed due to, I think, the volume of requests to update compute instances. So I'm going to have to break this up into two MRs, I think.

I've updated this MR: https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/merge_requests/3091 with all changes minus the generic-sv-with-group fleets. This is a more reasonable chunk of changes. Though there are some nodes that were running the v9 boot script and I am not sure why.

@alejandro I see that you provisioned patroni-zfs in this commit about 7 months ago. Was there a specific reason you chose to use bootscript version 9 instead of 10 or the default variable for the environment?

Is this just copy-paste from the file-zfs module from 2 years ago?

@cmcfarland yea, seems to have been copy-paste, so passing the ball to @mwasilewski-gitlab's regarding https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/commit/9226a085eab9bc795aa030e1cf70a1d8c8391126

here's the relevant MR: https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/merge_requests/861

and here's an MR for the bootstrap script around that time: https://ops.gitlab.net/gitlab-com/gl-infra/terraform-modules/google/bootstrap/-/merge_requests/2

@cmcfarland the bootstrap script MR was merged a couple of weeks before the zfs MR, I probably pinned it because all other envs and storage nodes were still using version 8 of the script and I didn't want to wait with testing zfs for the roll out of the script across the entire gstg fleet

If we have zfs nodes that successfully use the latest version of the bootstrap script than we can probably test unpinning this

Ok, I think I am going to need to perform a review of the changes to the script and just make sure I can properly note any changes before I change them in staging. Once the net difference is understood, I can proceed.

I can confirm that the code added for bootstrap-v9 is still present and working in the latest bootstrap script. So that should not cause any issues.

mentioned in issue #14492 (moved)

mentioned in issue #14537 (moved)

mentioned in issue #14594 (moved)

marked this issue as related to production#5807 (closed)

The change went ok for a patroni node. Chef did not run after a reboot when the node was rebooted. production#5807 (closed)

I ran into some issues with snapd that seem unrelated, but could be a problem for chef in the future.

@amoter

Next steps:

• Create a new issue to fix the snap problems in the ubuntu_advantage-metrics.sh script from gitlab-server cookbook. Done: https://gitlab.com/gitlab-com/gl-infra/infrastructure/-/issues/14640
• Apply the changes to everything else in GSTG that is not using the new bootstrap module. This should be a mostly harmless change after the work has been done testing on patroni and console nodes. This MR is that work: https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/merge_requests/3174
• Apply the same changes to GPRD after GSTG has seen a week of normal activity?

mentioned in issue #14632 (moved)

@cmcfarland MR was merged on Tuesday 11/23

mentioned in issue #14679 (moved)

I have an MR to make the same changes in GPRD: https://ops.gitlab.net/gitlab-com/gl-infra/config-mgmt/-/merge_requests/3189

changed milestone to %Corrective Actions Squad - December I

@cmcfarland Just want to make sure you're aware that other changes were made recently to the startup script.

mentioned in issue #14791

marked this issue as related to #14791

added workflow-infraDone label and removed workflow-infraIn Progress label

closed