F #-: VRouter: VRRP: Hooks on role-changes

appliances/VRouter/Failover/README.md

@@ -0,0 +1,45 @@
+# Failover service for the VRouter appliance
+
+## What it does
+
+The `one-failover` OpenRC service follows the VRRP role of the router. Keepalived writes its state changes to a FIFO (`/run/keepalived/vrrp_notify_fifo.sock`), and the service reacts to the event of the VRRP sync group (`GROUP`) only. Events of single instances are ignored.
+
+- **MASTER** (`up`): waits until Keepalived is ready, loads the VM context, then restarts the services that are enabled in the context and stops the others. The services are the ones in the `SERVICES` table of `execute.rb` (Router4, NAT4, LVS, HAProxy, SDNAT4, DNS, DHCP4, WireGuard, each controlled by its `ONEAPP_VNF_*_ENABLED` attribute).
+- **BACKUP** (`down`): waits until Keepalived is on standby and stops all of those services.
+- `FAULT`, `STOP` and `DELETED` change nothing. If the event loop fails, all services are stopped to avoid a conflict between the nodes.
+
+The last role is kept in `/run/one-failover.state` (`{"state":"MASTER"}`, `BACKUP`, ...). It is written before the `up` or `down` step runs.
+
+## Hooks
+
+Other services can follow the VRRP role without a change to this module. Put an executable file into `/etc/one-appliance/ha-hooks.d/`. The service runs every executable file of that directory, in name order, with one argument:
+
+| Argument | When | Order |
+|---|---|---|
+| `up` | the node became MASTER | after the stock services were restarted, so forwarding does not wait for a hook |
+| `down` | the node became BACKUP | first, before the stock services are stopped |
+
+Use a numeric prefix (`10-bgp`, `20-conntrack`) to fix the order. Files that are not executable, and directories, are skipped. A missing directory is not an error.
+
+Rules for a hook:
+
+- **Run again.** `down` runs for every BACKUP event, not only for a change of role (a node that boots as BACKUP, a repeated event, and after an error of the event loop). `up` runs only when the role changed to MASTER. A hook has to be safe to run twice in a row.
+- **Environment.** `up` hooks see the VM context in their environment (the service loads it before it runs the hooks). `down` hooks do not: a hook that needs a context value reads it itself, for example from `/run/one-context/one_env`, without sourcing it in a shell.
+- **Time.** A hook is killed after 30 seconds, together with its whole process group. Hooks run one after the other, so N hooks can delay the failover by up to N times 30 seconds. Keep them short and start long-running work in the background through the init system.
+- **Failure.** A hook never breaks the failover. A non-zero exit, a timeout, a signal or an error to start the hook is logged as an error in `/var/log/one-appliance/one-failover.log` (with the tail of its output, at most 4096 bytes) and the next hook runs.
+- **Output.** The output of a hook goes to a temporary file, not to a pipe, so a child that outlives the hook cannot block the failover. It is only logged when the hook fails.
+- **Role.** A hook can read the current role from `/run/one-failover.state`; the argument is what to do now.
+
+Example (`/etc/one-appliance/ha-hooks.d/50-example`, mode `0755`):
+
+```sh
+#!/bin/sh
+case "$1" in
+up)   logger -t ha-hook "became MASTER" ;;
+down) logger -t ha-hook "became BACKUP" ;;
+esac
+```
+
+## Tests
+
+`tests.rb` (RSpec) covers the hooks (order, argument, non-executable files, a missing directory, failing, signal-killed, hanging and unstartable hooks, and a background child that outlives its hook), the position of the hooks relative to the stock steps in `up` and `down`, and the handling of the VRRP events (single instances are ignored, the group event is acted on).

appliances/VRouter/Failover/execute.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: false
 
 require 'json'
+require 'tempfile'
 
 module Service
 module Failover
@@ -31,6 +32,9 @@ module Failover
 
     FIFO_PATH  = '/run/keepalived/vrrp_notify_fifo.sock'
     STATE_PATH = '/run/one-failover.state'
