Repository navigation
Conversation
Change-Id: I9a54c012f86ad1562da507e0cd415f6befd644ed
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes the background sweep mechanism (_sweep_stale_tempdirs) and its associated metadata files, constants, and tests from the Bigtable accelerator daemon. The cleanup of stale temp directories at startup is no longer performed, as the daemon is designed to exit and clean up its own socket when the parent process dies, making the sweep mechanism redundant and potentially hazardous. Unused imports and obsolete unit tests have also been removed. I have no feedback to provide as there are no review comments.
This branch has not been deployed
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
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.
Reverts the stale-tempdir sweep added in #18688, keeping the two
sweep-independent cleanups that came with it.
Why
The sweep walked
/tmpat everystart()and deletedbt-accel-*directories whose owning process looked dead. Reviewing the Go daemon
(
bigtable/internal/accelerator) shows most of what it was defending againstis already handled there, and the rest is not worth the mechanism:
Server.monitorParentPidpollsos.Getppid()every500ms and shuts the daemon down when it changes. A
SIGKILLof the Pythonprocess takes the daemon with it within half a second, independent of the
stdin-EOF watchdog.
sockon shutdown.daemon.log, plusidentity.jsonif the clientdied before reading it. A few KB of dead files per crashed client, in a
directory
systemd-tmpfilesalready clears on its own schedule (and whichis tmpfs or ephemeral on most hosts and in every container).
Against that, the sweep was the only code in the module that deletes files it
does not own, in a world-writable directory, by inferring the liveness of
processes in other namespaces. That inference was the source of every bug
found in review: two paths that could delete a live daemon's socket and
identity file, a PID-namespace confusion under a shared
/tmp, anOverflowErrorescaping from a planted pid file, and a test fixture thatswept the developer's real
/tmp. Each was fixable, and each fix added moremachinery to protect a few KB.
What's kept
_socket_is_live, factored out of the probe_wait_until_readyhadopen-coded, and the named filename/timeout constants. Both are independent of
the sweep.
A comment on
_TEMPDIR_PREFIXrecords the reasoning so this does not getreinvented.
Testing
61 tests in
test_accelerator_daemon.py, 2527 intests/unit/data. Verifiedthe result is identical to the pre-sweep baseline apart from the kept items.
Note for the stack
accel9 / accel10 still branch from the pre-squash commits and need a rebase.
This should make accel10's conflict substantially smaller, since it is the
only other branch that edits
_daemon.pyand most of what it collided withis removed here.