Fix data race issue on copyFn when there are multiple containers or initContainers #422
Conversation
Codecov ReportBase: 59.74% // Head: 67.34% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 59.74% 67.34% +7.60%
==========================================
Files 5 6 +1
Lines 313 392 +79
==========================================
+ Hits 187 264 +77
Misses 103 103
- Partials 23 25 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@Caomoji Would you mind rebasing and solving the conflicts? |
| The option `imageCopyDeadline` (default: `8s`) defines the duration after which the image copy if aborted. | ||
|
|
||
| This option only applies for `immediate` and `force` image copy strategies. |
There was a problem hiding this comment.
Isn't the default 8 really? Since the parameter is an integer of seconds? (Maybe I'm wrong about the configuration parsing). It might also be worth noting that a value of 0 will be treated as if no value was provided (although that's pretty normal for Go projects, the user may already expect this).
There was a problem hiding this comment.
Regarding parsing of this field, if an integer is passed then it will be interpreted as a duration expressed in nanoseconds, while if a string with a correct format (see https://pkg.go.dev/time#ParseDuration) is passed, then it's interpreted using that unit. For instance 8s is effectively converted to a duration of 8 seconds, just as 8000000000 would be too.
Please let me know if you think we should be more explicit about this in the documentation.
Notably, I've already done a very thorough review of this diff with @Caomoji in our fork, if you want to see what was discussed in the past. https://github.com/BackMarket/k8s-image-swapper/pull/4 |
|
@Caomoji Let me know if you need help resolving the conflicts. |
|
@estahn Sorry for the delay, I am monitoring this fix in one of our live environments to make sure it effectively works before rebasing here, but I will do so shortly 👍, thank you! |
|
@Caomoji All good, thanks so much for your efforts. |
|
@estahn We discovered that the context passed in the Besides, some updates were made on logs to adapt messages that were present on the main branch. We will resolve conflicts that this update will cause with this PR, please do not hesitate to provide additional feedback on those changes that, we believe, would open the path to a more registry-agnostic approach while allowing authentication to source registries 🙏 |
|
@Caomoji A bit confused, are you saying this PR should not be merged? |
|
🥳 🎊 Thanks for your help and support @estahn |
Fixes #421
Creates a new
ImageCopierobject with its ownimagePullPolicyto avoid a data race on thecontainerobject.This new object is dedicated to the image copy process as it was done before but with its own subcontext and abstracted from the core mutation webhook processing.
A new configurable
imageCopyDeadlineparameter has been added alongside an implementation of timeout throughout the different steps of the image copy. Every function called as part of the copy has been updated to include context evaluation and support abortion in case of context timeout.