fix: krane using lazy_static! caused uncleaned temp files #564
Conversation
tools/krane/src/lib.rs
Outdated
| let mut krane_reader = KRANE_BIN.reader(); | ||
| std::io::copy(&mut krane_reader, &mut temp_file)?; | ||
|
|
||
| use std::io::Write; |
There was a problem hiding this comment.
is there some reason this use statement is mixed in with the rest of the code? I tend to prefer to put use statements at the beginning of the block that is using it to make it clear where it applies, since it will apply to the whole block
There was a problem hiding this comment.
Yes this should be moved up.
tools/oci-cli-wrapper/src/lib.rs
Outdated
| impl ImageTool { | ||
| /// Uses the builtin `krane` provided by the `tools/krane` crate. | ||
| pub fn from_builtin_krane() -> Self { | ||
| let tools_dir = std::path::Path::new("./build/tools"); |
There was a problem hiding this comment.
this will create build/tools and install krane under whatever directory we run the command in, which isn't quite right - we want it installed under build/tools in the Twoliter project's root directory. you can find the right dir from within Twoliter if you have a Project by doing
let toolsdir = project.project_dir().join("build/tools");such as here. so maybe we can add an argument to this function that accepts a tools_dir - this should work for all of the Twoliter uses.
however, since we also use ImageTool::from_builtin_krane() in pubsys (here) this might be a bit tricky. maybe for now we can rely on the TWOLITER_TOOLS_DIR variable, since that should always be available when pubsys publish kit is invoked via twoliter?
There was a problem hiding this comment.
Yea you are right, updated!
…krane is extracted to correct place
3 participants
Issue number:546
Closes #546
Description of changes:
We began by analyzing a resource leak in Twoliter where temporary directories containing the krane binary (
/tmp/.tmp*/krane) were accumulating and not being cleaned up. We discarded lazy_static! approach which brings up the problem, and instead we extract the binary to a desired place (in tools directory) with atomic overwrite when needed.
Testing done:
Manual tests done
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.