Repository navigation
[database] Publish database_config.json atomically - #29464
Conversation
The initial jinja render already publishes through a temp file and rename, but the
remaining publication sites still write the live path directly:
- the copy from /etc/sonic/database_config*.json truncates the destination
- the final copy of the modified config truncates it again
- update_chassisdb_config reopens it with mode "w", which truncates before the
new content is written
Readers therefore see an existing but empty file and log:
ERR python3: :- parseDatabaseConfig: Sonic database config file syntax error >>
[json.exception.parse_error.101] parse error at line 1, column 1: attempting to
parse an empty input
The in-place rewrite kept the live file truncated for 285 us.
This window is independent of the platform slot queries: it also occurs on
platforms without chassisdb.conf, where the final copy is used instead.
Publish every site through a rename so a concurrent reader either sees the
previous content or the new content. The final publication renames the temp file
it already builds instead of copying it, so the cleanup that follows now tolerates
an already consumed temp file.
Signed-off-by: Charles Tsai <chartsai@nvidia.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
This PR has backport request for branch(es): 202605. ---Powered by SONiC BuildBot
|
|
The Tested branch section has been ticked and Test result is provided for branch(es): 202605. Added label(s): Tested for 202605 Branch. ---Powered by SONiC BuildBot
|
|
/azpw retry |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1217041: ✅Stage Test:
|
|
@qiluo-msft could you please help review this PR? thanks |
yxieca
left a comment
yxieca
left a comment
There was a problem hiding this comment.
Approving. Correct fix for the truncated-read race on database_config.json. Replacing in-place cp / open(...,"w") (which truncates the live file before streaming bytes) with temp-file + atomic rename (mv / os.replace) in all three write paths means concurrent readers — e.g. waitForAllInstanceDatabaseConfigJsonFilesReady — always see either the complete old file or the complete new one, never an empty/partial JSON.
The atomicity relies on rename(2) being atomic on the same filesystem, which holds here since the temp .json.new is created in the same directory as the destination. Extending the pattern already used in the rendered branch to the cp branch and update_chassisdb_config is consistent and closes the remaining gaps.
Minor (non-blocking): the temp name is a fixed .json.new rather than a PID/mktemp-suffixed name; fine here since the init path isn't run concurrently for the same file and the rename is still atomic.
|
Cherry-pick PR to 202605: #29693 |
mssonicbld
added
Included in 202605 Branch
and removed
Created PR to 202605 Branch
labels
5 participants
Why I did it
database_config.jsonis read concurrently during database startup. PR#28343 made the initial publication
atomic, but three later publication paths remained non-atomic and could truncate a live JSON file
before writing its replacement. This PR extends atomic publication to those remaining known paths.
Instrumentation on DUT measured a 285 us truncate-and-rewrite window in the live-file
update_chassisdb_configpath. The controlled 202605 A/B test reproduced the empty-read failurewith the installed writer and eliminated it with the atomic writer.
Work item tracking
How I did it
/etc/sonic/database_config*.jsonto a sibling temporary file and rename it onto the livepath.
it, and tolerate the consumed temporary file during cleanup.
update_chassisdb_configwrite a sibling temporary file and publish it withos.replace().How to verify it
Run concurrent JSON readers while repeatedly invoking the installed in-place writer and the
patched atomic writer against the same scratch configuration. Use identical writer iterations and
reader counts for both arms, and verify that the live database configuration remains untouched.
Which release branch to backport (provide reason below if selected)
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO):
N/A (internal tracking is not disclosed)
Failure type: other — existing non-atomic database configuration publication race
Tested branch
Test result
of a broader startup-path test and produced zero parser errors. Because that arm contained
additional startup-path changes, it is integration coverage rather than isolated proof for this
PR. Restoring the control produced three errors.
os.replace()writer: 132,972 successful reads and zero empty reads.Description for the changelog
Publish
database_config.jsonatomically to prevent readers from observing empty JSON duringnormal updates.