Repository navigation
fix(metrics): keep VMs cloned from one image apart in the instance label - #384
PrashantBtkl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for installing the agent across a fleet of hosts using Ansible or SSH loops, complete with documentation and a playbook. It also enhances the metrics instance label derivation by mixing in cloud instance IDs and hardware MAC addresses to prevent cloned VMs from overwriting each other's metrics, while providing a --legacy-instance-id flag to preserve the old behavior. Regarding the feedback, there is a type mismatch issue in the Ansible playbook where service.status.NRestarts (which can be an integer) is compared to the string '0', potentially causing false assertion failures on healthy hosts. It is recommended to use the int filter for a type-safe comparison.
instance = md5(machine-id + system-uuid) collapses clones that share /etc/machine-id when the hypervisor reports no system UUID or the same one, merging their series in the store. Mix in the cloud instance id and the permanent hardware NIC addresses (skipping virtual, bridge and boot-random ones). With no extras the value is unchanged. --legacy-instance-id keeps the previous derivation. Upgrading changes instance once on hosts that have a cloud instance id or a hardware NIC. Refs nudgebee/nudgebee-enterprise#40582 Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
9d428d7 to
c17188e
Compare
mayankpande88
left a comment
There was a problem hiding this comment.
Moving this to draft: the direction is right, but as written it changes every push-mode host's instance on upgrade, and the new identity isn't stable across restarts.
1. Default-on identity change for every push-mode host, mostly without benefit. StartAgent returns early without METRICS_ENDPOINT, so this affects push-mode (VM) installs, and all of them change on upgrade. Cloud hosts gain nothing: their SMBIOS system UUID is already unique per instance, yet the cloud id now changes their instance too. A collision needs an empty, bogus or duplicated UUID. Suggestion: keep the legacy value when the system UUID is present and plausible, and mix in extras only when it is empty, all-zero/all-F, or equal to machine-id. Handle "clones with an identical UUID" (e.g. VMware uuid.action=keep, a copied libvirt domain XML) with an opt-in flag, rather than a breaking default with an opt-out.
2. The identity can change between restarts of the same host.
- Cloud id:
NewCollectorfetches metadata synchronously with a 5s timeout; an IMDS token failure or timeout at startup returns nil metadata, so that run gets the no-cloud-idinstance, and a host can flip between two identities. Dropping the cloud id fixes this, and per point 1 it adds no collision protection. - Duplicate MACs (
hardwareMACs): with Azure accelerated networking the VF has the synthetic NIC's MAC and is removed/re-added during host servicing, so the list (and the identity) depends on whether the VF was present at start. Bond slaves all report the bond's MAC. Please deduplicate. - NIC set: attaching a second NIC or hot-plugging one changes the identity at the next restart.
- Non-permanent addresses: only
addr_assign_type == 1(random) is skipped;3(set from userspace, e.g. NetworkManager's per-boot cloned/random MAC) is just as unstable. Accept only0(permanent).
3. Clones still collide outside the series store. Every series still carries identical machine_id and system_uuid const labels, and logs/traces use host.id = machineId, so anything grouping by those still merges clones. That's fine for a partial fix, but the CHANGELOG entry reads as a complete one.
4. Not run end to end yet (the two unchecked test-plan boxes). Worth covering: two VMs sharing machine-id with no or identical system UUID get distinct instance; one host keeps the same instance across restarts with the metadata service unreachable and with a NIC added; --legacy-instance-id restores the old value.
5. PR description: it links an issue in a private repository; please drop that link from this public PR.
Minor:
Summary
Part of nudgebee/nudgebee-enterprise#40582 ("two VMs cloned from one image show as two VMs").
instancewasmd5(machine-id + system-uuid). Clones share/etc/machine-id; when the hypervisor reports no system UUID, or the same one for a copied disk, the hash is identical and the VMs' series merge in the store.prom.InstanceIDalso mixes in the cloud instance id (when in a cloud) and the permanent hardware NIC addresses from/sys/class/net. NICs with no backing device (docker0, veth, bridges) and boot-random addresses are skipped so identity doesn't change on restart.--legacy-instance-id/LEGACY_INSTANCE_ID=truekeeps the previous value.Upgrade impact
instancechanges once on every host that has a cloud instance id or a hardware NIC, so its series restarts and dashboards/alerts filtering on the old hash break. Use the legacy flag to avoid that. Noted in CHANGELOG.Test plan
go test ./prom/(legacy parity, clone separation, stability, delimiting, NIC filtering)go vet ./prom/ ./flags/main.gowiring not compiled locally: the repo doesn't build on my machine either way (missingsystemd/sd-journal.h); CI will cover itinstance; confirm--legacy-instance-idrestores the old value🤖 Generated with Claude Code