feat(watcher): move the decision engine onto prometheus and supply the instance metrics it needs - #1448
Conversation
traviswu-bigstack
left a comment
There was a problem hiding this comment.
Let's use .diff for all following openstack patches and drop the .py file
224ae2f to
2d51173
Compare
SekiXu
left a comment
There was a problem hiding this comment.
Verified the three claims this PR rests on against the sources rather than taking them from the body.
The patch convention checks out, byte for byte.
- All three
.origfiles are byte-identical to upstreampython-watcher12.1.0 (watcher/conf/__init__.py,watcher/decision_engine/datasources/manager.py,watcher/decision_engine/strategy/strategies/workload_balance.py), so "pristine" is literally true and a 12.2.0 bump will conflict loudly instead of being absorbed. datasources/prometheus.pyandconf/prometheus_client.pyare byte-identical to upstreamstable/2025.1. The "taken verbatim" claim holds, which is what makes the backport droppable on Epoxy.- All three
.py.patchfiles apply to their.origat rc=0. On a second run thepatch --dry-run --reversepre-check incore/watcher/watcher.mkcorrectly reports every one as already applied and skips it — so the deliberate divergence fromcore/nova's|| exit 1+--forwarddoes what the comment says it does, and an incremental workspace no longer aborts the build. - Applying the three patches reproduces develop's whole-file copies with exactly one difference: the
* 1024.0removal inworkload_balance.py. Nothing else changed under cover of the format switch.
python-observabilityclient is pinned — ===0.1.1 is in core/heavyfs/os-caracal-pip-upper-constraints.txt, so the -c on the pip install covers it and the unpinned-looking name in watcher.mk is not actually unpinned.
Three non-blocking notes. The first two are already fixed by #1457, so they only surface if this ships on its own:
core/sdk_sh/modules/sdk_watcher.sh— the# HELP ceilometer_memory_usage Resident memory of the instance in megabytes.line contradicts both the code (available - unused) and the file's own header, which explicitly rejectsballoon.rssbecause it is the resident set. It also says megabytes where the header says MEBIBYTES. #1457 rewrites this line correctly.- Same file —
$(( (avail - unused) / 1024 ))truncates to whole MiB (28.8 → 28). On the 128 MiB lab guest that is nearly a percentage point. #1457's awk does this in floating point. - Still present in both: a
virsh listtimeout is indistinguishable from "no running domains" — theforloop just yields nothing, and the good textfile is then overwritten with headers only.rate()/avg_over_timeabsorb a one-sample gap so the impact is small, but a failed read and an empty host are worth telling apart.
Style nit: the watcher_prometheus frontend is emitted above the comment block in config_haproxy.cpp that says "Add new sections here, at the end." It parses fine and it is after both cube_cos frontends, so it does not hit the hazard that comment describes — but the instruction and the placement now disagree.
LGTM. This one should merge first; #1457 is stacked on it.
Backports the prometheus datasource from 2025.1 into caracal's watcher 12.1.0, which predates it. The two upstream files are verbatim so the backport stays trivially re-portable and drops out entirely on Epoxy, which ships them; the only edits are the registrations caracal needs -- an import, a METRIC_MAP entry and a property in manager.py, and register_opts in conf/__init__.py. datasources/base.py is functionally identical between the two, so nothing else moves. The datasource cannot be pointed at Prometheus directly. It builds host:port/api/v1/... and python-observabilityclient takes only a host and a port, with watcher validating the host against a hostname regex so a path cannot be smuggled in -- while our Prometheus runs with --web.external-url ending in /prometheus/, making a bare /api/v1/query a 404 on both :9091 and the thanos querier's :10904. config_haproxy now writes a loopback watcher_prometheus listener that prefixes the path and hands off to prometheus_backend, so watcher inherits the UI's routing: the local Prometheus on one node, the deduplicating thanos queriers in HA. Dropping the route prefix instead would break the UI and Grafana. Two monasca-only patches go with the move. common/clients.py only ever built the monasca client and datasources/monasca.py was the monasca datasource, so both are dead weight we would otherwise re-port every cycle. The risk was that the patch also made monascaclient an optional import while manager.py imports monasca at module level -- but monascaclient 2.8.0 is in the caracal venv, and with genuine upstream files restored on all three nodes manager.py imports and the decision engine stays active. workload_balance loses PR 548's util * 1024.0 and nothing else. That converted monasca's mem.used_gb from GiB into the MiB the strategy compares against node.memory; ceilometer_memory_usage is already MiB, so leaving it overstates every instance 1024x. Measured: the same node read 424.446 percent with the multiply still live against 0.4145 percent without it. This couples the two changes -- landing the datasource without the strategy reads 1024x high, landing the strategy without the datasource reads 1024x low, and neither fails loudly. PR 515's workload_cache.get() guard and the diagnostics stay: that guard is generic, and instances really can be absent from the cache. metric_map_path is no longer written. It renamed monasca's meters; the option still defaults to the same file and that file keeps its monasca section, which is right. A prometheus section must never be added -- the datasource dispatches on the meter name and raises Cannot process prometheus meter for anything else, so an override breaks the query rather than redirecting it. Verified on jim-1cc and accept-3cc. The backport registers as gnocchi, ceilometer, monasca, grafana, prometheus and the listener answers watcher's exact request shape with HTTP 200. Host metrics resolve on both: jim-1cc 45.21 percent CPU and 32090844 KiB RAM on a 32 GB box; accept-3cc discovers all three fqdn labels through the thanos queriers with cc1 at 50.81 percent and 28.92 GiB against free -g's 28. All three verified strategies -- workload_balance on instance_cpu_usage, on instance_ram_usage, and allocation_balance -- SUCCEED, matching their monasca baseline. Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prometheus datasource resolves host metrics from node_exporter but returns None for instance_cpu_usage and instance_ram_usage, because nothing in the stack produces ceilometer_cpu or ceilometer_memory_usage. That is the whole of what the monasca to prometheus move was missing -- host metrics already matched free -g on both clusters, and allocation_balance reads no datasource at all. A textfile rather than an exporter. No libvirt exporter passes the provenance bar the phase 1 fleet was chosen under -- the upstream one has had no commit since June 2021 and prometheus-community declined to adopt it, a harder fail than the kafka_exporter already rejected. node_exporter's textfile collector needs no new service, port or scrape job, and this node's target already carries the fqdn label. The names are Watcher's and are not negotiable: its METRIC_MAP hardcodes them and _build_prometheus_query dispatches on the meter name, raising Cannot process prometheus meter for anything else, so metric_map_path can rename a meter but only into a failure. Emitting the ceilometer names keeps the backported datasource byte-identical to upstream and lets it be dropped on Epoxy. Units follow Watcher's own arithmetic. ceilometer_cpu is a counter of cumulative nanoseconds, since Watcher computes rate(...)/10e+8 * (100/vcpus); seconds would read 1e9 low and clamp to nothing. ceilometer_memory_usage is a gauge in MiB of GUEST-used memory, available - unused, which is what ceilometer's inspect_memory_usage computes for memory.usage and what monasca's libvirt check computes for vm.mem.used_gb -- the series metric_map.yaml pointed instance_ram_usage at, so the existing threshold carries over. balloon.rss is a different quantity that monasca publishes separately as vm.mem.resident_gb; on a 128 MiB guest it measured 133 MiB against a guest-used 28 MiB. Where a guest has no virtio-balloon neither field is reported and the series is omitted, as monasca omits it, rather than substituted with rss. No nova call is needed for the label: nova sets the libvirt domain UUID to the instance UUID, so virsh list --uuid is already the value instance_uuid_label names. The writer lives in sdk_watcher.sh because Watcher is the only consumer and hex_sdk resolves the module from the function's first token, but config_prometheus.cpp writes the cron -- config_watcher.cpp returns early unless IsControl, while the instances this reports on live on compute nodes, and this module's Commit runs on every role for exactly that reason. Verified on accept-3cc. The collector emits both series on the node hosting the VM and headers only on the other two. ceilometer_memory_usage reads 28, matching virsh dommemstat's (112676-83196)/1024. CPU checks out against the counter: 439926000 ns over 181 s on 1 vcpu is 0.243 percent, which is what the datasource returned. Through the strategy, node 26e1681d reads host_cpu_usage_percent 0.0253 and host_ram_usage_percent 0.0873 -- 28 MiB of 32086 -- against 0.0 on the two nodes with no instances, and the monasca baseline's 0.0594. All three verified strategies SUCCEED. Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file only ever carried a monasca: section, renaming that datasource's meters onto the measurements CubeCOS's agent publishes. With the decision engine on prometheus it is inert, and it should not be revived for the new datasource: prometheus dispatches on the meter name and raises Cannot process prometheus meter for anything it does not recognise, so an override there breaks the query rather than redirecting it. The names are matched at the source instead, by hex_sdk watcher_instance_metrics. Removing the file is safe even though metric_map_path still defaults to its path: upstream's load_metric_map guards with os.path.exists and returns an empty map, so nothing warns. The comment in config_watcher.cpp is corrected to say so -- it claimed the file was still installed and still read, which stopped being true here. Verified on accept-3cc: file removed on all three nodes, every decision engine active, zero metric_map or Could not load complaints in any log, and all three verified strategies still SUCCEED with the readings unchanged -- host_ram_usage_percent 0.0873 and host_cpu_usage_percent 0.0253 on the node hosting the instance, 0.0 on the two without. Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adopts the convention core/nova and core/masakari already use: a reviewable unified diff at <rel>.py.patch beside the pristine <rel>.py.orig it applies to, with brand-new downstream files installed verbatim. Watcher was the outlier, overlaying whole modified copies with cp -rf. The overlay hid what had actually changed -- three files totalling 617 lines in the tree to express 12 hunks -- and worse, it silently absorbs upstream edits on a version bump: a copy taken from 12.1.0 keeps overwriting 12.2.0's file with the old content, and nothing reports it. A diff fails loudly instead. The three brand-new files (prometheus.py, prometheus_client.py, allocation_balance.py) keep installing verbatim, which is the same rule nova applies to its own downstream-only files. One deliberate difference from nova and masakari, and it is a fix rather than a style choice. Both guard the apply with || exit 1 and rely on --forward for idempotence, but --forward only skips the hunks -- measured here, it still exits 1 when every hunk is already applied, so a re-run against an already-patched tree aborts the build. It never fires for them because each build reinstalls the venv from pip first, leaving the target pristine; it would fire in an incremental workspace. Testing with --dry-run --reverse first detects that case and skips it. Verified in the jim build container against a tree reconstructed from the .orig files: all three patches apply cleanly to pristine, a second run reports every hunk already applied and exits 0, and the three verbatim files install. The generated content is identical to what cp -rf produced, so the deployed behaviour is unchanged -- confirmed by re-running all three verified strategies on accept-3cc afterwards: workload_balance on cpu and on ram and allocation_balance all SUCCEEDED, with host_ram_usage_percent 0.0873 and host_cpu_usage_percent 0.0244 on the instance's node, byte-identical to the pre-refactor readings. Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback, and the better placement. The cron runs hex_sdk watcher_instance_metrics, which reads the local libvirt domains -- so it belongs to the module that runs where the domains are. config_nova.cpp gates it on IsCompute, exactly as nova's own compute services are gated a few lines below. It sat in config_prometheus.cpp for a weaker reason: config_watcher.cpp returns early unless IsControl, while the instances live on compute, and config_prometheus.cpp's Commit happened to run on every role. That made it work rather than made it right, and it wrote the cron on control-only and storage nodes too, where the generator has no virsh and produces an empty file. Now those nodes get no cron at all, and the writer unlinks it if the role or the service is disabled. config_prometheus.cpp keeps only what is genuinely its own: node_exporter's --collector.textfile.directory flag and the scrape. Verified on accept-3cc and jim-1cc. All four nodes are control-converged, so IsCompute matches and each gets the cron; the node hosting the instance emits both series and the rest emit none, which is the intended no-op. config_nova.o carries the cron literals and config_prometheus.o no longer references them. Re-running the three verified strategies afterwards gives host_ram_usage_percent 0.0873 and host_cpu_usage_percent 0.0244 on the instance's node, unchanged. Also closes a gap: jim-1cc had the datasource half deployed but never the collector, so it now matches accept-3cc. Not exercised: the control-only path where the cron should be absent. Every node on both lab clusters is control-converged, so no topology here can reach it; the unlink branch is reasoned from the same IsCompute gate nova already relies on. Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jim Lin <jim.lin@bigstack.co> Co-authored-by: Eandalf <clinah@connect.ust.hk>
2d51173 to
f351c68
Compare
What type of PR is this?
What this PR does / why we need it
Watcher is the last consumer that still reads Monasca. This moves its decision engine onto Prometheus and supplies the only metrics that move did not already have, so Monasca can be deleted without taking Watcher with it. This is issue #673.
host:port/api/v1/...andpython-observabilityclientaccepts only a host and a port — there is nowhere to put our/prometheusroute prefix, and Watcher validates the host against a hostname regex so one cannot be smuggled in. A loopback haproxy listener prefixes the pathceilometer_cpuandceilometer_memory_usagecome from anode_exportertextfile written byhex_sdk watcher_instance_metrics, cronned byconfig_nova.cppon compute-capable nodes. No new service, port or scrape jobclients.py,datasources/monasca.pyandmetric_map.yamlall go, along with PR #548's unit conversioncore/novaandcore/masakarialready useWhich issue(s) this PR fixes
Special notes for your reviewer
The first two commits are a pair and should not be split across releases. The datasource move alone leaves
instance_cpu_usageandinstance_ram_usageresolving toNone, and the failure is silent:get_backendonly rejects a datasource when a metric maps toNone, butceilometer_cpuis mapped — it is simply absent from the store. So audits still SUCCEED, still produce action plans, and decide from no per-instance data with nothing logged. The unit change below has the same coupling in the other direction.Watcher's metric names are not configurable, and
metric_map.yamlcannot help.METRIC_MAPhardcodesinstance_cpu_usage -> ceilometer_cpuandinstance_ram_usage -> ceilometer_memory_usage, and_build_prometheus_querythen dispatches on the meter name, raisingCannot process prometheus meterfor anything it does not recognise.metric_map_pathdoes override the map (manager.py:182), but renaming a Prometheus meter only breaks the dispatch. That is why the collector emits the ceilometer names rather than our own: it keeps the backported datasource byte-identical to upstream.metric_map.yamlis removed rather than repointed, and the option's default path is safe to leave dangling because upstream'sload_metric_mapguards withos.path.exists.Why
× 1024.0had to go, and why it is coupled. PR #548 added it because Monasca'smem.used_gbis GiB whileworkload_balancecompares againstnode.memoryin MiB.ceilometer_memory_usageis already MiB. Measured on the same node: 424.446 % with the multiply still deployed, 0.4145 % without. Land the datasource without the strategy change and memory reads 1024× high; land the strategy change without the datasource and the Monasca era reads 1024× low. Neither fails loudly — both just migrate wrongly.Memory is the guest's own figure, not the host resident set.
available - unused, which is exactly what Ceilometer'sinspect_memory_usagecomputes formemory.usageand what Monasca's libvirt check computes forvm.mem.used_gb— the seriesmetric_map.yamlpointedinstance_ram_usageat, so an existingthresholdcarries over unchanged.balloon.rssis a different quantity that Monasca publishes separately asvm.mem.resident_gb; on a 128 MiB guest it measured 133 MiB against a guest-used 28 MiB. Where a guest has no virtio-balloon neither field is reported and the series is omitted, as Monasca omits it, rather than silently substituted.Why a textfile and not an exporter. No libvirt exporter passes the provenance bar the phase 1 fleet was chosen under: the upstream one has had no commit since June 2021 and prometheus-community declined to adopt it — a harder fail than the
kafka_exporteralready rejected. The textfile collector needs no new service, port or scrape job, and the node'snode_exportertarget already carries thefqdnlabel. No nova call is needed for the instance label either: nova sets the libvirt domain UUID to the instance UUID, sovirsh list --uuidis already the valueinstance_uuid_labelnames.Addressing review — where the cron lives. It now sits in
config_nova.cpp, gated onIsCompute, because it reads the local libvirt domains and so belongs to the module that runs where the domains are — the same gate nova's own compute services use a few lines below. It was inconfig_prometheus.cppfor a weaker reason:config_watcher.cppreturns early unlessIsControlwhile the instances live on compute, andconfig_prometheus.cpp'sCommit()happened to run on every role. That made it work rather than made it right, and it wrote a cron on control-only and storage nodes where the generator has novirsh.config_prometheus.cppkeeps only what is genuinely its own: node_exporter's--collector.textfile.directoryflag and the scrape. The writer stays insdk_watcher.sh, since Watcher is its only consumer andhex_sdkresolves the module from the function's first token.Addressing review — the patch format. The three modified files are now unified diffs at
<rel>.py.patchbeside the pristine<rel>.py.orig, with the three brand-new files installed verbatim: thecore/novaandcore/masakariconvention. The oldcp -rfoverlay took 617 lines of whole-file copies to express 12 hunks, and would have silently overwritten a future 12.2.0 file with 12.1.0 content on a version bump. One deliberate difference from nova and masakari: both guard the apply with|| exit 1and rely on--forwardfor idempotence, but--forwardonly skips the hunks — measured here, it still exits 1 when every hunk is already applied, so a re-run against an already-patched tree aborts the build. It never fires for them because each build reinstalls the venv from pip first; it would fire in an incremental workspace. A--dry-run --reversepre-check detects that case and skips it. Say the word if you would rather match nova byte-for-byte.What upstream is doing, for the record. The Prometheus datasource is deprecated as of 2026.1 in favour of Aetos, a Keystone-authenticated RBAC proxy in front of Prometheus, and the default
watcher_datasourceshas changed accordingly. Backporting the deprecated datasource is still right for us — Aetos is an additional service plus a Keystone endpoint, and our path is yoga → antelope → caracal → epoxy — but the next Watcher uplift should revisit it. Upstream also assumes Ceilometer + sg-core produce the instance metrics, which is why the names areceilometer_*; we deliberately do not adopt those two services for two metrics.Additional documentation
Verified on
accept-3cc(3-node HA) andjim-1cc(single control) unless stated otherwise.For the requirement "the backport loads in Caracal":
For the requirement "the datasource can reach Prometheus at all", which is what the haproxy listener exists for:
For the requirement "host metrics resolve", which needed no new collection:
All three
fqdnlabels are discovered through the thanos queriers on the HA cluster.For the requirement "the collector produces the two missing series", on the node hosting the instance:
and nothing on the nodes with no domains. Cross-checked against libvirt directly:
For the requirement "the strategies behave as they did on Monasca", the same three audits run on both datasources. Node
26e1681dis the one hosting the instance:Both memory figures are guest-used; they differ only by sample moment (19.1 MiB vs 28 MiB of a 128 MiB guest). The other two nodes report
0.0throughout, having no instances. Audit outcomes, identical across both datasources and unchanged after the review refactor:allocation_balanceis unaffected by the move by construction — its own header reads "compute model only, no metrics datasource".For the requirement "removing
metric_map.yamlbreaks nothing", with the file deleted on all three nodes:For the requirement "the patch format applies and re-applies", against a tree reconstructed from the
.origfiles in the jim build container:For the requirement "it builds": clean
CXXunder-Wall -Werrorand a cleanLD hex_configforconfig_nova.o,config_prometheus.o,config_watcher.oandconfig_haproxy.o, with the cron literals now inconfig_nova.oand zero references remaining inconfig_prometheus.o.Not verified here, in two places. An instance without virtio-balloon, where the memory series is omitted by design — the lab guests are Cirros images that all report balloon statistics. And the control-only path where the cron should be absent: every node on both lab clusters is
control-converged, soIsComputematches everywhere and no topology available here reaches the unlink branch.