cri: Don't use rel path for image volumes#8885
Conversation
|
Hi @rata. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
mikebrow
left a comment
There was a problem hiding this comment.
see suggestion..
tagging related PR #8524 @gabriel-samfira
|
As discussed in the community meeting, perhaps a switch is nec. in runc, deprecated/temporary, to enable ignoring/disabling the abs check, until such time as the affected container runtimes have been patched and deployed and/or are no longer in support. |
|
/ok-to-test |
gabriel-samfira
left a comment
There was a problem hiding this comment.
Handling paths is tricky. Generally speaking, paths should be "normalized" based on target OS. The target OS in containerd is generally set by the sandbox image.
In the case of this PR, I think we can get the desired result if we move the check for relative paths closer to where those volumes need used.
|
@gabriel-samfira thanks for the review and catching the issue with Linux containers on Windows hosts! I think this changes now should fix that and make everyone happy. PTAL @gabriel-samfira @fuweid @mikebrow |
gabriel-samfira
left a comment
There was a problem hiding this comment.
LGTM. Some tests seem to be failing, but otherwise seems ok.
|
@gabriel-samfira @fuweid thanks! Re-pushed so tests run again, it didn't seem related to this PR. |
b11717c to
ae719ea
Compare
|
@gabriel-samfira @fuweid fixed the CI now, PTAL (the tests I added were quite useful!). The issue was that when executing on Windows, filepath.Join() does a clean, which changes from slash to backslash, and didn't match the expected paths. I've adjusted the code to work fine on windows and linux, and added a log line as @mikebrow suggested. The changes were basically this. |
|
@rata would you update with mike's suggestion? after update, I will merge it. Thanks |
Runc 1.1 throws a warning when using rel destination paths, and runc 1.2 is planning to thow an error (i.e. won't start the container). Let's just make this an abs path in the only place it might not be: the mounts created due to `VOLUME` directives in the Dockerfile. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
|
@fuweid just changed the log line from info to debug, thanks! |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Runc 1.1 throws a warning when using rel destination paths, and runc 1.2 is planning to thow an error (i.e. won't start the container).
Let's just make this an abs path in the only place it might not be: the mounts created due to
VOLUMEdirectives in the Dockerfile.We discussed this issue in today's containerd community meeting and decided to make this small PR that should fix it, and if this is merged without concerns, backport it to 1.6 and 1.7.
cc @mikebrow