+    HOOKS_DIR  = '/etc/one-appliance/ha-hooks.d'
+    HOOK_TIMEOUT = 30
+    HOOK_LOG_TAIL = 4096
 
     STATE_TO_DIRECTION = {
         'BACKUP'  => :down,
@@ -143,6 +147,58 @@ def execute
         process_events
     end
 
+    # Runs /etc/one-appliance/ha-hooks.d/* with "up" (MASTER) or "down" (BACKUP).
+    # Hooks must never break the failover itself, so every failure is only logged.
+    # Each hook runs synchronously but bounded: output goes to a file (not a pipe,
+    # so leftover children cannot block us) and a hook past the timeout is killed
+    # together with its whole process group.
+    def run_hooks(direction, dir: HOOKS_DIR, timeout: HOOK_TIMEOUT)
+        Dir.glob(File.join(dir, '*')).sort.each do |hook|
+            next unless File.file?(hook) && File.executable?(hook)
+
+            run_hook hook, direction, timeout
+        end
+    end
+
+    def run_hook(hook, direction, timeout)
+        Tempfile.create('one-ha-hook') do |out|
+            pid = Process.spawn hook, direction.to_s, pgroup: true, [:out, :err] => out
+            status = wait_hook pid, Process.clock_gettime(Process::CLOCK_MONOTONIC) + timeout
+
+            if status.nil?
+                msg :error, "hook #{hook} #{direction} timed out after #{timeout}s, killed"
+            elsif !status.success?
+                msg :error, "hook #{hook} #{direction} failed (#{status.exitstatus || status}): #{hook_output out}"
+            end
+        end
+    rescue StandardError => e
+        msg :error, "hook #{hook} #{direction} raised: #{e.message}"
+    end
+
+    # Returns the exit status, or nil after killing the process group on timeout.
+    def wait_hook(pid, deadline)
+        loop do
+            _, status = Process.wait2 pid, Process::WNOHANG
+            return status if status
+            break if Process.clock_gettime(Process::CLOCK_MONOTONIC) >= deadline
+
+            sleep 0.05
+        end
+        begin
+            Process.kill 'KILL', -pid
+        rescue Errno::ESRCH, Errno::EPERM
+            nil
+        end
+        Process.wait pid
+        nil
+    end
+
+    def hook_output(file)
+        file.flush
+        file.seek [file.size - HOOK_LOG_TAIL, 0].max
+        file.read.to_s
+    end
+
     def stay
         msg :debug, ":STAY (pid = #{Process.pid})"
     end
@@ -169,11 +225,15 @@ def up
         end
 
         puts bash 'rc-update -v -u ||:', terminate: false
+
+        run_hooks :up # after the stock services: forwarding must not wait for hooks
     end
 
     def down
         msg :debug, ":DOWN (pid = #{Process.pid})"
 
+        run_hooks :down
+
         wait_ready :standby
 
         12.times do |attempt|

appliances/VRouter/Failover/tests.rb

@@ -0,0 +1,175 @@
+# frozen_string_literal: true
+
+require 'rspec'
+require 'tmpdir'
+require_relative '../../lib/helpers.rb'
+require_relative 'execute.rb'
+
+RSpec.describe Service::Failover do
+    let(:dir) { Dir.mktmpdir }
+    let(:out) { Dir.mktmpdir }
+    let(:log) { File.join(out, 'log') }
+    let(:errors) { [] }
+
+    before do
+        allow(described_class).to receive(:msg) { |level, text| errors << text if level == :error }
+    end
+
+    after do
+        FileUtils.remove_entry(dir)
+        FileUtils.remove_entry(out)
+    end
+
+    def alive?(pid)
+        Process.kill(0, pid)
+        true
+    rescue Errno::ESRCH
+        false
+    end
+
+    def hook(name, body, mode: 0o755)
+        path = File.join(dir, name)
+        File.write path, "#!/bin/sh\n#{body}\n"
+        File.chmod mode, path
+    end
+
+    it 'runs the hooks in name order with the direction as argument' do
+        hook '20-b', %(echo "b $1" >> #{log})
+        hook '10-a', %(echo "a $1" >> #{log})
+
+        described_class.run_hooks :up, dir: dir
+
+        expect(File.read(log)).to eq "a up\nb up\n"
+    end
+
+    it 'skips files that are not executable' do
+        hook '10-a', %(echo a >> #{log}), mode: 0o644
+
+        described_class.run_hooks :down, dir: dir
+
+        expect(File.exist?(log)).to be false
+    end
+
+    it 'keeps going and does not raise when a hook fails' do
+        hook '10-fail', 'exit 3'
+        hook '20-ok', %(echo ok >> #{log})
+
+        expect { described_class.run_hooks :down, dir: dir }.not_to raise_error
+        expect(File.read(log)).to eq "ok\n"
+    end
+
+    it 'does nothing when the hook directory does not exist' do
+        expect { described_class.run_hooks :up, dir: File.join(dir, 'missing') }.not_to raise_error
+    end
+
+    it 'logs a failing hook and runs the next one' do
+        hook '10-fail', 'echo boom; exit 3'
+        hook '20-ok', %(echo ok >> #{log})
+
+        described_class.run_hooks :down, dir: dir
+
+        expect(errors.join).to match(/10-fail.*failed \(3\).*boom/m)
+        expect(File.read(log)).to eq "ok\n"
+    end
+
+    it 'logs a signal-killed hook without an empty exit status' do
+        hook '10-sig', 'kill -9 $$'
+
+        described_class.run_hooks :down, dir: dir
+
+        expect(errors.join).to match(/10-sig/)
+        expect(errors.join).not_to match(/\(\)/)
+    end
+
+    it 'kills a hanging hook after the timeout and runs the next one' do
+        pidfile = File.join(out, 'pid')
+        hook '10-hang', %(echo $$ > #{pidfile}\nsleep 60)
+        hook '20-ok', %(echo ok >> #{log})
+
+        started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+        described_class.run_hooks :up, dir: dir, timeout: 1
+        elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
+
+        expect(elapsed).to be < 5
+        expect(alive?(File.read(pidfile).to_i)).to be false
+        expect(errors.join).to match(/10-hang.*timed out/)
+        expect(File.read(log)).to eq "ok\n"
+    end
+
+    it 'does not wait for a background child that outlives the hook' do
+        pidfile = File.join(out, 'pid')
+        hook '10-bg', %(sleep 30 &\necho $! > #{pidfile})
+
+        started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+        described_class.run_hooks :up, dir: dir, timeout: 10
+        elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
+
+        expect(elapsed).to be < 5
+    ensure
+        Process.kill('KILL', File.read(pidfile).to_i) if pidfile && File.exist?(pidfile)
+    end
+
+    it 'logs a hook that cannot be executed and does not raise' do
+        path = File.join(dir, '10-noshebang')
+        File.write path, "not a program\n"
+        File.chmod 0o755, path
+        hook '20-ok', %(echo ok >> #{log})
+
+        expect { described_class.run_hooks :up, dir: dir }.not_to raise_error
+        expect(errors.join).to match(/10-noshebang/)
+        expect(File.read(log)).to eq "ok\n"
+    end
+
+    describe 'hook ordering' do
+        let(:calls) { [] }
+
+        before do
+            allow(described_class).to receive(:wait_ready) { |role| calls << [:wait_ready, role] }
+            allow(described_class).to receive(:load_env)
+            allow(described_class).to receive(:puts)
+            allow(described_class).to receive(:sleep)
+            allow(described_class).to receive(:bash) { |script, **| calls << [:bash, script]; '' }
+            allow(described_class).to receive(:run_hooks) { |direction| calls << [:hooks, direction] }
+        end
+
+        it 'runs the up hooks only after the stock services were restarted' do
+            ENV['ONEAPP_VNF_ROUTER4_ENABLED'] = 'YES'
+
+            described_class.up
+
+            restarts = calls.each_index.select { |i| calls[i].first == :bash && calls[i].last.include?('restart') }
+            expect(restarts).not_to be_empty
+            expect(calls.index([:hooks, :up])).to be > restarts.max
+            expect(calls.last).to eq [:hooks, :up]
+        ensure
+            ENV.delete 'ONEAPP_VNF_ROUTER4_ENABLED'
+        end
+
+        it 'runs the down hooks first, before waiting for keepalived and stopping services' do
+            described_class.down
+
+            expect(calls.first).to eq [:hooks, :down]
+            expect(calls.index([:wait_ready, :standby])).to eq 1
+        end
+    end
+
+    describe 'VRRP instance events' do
+        it 'ignores the events of a single instance' do
+            allow(described_class).to receive(:load_state).and_return(state: +'BACKUP')
+            expect(described_class).not_to receive(:save_state)
+
+            task = described_class.to_task(described_class.to_event('INSTANCE "ETH1" MASTER 100'))
+
+            expect(task).to include(ignored: true, direction: :stay)
+        end
+
+        it 'reacts to the group event only' do
+            allow(described_class).to receive(:load_state).and_return(state: +'BACKUP')
+            allow(described_class).to receive(:save_state)
+
+            task = described_class.to_task(described_class.to_event('GROUP "VRouter" MASTER 100'))
+
+            expect(task).to include(ignored: false, direction: :up)
+        end
+    end
+end