fix(metallb): verify the metallb-system namespace actually exists (#702)

* fix(metallb): verify the metallb-system namespace actually exists

- change the Test metallb-system namespace task to run
  k3s kubectl get namespace metallb-system instead of the bare
  -n metallb-system, which printed kubectl usage and always exited 0
- add a regression test that asserts the task uses an explicit get
  and fails if it ever regresses to the usage-only form
- wire the new test into pre-commit

* fix(metallb): retry the namespace check on transient kube API errors

- retry the explicit get namespace check until it succeeds
- extend the regression test to assert the retry wiring

* fix(metallb): retry the webhook endpoint check on transient kube API errors

- retry the webhook-service endpoint get until it succeeds
- extend the regression test to cover the webhook task too

.github/scripts/test-disable-swap.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+set -Eeuo pipefail
+
+repo_root="$(git rev-parse --show-toplevel)"
+prereq_defaults="$repo_root/roles/prereq/defaults/main.yml"
+prereq_tasks="$repo_root/roles/prereq/tasks/main.yml"
+
+# #670: k3s recommends swap be disabled on all nodes. The prereq role must expose
+# a disable_swap toggle (defaulting to true) that turns swap off now and comments
+# out the /etc/fstab swap entries so swap stays off across reboots.
+grep -Eq -- '^disable_swap: true' "$prereq_defaults" || {
+  printf 'prereq defaults are missing disable_swap: true\n' >&2
+  exit 1
+}
+
+grep -Fq -- 'Disable swap on all cluster nodes' "$prereq_tasks" || {
+  printf 'prereq tasks are missing the swap-disable block\n' >&2
+  exit 1
+}
+
+grep -Fq -- 'swapoff -a' "$prereq_tasks" || {
+  printf 'swap-disable block does not run swapoff -a\n' >&2
+  exit 1
+}
+
+grep -Fq -- '/etc/fstab' "$prereq_tasks" || {
+  printf 'swap-disable block does not comment out /etc/fstab swap entries\n' >&2
+  exit 1
+}
+
+if ! grep -Eq -- 'when: disable_swap' "$prereq_tasks"; then
+  printf 'swap-disable block is not gated on the disable_swap toggle\n' >&2
+  exit 1
+fi
+
+printf 'Swap disable regression test passed\n'

.github/scripts/test-kube-vip-manifest.py

@@ -123,6 +123,38 @@ def main():
     if "name: bgp_peers" in output:
         fail("bgp_peers present even though the peer list is empty")
 
+    # kube_vip_endpoint defaults to null (defined in role defaults): the
+    # address and subnet must fall back to the apiserver endpoint. default()
+    # without a truthy flag does NOT fall back on null, only on undefined, so
+    # this case pins the null runtime condition to prevent that regression.
+    output = render(
+        env,
+        {
+            "_kube_vip_bgp_peers": [],
+            "kube_vip_endpoint": None,
+            "kube_vip_arp": True,
+            "kube_vip_bgp": False,
+        },
+    )
+    if "value: 192.168.30.222" not in output:
+        fail("null kube_vip_endpoint does not fall back to apiserver_endpoint")
+
+    # kube_vip_endpoint set: overrides the internal listening address AND the
+    # subnet derivation while the advertised apiserver_endpoint stays separate.
+    output = render(
+        env,
+        {
+            "_kube_vip_bgp_peers": [],
+            "kube_vip_endpoint": "10.66.1.5",
+            "kube_vip_arp": True,
+            "kube_vip_bgp": False,
+        },
+    )
+    if "value: 10.66.1.5" not in output:
+        fail("kube_vip_endpoint did not override the address")
+    if "value: 192.168.30.222" in output:
+        fail("apiserver_endpoint leaked into address when kube_vip_endpoint set")
+
     print("kube-vip manifest regression test passed")
 
 

.github/scripts/test-metallb-namespace.py

@@ -0,0 +1,108 @@
+#!/usr/bin/env python3
+"""Regression test for the MetalLB converge checks.
+
+The MetalLB tasks in roles/k3s_server_post/tasks/metallb.yml must actually
+verify resources through an explicit kubectl get, and must retry on a
+transient kube API error while MetalLB converges.
+
+The "Test metallb-system namespace" task previously ran `k3s kubectl -n
+metallb-system` with no subcommand, which only printed a usage page and always
+exited 0, so it always succeeded even when the namespace did not exist (issue
+#350). It must instead run an explicit `get namespace metallb-system`, which
+returns non-zero when the namespace is absent.
+
+An explicit get actually contacts the API server, so these tasks need the same
+retry wiring as their siblings (register, until rc == 0, retries, delay). A
+bare get with no retry would otherwise abort the converge play on a transient
+kube API error while MetalLB converges.
+"""
+
+from __future__ import print_function
+
+import os
+import subprocess
+
+import yaml
+
+
+def repo_root():
+    return subprocess.check_output(
+        ["git", "rev-parse", "--show-toplevel"], text=True
+    ).strip()
+
+
+def fail(message):
+    raise SystemExit("MetalLB namespace test failed: " + message)
+
+
+def find_task(tasks, name):
+    for entry in tasks:
+        if entry.get("name") == name:
+            return entry
+    fail("could not find the '{0}' task".format(name))
+    return None
+
+
+def command_text(task):
+    cmd = task.get("ansible.builtin.command")
+    if not cmd:
+        cmd = task.get("command")
+    if not cmd:
+        fail("task does not use ansible.builtin.command")
+    return cmd if isinstance(cmd, str) else " ".join(cmd)
+
+
+def check_explicit_get(task, name, needle):
+    text = command_text(task)
+    if needle not in text:
+        fail(
+            "command does not run '{0}'; the task would only print usage and "
+            "never verify the resource (got: {1!r})".format(needle, text)
+        )
+
+
+def check_retry_wiring(task, name):
+    # The sibling k3s_server_post metallb tasks retry kubectl because the kube
+    # API can briefly be unavailable while MetalLB converges. Without the same
+    # retry, a transient API error aborts the whole converge play.
+    if not task.get("register"):
+        fail(
+            "{0} does not register a result; without retry wiring a transient "
+            "kube API error aborts the converge play".format(name)
+        )
+    if not isinstance(task.get("until"), str) or "rc == 0" not in task["until"]:
+        fail(
+            "{0} does not retry on rc == 0; the kube API can transiently fail "
+            "while MetalLB converges and abort the play".format(name)
+        )
+    if task.get("retries") is None:
+        fail("{0} is missing retries".format(name))
+    if task.get("delay") is None:
+        fail("{0} is missing delay".format(name))
+
+
+def main():
+    task_file = os.path.join(
+        repo_root(), "roles", "k3s_server_post", "tasks", "metallb.yml"
+    )
+    with open(task_file, encoding="utf-8") as handle:
+        tasks = yaml.safe_load(handle)
+
+    namespace_task = find_task(tasks, "Test metallb-system namespace")
+    # A bare `-n metallb-system` with no subcommand prints kubectl usage and
+    # always exits 0, so it never proves the namespace exists. The fix must
+    # use an explicit get.
+    check_explicit_get(namespace_task, "Test metallb-system namespace",
+                       "get namespace metallb-system")
+    check_retry_wiring(namespace_task, "Test metallb-system namespace")
+
+    webhook_task = find_task(tasks, "Test metallb-system webhook-service endpoint")
+    check_explicit_get(webhook_task, "Test metallb-system webhook-service endpoint",
+                       "get endpoints")
+    check_retry_wiring(webhook_task, "Test metallb-system webhook-service endpoint")
+
+    print("MetalLB namespace check regression test passed")
+
+
+if __name__ == "__main__":
+    main()

.github/workflows/test.yml

@@ -88,7 +88,7 @@ jobs:
           trap stop_monitor EXIT
           /usr/bin/time -v -o "$timing_file" \
             molecule test --scenario-name ${{ matrix.scenario }}
-        timeout-minutes: 150
+        timeout-minutes: 180
         env:
           ANSIBLE_K3S_LOG_DIR: ${{ runner.temp }}/logs/k3s-ansible/${{ matrix.scenario }}
           ANSIBLE_SSH_RETRIES: 4

.pre-commit-config.yaml

@@ -68,6 +68,12 @@ repos:
         language: system
         pass_filenames: false
         files: ^site\.yml$|^\.github/scripts/test-unique-hostname-precheck\.sh$
+      - id: disable-swap-test
+        name: Disable swap test
+        entry: .github/scripts/test-disable-swap.sh
+        language: system
+        pass_filenames: false
+        files: ^roles/prereq/(tasks/main|defaults/main)\.yml$|^\.github/scripts/test-disable-swap\.sh$
       - id: cilium-bgp-manifest-test
         name: Cilium BGP manifest test
         entry: python3 .github/scripts/test-cilium-bgp-manifest.py
@@ -107,6 +113,14 @@ repos:
           - Jinja2>=3.1
         pass_filenames: false
         files: ^roles/k3s_server_post/templates/metallb\.crs\.j2$|^\.github/scripts/test-metallb-interfaces\.py$
+      - id: metallb-namespace-test
+        name: MetalLB namespace test
+        entry: python3 .github/scripts/test-metallb-namespace.py
+        language: python
+        additional_dependencies:
+          - PyYAML
+        pass_filenames: false
+        files: ^roles/k3s_server_post/tasks/metallb\.yml$|^\.github/scripts/test-metallb-namespace\.py$
       - id: metallb-deploy-condition-test
         name: MetalLB deploy condition test
         entry: python3 .github/scripts/test-metallb-deploy-condition.py

README.md

@@ -219,6 +219,7 @@ See the commands [here](https://technotim.com/posts/k3s-etcd-ansible/#testing-yo
 | `k3s_server` | `kube_vip_bgp_peers` | list | `[]` | Not required | List of BGP peer ASN & address pairs |
 | `k3s_server` | `kube_vip_bgp_peers_groups` | list | `['k3s_master']` | Not required | Inventory group in which to search for additional `kube_vip_bgp_peers` parameters to merge. |
 | `k3s_server` | `kube_vip_iface` | string | `~` | Not required | Explicitly define an interface that ALL control nodes should use to propagate the VIP, define it here. Otherwise, kube-vip will determine the right interface automatically at runtime. |
+| `k3s_server` | `kube_vip_endpoint` | string | `~` | Not required | Overrides the internal address kube-vip binds/listens on, which can differ from the announced apiserver_endpoint for complex routing/tunnels. Defaults to apiserver_endpoint. |
 | `k3s_server` | `kube_vip_tag_version` | string | `v1.2.2` | Not required | Image tag for kube-vip |
 | `k3s_server` | `kube_vip_cloud_provider_tag_version` | string | `v0.0.12` | Not required | Tag for kube-vip-cloud-provider manifest when enable |
 | `k3s_server`, `k3_server_post` | `kube_vip_lb_ip_range` | string | `~` | Not required | IP range for kube-vip load balancer |
@@ -261,6 +262,7 @@ See the commands [here](https://technotim.com/posts/k3s-etcd-ansible/#testing-yo
 | `reboot` (playbook) | `concurrent_reboots` | int/string | `100%` | Not required | Number (or percentage) of nodes to reboot at a time for a staggered reboot |
 | `reboot` (playbook) | `wait_seconds_after_reboot` | int | `0` | Not required | Pause in seconds between staggered reboot batches |
 | `prereq` | `system_timezone` | string | `null` | Not required | Timezone to be set on all nodes |
+| `prereq` | `disable_swap` | bool | `true` | Not required | Disable swap on all cluster nodes (swapoff + comment out /etc/fstab swap entries), all-or-nothing |
 | `proxmox_lxc`, `reset_proxmox_lxc` | `proxmox_lxc_ct_ids` | list | ❌ | Required | Proxmox container ID list |
 | `raspberrypi` | `state` | string | `present` | Not required | Indicates whether the k3s prerequisites for Raspberry Pi should be set up (possible values are `present` and `absent`) |
 

inventory/sample/group_vars/all.yml

@@ -7,6 +7,10 @@ systemd_dir: /etc/systemd/system
 # Set your timezone
 system_timezone: Your/Timezone
 
+# k3s recommends swap be disabled on every cluster node. Applied uniformly to all
+# nodes (all-or-nothing) in the prereq role. Set to false to leave swap enabled.
+disable_swap: true
+
 # interface which will be used for flannel
 # Defaults to each host's default IPv4 interface (e.g. eth0, enp1s0, ens3)
 # so KVM/cloud hosts without eth0 work out of the box. Override per-host if needed.
@@ -44,6 +48,11 @@ cilium_bgp_lb_cidr: 192.168.31.0/24 # cidr for cilium loadbalancer ipam
 # enable kube-vip ARP broadcasts
 kube_vip_arp: true
 
+# (optional) overrides the address kube-vip binds/listens on internally, which
+# can differ from the announced apiserver_endpoint for complex routing/tunnels.
+# Defaults to apiserver_endpoint. Also used to derive the kube-vip subnet.
+# kube_vip_endpoint: 10.66.1.5
+
 # enable kube-vip BGP peering
 kube_vip_bgp: false
 

roles/k3s_server/defaults/main.yml

@@ -7,6 +7,7 @@ group_name_master: master
 
 kube_vip_arp: true
 kube_vip_iface:
+kube_vip_endpoint:
 kube_vip_cloud_provider_tag_version: v0.0.12
 kube_vip_tag_version: v1.2.2
 

roles/k3s_server/meta/main.yml

@@ -78,6 +78,15 @@ argument_specs:
           - automatically at runtime.
         default: ~
 
+      kube_vip_endpoint:
+        description:
+          - Overrides the address kube-vip binds/listens on internally, which
+          - can differ from the announced apiserver_endpoint for complex
+          - routing and site-to-site tunnels.
+          - Defaults to apiserver_endpoint and is used to derive the kube-vip
+          - subnet.
+        default: ~
+
       kube_vip_tag_version:
         description: Image tag for kube-vip
         default: v1.2.2

roles/k3s_server/templates/vip.yaml.j2

@@ -37,7 +37,7 @@ spec:
           value: {{ kube_vip_iface }}
 {% endif %}
         - name: vip_subnet
-          value: "{{ apiserver_endpoint | ansible.utils.ipsubnet | ansible.utils.ipaddr('prefix') }}"
+          value: "{{ (kube_vip_endpoint | default(apiserver_endpoint, true)) | ansible.utils.ipsubnet | ansible.utils.ipaddr('prefix') }}"
         - name: cp_enable
           value: "true"
         - name: cp_namespace
@@ -55,7 +55,7 @@ spec:
         - name: vip_retryperiod
           value: "2"
         - name: address
-          value: {{ apiserver_endpoint }}
+          value: {{ kube_vip_endpoint | default(apiserver_endpoint, true) }}
 {% if kube_vip_bgp | default(false) | bool %}
 {% if kube_vip_bgp_routerid is defined %}
         - name: bgp_routerid

roles/k3s_server_post/tasks/metallb.yml

@@ -40,8 +40,14 @@
 
 - name: Test metallb-system namespace
   ansible.builtin.command: >-
-    {{ k3s_kubectl_binary | default('k3s kubectl') }} -n metallb-system
+    {{ k3s_kubectl_binary | default('k3s kubectl') }} get namespace metallb-system
   changed_when: false
+  # The kube API can briefly return ServiceUnavailable while MetalLB converges,
+  # which would otherwise abort the whole converge play on a transient error.
+  register: metallb_namespace_result
+  until: metallb_namespace_result.rc == 0
+  retries: "{{ download_retries }}"
+  delay: "{{ download_delay }}"
   with_items: "{{ groups[group_name_master | default('master')] }}"
   run_once: true
 
@@ -99,6 +105,12 @@
   ansible.builtin.command: >-
     {{ k3s_kubectl_binary | default('k3s kubectl') }} -n metallb-system get endpoints {{ metallb_webhook_service_name }}
   changed_when: false
+  # The kube API can briefly return ServiceUnavailable while MetalLB converges,
+  # which would otherwise abort the whole converge play on a transient error.
+  register: metallb_webhook_result
+  until: metallb_webhook_result.rc == 0
+  retries: "{{ download_retries }}"
+  delay: "{{ download_delay }}"
   with_items: "{{ groups[group_name_master | default('master')] }}"
   run_once: true
 

roles/prereq/defaults/main.yml

@@ -1,4 +1,6 @@
 ---
+disable_swap: true
+
 secure_path:
   RedHat: /sbin:/bin:/usr/sbin:/usr/bin:/usr/local/bin
   Suse: /usr/sbin:/usr/bin:/sbin:/bin:/usr/local/bin

roles/prereq/tasks/main.yml

@@ -4,6 +4,28 @@
     name: "{{ system_timezone }}"
   when: (system_timezone is defined) and (system_timezone != "Your/Timezone")
 
+# k3s recommends swap be disabled on all nodes. Disabling swap is all-or-nothing
+# across the cluster: leaving it enabled on some nodes but not others creates
+# uneven scheduling/latency behavior. This block turns swap off and comments out
+# the swap entries in /etc/fstab so it stays off across reboots. It is idempotent
+# and a no-op when swap is already disabled or swapoff is unavailable.
+- name: Disable swap on all cluster nodes
+  when: disable_swap
+  block:
+    - name: Turn off swap now
+      ansible.builtin.command: swapoff -a
+      register: swapoff_result
+      changed_when: false
+      failed_when: false
+
+    - name: Comment out swap entries in fstab
+      ansible.builtin.replace:
+        path: /etc/fstab
+        regexp: '^([^#][^\n]*\s+swap\s+)'
+        replace: '# \\1'
+      register: fstab_swap
+
+
 - name: Set SELinux to disabled state
   ansible.posix.selinux:
     state: disabled