dns - #1099
Open
cvaroqui wants to merge 54 commits into
Open
Conversation
Upgrade ebpf and dependencies to protect from CVE in cilium/ebpf commit e30ccb7bbbc3dfa7376773ab08f977fba4710e99. The vulnerability affects memory_unsafe.go which was added in v0.19.0. v0.18.0 is the highest version without this file, providing protection while maintaining compatibility with existing dependencies. Also upgrades: - containerd/cgroups: v1.0.1 -> v1.1.0 - containerd/cgroups/v3: v3.0.3 -> v3.1.3 - google/nftables: old -> v0.3.0 - Various transitive dependencies
- Remove the buggy getExistingRecords function that only indexed by record name - Use Record directly as map keys (since all fields are comparable) - Properly track existing and new records to detect actual changes - Publish ZoneRecordUpdated only when records are new or changed - Publish ZoneRecordDeleted only when records disappear
Instead of a []Record, so we naturally avoid duplicated and we can detect already existing records fast.
This is repeating every second for no reason: we have traces on socket state transitions already.
Protection against log flood and buffer DoS.
- Added validation to skip empty and invalid IP addresses - Deduplicate Records in Zone
The index is rebuild on: - InstanceStatusUpdated (Instance records added/updated) - InstanceStatusDeleted (Instance records removed) - ClusterConfigUpdated (SOA/NS records depend on clusterConfig.DNS)
Look at the event counter in the Poll() result to decide if reparsing is needed. Add a prom counter for total number of /proc/mountinfo parsing. $ curl -s -k https://localhost:1215/metrics| egrep ^opensvc_mntmon opensvc_mntmon_parse_mountinfo_total 4
The net.(*Dialer).Dial functions (which were consuming ~36% of CPU in the network category) have been reduced to ~5.88%, representing the connection management overhead for new connections (e.g., when nodes restart).
So we don't include in the zone records for scoped ip.host addr when the instance resource is not up.
- Avoid unnecessary string(b) - Avoid adding line feeds in traces
Because, on deadline the socket is closed and it forces tx to close the socket too, making the pool optimization inefficient.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Comment on lines
+204
to
+208
| clearConn := encryptconn.New(conn, crypto.Load()) | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| t.handle(clearConn) | ||
| t.handleLoop(clearConn) |
There was a problem hiding this comment.
🟡 Medium - Removing the RX deadline lets idle connections pin memory and block shutdown
After this deletion, every connection from an allowlisted peer IP is handed to handleLoop with no idle timeout, and each handler immediately retains a 10 MB buffer while blocking in ReadWithNode. A peer that opens many TCP connections without sending a frame can therefore consume unbounded goroutines and memory; because Stop only closes the listener and waits without closing accepted connections, one idle connection can also keep shutdown blocked indefinitely. The IP check is only an address allowlist, so this is reachable by a compromised/configured peer before any message authentication occurs.
Show fix
Track accepted connections and close them when the receiver context is cancelled, and retain a bounded idle/read deadline (or otherwise enforce per-peer connection limits) so unauthenticated idle sockets cannot retain 10 MB handlers indefinitely. Keep the deadline long enough for the configured heartbeat interval, but ensure shutdown actively closes all accepted connections.
More info - Reply on this comment to give feedback or ignore the issue.
Member
Author
There was a problem hiding this comment.
deadline at hbrx.go:305-324, shutdown close at hbrx.go:241-250, per-peer limit at hbrx.go:225-232, and the pre-auth retention now 1 KB instead of 10 MB.
At least the SOA and NS will be available.
Do not hold a global mutex while dialing. Use per-peer connection locks so one peer's dial timeout cannot block unrelated peers.
…umulation When a node (e.g. dev2n2) is in a network blackhole, the hb.ucast tx keeps starting goroutines for every send message. Due to the blackhole, the send never finishes and goroutines accumulate, leading to a memory leak. This change implements per-node send serialization: - Each node gets its own buffered channel (size 1) for send requests - A single worker goroutine per node processes sends sequentially - If a send is already in progress, new sends to the same node wait in the queue (or are dropped if the queue is full) - Other nodes are not affected by a blackholed node - Proper cleanup of send queues and worker goroutines on Stop() This ensures that even if a node is unreachable, only one goroutine per node is blocked, preventing unbounded goroutine accumulation.
…ions Since we now have per-peer sender workers that serialize sends to each node, the connection pool infrastructure (peerConns, peerLocks, getPeerConn, removePeerConn) is no longer necessary. Each worker goroutine now maintains its own persistent connection to its assigned node. This is simpler and more efficient: - No need for sync.Maps to track connections per peer - No need for per-peer locks to prevent concurrent dial attempts - The worker's sequential nature guarantees connection safety - Connections are still reused across multiple sends to the same node - Connection cleanup happens automatically when the worker exits This reduces code complexity while maintaining the same functionality (persistent TCP connections) and the goroutine accumulation fix.
The getMessage function was creating a new bufio.Scanner for each ReadWithNode call. When TCP coalesces multiple authenticated writes, the scanner's internal buffer can contain additional frames after the first NUL delimiter. Discarding the scanner after reading the first frame loses these buffered bytes, causing valid heartbeat/state messages to be silently dropped. This change makes the scanner persistent per connection: - Store scanner and its buffer in the T struct - Initialize scanner once when creating the T connection wrapper - Reuse the same scanner for all ReadWithNode calls on that connection - Return buffer to pool when connection is closed This preserves the existing NUL framing and ensures all frames are read, even when TCP coalesces multiple writes. Each frame is still decrypted independently after being read from the scanner.
Comment on lines
+67
to
+74
| // Close all send queues to unblock workers | ||
| t.sendQueues.Range(func(key, value any) bool { | ||
| q := value.(chan sendRequest) | ||
| close(q) | ||
| return true | ||
| }) | ||
| t.Wait() | ||
| t.sendWorkers.Wait() |
There was a problem hiding this comment.
🟡 Medium - Stopping the unicast transmitter waits on live peer connections
Stop now closes only the per-peer request channels and then waits for every worker, but it no longer closes the worker's active net.Conn. A worker blocked in Dial or in the deadline-bounded Write can therefore keep sendWorkers.Wait blocked for up to the heartbeat timeout, instead of being interrupted as in the previous connection-map implementation; if a worker reaches the unconditional cmdC send after the controller has stopped, the wait can become indefinite. This makes heartbeat shutdown materially slower or capable of hanging when a peer is slow or unreachable.
Show fix
Track each worker's active connection and close it during Stop, or give workers a cancellation-aware connection lifecycle that interrupts in-flight dial/write operations. Also make post-send controller notifications select on the transmitter context so a worker cannot block shutdown after the command consumer exits.
More info - Reply on this comment to give feedback or ignore the issue.
Member
Author
There was a problem hiding this comment.
the dial is interrupted by the context (hbtx.go:210-217, DialContext on a t.ctx-derived context, since c063b34); the worker's cmdC notification already selects on <-t.ctx.Done() (hbtx.go:252-260); and nothing here can hang indefinitely — hbctrl runs on the main context, not the one Stop cancels, hb.Stop() stops it last, and daemon.Stop() runs component stops before cancelling the root context. The ceiling was one timeout, and now there isn't one
The previous implementation had a shutdown race condition: - Stop() closes send queues and waits for workers via sendWorkers.Wait() - But workers blocked in Dial() or Write() would not be interrupted - Workers could also block indefinitely on cmdC send after controller stopped This fix makes workers cancellation-aware: 1. Worker loop now selects on t.ctx.Done() as well as the queue 2. Dial uses DialContext with a timeout that respects t.ctx 3. cmdC send is wrapped in a select that also checks t.ctx.Done() 4. On context cancellation, workers close their connection and exit immediately This ensures that: - Workers are interrupted when the transmitter is stopped - sendWorkers.Wait() completes promptly (within timeout or immediately on cancel) - Heartbeat shutdown is fast and won't hang on slow/unreachable peers - Post-send controller notifications don't block after controller exits
…nn on reconnect Instead of allowing unlimited connections from each peer IP, enforce one handler goroutine per peer. When a peer reconnects: - Close the old connection (old handler exits on next read) - Accept the new connection and start a new handler - Peer doesn't see errors or retry The accept loop (single-threaded) manages a peerConns map: - On new connection: check if peer has entry, close old conn if so - Store new connection and start handler - Handlers don't touch the map; only read their own connection Combined with read deadline (3x timeout, min 10s), this ensures: - Bounded goroutines (one per cluster peer) - Bounded memory (10MB per peer) - Seamless connection replacement on reconnect - No retry storms from peers
The persistent scanner introduced with the per-connection scanner change brought a Close() that returned the scanner buffer to msgPool and nilled the scanner. Both are unsafe while a reader goroutine is still using the connection, and hb.ucast rx has exactly that pattern: the accept loop calls oldConn.Close() when a peer reconnects, while the old handler goroutine is still in its read loop (and the handler itself closes the connection again on exit). Two failure modes: - Nil dereference: a handler parked on the msgC send resumes after Close() has set t.scanner to nil, then calls scanner.Scan() and panics the daemon. - Cross-connection buffer sharing: the "scannerBuf != nil" check is not atomic, so both closers can Put the same 10MB slice back to the pool. Two later connections then get the same backing array and their scanners write into it, corrupting frames and failing decryption. Give the buffer to the *T for the lifetime of the connection instead: - Remove msgPool and the scannerBuf field - Remove the Close() override, so Close() resolves to the embedded net.Conn, which is race-free and safe to call twice - Size the buffer at msgUsualSize and let bufio.Scanner grow it, up to msgMaxSize, only when a bigger message is read Dropping the pool loses buffer reuse across connections, but not memory: the pooled buffer was held for the whole connection lifetime anyway. In hb.ucast connections are one per peer and long-lived, so the allocation churn is negligible, and the steady-state footprint per connection drops from 10MB to msgUsualSize.
Stop() closed every per-node send queue before waiting on the Start goroutine. That goroutine is the only sendToNode caller, and it is still running at that point: if it had already passed its select on msgC when the context was cancelled, it goes on to send on a queue Stop() just closed, panicking the daemon. The CmdDelWatcher sends in between make the window comfortably wide. Wait for the Start goroutine first, so the queues provably have no writer left, then close them. Also delete each queue from sendQueues before closing it, so a second Stop() doesn't close an already closed channel: a tx can be stopped both by the daemonctl path and by the config change path. Worst case Stop() still waits up to timeout for a worker parked in conn.Write. Closing the queues never unblocked a write either.
The send worker dials inside its loop and deferred the context cancel. The worker goroutine lives as long as the transmitter, so those deferred cancels are never run until stop: every reconnect leaves another one on the goroutine stack, growing without bound on a flapping peer. Cancel right after the dial returns instead. Cancelling the dial context once the connection is established doesn't affect the connection, so the dial deadline still applies and nothing else changes.
Each send worker read t.localIP from its own goroutine to build the source address it dials from, while the Start goroutine refreshes t.localIP every 30s: a data race on the net.IP, and the refresh never reached an already running worker, which kept binding the source address it snapshotted when it was created. Carry the local ip in the sendRequest instead. The Start goroutine is the only sendToNode caller, so t.localIP is now read and written by that goroutine only, and the value the worker dials from is the one current when the message was queued. A worker whose connection is bound to a source address that is no longer the local ip now closes it and redials, instead of waiting for the write to fail once the address is gone from the interface.
Stop() cancelled the context and waited, but left the established peer connections open, so a handler parked in ReadWithNode held the shutdown until its read deadline expired: up to 3x timeout, 45s with the defaults. A peer that keeps beating wakes its handler within an interval, but a silent or partitioned peer didn't, and stopping or reconfiguring the heartbeat stalled for that long. Close the tracked connections at the end of the accept loop, right before waiting for the handlers. Doing it there rather than in Stop() keeps the invariant that only the accept loop writes peerConns: the loop has stopped accepting by then, so no connection can be added behind our back, and closing a connection unblocks the read its handler is in.
rebuildNameIndex() reallocated the whole name to records index from all of t.state, and was called on every InstanceStatusUpdated, one of the highest frequency events in the daemon, even when the handler had published no record change at all. That traded a per lookup cost for a per event cost that grows with the number of objects in the cluster. Keep the index in sync where the state changes instead: - setStateRecords() replaces the records of one state key, unindexing the ones it had and indexing the new ones, so the cost is scoped to the object that changed - setClusterRecords() refreshes the SOA and NS records only, on cluster config change - delIndexRecord() removes a single occurrence: the same record can be indexed once per state key that yields it, and the others must stay Record.Key() replaces the recordKey literals built at each site, and the cluster records construction is now shared by the index and zone(), which had two copies of it. TestNameIndexIsMaintainedIncrementally checks the invariant the full rebuild gave for free: after any sequence of state and cluster config changes, the index holds exactly the records a rebuild from scratch would, duplicates included.
Each worker appended the null frame terminator to the buffer it got from the send request. That buffer is the single copy the sender shares with every per-node worker, so the appends were only safe because make([]byte, len(b)) returns a slice with no spare capacity, forcing append to allocate. A buffer allocated with room to spare would have had the workers writing their terminator into the shared array at once. Allocate the extra byte in the sender instead, where the copy is made, and let the workers write the buffer as is. The bytes on the wire are unchanged.
sendToNode() passed a freshly made channel to LoadOrStore on each call, so every send allocated a queue only to discard it, the node having had one since its first message. The lazy creation isn't needed at all: t.nodes is fixed for the lifetime of the transmitter, so Start can create the queue and the worker of each peer node up front, before the sender can reach them. sendQueues becomes a plain map, written once and only read afterwards: no sync.Map, no create on first send, no worker start race. sendToNode() is left with a queue lookup and a non blocking send, and the worker moves out of it to startSendWorker(). The node and addr fields of sendRequest go away with the same move, the worker having them from Start.
The read error handling compared err.Error() to "EOF" and logged both branches identically, at trace level. A peer whose frames stop decrypting, after a heartbeat secret rotation for instance, was therefore invisible at the default log level, the connection just silently dropping and being redialed. Match the errors with errors.Is and keep quiet only for the two expected ones: the peer closing the connection, and us closing it on a peer reconnect or a stop. A read deadline expiry, which means the peer went silent for 3x timeout, and any other read failure are now warnings.
A received message logged four trace lines, a sent one two, all of them saying much the same thing. Keep one per message on each side: the read one, now carrying the size, the peer node, the message kind and the count, and the write one. The rest of the traces are per connection or per error, and stay. Drop the "handleLoop:" prefix while there: the logger prefix already says which node, subsystem, direction and heartbeat the line comes from.
The receiver captured the crypto when accepting the connection, and the connections are now long lived, so it kept decrypting with the secret that was current back then. A heartbeat secret rotation then broke every established connection: the reads failed until the peer noticed its writes failing and redialed. Those failures are logged as warnings since "Tell hb.ucast rx read failures apart", which is alarming for a rotation the cluster admin asked for and which is meant to be transparent. Decrypt through hbcrypto.Loader, which resolves the crypto at each call, the way hbdisk and hbrelay refresh theirs at each tick. Nothing is dropped anymore: nmon only commits the rotation once every node has acknowledged the candidate secret, so every node then holds both keys, and the read failure warnings are back to meaning a real anomaly.
The receiver dereferenced the crypto pointer into its own field, copying the atomic.Pointer. The copy keeps the value the pointer held at Start, and the stores the hbcrypto worker does on a heartbeat secret rotation land on the original, so the receiver decrypted with the secret current when the daemon started, forever. It survives the first rotation, the peers new key usually being the alternate key it already holds, then stops decrypting anything on the next one, until the daemon is restarted. go vet says nothing: atomic.Pointer has no noCopy. Use the hbcrypto.Loader added for hb.ucast, which resolves the pointer at each call.
The tests starting a daemon bound the cluster default listener port, so they failed with "listen tcp :1215: bind: address already in use" on a development node running a real daemon. daemontesthelper.SetFreeListenerPort() sets the listener.port cluster config keyword to a free port before the daemon is started. daemon.Start picks it up from the config, and republishes it to daemonenv.HTTPPort, where the test clients find it. It goes in daemontesthelper because both the daemon and the integrationtest packages can import it. TestDaemonStartupWithoutConfig has no config to set the port in, its daemon binding the listener.port keyword default, so it now skips when that port is taken, saying why.
Test_TcpPortAvailable asserted 1215 was available, which it isn't on a node running a daemon. The test only needs a port nobody listens on: ask the kernel for one.
The receiver resolved the peer node names once, at start, and rejected every connection coming from an address absent from that snapshot. The transmitter, on the other hand, refreshes its local address every 30s and rebinds its connections to it. So a node whose address changed kept beating, from an address its peers refused, until their receiver was restarted. Both sides read the same source of truth: the transmitter dials from the address the local node name resolves to (defaultLocalIP), and the receiver allows the addresses that same name resolves to. A connection from an unknown address is therefore the signal that a peer may have moved, and resolving the names again is enough to confirm it. Do it in the accept loop, on the rejection path: - the accept loop is the only owner of the allow list, so the refresh needs no locking, unlike a ticker in a goroutine of its own - a known address never triggers a lookup, so the steady state is unchanged - one refresh per timeout at most, so a stranger hammering the port can't turn into a lookup storm - a node whose lookup fails keeps its known addresses, so a resolver hiccup doesn't empty the allow list The listener closing goroutine now gets its own copy of the address list, which the accept loop writes to.
Each handler took a msgMaxSize buffer from a pool, and held it for the life of the connection, because ReadWithNode copies the message into a caller buffer that must be large enough for the biggest message to come. So a peer holding an idle connection held 10MB, before sending anything, and before any message authentication: the accept loop only checks the source address against the peer address list. MessageWithNode returns the decrypted message instead of copying it into a caller buffer. The slice is sized for the message read, so an idle connection holds nothing but the scanner buffer, which starts at the usual message size, and the copy is one less. The 10MB stays reachable, the scanner growing to the max message size for a peer that really sends one, which is what the max is for. It now takes sending it.
Stop closed the send queues and waited for the workers, but not their connections, which the conn pool removal had taken away. A worker parked in a write to a peer that stopped reading is only released by its own deadline, so stopping or reconfiguring the heartbeat took a timeout, 15s with the defaults, instead of being immediate. The connection each worker dials is now published to its sendWorker, behind a mutex, so that Stop can close it and end the write. The worker keeps owning the dial and the send: the mutex guards the hand off to Stop only, and closing a connection under a blocked write is the way to interrupt it. handleSendError returns early once the context is done, so the write Stop just interrupted doesn't open a send error period on the way out. The dial was already interrupted by the context, and the send success notification already gave up on it, contrary to what the report says.
|
|
||
| o, err := object.NewCluster() | ||
| require.NoError(t, err) | ||
| require.NoError(t, o.Config().Set(keyop.ParseList(fmt.Sprintf("listener.port=%d", port))...)) |
There was a problem hiding this comment.
🔵 Low - Free-port helper fails when the cluster config has no listener port key
SetFreeListenerPort calls o.Config().Set with listener.port, but the cluster configurations used by its callers do not contain a [listener] port key and xconfig.Set rejects a missing key instead of creating it. The helper therefore returns an error through require.NoError during setup, so the daemon and integration tests that now invoke it fail before they can start. This makes the new test isolation change unusable unless the config text includes the key or the helper adds the section/key before calling Set.
Show fix
Ensure the test configuration contains a [listener] section with a port key before calling Config().Set, or update the helper to add the missing section/key through the supported xconfig construction API and then persist the value.
More info - Reply on this comment to give feedback or ignore the issue.
Comment on lines
29
to
33
| ConnNoder interface { | ||
| net.Conn | ||
| ReadWithNode(b []byte) (n int, nodename string, err error) | ||
| MessageWithNode() (b []byte, nodename string, err error) | ||
| } |
There was a problem hiding this comment.
🔵 Low - Adding MessageWithNode breaks existing ConnNoder implementations
ConnNoder is an exported interface, and this change adds MessageWithNode to its method set. Any downstream package or external test double that previously implemented the exported net.Conn plus ReadWithNode contract will stop compiling even though it does not use the new persistent-message API. The repository has no alternate implementation to expose the break, but consumers implementing this public interface are reachable outside the repository.
Show fix
Keep the existing ConnNoder interface unchanged and define a separate interface for handlers that need MessageWithNode, or expose the new method on a concrete type and type-assert it only where required. If the breaking interface change is intentional, document and version it as an API-breaking change.
More info - Reply on this comment to give feedback or ignore the issue.
tview's Table page-up/page-down handlers snapshot the current selection before clamping it and use it as the sentinel terminating their cell scan. When a table has selection enabled but not a single selectable cell, Table.Draw() has already pushed the selection one row past the last one looking for a selectable cell, so that sentinel is never reached and the scan loops forever, wedging the whole application. createTable() enabled selection unconditionally, while the heartbeats view declares no selectable column (and the relay view declares none at all). Both were a page down away from a hang, whatever their row count. A view declaring no selectable column is a plain scrollable table: don't enable selection on it. Page up and page down then move the row offset, which is the wanted behaviour there. As those views have no selection to restore from t.position, carry the scroll offset over the periodic table rebuild, so a heartbeat status change does not snap the reader back to the first row.
Adding a view meant editing six places: the viewId const block, its String(), viewPrimitive(), the two switches of navFromTo() (teardown and setup) and the refresh switch of do(). They had already drifted apart: String() had no viewEvents case, so the events view had no head bar title, and viewPrimitive() had no case for any of the createTable based views, returning the object table for all of them. It was also dead code. Declare instead one viewDefs entry per view, holding its title and its enter, refresh and leave hooks. viewId.String(), the nav dispatch and the data update dispatch all read that single table, and a view owning a resource can no longer forget to release it in a switch case nobody remembers to extend. Four places built the flex layout by hand: navFromTo(), createTable(), listContexts() and the instance view postamble, and they disagreed. createTable() dropped the errors bar on every refresh, and so did the instance view. Funnel them all through mount(), which lays out the head bar, the optional banners, the view body and the errors bar, and remembers the layout so the help popup can restore it. The instance view restore was broken: it put the summary banner back in place of the resources table. This also removes the flex index guessing (GetItem(1) is the body, GetItem(2) is the instance table) from the popups and from the command line handler. The navigation stack now holds frames rather than bare view ids. A frame carries the cursor position and the element the view drilled down into, so: - coming back to a view lands where the user left it, instead of on the first cell: t.position was a single App field, shared by three views and reset on every navigation. - the pool drill down no longer needs previousSelectedElement: pushing a frame saves the parent element, popping restores it. - navigating to the frame already on top is a no-op, so hitting the log key twice no longer stacks two log frames, each needing its own ESC, while entering a pool from the pool list still pushes a frame. The stack is never empty: its root frame is the object view, or the context view for a user only granted the relay view, which is what backToContext was emulating. listContexts() becomes the context view enter hook, so t.focus() no longer lies while the context list is displayed. Also fixes, found on the way: - the instance view set two selection changed funcs on its table, and tview only keeps the last one: t.viewRID was never set, so the 't' and 'T' container terminal shortcuts never triggered. - entering a view left the head bar naming the previous one until the next cluster data change. - printf() added a second errors bar for 5s, then removed both.
Entering the log view opens one log reader per node, a blocking daemon call, and it ran on the tview loop. That call is served on the same connection as the event stream, and the only reader of that stream is do(), which is itself blocked in QueueUpdateDraw waiting for the tview loop to run the update. The connection stops being drained, its flow control window fills, the log request never completes, and the whole application is wedged. Open the readers from a goroutine instead, and hand it the node list and the object path it needs, so it does not race with the view fields. A reader can now be opened after the user has already left the log view and CloseAll() has run. Have AtomicCloserSlice refuse a closer in that case, and let the caller close it, instead of leaking a followed log stream. The log view enter hook re-arms the slice.
t.focus() reads the navigation stack, which nav() and back() write from the tview loop, and do() called it from its own goroutine to decide whether to refresh the object view clock. Mirror the focused view id in an atomic and read that off the loop. The events view goroutine read t.textView, which the view leave hook nils from the tview loop, guarding it with a plain nil check that happens to be a data race. Hand the text view and the context over to the goroutine, like the log view now does with its writer. t.stopEvents, toggled from the tview loop and read by that goroutine, becomes an atomic too. Also make the event fan out to the events view a non blocking send. The channel is only drained while that view is displayed, and the producer stops one check after the view is left: a stale event stays in the buffer at every visit, and once a hundred of them piled up the send blocked the whole event pipeline for good. An event dropped there only misses a line in the live tail, the cluster data is applied regardless. The remaining known race is the log and events views streaming into their tview TextView from a goroutine, concurrently with the draw loop. Fixing it means funneling those writes through QueueUpdateDraw. Until then the smoke test skips under the race detector.
The log and the events views are written into by the goroutines streaming their content. TextView.Write() takes the text view lock, so that part is fine, but both views also called TextView.ScrollToEnd() off the tview loop, and that one writes trackEnd and columnOffset with no lock at all, concurrently with TextView.Draw() reading them. The log view called it from the changed handler, which tview invokes in a goroutine of its own, and the events view called it straight from its streaming goroutine. Set the follow flag once instead, on the tview loop, when the view is entered: nothing clears it until the user scrolls up, so following the tail does not need a call per line. That leaves the changed handlers with the one thing tview documents as allowed there, Application.Draw(), which neither view was doing: writing into a text view does not refresh the screen, and the tui only redrew on a cluster data change. Both tails froze on an idle cluster. They now repaint as the lines land. Also stop clearing the changed handler when leaving the log view: SetChangedFunc() is an unlocked field write too, and the readers are still reading it while they wind down. The handler only asks for a redraw, so it costs nothing on a text view left behind. TestStreamedTextViewRedraws covers both views, and the smoke test no longer needs to skip under the race detector. It reads the application state from the tview loop, which owns it.
printf() wrote the errors bar wherever it was called from, and errorf() is called from the goroutines nobody thinks about: the event reader, the event to message conversion, the log readers and the events view. Two of its writes are tview.Box.SetBackgroundColor(), an unlocked field write, concurrent with the draw loop reading it. The second one came from a time.AfterFunc, so it was off the tview loop even for the callers that were on it. Give the bar a single writer: printf() now queues the message and runErrsBar() displays it, from the tview loop, through QueueUpdateDraw. The queueing is a non blocking send, so the goroutines feeding the event pipeline can no longer be held up by the display. Dropping a message the bar has no room for is what the user got anyway: each printf() used to schedule its own expiry timer, and an early one cleared a later message. A single timer, reset on each message, gives every message its full lingering time.
ReadCloser.closed was a plain bool, written by Close() and read by Read(). Those two run on different goroutines whenever a stream is closed by someone other than its reader, which is what the tui does to the log readers when the user leaves the log view. Make it an atomic, and let Close() claim it with a swap so a concurrent second Close still gets ErrClosed.
t.Current, t.Nodename and t.eventCount were assigned by the goroutine draining the cluster data, then the views were told to repaint through QueueUpdateDraw. That orders the assignment against the repaint, but not against the key handlers, which read the same snapshot from the tview loop: navigating to a view while new data lands has the view build its table from a snapshot being overwritten under it. Assign inside the queued function, so the snapshot is written and read from the tview loop only.
The daemon publishes one instance.status.running entry per resource run in progress, a task or a sync, but nothing surfaced it in the daemon status renderer: users had to open the instance status of every instance to find out where a run was going on. Add an R flag to the instance cell of the objects section, right after the availability and warning icons, so it shows in the om and ox monitor output and in the ox tui main page, which share the renderer. Count the encapsulated instances runs too. Their run info lives in the encap status, which nothing reads: ResourceFlagsString() looks the encap rids up in the running list of the outer instance, where they are not.
cobra walks the command chain and, when the deepest command it reaches has no Run function, prints that command help and returns no error, silently dropping whatever was typed after it. It does so before validating the leftover args, so there is no hook to reject them. A stale or mistyped command path exits 0 that way. That is how the daemon, which execs om with hard-coded argv from its scheduler and from its api action handlers, kept reporting successful runs of commands that had been renamed under it. Resolve the args before handing them to cobra, and refuse the ones leaving a command name behind a command that can not run. The leftover args have to be told from the flags to find that name, and cobra parses the same args again right after, so the parse is done on a throwaway flag set holding copies of the flag shapes, values discarded: parsing the real ones twice would append twice to the slice flags. setExecuteArgs() now returns the args it hands to cobra. Its rewriting of an object selector into a subsystem command is untouched.
The scheduler and the api action handlers exec om with a hard-coded argv. Four of them named no command: push_resinfo om <path> instance resource info push sync_update om <path> instance sync update api push resinfo om <path> instance push resinfo api sync ingest om <path> instance sync ingest The resource subsystem hangs off the object, not off the instance, and the sync one likewise. om answered those with the instance help text and exit 0, so the hourly resinfo refresh, the scheduled sync updates, and both api actions have been silent no-ops. On a cluster where nobody ran the working spelling by hand, the resinfo cache the object and instance resource info endpoints read was never written. sync_update now passes the entry rid: those entries are created one per sync resource, from the resource schedule keyword, so the whole object was never the target. Drop the pushstats and reboot cases: they name commands om does not have either, and no schedule entry builder emits them. An unknown action at least returns an error the scheduler logs, instead of a successful run. TestSchedulerCmdArgsResolve and TestDaemonAPIExecArgsResolve resolve every one of these argv against the om command tree. Nothing else keeps them in sync: they are strings, so neither the compiler nor the type system has anything to say about them. The api ones are read back from the source, being built inline in each handler.
"om svc resource --help" printed an empty "Subsystems:" section, and 24 other sections across the tree were empty too: the object print commands with their hidden children only, the pg commands, and the instance commands of the kinds having no replication resource. Two causes. The usage template prints a group title for every group a command declares, whether or not a command of that command belongs to it. That is inherited from the cobra default template. Ask first. And "resource info", holding the list and push commands, declared no group at all. It landed in the "Additional Commands" section, right below the "Subsystems:" title it was meant to fill. The help of the 22 commands with populated groups lists exactly what it listed before, empty titles aside.
node push, sec and usr certificate, and the daemon dns, hb, listener and relay commands hold subcommands but belonged to no section: they landed in the "Additional Commands" one, among the verbs of their parent. The daemon command had no subsystems section at all to put them in. TestSubsystemCommandsAreGrouped now walks the tree instead of naming the resource info command: wherever a command offers a subsystems section, the commands holding subcommands under it must belong to a section, be it that one or the resource groups one. The root command is left out: its object kinds and its subsystems are all still ungrouped, and naming their sections is a question of its own. Two bugs found on the way, both of them a copy-pasted constructor: - "om daemon" registered two run commands, the running one having been built from the run constructor. Cobra runs the first of the two, so "om daemon running", which exits 0 when the daemon runs and 1 when it does not, could not be reached at all. - the instance unprovision command aliased itself "prov", the alias of the provision command. "om <path> instance unprov" named no command, and "prov" named two. TestNoDuplicateCommandName covers both. It skips the hidden commands a visible one shadows: those are backward compatibility spellings, not duplicates.
The sync ingest command carries a sync resource selector by default, where the ingest action is not the sole business of the sync resources: a disk resource group can support it too. The instance ingest command selects them all, and the handler passes the rid, subset and tag parameters through, so the caller keeps narrowing the selection down.
The ccfg command file added a monitor command to the ox root, which the monitor command file already does: the ox help listed monitor twice, and cobra ran the first of the two.
The om and ox root help listed every command in a single flat section, where the object kinds, the subsystems and the query commands are three different things to reach for. The root command now declares the three sections. It is built from a package level variable initializer, which the runtime runs before every init() of the package, so the command files register into those sections whatever order they run in.
The checks lived in the om test file, where the ox tree, which has its own commands, went unchecked: it had an ungrouped schedule and node system command, and a duplicate monitor. They move to a helptest package, outside of commoncmd so both root commands, which commoncmd knows nothing about, can run them on their own tree.
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
2 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.