Fix order of file copies in entrypoint.sh #935
Conversation
Fixes this error when you upgrade the CNI because the CNI starts prior to portmap is copied and portmap is then sometimes in use by the time the cp commands execute
|
Hi @dthorsen, good catch finding that cp error message! In this case, we would like to only copy the portmap and not the CNI binary and config before ipamd starts, since that might make a new node ready even if it cannot start. Could you update the PR to only copy the portmap early? That said, I'm working on a PR to add an init container instead, to make the setup part more explicit. (Issue #158) |
|
Taking a second look at this, I wonder if we should check if the file exists, if so rename it to |
|
Thanks @mogren! That makes sense I think. I moved only the portmap binary to copy early. |
|
Oh that might make sense. We can totally do that. |
|
I think this does what you are asking. The timing difference here is probably milliseconds though. I am not certain it buys much over simply copying over the file. I suspect that if something was actually executing portmap it would be cached in RAM anyway and I don't think those few extra milliseconds will protect from much. |
There was a problem hiding this comment.
Thanks @dthorsen, looks good to me. I wonder if we might have to do the same thing for the aws-cni as well, but that would be a separate PR. I'll let someone else take a quick look at this as well before merging. Thanks a lot for finding and fixing this issue!
scripts/entrypoint.sh
Outdated
| HOST_PORTMAP="$HOST_CNI_BIN_PATH/portmap" | ||
| if [[ -f "$HOST_PORTMAP" ]] | ||
| then | ||
| mv "$HOST_PORTMAP" "$HOST_PORTMAP.old" | ||
| cp portmap "$HOST_CNI_BIN_PATH" | ||
| rm "$HOST_PORTMAP.old" | ||
| else | ||
| cp portmap "$HOST_CNI_BIN_PATH" | ||
| fi |
There was a problem hiding this comment.
Having taken another look, I think this could be simplified even more. What do you think about:
HOST_PORTMAP="$HOST_CNI_BIN_PATH/portmap"
if [[ -f "$HOST_PORTMAP" ]]; then
rm "$HOST_PORTMAP"
fi
cp portmap "$HOST_CNI_BIN_PATH"
There was a problem hiding this comment.
Simplification engaged
* copy only portmap early before cni starts in entrypoint.sh Fixes this error when you upgrade the CNI because the CNI starts prior to portmap is copied, and portmap is then sometimes in use by the time the cp commands execute Co-authored-by: Dane Thorsen <dthorsen@newrelic.com> Co-authored-by: Claes Mogren <mogren@amazon.com> (cherry picked from commit 2487803)
* copy only portmap early before cni starts in entrypoint.sh Fixes this error when you upgrade the CNI because the CNI starts prior to portmap is copied, and portmap is then sometimes in use by the time the cp commands execute Co-authored-by: Dane Thorsen <dthorsen@newrelic.com> Co-authored-by: Claes Mogren <mogren@amazon.com> (cherry picked from commit 2487803)
Fixes this error when you upgrade the CNI because the CNI starts prior to portmap
is copied and portmap is then sometimes in use by the time the cp commands execute
Issue #, if available:
Description of changes:
This fixes the following error from causing the CNI pod to enter crashloopbackoff:
cp: cannot create regular file '/host/opt/cni/bin/portmap': Text file busyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.