Aiodocker Helper #341

rjt-gupta · 2019-07-17T16:57:26Z

Changes made:

Cmd exec emulator
Lfi emulator
Tests

coveralls · 2019-07-17T17:08:42Z

Pull Request Test Coverage Report for Build 1058

64 of 76 (84.21%) changed or added relevant lines in 4 files are covered.
8 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.3%) to 77.014%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tanner/session.py	1	2	50.0%
tanner/utils/aiodocker_helper.py	47	58	81.03%

Files with Coverage Reduction	New Missed Lines	%
tanner/utils/mysql_db_helper.py	8	92.0%

Totals
Change from base Build 1040:	0.3%
Covered Lines:	1310
Relevant Lines:	1701

💛 - Coveralls

afeena · 2019-07-18T14:12:55Z

tanner/utils/aiodocker_helper.py

+            self.logger.exception('Error while fetching %s container %s', container_name, server_error)
+        return container
+
+    async def create_container(self, container_name, cmd=None, image=None):


This function, for now, is used only in tests, why you need it?

Yes, because with aiodocker we can execute commands in both ways, by using client.containers.create and with docker_client.containers.run as well. But, I think its good to have this method if we need to create container at some point then it can help :)

afeena · 2019-07-18T14:15:58Z

tanner/emulators/cmd_exec.py


-    async def create_attacker_env(self, session):


why you don't associate docker with a session anymore?

For the above reason, I only followed one method to execute commands (containers.run) :) so haven't used a container like the previous implementation.

I am interested more in re-using containers. Creating a new container each time might be critical for small instances

You're right, got your point.

afeena · 2019-07-18T14:18:34Z

tanner/utils/aiodocker_helper.py

+                image = self.host_image
+
+            config = {"Cmd": cmd, "Image": image}
+            container = await self.docker_client.containers.run(config=config)


As far as I understood, you create all the time new containers with a random name, how you suppose to clean it?

For now these containers have to be deleted manually because of the risk other containers can also be deleted if we try to delete them all.
Do you have something else in mind?

Previously we had a procedure to delete containers in session routine, the idea is to have an associated container with session and delete it by name while deleting the session

Ahh! Yes, then in this case we should use create method to run commands in emulators right?

You can use create, get and start methods
or
You can still create the containers with run and delete it right after the execution.

I don't know what is better in terms of architecture and performance, you can test it :)

I think the second method works best. :)

The only problem I can imagine with a second approach is a (possible) inconsistency in the sequence of requests. And also, have you measured what is faster: always create and delete new one or get existing and start it? I would be happy to see such comparison

So, I analyzed the performance of both methods - (averaged multiple runs)

create_or_replace: gets the container and replaces new config or creates a new one + starting the container - 1.4206 sec (deleting time and logs output time has to be added)

run + delete: creates a new container and deleting [ had to force :( ] container + checking output using logs- 1.5324 sec

I think it's not a significant performance difference, but the method you will prefer would be final :)

afeena · 2019-07-18T14:19:53Z

tanner/utils/aiodocker_helper.py

+        except aiodocker.exceptions.DockerError as server_error:
+            self.logger.exception('Error while removing %s container %s', container_name, server_error)
+
+    async def close_docker(self):


used only in tests

Should I close the client directly in tests and remove this?

I think in test yes and on tanner shutdown either

afeena · 2019-07-18T14:23:56Z

tanner/emulators/lfi.py

-        execute_result = execute_result.replace('\x00', '\n')
-        return execute_result
+        cmd = ["sh", "-c", "cat {file}".format(file=shlex.quote(file_path))]
+        execute_result = await self.helper.execute_cmd(cmd)


since in lfi we are only read the files, it's better to create only 1 container (pass the same name) and on a shutdown of tanner remove this container

As such we don't really need to create a container before to execute some commands. I guess run method creates them itself.

Run method creates the containers, yes, but it will create each time a new container, which is not ok.

rjt-gupta added 2 commits July 17, 2019 22:25

Aiodocker helper

fce4620

Aiodocker tests

fec2e4a

afeena reviewed Jul 18, 2019

View reviewed changes

afeena requested changes Jul 18, 2019

View reviewed changes

rjt-gupta added 2 commits July 19, 2019 11:18

deleting used containers

0ee9859

close docker removed

be7dfc9

afeena merged commit d272dcd into mushorg:master Jul 24, 2019

rjt-gupta deleted the aiodocker branch July 24, 2019 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aiodocker Helper #341

Aiodocker Helper #341

rjt-gupta commented Jul 17, 2019

coveralls commented Jul 17, 2019 •

edited

Loading

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019 •

edited

Loading

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019

afeena Jul 18, 2019 •

edited

Loading

rjt-gupta Jul 18, 2019 •

edited

Loading

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019

afeena Jul 18, 2019

afeena Jul 18, 2019

rjt-gupta Jul 19, 2019 •

edited

Loading

afeena Jul 20, 2019 •

edited

Loading

rjt-gupta Jul 21, 2019

rjt-gupta Jul 23, 2019

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019

afeena Jul 20, 2019

rjt-gupta Jul 22, 2019

afeena Jul 18, 2019

rjt-gupta Jul 18, 2019 •

edited

Loading

afeena Jul 18, 2019 •

edited

Loading

Aiodocker Helper #341

Aiodocker Helper #341

Conversation

rjt-gupta commented Jul 17, 2019

coveralls commented Jul 17, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1058

💛 - Coveralls

Choose a reason for hiding this comment

rjt-gupta Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afeena Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

rjt-gupta Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjt-gupta Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

afeena Jul 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjt-gupta Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

afeena Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 17, 2019 •

edited

Loading

rjt-gupta Jul 18, 2019 •

edited

Loading

afeena Jul 18, 2019 •

edited

Loading

rjt-gupta Jul 18, 2019 •

edited

Loading

rjt-gupta Jul 19, 2019 •

edited

Loading

afeena Jul 20, 2019 •

edited

Loading

rjt-gupta Jul 18, 2019 •

edited

Loading

afeena Jul 18, 2019 •

edited

Loading