fix(metrics): keep VMs cloned from one image apart in the instance label

CHANGELOG.md

@@ -24,6 +24,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- The metrics `instance` label now also mixes in the cloud instance id and the
+  permanent hardware NIC addresses (virtual, bridge and boot-random addresses
+  are ignored). VMs cloned from one image share `/etc/machine-id`, and when the
+  hypervisor gives them the same or no system UUID they previously collapsed
+  into one `instance` and overwrote each other's series. **Upgrading changes
+  `instance` once on every host that has a cloud instance id or a hardware NIC.**
+  Start with `--legacy-instance-id` (`LEGACY_INSTANCE_ID=true`) to keep the
+  previous value.
 - Prometheus `job` label and outbound `User-Agent` are now
   `nudgebee-node-agent`. Update any dashboards or alerts that filter on
   `job="coroot-node-agent"`.

flags/flags.go

@@ -71,6 +71,7 @@ var (
 
 	ScrapeInterval                        = kingpin.Flag("scrape-interval", "How often to gather metrics from the agent").Default("15s").Envar("SCRAPE_INTERVAL").Duration()
 	WalDir                                = kingpin.Flag("wal-dir", "Path to where the agent stores data (e.g. the metrics Write-Ahead Log)").Default("/tmp/nudgebee-node-agent").Envar("WAL_DIR").String()
+	LegacyInstanceID                      = kingpin.Flag("legacy-instance-id", "Derive the metrics `instance` label from machine-id and system UUID only, as releases before cloned-VM support did. Keeps existing series after an upgrade, but VMs cloned from one image whose system UUID is missing or identical share an instance").Default("false").Envar("LEGACY_INSTANCE_ID").Bool()
 	MaxSpoolSize                          = kingpin.Flag("max-spool-size", "Maximum size of the on-disk spool used to buffer data when it cannot be sent to collector. Supports size suffixes like KB, MB, or GB.").Default("500MB").Envar("MAX_SPOOL_SIZE").Bytes()
 	ResolveDns                            = kingpin.Flag("resolve-dns", "should resolve DNS").Default("false").Envar("RESOLVE_DNS").Bool()
 	IgnoreControlPlane                    = kingpin.Flag("ignore-control-plane", "ignore control plane like loki").Default("karpenter,loki,prometheus,grafana,kubelet,etcd,apiserver,victoria,nudgebee-agent,kube-system").Envar("IGNORE_CONTROL_PLANE").String()

main.go

@@ -261,7 +261,12 @@ func main() {
 	profiling.Start()
 	defer profiling.Stop()
 
-	if err := prom.StartAgent(registry, machineId, systemUuid); err != nil {
+	cloudInstanceID := ""
+	if md := nodeCollector.Metadata(); md != nil {
+		cloudInstanceID = md.InstanceId
+	}
+	identityExtras := prom.HostIdentityExtras(cloudInstanceID, proc.HostPath("/sys/class/net"))
+	if err := prom.StartAgent(registry, machineId, systemUuid, identityExtras); err != nil {
 		klog.Exitln(err)
 	}
 

prom/instance.go

@@ -0,0 +1,91 @@
+package prom
+
+import (
+	"crypto/md5"
+	"encoding/hex"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+)
+
+// InstanceID derives the `instance` label that tells one machine's series from
+// another's in the metrics store.
+//
+// machine-id alone is not enough: VMs cloned from one image share it. The system
+// UUID (SMBIOS product_uuid) usually differs per clone, so it is folded in. But
+// when the hypervisor reports no UUID, or the same one for a copied disk, two
+// clones collapse into one instance and their series overwrite each other. The
+// extras (cloud instance id, hardware NIC addresses) are per-machine inputs that
+// still differ in those cases.
+//
+// With no extras the result is byte-for-byte what earlier releases produced, so a
+// host that has nothing extra to add keeps its existing series.
+func InstanceID(machineID, systemUUID string, extras ...string) string {
+	uuid := strings.ReplaceAll(systemUUID, "-", "")
+	var parts []string
+	if uuid != "" && uuid != machineID {
+		parts = append(parts, uuid)
+	}
+	for _, e := range extras {
+		if e != "" {
+			// A separator keeps ("ab","c") and ("a","bc") from hashing alike.
+			parts = append(parts, "\x00"+e)
+		}
+	}
+	if len(parts) == 0 {
+		return machineID
+	}
+	h := md5.New()
+	h.Write([]byte(machineID))
+	for _, p := range parts {
+		h.Write([]byte(p))
+	}
+	return hex.EncodeToString(h.Sum(nil))
+}
+
+// HostIdentityExtras collects the per-machine inputs InstanceID mixes in beyond
+// machine-id and system UUID: the cloud instance id when running in a cloud, and
+// the permanent hardware addresses of the host's NICs.
+func HostIdentityExtras(cloudInstanceID, sysClassNet string) []string {
+	var extras []string
+	if cloudInstanceID != "" {
+		extras = append(extras, "cloud:"+cloudInstanceID)
+	}
+	for _, mac := range hardwareMACs(sysClassNet) {
+		extras = append(extras, "mac:"+mac)
+	}
+	return extras
+}
+
+// hardwareMACs lists the sorted, permanent MAC addresses of real NICs under
+// /sys/class/net. Interfaces without a backing device (bridges, veth, docker0,
+// tunnels) are skipped, as are addresses the kernel generated at boot
+// (addr_assign_type 1, random): those would change identity on every restart.
+func hardwareMACs(sysClassNet string) []string {
+	entries, err := os.ReadDir(sysClassNet)
+	if err != nil {
+		return nil
+	}
+	var macs []string
+	for _, e := range entries {
+		dir := filepath.Join(sysClassNet, e.Name())
+		if _, err := os.Stat(filepath.Join(dir, "device")); err != nil {
+			continue
+		}
+		if t, err := os.ReadFile(filepath.Join(dir, "addr_assign_type")); err == nil && strings.TrimSpace(string(t)) == "1" {
+			continue
+		}
+		b, err := os.ReadFile(filepath.Join(dir, "address"))
+		if err != nil {
+			continue
+		}
+		mac := strings.ToLower(strings.TrimSpace(string(b)))
+		if mac == "" || mac == "00:00:00:00:00:00" {
+			continue
+		}
+		macs = append(macs, mac)
+	}
+	sort.Strings(macs)
+	return macs
+}

prom/instance_test.go

@@ -0,0 +1,139 @@
+package prom
+
+import (
+	"crypto/md5"
+	"encoding/hex"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// legacy is the derivation earlier releases used; InstanceID must reproduce it
+// when there is nothing extra to add, so hosts keep their existing series.
+func legacy(machineID, systemUUID string) string {
+	instance := machineID
+	if s := systemUUIDNoDashes(systemUUID); s != "" && s != machineID {
+		h := md5.New()
+		h.Write([]byte(machineID))
+		h.Write([]byte(s))
+		instance = hex.EncodeToString(h.Sum(nil))
+	}
+	return instance
+}
+
+func systemUUIDNoDashes(s string) string {
+	out := ""
+	for _, r := range s {
+		if r != '-' {
+			out += string(r)
+		}
+	}
+	return out
+}
+
+func TestInstanceID_MatchesLegacyWithoutExtras(t *testing.T) {
+	for _, c := range []struct{ machine, uuid string }{
+		{"aaaa", "11111111-2222-3333-4444-555555555555"},
+		{"aaaa", ""},
+		{"aaaa", "aaaa"},
+		{"", ""},
+	} {
+		if got, want := InstanceID(c.machine, c.uuid), legacy(c.machine, c.uuid); got != want {
+			t.Errorf("InstanceID(%q,%q) = %q, legacy %q", c.machine, c.uuid, got, want)
+		}
+	}
+}
+
+// The failure this exists for: clones share machine-id and the hypervisor gives
+// them the same (or no) system UUID, so the legacy derivation cannot tell them apart.
+func TestInstanceID_ClonesWithSameMachineAndUUIDDiffer(t *testing.T) {
+	const machine, uuid = "aaaa", "11111111-2222-3333-4444-555555555555"
+	if legacy(machine, uuid) != legacy(machine, uuid) {
+		t.Fatal("legacy must be deterministic")
+	}
+	a := InstanceID(machine, uuid, "mac:52:54:00:00:00:01")
+	b := InstanceID(machine, uuid, "mac:52:54:00:00:00:02")
+	if a == b {
+		t.Fatal("clones with different NICs must get different instances")
+	}
+	c := InstanceID(machine, "", "cloud:i-0abc")
+	d := InstanceID(machine, "", "cloud:i-0def")
+	if c == d {
+		t.Fatal("clones with different cloud instance ids must get different instances")
+	}
+}
+
+func TestInstanceID_Stable(t *testing.T) {
+	a := InstanceID("m", "u", "cloud:i-1", "mac:aa", "mac:bb")
+	if b := InstanceID("m", "u", "cloud:i-1", "mac:aa", "mac:bb"); a != b {
+		t.Fatalf("not stable: %q vs %q", a, b)
+	}
+}
+
+func TestInstanceID_ExtrasAreDelimited(t *testing.T) {
+	if InstanceID("m", "", "ab", "c") == InstanceID("m", "", "a", "bc") {
+		t.Fatal("extras must not run together")
+	}
+}
+
+func TestInstanceID_EmptyExtrasIgnored(t *testing.T) {
+	if InstanceID("m", "u", "", "") != InstanceID("m", "u") {
+		t.Fatal("empty extras must not change the instance")
+	}
+}
+
+func writeNIC(t *testing.T, root, name string, device bool, address, assign string) {
+	t.Helper()
+	dir := filepath.Join(root, name)
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if device {
+		if err := os.Mkdir(filepath.Join(dir, "device"), 0o755); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := os.WriteFile(filepath.Join(dir, "address"), []byte(address+"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if assign != "" {
+		if err := os.WriteFile(filepath.Join(dir, "addr_assign_type"), []byte(assign+"\n"), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+}
+
+func TestHardwareMACs(t *testing.T) {
+	root := t.TempDir()
+	writeNIC(t, root, "ens3", true, "52:54:00:AB:CD:02", "0") // real NIC, upper case
+	writeNIC(t, root, "ens4", true, "52:54:00:ab:cd:01", "0")
+	writeNIC(t, root, "docker0", false, "02:42:ac:11:00:01", "") // no backing device
+	writeNIC(t, root, "veth1", false, "de:ad:be:ef:00:01", "1")  // virtual, random
+	writeNIC(t, root, "ens5", true, "7a:00:00:00:00:09", "1")    // random at boot: unstable
+	writeNIC(t, root, "ens6", true, "00:00:00:00:00:00", "0")    // unset
+	writeNIC(t, root, "lo", false, "00:00:00:00:00:00", "")
+
+	got := hardwareMACs(root)
+	want := []string{"52:54:00:ab:cd:01", "52:54:00:ab:cd:02"}
+	if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] {
+		t.Fatalf("got %v, want %v", got, want)
+	}
+}
+
+func TestHardwareMACs_MissingDir(t *testing.T) {
+	if got := hardwareMACs(filepath.Join(t.TempDir(), "nope")); got != nil {
+		t.Fatalf("got %v", got)
+	}
+}
+
+func TestHostIdentityExtras(t *testing.T) {
+	root := t.TempDir()
+	writeNIC(t, root, "ens3", true, "52:54:00:00:00:01", "0")
+	got := HostIdentityExtras("i-0abc", root)
+	if len(got) != 2 || got[0] != "cloud:i-0abc" || got[1] != "mac:52:54:00:00:00:01" {
+		t.Fatalf("got %v", got)
+	}
+	if got := HostIdentityExtras("", filepath.Join(root, "missing")); len(got) != 0 {
+		t.Fatalf("got %v", got)
+	}
+}

prom/remote_writer.go

@@ -2,8 +2,6 @@ package prom
 
 import (
 	"bytes"
-	"crypto/md5"
-	"encoding/hex"
 	"errors"
 	"fmt"
 	"net/http"
@@ -39,7 +37,9 @@ type Agent struct {
 	maxSpoolSize int64
 }
 
-func StartAgent(reg *prometheus.Registry, machineId, systemUuid string) error {
+// StartAgent starts remote-writing reg. identityExtras are per-machine inputs
+// (see HostIdentityExtras) that keep clones of one image apart in the store.
+func StartAgent(reg *prometheus.Registry, machineId, systemUuid string, identityExtras []string) error {
 	if *flags.MetricsEndpoint == nil {
 		return nil
 	}
@@ -49,13 +49,11 @@ func StartAgent(reg *prometheus.Registry, machineId, systemUuid string) error {
 	up.Set(1)
 	reg.MustRegister(up)
 
-	instance := machineId
-	if s := strings.ReplaceAll(systemUuid, "-", ""); s != "" && s != machineId {
-		hash := md5.New()
-		hash.Write([]byte(machineId))
-		hash.Write([]byte(s))
-		instance = hex.EncodeToString(hash.Sum(nil))
+	if *flags.LegacyInstanceID {
+		identityExtras = nil
 	}
+	instance := InstanceID(machineId, systemUuid, identityExtras...)
+	klog.Infoln("metrics instance:", instance)
 
 	a := &Agent{
 		reg: reg,