Add endpoint for uploading docker image #232
Conversation
|
This PR is updated and ready for review. |
|
@CodeRabbit review |
|
At the very least, this operation should verify that it's using the localDocker backend, since it can only possibly work with that backend. |
WalkthroughWalkthroughThe recent updates involve enhancing a system with Docker capabilities, including building Docker images and handling larger input files. A new Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -87,7 +87,7 @@ class Config(object): | |||
| DOCKER_HOST_USER = "" | |||
|
|
|||
| # Maximum size for input files in bytes | |||
| MAX_INPUT_FILE_SIZE = 250 * 1024 * 1024 # 250MB | |||
| MAX_INPUT_FILE_SIZE = 10 * 1024 * 1024 * 1024 # 10GB | |||
There was a problem hiding this comment.
The increase in MAX_INPUT_FILE_SIZE to 10GB is a significant change that could impact system resources. Ensure that the system has been tested to handle such large uploads without running into issues like disk space or memory constraints. Additionally, consider the potential impact on dependents and whether the MAX_OUTPUT_FILE_SIZE should also be adjusted to accommodate larger outputs.
restful_tango/tangoREST.py
Outdated
| else: | ||
| self.log.info("Key not recognized: %s" % key) | ||
| return self.status.wrong_key | ||
|
|
||
| def build(self, key, tempfile): | ||
| self.log.debug("Received build request(%s)" % (key)) | ||
| if Config.VMMS_NAME != "localDocker": | ||
| self.log.error("Not using Docker backend, so cannot build image") | ||
| os.unlink(tempfile) | ||
| return "test" |
There was a problem hiding this comment.
The hardcoded return value "test" in the build method is likely a mistake and should be replaced with an appropriate status object or message.
- return "test"
+ return self.status.create(-1, "Docker backend not in use")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| else: | |
| self.log.info("Key not recognized: %s" % key) | |
| return self.status.wrong_key | |
| def build(self, key, tempfile): | |
| self.log.debug("Received build request(%s)" % (key)) | |
| if Config.VMMS_NAME != "localDocker": | |
| self.log.error("Not using Docker backend, so cannot build image") | |
| os.unlink(tempfile) | |
| return "test" | |
| else: | |
| self.log.info("Key not recognized: %s" % key) | |
| return self.status.wrong_key | |
| def build(self, key, tempfile): | |
| self.log.debug("Received build request(%s)" % (key)) | |
| if Config.VMMS_NAME != "localDocker": | |
| self.log.error("Not using Docker backend, so cannot build image") | |
| os.unlink(tempfile) | |
| return self.status.create(-1, "Docker backend not in use") |
| client = docker.from_env() | ||
| imageTarStr = open(tempfile, "rb").read() |
There was a problem hiding this comment.
Use a context manager to ensure the temporary file is properly closed.
- imageTarStr = open(tempfile, "rb").read()
+ with open(tempfile, "rb") as file:
+ imageTarStr = file.read()Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| client = docker.from_env() | |
| imageTarStr = open(tempfile, "rb").read() | |
| client = docker.from_env() | |
| with open(tempfile, "rb") as file: | |
| imageTarStr = file.read() |
restful_tango/tangoREST.py
Outdated
| if Config.VMMS_NAME != "localDocker": | ||
| self.log.error("Not using Docker backend, so cannot build image") | ||
| os.unlink(tempfile) | ||
| return "test" | ||
| if self.validateKey(key): |
There was a problem hiding this comment.
The key validation should occur before checking the VMMS_NAME to avoid unnecessary processing for invalid keys.
- if Config.VMMS_NAME != "localDocker":
- self.log.error("Not using Docker backend, so cannot build image")
- os.unlink(tempfile)
- return "test"
- if self.validateKey(key):
+ if self.validateKey(key):
+ if Config.VMMS_NAME != "localDocker":
+ self.log.error("Not using Docker backend, so cannot build image")
+ os.unlink(tempfile)
+ return self.status.create(-1, "Docker backend not in use")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if Config.VMMS_NAME != "localDocker": | |
| self.log.error("Not using Docker backend, so cannot build image") | |
| os.unlink(tempfile) | |
| return "test" | |
| if self.validateKey(key): | |
| if self.validateKey(key): | |
| if Config.VMMS_NAME != "localDocker": | |
| self.log.error("Not using Docker backend, so cannot build image") | |
| os.unlink(tempfile) | |
| return self.status.create(-1, "Docker backend not in use") |
| images = client.images.load(imageTarStr) | ||
| id_list = ", ".join(image.short_id for image in images) |
There was a problem hiding this comment.
Handle the case where no images are loaded from the tar file by checking the length of the images list and returning an appropriate status message if it is empty.
+ if not images:
+ self.log.error("No images were loaded from the tar file")
+ os.unlink(tempfile)
+ return self.status.create(-1, "No images loaded from file")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| images = client.images.load(imageTarStr) | |
| id_list = ", ".join(image.short_id for image in images) | |
| images = client.images.load(imageTarStr) | |
| if not images: | |
| self.log.error("No images were loaded from the tar file") | |
| os.unlink(tempfile) | |
| return self.status.create(-1, "No images loaded from file") | |
| id_list = ", ".join(image.short_id for image in images) |
| @tornado.web.stream_request_body | ||
| class BuildHandler(tornado.web.RequestHandler): | ||
| def prepare(self): | ||
| """set up the temporary file""" | ||
| tempdir = "dockerTmp" | ||
| if not os.path.exists(tempdir): | ||
| os.mkdir(tempdir, 0o700) | ||
| if os.path.exists(tempdir) and not os.path.isdir(tempdir): | ||
| tangoREST.log("Cannot process uploads, %s is not a directory" % (tempdir,)) | ||
| return self.send_error() | ||
| self.tempfile = NamedTemporaryFile(prefix="docker", dir=tempdir, delete=False) | ||
|
|
||
| def data_received(self, chunk): | ||
| self.tempfile.write(chunk) | ||
|
|
||
| def post(self, key): | ||
| """post - Handles the post request to build.""" | ||
| name = self.tempfile.name | ||
| self.tempfile.close() | ||
| self.write(tangoREST.build(key, name)) |
There was a problem hiding this comment.
The BuildHandler class correctly implements the streaming of request bodies to a temporary file. However, there are a few points to consider:
- The
preparemethod creates a temporary directorydockerTmpwithout checking for permissions or handling potential exceptions that could occur during directory creation. - The
postmethod does not perform any cleanup of the temporary file after the build process, which could lead to storage issues if not handled elsewhere. - There is no error handling for the
buildmethod call in thepostmethod, which could lead to unhandled exceptions if the build process fails.
| tempdir = "dockerTmp" | ||
| if not os.path.exists(tempdir): | ||
| os.mkdir(tempdir, 0o700) |
There was a problem hiding this comment.
The tempdir is hardcoded as dockerTmp. Consider making this a configurable value in the Config class to improve maintainability and flexibility.
| def data_received(self, chunk): | ||
| self.tempfile.write(chunk) |
There was a problem hiding this comment.
The data_received method does not update a hash or checksum for the received data. It might be beneficial to calculate a checksum to verify the integrity of the uploaded file.
| def post(self, key): | ||
| """post - Handles the post request to build.""" | ||
| name = self.tempfile.name | ||
| self.tempfile.close() | ||
| self.write(tangoREST.build(key, name)) |
There was a problem hiding this comment.
The key parameter in the post method is not sanitized or validated before being passed to the build method. Ensure that the key is properly validated to prevent potential security issues.
| """post - Handles the post request to build.""" | ||
| name = self.tempfile.name | ||
| self.tempfile.close() | ||
| self.write(tangoREST.build(key, name)) |
There was a problem hiding this comment.
The post method does not check the result of the tangoREST.build call before writing it to the response. Consider adding error handling to provide appropriate feedback to the client in case of a build failure.
Description
docker save.Testing
clients/tango-cli.pyto upload and build a new docker image.