gce: make append_or_replace.. atomic #49897
Conversation
Before this change, * the final echo is not atomically written to the target file * two concurrent callers will use the same tempfile
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
approved
mikedanese
added
release-note-none
| echo "${prefix}${suffix}" >> "${file}" | ||
| awk "substr(\$0,0,length(\"${prefix}\")) != \"${prefix}\" { print }" "${file}" > "${tmpfile}" | ||
| echo "${prefix}${suffix}" >> "${tmpfile}" | ||
| mv "${tmpfile}" "${file}" |
There was a problem hiding this comment.
this is still not threadsafe... if two processes both write to different tmp files, one's mv back to the original can stomp the other's change. where are we calling this from multiple threads on a single file?
There was a problem hiding this comment.
As said in #49895 we're seeing corruption in this file - how exactly that happens is still unclear - this should at least prevent corruption. You're right in assessing that we now might loose changes to this file, I tend to believe that is a lesser evil.
There was a problem hiding this comment.
It's one theory. We are scrambling to find a fix for #49895. Copied from that bug:
Very rarely we are seeing known tokens files filled with the NULL character, with ASCII value 000. Restarting the master doesn't fix the issue because the known_tokens.csv is saved between restarts.
There was a problem hiding this comment.
ok, as long as we're clear that this isn't actually making the bash function atomic. the other thing this change does is make it so that a partially failed run of the awk command will still overwrite the file. previously, the mv command would only run if the awk command succeeded.
There was a problem hiding this comment.
ok, as long as we're clear that this isn't actually making the bash function atomic
I'm considering adding an flock around "${file}".
previously, the mv command would only run if the awk command succeeded.
This behavior should still be preserved since the script runs with set -o errexit.
|
[ Quoting <notifications@github.com> in "Re: [kubernetes/kubernetes] gce: ma..." ]
> ok, as long as we're clear that this isn't actually making the bash function atomic
I'm considering adding an flock around "${file}".
As we are not sure what is causing the corruption (i.e. multiple processes?)
using flock could lead to things just hanging. I prefer to do something relative
simple first.
Another item might be getting more visibility; i.e. add logging as well?
|
|
I don't object to the change, but I don't really see this changing anything that would explain nulls in the file. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gmarek, mikedanese Associated issue: 49895 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 49898, 49897, 49919, 48860, 49491) |
wojtek-t
added
the
cherry-pick-approved
|
Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
enisoc
added
release-note
Before this change,
Helps with #49895
cc @miekg