idle daemon tuning and misc fixes - #1102
Merged
Merged
Conversation
Every node pushed the resource info of its own instances straight to the collector, from the cli process running the push_resinfo schedule entry. That bypassed the collector speaker, which exists so a single node talks to the collector and can coalesce and rate-limit what it reports. The resinfo.json cache keeps its own schedule, which is the reason the info never moved into resource.Status. Only the reporting path changes: a refresh now signals the local daemon, which publishes a small InstanceResourceInfoUpdated carrying a checksum and no key-values, so peers replicate a signal rather than a payload. The speaker subscribes, and on its refresh ticker fetches the key-values from the instance node with the GetInstanceResourceInfo api before posting them, deduplicating on the checksum it persists under collector/resinfo_sent. This is the post_feed_object_config pattern, which already feeds a toSend map from a bus event and fetches the heavy payload from the holder node, combined with the imon fetch_resource_files one, where the frequent event is only a change signal. instance.Status is left alone: it rides InstanceStatusUpdated, which is far too frequent to grow. The push command disappears with the push path it drove: resource info reports the cache, resource info -r refreshes it.
The info command only existed under the resource subsystem, where every sibling query command is also mounted per driver group, and the push parent it replaced still stood next to it with a single hidden child. info becomes a group command, built like the list one it now sits next to: it takes the group and a PATTERN positional, so disk info reports the disk key-values and instance info reports them all. The object push parent goes away with its last child, the hidden push resinfo. A group selects a subset of the resources, which the refresh must not mistake for the whole instance: refreshing a subset merges over the cache instead of replacing it, so the rids left out keep their key-values. A full refresh still drops what the configuration no longer holds. The scheduler entry and the api action both run instance info, the canonical spelling for data the collector reports per instance, and the action forwards its rid to it.
The file was named after the push action it implemented, which no longer exists: it now refreshes, merges and loads the resource info cache, and feeding the collector moved to the collector speaker. It follows actor_status.go, named after the data rather than an action, for the same reason.
The action was named after the push it used to do, which the collector speaker took over: PostInstanceActionPushResourceInfo, at action/push/ resource/info, ran a command that no longer pushes anything. It becomes PostInstanceActionInfo, at action/info, and moves up to where the alphabetically ordered instance actions expect it. This reads like its PostInstanceActionStatus neighbour, which runs instance status -r the way this one runs instance info --refresh. The rename breaks the generated clients, which nothing in production calls yet. The api version takes a minor bump: this release also adds the POST resource/info signal endpoint and the action rid parameter.
The keyword was named after the push the collector speaker took over, and it drove a schedule entry, a statefile and a scheduler action all still spelling that push. resinfo_schedule stays as a keyword alias, so the configurations setting it keep working. The rest is free to follow the command: the entry is the info action, next to the status one, and runs instance info --refresh. The entry base changes with it, so the last run is recorded in last_info rather than last_push_resinfo. An upgraded object has no last run for the new name and refreshes its resource info once, which is what the entry does hourly anyway.
The moved-commands list still sent readers to a resource info list and a resource info push that no longer exist, one renamed and the other removed with the push path it drove.
It described the push resinfo action, which no longer exists, and left out the two traps of writing an info command: a value containing a colon is dropped like any other invalid line, and a non-zero exit aborts the refresh of the whole instance.
children and parents accept ./svc/svc2, resolved in the namespace of the object declaring it, which is what keeps a set of relations intact when the objects are cloned into another namespace. The keywords documented the bare name and the fully qualified path, and not that one. The children text also described what parents do, having been copied from it. hb.relay.password takes the same notation, resolved in the system namespace rather than the object's, node configuration not being namespaced.
It advertised json|flat|auto|tab and left out three renderers the commands accept: yaml, jsonline and template. auto moves to the front, being the default. table and csv are not added: the renderer implements neither, tab having replaced table, so passing them silently falls back to the human rendering like any unknown value. diff stays on the events flag alone, which is the only command that implements it.
It pointed at the pg_cpu_shares text, so the keyword reference described shares under quota: two different mechanisms, one arbitrating a contended cpu and the other capping regardless of contention. The accepted expressions were documented nowhere. They are 50%, 50%@ALL, 10%@2 and a raw microsecond quota, per the parser.
om node config eval failed on DEFAULT.id, a key the agent writes itself in every node.conf, so the command was broken on a clean install. The node keyword store declares id in the cluster section only, and eval stops at the first key it can not resolve, so one key hid the two hundred that evaluate fine. The node and cluster stores are not the core objects store, so a keyword has to be declared in both. Two were missing: * DEFAULT.id for the node, mirroring the ccfg one. It goes to the private list, not the common one, because the ccfg store appends its own and a second declaration would shadow it with the wrong text. * comment, for the node and the cluster. It has no section, like the core objects counterpart, so a comment is accepted in any section rather than in DEFAULT alone. Declaring keywords does not make the command robust, though, as any configuration can hold a key no keyword declares. So the two config handlers now split on how the keys were selected. A keyword selection is a user input, and asking for a key that does not exist stays a 400. The whole configuration is not a user input, and each failure is now reported on its own item, the listing continuing. This covers every eval error there, not just the unknown keyword one: a bad reference in one key had the same power to blank all the others, and a caller can no more act on a 500 than on a 400 in that case. The node handler was also still returning a 500 for an unknown keyword, as ddfdbc9 only patched the object one. The om and ox get commands grow an ERROR column, added to the default output only when an item carries one.
(cherry picked from commit fc8a98e)
…alues (cherry picked from commit d05793b)
…figFile call (cherry picked from commit fe9119c)
The two config file handlers declared a redact-secrets query parameter, plumbed it through the proxy call and the clients that reach them, then redacted every response whatever its value. So `om <path> config show` had no way back to the real values, and neither had its callers. `om <path> config edit` on an object with no local instance fetches the file through that handler and writes the buffer back on exit, which would have replaced cluster.secret, node.uuid and every sec or usr data key with the eight asterisks the editor was shown. The collector feed would have reported the redacted configuration as the object configuration. The default path also returns to serving the file rather than reading it whole into memory, as it did before. Cluster config synchronization is unaffected: peers fetch through GetInstanceConfigFile, a handler this feature does not touch. Also drop the sectionName copy the redact package carries. Only the core/object one has a caller.
The KeywordItem unstructured map is hand written, and I added the error field to the schema without adding it there. So the ERROR column the get commands ask for had nothing to read and printed a dash on the very rows it was added for. The json output was correct, as it marshals the struct.
The keyword was openid_authority until ddbaa83 renamed it, in July 2025, with no alias left behind. A configuration written before that does not merely fail to validate: getClusterConfig reads the new name only, so the issuer resolved empty and the daemon stopped advertising the openid auth method, silently, on clusters that had it configured. The alias mechanism is resolved by mayDescope and descope, so declaring the old name restores the value as well as the lookup. This cluster now advertises the issuer it has been configured with all along. The rename to openid_issuer was not the documented removal of openid_well_known, which the changelog covers. That one stays removed.
github.com/cvaroqui/ini re-encodes every line it writes, so a document that went through it came back reformatted whether or not anything was modified. The formatting was steered by package level globals, which xconfig set on each write. util/ini keeps, for every element, the source bytes it was parsed from, and writes those back verbatim for the elements nothing touched. Only a modified element is encoded again, from a Format carried by the file rather than by the package. A document nobody modified is reproduced byte for byte, comments, blank lines, alignment and multiline layout included. Its accepted syntax is the dialect the OpenSVC configuration files use: the "=" and ":" delimiters, python multiline values, backslash continuations, quoted and triple quoted values, inline comments requiring a leading space, child sections and the DEFAULT section. A document defining a key twice is normalised to its last definition, which is what the previous library resolved it to, and duplicate sections are merged under the same rule. compat_test.go keeps the old library as a test only dependency and checks the two read the same values out of the same documents, over a corpus of the syntax corners plus a fuzz target. Two more fuzz targets cover the byte identical write back and the value substitution.
A value is written to the configuration file as it is given, and read back through the ini quoting rules, which strip a surrounding quote pair. So `--set x="1 2"` stored `"1 2"` and read back `1 2`, while `--set x=1 2` stored and read back the same thing: the quotes a shell user reaches for to protect a space silently became part of the value on the way in and were dropped on the way out. Parse now strips the pair the way the parser does, on the same rule: only when the quote appears nowhere else in the value, so a value holding a quote keeps it.
RedactSecrets read the configuration file with three regexps, rebuilt every line from the submatches, and wrote the whole document back. Two of its readings leaked the secret it was called to hide: * A key delimited with a colon did not match the key/value regexp, so the line was written out raw. xconfig accepts "=" and ":", so "secret:s3cr3t" was served whole. * The regexp cut the value at the first "#" or ";" and kept the remainder as an inline comment, so "secret = a#b" was served as "secret = ********#b". It also rewrote a CRLF document with LF endings, and appended a newline to a document that ended without one. The parser knows where a value starts and ends, so the redaction is now a substitution in place: load, walk the sections, replace the value of the keywords declared with RedactSecret and of every key of a sec or usr data section, write. Everything else is written back verbatim, and the substitution is refused if it would not parse back to the redacted value. RedactSecrets returns an error rather than a document it could not parse and therefore could not redact. The handlers answer 500 on it, the show commands report it. Serving the configuration unredacted is not an option worth having for this function. util/redact held the line and section types the hand rolled reassembly needed, and has no remaining caller.
The keyword doc rendered required, scopable, candidates, depends, default and convert, and nothing else. Three things a reader needs were held in the keyword and never printed: * the aliases, 213 keywords having one. A configuration written with the name a keyword had before a rename validates and evaluates, but its doc gave no clue the name was accepted, so the reader could not tell a working configuration from a broken one. * the deprecation, 13 keywords carrying one. config validate warns about them with the release and the replacement, the doc said nothing. * the replacement of a deprecated keyword, and whether the value is a secret. Both were also dropped by the API the doc command reads its definitions from: ConvertKeywordStore never set them and the schema had no field to carry them, so plumbing them through was needed for the keyword to reach the renderer with them. The deprecation is phrased as the config validate alert phrases it. Also correct the text of three keywords: * comment now serves the node and cluster configurations, where there is no object and no resource, which is all its text described. It says what it covers, and that it is accepted in any section, which the empty Section of its declaration means and nothing said. * cluster.secret advised that the cluster *name* be unique site-wide, copied from the cluster.name text. The advice is about the secret, and now says what changing it later costs. * listener.openid_issuer mentioned only the openid_well_known it replaced, not the openid_authority name it answers to.
Auditing all 693 keyword text references turned up the same class of bug the pg_cpu_quota fix was about, in three places. The whole non-blocking trigger block of the resource keywords pointed at the post_provision text, so pre_start was documented as "a command to execute after the resource provision action", and every trigger claimed to run on leaders only, which is true of the provision ones alone. The ten correct texts were on disk, unreferenced. restart_delay and subset pointed there too, a duration and a resource grouping documented as a trigger command. subset had no text at all and now has one. pool.freenas.blocksize carried the insecure_tpc text, so a zvol block size was documented as an xcopy authentication setting. ressynczfs recursive carried the target text. Four keywords had no text at all, NewText discarding the read error, so they rendered an empty description: cluster.envs, node.max_key_size, and task.host stop, plus pool.drbd.max_peers whose reference was missing the node/ path component. resipsgcp_dnsalias target wrote its default text reference with an underscore where the file has a dot. 34 texts told the reader to run pushasset, hidden and kept for compatibility, rather than `om node push asset`. The stonith keyword and the stonith section command keyword shared a text explaining the fencing mechanism without saying what either keyword holds. They now open with that, and the object one points at the section holding the callout. Left alone and reported: 19 text files no keyword references, among them cluster.hb_secret, whose keyword was removed with the mechanism, and resdiskhp3par method, which may be wanted by the array driver in progress.
…gnal The command help did not say what a killable pid is. The handler only accepts the pids the daemon registered for its own spawned processes, which is exactly the set `daemon ps` lists, and refuses any other with a 400. Nothing said so, leaving the reader to find the target list by guessing. The long description now names `daemon ps` as the source of the pids, and says a pid only makes sense on the node the process runs on, so --node is what reaches a process listed on a peer. The pids move from a --pid option to positional arguments, the command requiring at least one. `om daemon kill 3924688` reads as kill(1) does, and a bad argument is now reported as "invalid pid abc" before any request is sent, where IntSliceVar reported a cobra parse error naming the flag. --signal is new. The handler sent SIGKILL and nothing else, so a task that installed a signal handler could not be asked to wind down. The signal travels as a name or a number in a new query parameter of the DeleteDaemonProcess operation, and is parsed server side, where the allowed pid check already lives: a number is refused unless it names a signal, a name is accepted bare or SIG prefixed, in any case, the way volsignal reads the resource signal routes. An absent or empty parameter keeps SIGKILL, so the existing behavior and the older clients are unchanged. --pid is removed rather than deprecated.
The driver spelled the same directory two ways. objectNSPIDFile built /var/run/netns/<object id>, the path handed to `ip netns add`, `ip netns del` and netns.GetFromPath. purgeCNIVarFile stated /run/netns/<name> to decide whether a namespace still exists before dropping its address from the CNI cache. The first spelling dates from the original port, the second from the ipv6 purge fix, and nothing ever reconciled them. They resolve to the same inode wherever /var/run is the symlink to /run that the merged layout installs, which is why the split never showed. It still has to agree with iproute2's NETNS_RUN_DIR, since `ip netns add` is what creates these files: were /var/run a real directory, the two call sites would part ways and the purge would keep stale addresses forever, silently. Both now read a netnsRunDir constant holding /run/netns, the canonical FHS path, and the one compiled into the iproute2 the driver calls. resfsflag keeps its /var/run/opensvc, reached only on solaris, which has no /run and where it is the correct volatile directory.
An object whose only resources are tasks or syncs came out with an avail of undef. The aggregation skips those two driver groups, so nothing ever called Avail.Add and the field kept its zero value, and the guard that answered n/a only covered the object with no resources at all, which this is not. undef is not a status a reader can act on, and the daemon reads it as a state to leave rather than a state that is fine. omon aggregates a failover object of undef instances to down, because the n/a shortcut wants every instance n/a. imon's isLocalStarted answers false on undef and true on n/a, so an orchestrated task only object never settled: it was told to start, had nothing to start, and was asked again. The two aggregates are now settled once, at the end of the resource walk, where the object with no resources and the object with no contributing resource are the same case: an avail or an overall still undef there had no contributor and is reported n/a. Doing it before the statusbus posts also hands the corrected value to the action log status icon. Optional keeps its own reading. It is never aggregated from the resources, so widening it to every object would have started serving an "optional" field that the omitempty of the undef zero value drops today. Verified against a task only object, which now reports avail n/a, and against a running and a stopped service, unchanged.
Profiling an idle three node cluster, the daemon allocated 1.54MB/s while doing nothing but refreshing instance status, and spent 7.6% of its cpu collecting that garbage. More than half of it came from here: compress called zlib.NewWriter and decompress called zlib.NewReader once per message, and a flate writer carries about 800KB of compressor state. The heartbeat messages alone made flate.NewWriter the single largest source of garbage in the daemon, ahead of every piece of real work. Both codecs are documented as resettable for reuse, so they are now held in a sync.Pool and reset onto each message. The compression itself is untouched: the same level, the same Flush before Close, the same bytes on the wire. That last point is what the new package tests are mostly about. A reused writer must emit the stream a fresh one emits, or a peer running another release decodes something else, so the test asserts the output against a copy of the previous implementation. The rest covers the round trip over five payload sizes, concurrent use, and that a malformed message does not poison the pooled reader for the message after it. The package had no tests. Measured on the profiled cluster, over a 180s idle window, against the same window before the change: * the daemon allocates 117MB where it allocated 277MB, 58% less * compress drops off the profile entirely, from 96MB * decompress allocates 9MB where it allocated 55MB, 84% less * the gc goes from 7.6% to 4.5% of the daemon cpu, and the daemon from 2.01% to 1.80% of a core The unchanged paths hold still across the two windows, which is what makes the comparison worth reading: the metrics handler 0.71s -> 0.74s, the cluster data deep copy 0.72s -> 0.65s, the heartbeat receive loop 0.63s -> 0.60s. The cpu spent compressing is unchanged at 0.08s, as expected. Only the garbage is gone. A pooled reader keeps a reference to the last message until its next use, because zlib's Resetter reads the header eagerly and cannot be pointed at a dummy source the way the writer can be pointed at io.Discard. It is bounded by GOMAXPROCS times the message size.
The sender encrypts a message once and hands the very same bytes to every tx driver, so a cluster running n heartbeats delivers n byte identical frames per message. Every rx driver decrypted, decompressed and unmarshalled its copy in full, and msgFromRx then dropped all but the first on their UpdatedAt. The work was already being discarded; it just was not being skipped, and on an idle cluster it was the most expensive thing the daemon did. The rx drivers now hash the frame and ask hbdedup whether another link already delivered it, before paying its decoding. Two invariants shape it. A link is still reported alive on a duplicate: one delivering nothing but copies is a working link, and dropping its liveness report would have it declared stale, which is why the cache holds the sender nodename and hands it back on a hit. And a frame is recorded after it reached the daemon, never before: a frame recorded by a link that then failed to deliver it would be skipped by the links still to come, and the message lost. Recording late only costs the decoding of a copy arriving in the race window. Entries expire by generation rather than one by one, so a daemon running for months does not remember every frame it ever received, and no sweep walks the map. The disk and relay receivers verify a frame came from the node whose slot it sat in. They keep that check on the skipped path, against the nodename the cache holds. The unicast receiver decrypted inside encryptconn.MessageWithNode, with no frame to hash in between, so encryptconn grew Frame and DecryptFrame. MessageWithNode is unchanged, and is now the two composed. Measured on the profiled cluster, five heartbeats, per 300s window: * the unicast receive loop goes from 11.13% of the daemon cpu to 4.94%. Inside it, decryption 0.24s -> 0.02s and unmarshalling 0.34s -> 0.10s: what is left is the frames this link wins the race for, plus reading and hashing, which no dedup can avoid. * the disk receiver drops off the profile, from 2.41%. * the dedup itself costs 0.02s, hashing at 14GB/s with no allocation. The multicast receiver does not benefit, and the profile says why: its cost is the per datagram fragment framing, a json unmarshal of a 50KB chunk carried as base64, not the message decoding the dedup skips. That framing sits upstream of the reassembly, the first point where a frame exists to be keyed, so no dedup reaches it. Worth its own look. Verified on the cluster: 30 of 30 rx peer links beating across the four driver kinds, and stopping then starting a receiver has the dedup follow the live driver set, the unicast loop moving from Seen to Delivered when the multicast one goes away, and back when it returns.
daemondata.ClusterData is behind a package level singleflight.Group, so callers whose requests overlap are handed the very same *clusterdump.Data pointer rather than a copy each. WithSelector and WithNamespace purged the dataset they were given, deleting from Cluster.Object and from every node's Instance map, so a caller that filtered was writing to a struct its neighbours were reading. The api handler defended against that, but only on one of the two ways it filters: it deep copied when selector or namespace was passed in the query, and not before the rbac filtering, which is the branch a plain om mon from a namespace scoped user takes. A guest:ns1 request overlapping the collector feed could therefore strip every non ns1 object out from under it, and the collector would post a cluster that looks like it just lost a hundred objects. get_daemon_events replay shares the pointer the same way. The window is the channel round trip to the daemondata goroutine plus the deep copy itself, a json marshal and unmarshal of the whole dataset, so it is widest exactly when the daemon is busy. The filters now build a view instead: Cluster.Object and each node's Instance map are rebuilt with the paths kept, and everything the filter does not look at is shared with the receiver, which is left alone. The sharing is sound because both datasets are only ever serialized, and the doc comment says so, since that is the constraint the next edit has to respect. Both filters were doing the same two loops over the same two maps around a different predicate, so the loops moved into one filter method and the predicates are what is left of each. The defensive deep copy goes away with the mutation it was guarding, and with it one of the five full serializations the filtered path used to pay. It is also what would let a single json rendering of the dataset be shared between callers of different privilege rather than recomputed per request, which is the direction the deep copy cost wants to be taken in. Removing it does mean a nil dataset is no longer quietly rescued: json marshalling a nil *Data yields "null" and unmarshalling that gave back an empty Data. The rbac branch already panicked on nil, so the handler now answers 500 instead, on both branches. TestFiltersDoNotModifyTheReceiver pins the invariant the handler rests on. It fails against the previous in place implementation, checked by restoring it. Verified on the profiled three node cluster after redeploy, on both filtering branches. The query parameter branch: namespace and selector responses are exact subsets of the unfiltered one, set compared. The rbac branch, as a user granted admin:test and operator:mon and nothing cluster wide: 18 objects over exactly the test and mon namespaces, set equal to that subset of the unfiltered dump, no leak and nothing missing, all three nodes still present with their instance maps filtered. Then the shape of the bug itself, 60 unfiltered root requests interleaved with 60 of that user's, 24 in flight: every root caller saw all 103 objects and every rbac caller saw only its 18, no cross contamination. The requests do overlap, which is what makes that worth reading: one takes 21ms, and 24 concurrent finish in 194ms where serializing them would take 500ms, so the singleflight is collapsing them onto a shared pointer under exactly this load.
clusterdump.Data.DeepCopy marshalled the whole dataset to json and unmarshalled it back. On the profiled cluster that was 12% of the daemon cpu and 77% of everything the daemondata goroutine did, and it ran inside opGetClusterData, so every other cluster data operation waited behind it. It was not the only one. node.Node.DeepCopy and resource.Status.DeepCopy were json round trips too, and the dataset is mostly made of those: 3 nodes carrying 253 instances carrying 594 resource statuses here. A real Data.DeepCopy calling into them would have kept nearly all of the cost, so all three are hand written now, along with the instance.Instance and node.Os methods they needed. instance.Monitor.DeepCopy is deliberately left alone. Its json marshal is inside the GlobalExpectOptions branch and is not a copy: it converts a peer's map[string]any into the typed MonitorGlobalExpectOptionsPlacedAt the orchestration reads. It is also nil in all 253 instance monitors here. Benchmarked against a captured 103 object dataset, the copy goes from 16.44ms to 1.02ms, from 2.91MB to 0.94MB, and from 37210 allocations to 8285. The time matters twice: it is cpu, and it is 16ms data.run spends answering nothing else. The any typed fields are what a hand written method cannot reach, since the concrete type is only known at run time: resource.Status.Info holds strings, ints and lists set by drivers here, 226 of the 594 statuses have one. util/deepcopy covers them by reflection, and Slice covers the plain slices while keeping nil apart from empty, which matters because several of these fields carry no omitempty and so serialize the two differently. Routing the copy through the hand written methods exposed six bugs in them, all pre existing, none of which had been visible because the json round trip never called them: * daemonsubsystem.Daemon.DeepCopy dropped Nodename * daemonsubsystem.Heartbeat.DeepCopy dropped SecretVersion and UpdatedAt * pool.Status.DeepCopy dropped Usage.Shared * cluster.Config.DeepCopy dropped Issues * instance.ActorConfig.DeepCopy dropped Children, Parents and MonitorAction, and shared the Flex pointer * instance.ResourceConfigs.DeepCopy shared RestartDelay, through &(*cfg.RestartDelay), which is the pointer it was handed Five are the same mistake, building the copy by listing the fields wanted, which silently stops copying every field added after it was written. They are all n := *t and then deepen now, which cannot drop a field. The sixth is the &(*p) idiom, which looks like it dereferences and re-addresses and does neither. The daemonsubsystem methods keep one thing they were doing that a copy would not: their collections come out non-nil even when they went in nil. That is deliberate and now documented and tested. Those fields carry no omitempty, and hbctrl builds Alerts by appending to a nil slice, so an alert-free heartbeat stream is nil by the time it gets here. A faithful copy served "alerts": null where an api client had always been served "alerts": [], which a redeploy of an earlier draft of this change showed on the wire. Normalizing in DeepCopy rather than at the sources is what also covers a peer sending null over the heartbeat. The tests drive the dataset by reflection rather than from a fixture, because a fixture covers the fields its author thought of and stops covering the ones added later. A filler writes a distinct non-zero value into all ~1900 fields, choosing a valid member for the enum types, whose MarshalText rejects out of range values. Then the copy must serialize byte for byte like the json round trip it replaces, since the collector feed and GET /daemon/status put it on the wire; separately with every collection nil, which the filler cannot produce; and a second reflection pass mutates the original in place, following maps, slices and pointers rather than replacing them, after which the copy must not have moved. That last one asserts the mutation was real, or it would pass against a copy that shares everything. Setting OM_TEST_CLUSTER_DUMP to a captured GET /api/cluster/status body replays both properties against real data. On this cluster's 612KB dump the two serializations are identical, which is the wire compatibility this change needed to show. It is also what caught Daemon.Nodename, the only one of the six that was non-zero in the live data.
Two things pgmetrics got wrong about cgroup v2, both of which show as a metric that is silently not there. A cgroup v2 limit file holds the literal "max" when nothing is limited, which is the default. parseUint rejected it, and the caller dropped the value on error, so cgroup_memory_max_bytes was empty on every cgroup on the node rather than on an unusual one. It now reads +Inf, which is what prometheus uses for no limit and what the text exposition renders, so a query can ask which cgroups are capped instead of guessing from an absent series. 0 would not do, being a limit of zero. The help string claimed 0 = unlimited and described nothing the code could produce. The other is that nothing was ever removed. Prometheus vectors do not forget a label combination on their own, and collect() skipped an object whose cgroup had gone with a bare continue, so its series went on serving the values of the last collection that found it. A stopped object looked like a running one, indefinitely, and cgroup_exists in particular kept answering 1 though its own comment says "(1) or not (0)" and Set(0) was never called anywhere. The series count also only ever grew, for as long as the daemon ran, over every object it had ever seen. Now an object with no cgroup reports exists 0 and has its usage series dropped, and an object gone from the cluster keeps nothing at all. That bounds the endpoint by the number of objects, which is the bound it should have had. Dropping the series needs the list of metrics to drop, and register and unregister each had their own hand written copy of that list. There is one now. Three enumerations of the same fourteen metrics is the same failure mode a DeepCopy that lists the fields it wants has: the copy that is not updated stops covering what is added later. Not addressed, and worth its own look: cgroup_blkio_weight is empty for the same shape of reason, io.weight being "default 100" rather than a bare number under cgroup v2, and cgroup_cpu_shares, cgroup_cpu_quota_us and cgroup_cpu_period_us are empty because they read the cgroup v1 files, which do not exist under v2 where the information is in cpu.max and cpu.weight. Five of the fourteen metric families publish nothing at all on this node.
hbmcast sent each fragment of a message as a json envelope with the chunk inside it as a []byte, which json encodes as base64. The receiver paid a json parse and a base64 decode per datagram, before it could know which message the fragment even belonged to. A message is parsed once per fragment and decrypted once in total, so the framing was the cost, not the message: across the two windows that measured the hb dedup, this driver's json.Unmarshal was 0.31s in both, unmoved by a change that demonstrably removed the equivalent work from the unicast receiver. The whole receiver was 8.75% of the idle daemon. A datagram is now a 24 byte header and the chunk, unaltered: magic, framing version, the 16 bytes of the message uuid rather than its 36 character string, and the index and count as uint16. Decoding one goes from 393us to 13.7us on a full size chunk, 174MB/s to 3.7GB/s, and from 7 allocations to 2. What is left is almost entirely the copy of the chunk out of the read buffer, which is not optional: the receiver reads every datagram into one buffer and holds fragments in the assembly map until their message is complete, so a chunk pointing into that buffer would be overwritten by the datagram after it. The json path got that copy for free, out of base64 decoding into a fresh slice. TestDecodeFragmentCopiesTheChunk is there because it would be silent corruption rather than a failure. The base64 also broke the size arithmetic, which is the stronger reason to change the framing. MaxChunkSize is 51200, and every fragment but a message's last carries a full one. As base64 in the envelope that is 68333 bytes on the wire: larger than the receiver's 61440 read buffer, and larger than a UDP payload may legally be. So any message over 51200 bytes had at least one fragment that could not be sent at all, and the write error was logged at Trace before abandoning the whole message, saying nothing at any usual log level. Raw, the same chunk is 51224 bytes and MaxChunkSize means what it says. That write failure is a Warnf now. MaxDatagramSize is the largest a UDP payload can legally be, so whatever reaches the socket is read whole. It was 61440, and Go truncates a datagram that does not fit the buffer without saying so, which for a fragment means a corrupted message. The binary framing needs 51224 of it; the rest is headroom for a peer still sending json, whose datagrams are a third larger. On compatibility: the receiver reads both framings, telling them apart by the magic, so a peer on an older release goes on being understood, and the version digit keeps the next change from being a flag day. The transmitter sends binary only. That is the half worth knowing about during a rolling upgrade: a node that has not been upgraded yet cannot read an upgraded node's fragments, and will see it as stale on the multicast link until it is upgraded too. On a cluster where multicast is the only heartbeat that is a membership risk for the length of the upgrade. Sending both framings for a release would remove it, at the cost of the traffic and of the parse this change is removing.
A multicast datagram is delivered to every member of the group, the sender included, so on this three node cluster a third of what the receiver reads is its own. It was recognised as such from the nodename inside the message, which is the last thing the receiver can reach: every fragment reassembled, the frame hashed against the dedup cache, and the whole message decrypted, before finding out it was never for us. The transmitter now records the id of each message it sends, and the receiver drops a fragment carrying one on its header, a map lookup after the 24 bytes it has already read. The pair share the ring, which is what it has to be: a datagram only comes back on the group its own tx sent it to, so a per heartbeat ring is exactly right and a shared one would be wrong. Sixteen ids are kept, evicted oldest first, no clock. Our own datagrams come back at once and a message's fragments arrive together, so nothing older is ever asked about. The nodename check stays, as the backstop for what the ring cannot see: a datagram from a tx that has since restarted, or one whose id aged out because a great many messages were sent while its fragments were still arriving. Not done, and the note that suggested it was wrong: skipping the interval re-send when nothing changed. That re-send is the beat. Every driver has it, ucast and disk as well as this one, and with an interval of 5s against a timeout of 15s it is what keeps a peer from declaring this node stale while daemondata has nothing new to say. Skipping it would not be an optimisation, it would be a liveness change, and on a quiet cluster it would take a node out of the membership rather than save it any work.
The two callers that put the dataset on the wire were each paying three
serializations of it. daemondata deep copied it, which since the copy
became a real one is a walk of the whole dataset, and then the caller
marshalled what came back: GET /daemon/status through echo, the collector
feed into its request body. The copy exists only so that the caller can
marshal something that will not change under it, so marshalling where it
cannot change removes the need for it.
ClusterDataJSON marshals on the daemondata goroutine and returns bytes.
No copy, and nothing for the caller to do but write them: JSONBlob for
the api, a json.RawMessage field for the collector body, so embedding it
copies bytes rather than walking the dataset a second time. It stays
behind the same singleflight, and sharing is now unconditionally safe:
sharing the struct was what made concurrent callers a bug, bytes cannot
be modified by anyone.
Measured on the captured 103 object dataset, per caller per cycle:
copy then marshal 5.79ms 2.75MB 21780 allocs
marshal only 4.44ms 1.68MB 13492 allocs
The api keeps the struct path for a request that filters, by selector, by
namespace, or by an rbac grant that is not cluster wide, since filtering
needs a struct. That predicate is evaluated before asking daemondata,
which is what lets the common request take the cheap path.
The trade this makes, which the note proposing it did not mention: the
marshal moves onto the daemondata goroutine, where it blocks every other
cluster data operation while it runs. That goroutine is busy 4.44ms per
reader now where it was busy 1.02ms doing the copy, so its occupancy goes
up more than fourfold even as total cpu drops by 19%.
Worth it on the numbers, and worth saying out loud. Two readers at the
rates this cluster runs, a poller every 6s and the collector every 10s,
put that at about 0.11% of wall time against 0.026%, and each individual
block is 4.44ms where before P1 it was the 16ms of a json round trip
deep copy. So this is still well under half of what the same goroutine
was blocked for a few commits ago, while allocating less than either. If
a reader ever arrives that polls hard, the answer is not to go back to
copying but to stop marshalling per read.
pgmetrics walked 24 cgroups on a ticker, opening about ten sysfs files in each, whether or not anyone was looking. On the profiled cluster that was 6.8% of the daemon's cpu and 10.8% of its idle allocation, and 4.4% and 7.8% of those were os.ReadFile alone. Splitting the endpoints made that worse in principle: /metrics/pg is meant to be scraped on its own, much longer interval, or not at all until someone is looking, while the collection went on at fifteen seconds regardless. The metrics are now const metrics, built during the walk by the collector that emits them, and the walk happens when a scrape asks. A snapshot is reused for minInterval, which is the ticker period this replaces, so the data is no less fresh than it was and a /metrics scrape every few seconds does not walk every few seconds. Nothing scraping means nothing walking. The obvious shape, a collector that refreshes the gauge vectors and emits nothing, does not work: a registry gathers its collectors concurrently and in no defined order, so the vectors could be read either side of the refresh. Emitting what you just read, from one collector, has no such question in it. The hints on /metrics are the reason this needed a second collector rather than one. They have to stay fresh on an endpoint scraped every few seconds, and making that trigger the full walk would have saved nothing. So the walk takes a full flag: the cheap one reads the two files the memory utilization ratio needs and counts what it stats, the full one reads the other eight and emits the detail. Benchmarked over 100 objects of which 24 have a cgroup, 591us against 1929us, and 131KB against 526KB. Const metrics also delete a class of bug rather than fixing it. A vector remembers every label combination it was ever given, which is why a vanished cgroup had to be deleted from each of fourteen of them by hand, why the manager carried two maps to know which cgroups to run that pass for, and why doing it on every tick for every object without a cgroup cost 9.4% of the daemon a few commits ago. A walk emits what it found, so a cgroup that is gone is simply not in the next one. forgetUsage, the maps, the deletion pass and the ticker are all gone with it, and the file is 180 lines shorter. New takes no queue sizer: the manager subscribes to nothing now, having no goroutine left to receive on.
The objects table sets viewPath from the selected row and viewNode from
the selected column, in two independent ifs, so a cell at the crossing of
an object row and a node column has both. The config view read them in
the order node, cluster, object, and so answered such a cell with the
node's configuration.
'e' read them in the order object, node, cluster, and so opened the
object's configuration for the very same cell. One of the two orders is
right and it is 'e' that had it: a cell that names an object is about
that object, whichever node's column it sits in.
The order is now written once, in configTargetFor, which both read. That
is the actual fix. Two copies of a precedence is what let them disagree,
and reordering the switch without removing the second copy would leave
the next pair free to drift the same way.
Two tests, because they fail for different reasons. The unit test pins
the decision, including the case the bug was about. The other drives the
real application against the node's daemon with a simulation screen,
lands on an instance cell, presses 'c' and reads the title off the view:
against the old order it reports 'c' showed the node configuration
("dev2n1 configuration") for a cell naming object appstatuslog, which is
the report this fixes, in the words it was reported in.
The commit that made daemondata hand out the dataset already marshalled
put it in the request body as a json.RawMessage field, on the assumption
that embedding it there would copy bytes. It does not. encoding/json
compacts a RawMessage, which validates and copies it byte by byte, and
appendCompact was 7.5% of the daemon in the profile after that deploy.
Measured over the 103 object dataset, the whole body:
deep copy, then marshal the struct 5.45ms 2.57MB 21780 allocs
marshal, then a RawMessage field 8.22ms 1.91MB 13493 allocs
marshal, then splice 4.25ms 1.55MB 13494 allocs
So the change traded a deep copy for a compaction that costs more than
the copy did: it was a 51% regression on cpu, bought with a 26% drop in
allocation. Splicing is what it was supposed to be, and is 22% below
where it started on cpu as well as 40% below on allocation.
The dataset is no longer a field of the body. The envelope is marshalled
without it and the bytes go in before its closing brace, through an
io.MultiReader, so nothing walks the dataset between daemondata and the
socket.
Escaping survives, which is the thing to check when stepping around a
serializer: the dataset comes out of a json.Marshal that has already
escaped what needs escaping, so compacting it again only copied it. The
test covers a dataset holding & and <, and asserts the body parses to the
same thing the RawMessage field produced.
The echo middleware breaks each of its four metrics down by url, and two of them are histograms, so one route reached with one method and one status code is 36 series. On a cluster whose nodes talk to each other over the api that was 1025 of the 1100 series /metrics answered with, and it grows with the number of routes exercised rather than with anything an operator asked to watch. The middleware now registers into its own registry, with its names prefixed route_ so the plain ones are free for the aggregates, the way the scheduler's object_runs_total leaves runs_total to the hint. The rate limiter's denials by method and path follow, being the same kind of series about the same requests. What is left on /metrics is the request rate by code and method and the latency of all routes taken together, which is what an alert fires on, plus the unlabelled denial counter, whose value is the sum over the vector it replaces. Each says where the breakdown is in its help text. DoNotUseRequestPathFor404 comes along with the move: without it a request no route matches is labelled with the path it asked for, so anything scanning the listener writes series nothing ever removes, and the endpoint they now write to is the one meant to be scraped rarely. /metrics goes from 1100 to 85 series on a dev node, 16 of them the api hints, against 123 at /metrics/api.
Someone reading a scrape of /metrics has no way to learn that /metrics/pg, /metrics/pubsub and /metrics/scheduler exist, and the aggregates that were left behind are the place they would look: an operator wondering which object is running the schedule entries counted by opensvc_scheduler_runs_total is already holding the metric that knows the answer is one endpoint away. So each hint whose breakdown is served apart names the endpoint and the dimension it is broken down by, in the parenthetical the api hints introduced. Only the hints that actually have a counterpart there get one: the pubsub queue counters have no detail, and pointing at /metrics/pubsub from them would send a reader looking for something that is not there.
Five call sites reached a peer's api as the local node by sending the cluster secret as a basic auth password, which the peer compared to its own copy. That secret is also the key every sec object is encrypted with, so a call that resolved to the wrong listener handed over rather more than the config file it came for, a hazard both core/client and daemon/discover carried a TODO about. They now mint a five second token signed with the cluster CA private key, which every node has, so any peer verifies it with the jwt strategy it already runs. This is what the api proxy has been doing since it learned to forward a request to the node that can answer it, and what the collector's dequeue action does; CreateNodeToken is that, named, and those two now call it. With the last user gone the basic node strategy is removed, along with NodeAuthenticater, ccfg.NodeDB and the client's habit of calling itself by the local hostname when it had no other identity to offer. A peer running the previous version still authenticates with the secret, so this expects the cluster to be upgraded before node to node calls work again. CreateToken no longer returns an empty token and no error when the sign key failed to load: the caller sent the empty string as its bearer and read the 401 back as a credentials problem, one node away from the message saying why.
The daemon compiled about a thousand lines of go-guardian to run six strategies, of which it wrote the interesting half itself: ux was already hand written, and jwt, x509 and basic user were our validate functions wrapped in a header parser and a cache. The library's own contribution was r.BasicAuth, an Authorization header split, Certificate.Verify against a pool, a loop over the chain, and the openid id token verifier. Those are here now, at about a third of the size, and with tests, which the package had none of. It also settles the two jwt packages: go-jose came in only under go-guardian's openid strategy, and the id tokens are now verified with golang-jwt, the one this repository already signs its own tokens with in daemonauth, omcmd and the pure array driver. The jwks client that go-jose was there for is jwks.go: it fetches the provider's key set, caches it for what its Cache-Control asks, and refetches when a token names a key id it does not know, which is what a key rotation looks like from here. auth.Info and its map of string lists become the Info struct. The extensions map was how a fixed library interface carried what this daemon wanted, and every reader had to know which key held the grants; the fields say it instead. Verification got stricter on the way. Both jwt strategies now name the signing methods they accept, so a token asking to be verified with something we never issue is refused before a key is fetched for it, and both require an expiration claim, which also removes a nil dereference on a token that carried no registered claims at all. The openid strategy refuses a token whose alg does not match the alg its key was published for. The authentication cache is keyed by a digest of the credential rather than by the username, so a heap dump does not hand over the passwords presented to the daemon, a wrong password cannot be answered by the entry a right one left behind, and an entry lives at most a few seconds however long the token it came from is valid for. It is also bounded: its keys come from the request.
The 401 body carried what each strategy made of the request, and those errors are not equally uninformative: "read password from system/usr/bob: key does not exist" for a name nobody has, "wrong password" for one that exists. Any client could walk a list of names through it and learn which are real, and read off which strategies the listener runs while doing it. The body now says the credentials were refused and nothing else. The line above it already logs the full joined error with the peer address, so the detail an operator needs to debug a client is still written down, at the daemon rather than at whoever asked. The password comparison in usr.go becomes constant time in passing. It is not what was leaking: the timing difference between the two paths above is nanoseconds behind a tls handshake, a file read and a decrypt, and fifteen requests of each measure the same 12ms. It is one line, and it removes the question.
Creating a client certificate for a usr object took two keywords set first, and neither had a default: cn, because the api reads the common name as the username, and ca, because a certificate the cluster authority did not sign is refused. The documented procedure did not mention them, and what it produced was a self signed certificate naming nobody, refused twice over by the listener it was made for. They now default to the name of the object and to system/sec/ca, so the certificate a usr object generates authenticates as that user, which is the only thing it could have been for. The sec keywords keep no default: system/sec/ca signs itself, and a listener certificate is named after a host rather than after its object.
The cluster `ca` keyword is documented as a list: "the listener accepts a x509 client certificate if it is trusted by any CA certificate found in these sec objects". The listener writes the certificate chain of each of them into the ca file, and the x509 strategy then pem decoded the file once, took the certificate that came out and made a pool of it. So the first authority worked, and an operator who added a second watched certificates it signed be refused as signed by an unknown authority, with the keyword documentation saying otherwise. The pool is now built from the whole file. A certificate signed by any authority in it is accepted, one signed by anything else is still refused, and a file with no certificate in it is an error rather than a pool that trusts nothing.
The speaker seeds its send queue from every instance status it holds, and a datastore or a configuration object has an instance status like anything else. It has no resources though, so what followed was one warning per object per speaker change: "object does not support resource info" for the local instances, whose object cannot answer, and "unexpected response: 400 Bad Request" for the peer ones, whose api refuses the question. Both are the same mistake read from the two ends. The queue now takes svc and vol instances, which are the kinds whose objects have resources. The two errors stay where they are, as the answer to a question this daemon no longer asks.
Since the journald writer was capped at info, five days after the audit endpoint landed, "om daemon log --level debug" has set a global that nothing downstream can honor: the daemon runs with --quiet, so journald is its only writer, and the writer drops what is below info. The request succeeded and changed nothing, and the command printed nothing either way, so there was no way to tell. The endpoint now refuses a level below info and names the audit endpoint in the refusal, which is where a debug or trace feed comes from: audit reads the messages before they reach a writer, which is why it works and this cannot. It also answers with the level in effect, and a GET reports it without changing it. Nothing reported it before, so a node left silent by an earlier --level none had nothing on it to say why. "om daemon log" with no --level now prints the level of each selected node, and with one prints what the level became. Sub-actions in general say more when they fail: doNode read the status code and dropped the body, so a 400 explaining itself was reported as "unexpected status code 400". It now renders the problem document, and closes the response body, which it never did.
The listener log level set two things: the level of that listener's own logger, and daemonapi.LogLevel, a package global the request middleware gave to every api handler logger. So the half of it that worked silenced the info lines api handlers emit, and it did so for both listeners at once, whichever one --name selected. The per listener control it offered was one global with two writers. The other half never worked. Asking for debug or trace set a logger level that the global level and the journald writer, capped at info, both filter afterwards, and the audit endpoint reads the messages before any of that, so it was neither necessary there nor sufficient anywhere. What is left covers it: om daemon log --level warn silences the daemon, and om daemon audit is the debug and trace feed. If listener scoped quieting is wanted again, it should come back as a setting each listener owns rather than as a global they share. The ux listener now has nothing to do with a DaemonCtl message: the log levels were the only actions it acted on, and it has no start, stop or restart of its own. It says so rather than falling through a switch with one unreachable branch left in it.
A listener answered to two names. Audit called it api.inet, from the label it registers when a session starts. The actions called it http-inet, which the api prefixed with lsnr- to reach the name the listener subscribes to. So the taxonomy a client learns from "om daemon audit --sub" did not address the same listener through "om daemon listener restart --name", and the name that did work was documented nowhere but an api.yaml example. Both now come from daemonenv: the listener labels its audit sessions and its control subscription with the same constant the api validates the path name against, so the name in one place is the name in the other. An unknown name is refused with the two that exist, where it used to be accepted, queued and matched by no subscriber. This renames what the actions take: --name api.inet, not http-inet. The heartbeat actions are left as they are. Audit spells that subsystem hb:1 and the action name is 1, prefixed to hb#1, and the reason the audit label is not hb#1 is that a subsystem list travels in a query string, where # ends it. The log control body loses debug from its enum: the endpoint refuses everything below info now, so the schema said a value that always fails. The flag help says the same, and both gain a sentence pointing at the audit endpoint for a debug or trace feed.
The daemon commands were of two minds about what a verb acts on. "om
daemon kill" takes its pids as arguments, with --node and --signal as
the flags they are, while the listener, heartbeat and audit commands
asked for the thing itself through --name and --sub. The first reads
like the rest of the world's command lines, so the others follow it:
om daemon listener restart api.inet
om daemon hb restart 1.rx
om daemon audit imon:* icfg:*
The flags are gone rather than deprecated: --name silently ignored an
argument given in its place, and a spelling that quietly does nothing is
what made this worth touching. Cobra now says "accepts 1 arg(s),
received 0" where the missing name used to be a required flag error.
The audit subsystems travel as a repeated query parameter instead of one
comma joined string, so the client stops joining what the daemon splits.
The subsystem names get a list of their own, which the help text and the
shell completion both read: the help had been written out by hand beside
them and had lost mntmon, and the listener names in it now come from the
same daemonenv constants the actions and the listeners use. Completing
"om daemon audit hb" offers hb, hb.ctrl, hb.main, hb.peer_dropper and
hb:, which was until now discoverable only by reading the help.
Left as a flag: "om daemon ps --sub". It selects what to show rather
than what to act on, and it reads better beside the other filters it
will grow than as the argument of a listing.
v2 read the identity of an app action from the file it ran: a script owned by a user other than root ran as that user, with the group of the file, and the user and group keywords were what decided for a script owned by root. That is what let an operator install a script as themselves and have it run as themselves, without holding the right to edit the object configuration. v3 kept the keywords and dropped the file: every action ran as root unless a keyword said otherwise, and a v2 configuration that relied on the ownership silently gained root privileges it never asked for. The policy is back, from drivers/resource/app/__init__.py of the v2 tree, including the cases around it: a uid with no passwd entry is not an identity to demote to, so it falls back to the keywords, and a command needing a shell is run through /bin/sh, whose owner says nothing about who wrote the command, so it does too. Two things v2 did in the same breath come with it. The group follows the user when only the user is named, where om exec demoted the user and left the command in the group of the daemon, which is root's. And HOME, USER and LOGNAME name the user the command runs as, where a script demoted to its owner read the HOME of the daemon that started it. An env keyword naming one of the three still wins. Verified on a node: a script owned by daemon runs as daemon:daemon with HOME=/usr/sbin, with no keyword and even against user=root; the same script owned by root runs as root, or as the user keyword when there is one.
…n act on The relay heartbeat sent a message every time the daemon produced one, which is one per 250ms propagation tick whenever anything changed, and the interval only said how long to wait when nothing did. Audited on an idle three node cluster, one node posted 19 times in 12 seconds to a stream configured with interval=60s. All but the last of those were wasted by construction: the relay keeps one message per node, overwritten by each post, and a peer reads it once per its own interval. What they were not wasting was the relay's rate limiter, which every cluster using that relay shares by source address. The transmitter now keeps the freshest message and posts it at the interval: at most once, so a burst of changes is one post, and at least once, since a post refreshes the timestamp the peers read this stream's liveness from. A message arriving after a quiet interval still goes out at once, so a node that just started does not wait a minute to be seen. Measured after the change: 2 posts in 130 seconds, a minute apart, with every relay stream still beating. A refused caller is now told what happened. The rate limiter answered 403 with an empty body, which reads as a permission problem and leaves a client nothing to pace by; it answers 429 with a Retry-After of the time it needs to grant one token. The handler for an identifier it could not extract, which is this listener's fault rather than the caller's, answers 500 instead of the 429 it used to borrow. And a relay stream that is refused or unreachable says so: both directions logged every failed beat at trace, which no writer of this daemon emits, so a throttled heartbeat looked like peers going stale for no reason. The first failure of a run is a warning, the rest are traces, and the recovery is logged too.
Pacing the posts to the interval left the stream one second of margin. The receiver marks a peer alive only when it reads a timestamp it has not seen, and both sides tick at the interval, so one failed post stretches the gap between two liveness marks to twice the interval, against a stale timeout of twice the interval plus a second. A refused post and a moment of jitter were a stale peer. Before the pacing the transmitter posted about once a second whatever the interval said, and that accidental margin was what covered this. So the failed post is retried rather than skipped. The interval paces the posts that succeed; one that does not is tried again a quarter of an interval later, or after the delay a refusing relay asked for, whichever is longer, and never later than the next beat was due anyway. The attempts are paced too: a relay that is down would otherwise be posted to once per message the daemon produces, which is what this all started from. And the stale timeout floor goes from twice the interval to four times it, so three missed beats are tolerated and the fourth is the one that declares the peer gone. A relay is reached over a network this cluster does not own, where one lost post is not news. On a node: reajust timeout: 15s => 4m1s for the interval=60s stream, with every relay stream beating. The unicast and disk drivers keep the two interval floor. They still send per message, so their beats are far more frequent than their interval and the floor is not what protects them.
The metrics a node exposes were reachable with a curl to the listener's
unix socket, or with a token and a curl to its api. Both are a lot of
typing for something an operator wants while looking at something else,
and neither reads a peer without an address, a certificate and a
credential.
The api endpoint was already node addressed and already proxied to the
node named in the path, so this is the command that was missing in front
of it:
om node metrics
om node metrics --node dev2n2
om node metrics --node '*'
Several nodes are printed one after the other, each preceded by a
comment naming it, so the output stays readable by what reads an
exposition format.
The proxy carries the grants of the caller, so what a user without the
root grant reads is what the endpoint gives them: the metrics of the
objects in the namespaces they have a grant on, and none of the
daemon's own. The command help says so, since the output of a guest is
otherwise a puzzle.
The recover in the om main wrote the stack of the panicking goroutine to om.stack, and it caught the panics of the command line, where the panic is on goroutine 1 and the file is a copy of what the terminal already showed. It caught nothing of what crashes the daemon this same binary runs. A recover is per goroutine, so a panic in any of the daemon's other goroutines never reached it and wrote no file at all, and a fatal error, which is what a concurrent map write or a deadlock detection is, is not recoverable at all. Both were verified rather than assumed. The daemon now asks the runtime for the report instead. SetTraceback makes it carry every goroutine, where the default prints only the one that crashed and the service unit sets no GOTRACEBACK, and SetCrashOutput copies it to om.stack, for a node whose journal was rotated or never kept. Both cover a crash on any goroutine and the fatal errors, which is the half that was missing. The file is truncated when the daemon starts, and a daemon that starts is often a daemon that just crashed, restarted by its service manager seconds later. So a report found there is moved to om.stack.1 first, and only an empty file, left by a daemon that did not crash, is overwritten in place. Verified on a node: SIGQUIT to the daemon, systemd restarts it, and the report of the crash is in om.stack.1 with its 900 goroutines, next to the empty om.stack the new daemon created. The recover is gone from both mains. A panicking command prints its stack to stderr, where the operator running it is looking.
It printed under the ungrouped commands, away from list, logs and drivers, which is where a reader looks for a command that reads something and changes nothing.
A prkey identifies a node to the storage, and two nodes holding the same one can each preempt the reservations of the other, which is what a scsireserv resource relies on to keep a peer away from a device it was not given. It happens when a node.conf is copied to a peer without redacting the prkey, and nothing said so until a takeover did the wrong thing. The daemon says so now. The prkey is already in the node config every node gossips, so nmon compares the peers' to its own on the config changes it already subscribes to, and writes a warning naming the peer it collides with. The warning is written when the collision appears and when it clears, not on every config change of every node in between. "om node prkey" and "ox node scsi prkey" say so too, on the reading they already print: the key stays on stdout, the explanation goes to stderr, and the exit code is not zero. A key the daemon could not be asked for is not a duplicate: the caller is told the check did not run and is not failed for it. Verified on a cluster by giving two nodes the same key: both daemons logged it, the command exited 1 with the value still on stdout, and setting the key back logged that it is not used by a peer anymore. This sees the nodes of one cluster. Another cluster registering its own keys on the same luns collides in the same way and is invisible from here, which is why the message says what it found rather than that the prkey is unique.
The daemon knew a node's prkey collided with a peer's, and said so in the journal. An operator reads the journal after something went wrong, which for a duplicate prkey is after a takeover preempted the wrong reservation. So the finding travels with the configuration it is about. node.Config gains Issues, the node config equality gates the publication of it as it does the rest, and nmon recomputes the list rather than appending to it: an issue that was corrected has to leave, and looking again is the only way to know it was. A peer changing its configuration can create or clear an issue here without a local file changing, which is why the peer event refreshes and republishes it. It converges, because the issues of a node are computed from the peers' keywords and never from their issues. The api carries them too. GetNode maps the node config field by field into the api type, so a field added to one and not the other is a field "ox node ls" cannot see. The mark is on the states line, which both renderers already share: om mon writes it there, and the tui does not have to spell it. In the tui that line becomes selectable, and following the mark opens the issues of that node in a view of its own. The node name cell is left alone. The mark had been appended to it, which is not only uncolored there: the selection handler reads the node name from that cell's text, so a marked node was a node whose name had a "!" in it. Verified on a cluster: two nodes given the same prkey are marked on the states line of om mon, carry the issue in the dataset the tui reads and in the api "ox node ls -o flat" reads, and setting the key back clears the mark and the list.
The tab renderer walked a map every type built of itself, by hand, in an Unstructured method. The jsonpath it feeds resolves a struct field by its json tag already, which is the name the tab expressions are written in, so the maps were a second serialization of types that already had one, with nothing to keep the two together. They drifted, as a second copy does. The node config gained an issues field yesterday, in the struct, in the api schema and in the api mapping: "ox node ls -o flat" printed it and a tab column of it printed nothing. The pool map had lost "shared" some time before that. A map also has to hand write what the language does for free: Node's dereferenced a nil Monitor, so a tab render of a node whose monitor data had not arrived yet was a segmentation fault. So the renderer hands the jsonpath what it was given. A type whose columns are not its fields still composes a map, and that is what the interface is for now: ObjectData is a union, a raw message with no fields to select, and the heartbeat table entry turns states into the words a reader reads. Both keep their Unstructured method. The 52 mirrors of the api types are gone, and the GetItems methods that lived in the same file are in one of their own, since the renderer does need to be told which field of a list holds the rows. The commands that were building maps to add a column now add it in a type: a pool line is the pool the api sent with the sizes in the units a reader reads, embedded so that the pool fields keep their names. The listings that were building maps for no reason at all hand their items over. Two fixes came with it. The optional fields of the api types are pointers, and a cell is the value, not its address. And a body already read, as the generated clients with a typed response leave it, is no longer an error in the sub action runner: it broke "om daemon ps" when that runner started reading bodies. Checked against the previous binary on a live cluster, sorted, for node, pool, array, network, capabilities, drivers, relay, hb, object, instance, schedule, config and status listings, in tab, json and flat: identical, but for "shared" appearing in the pool output and the heartbeat times keeping their nanoseconds.
The heartbeat table composed its changed_at and last_beating_at as RFC3339Nano text, and the three other times of the same map as values. So a listing showed two columns to the nanosecond and three to the second, and no caller could ask for anything else: by the time the renderer saw them they were strings. They are values now, like their neighbours. The renderer prints a time to the second, which is what a column of a listing wants, and the datasets keep the nanoseconds the type carries, which is what a reader of json wants. Both from one place, rather than from a decision taken in the middle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.