Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Building docker image using docker api #43

Closed
wants to merge 4 commits into from

Conversation

gurneesh
Copy link

@gurneesh gurneesh commented Aug 20, 2021

Changing the image build process to use docker API.
Changes Made to

  1. harvey/images.py
    Changing variables to the values only rather than making them command-line arguments that are run through subprocess.
    Under if condition for context == 'test'
    i. We are changing the language variable to the value of the language key provided in config dictionary, same procedure is
    followed for variables version and project.
    ii. We are changing tag_arg variable to get the tag for the image from the webhook.
    iii. Initiating docker client, for now I have only allowed to use docker on host machine.
    (I am not sure but I think harvey has no support to connect to docker on other machines to build images or run containers)
    iv. Lastly, we are creating the image using image.build method. Now we don't have to cd into the directory as we have
    provided the path for dockerfile as parameter to the method and then we are returning the tag of newly created image.

  2. test/unit/test_images.py

    1. We also have to change the test_build_image method as now no subprocess is being run. So we are mocking the build
      object (https://stackoverflow.com/questions/64089226/how-to-mock-call-to-python-docker-sdk-using-mockito)

I'll change the double quotes to single quotes.
Also, clean up the docstrings and comments and I'll also add docker to setup.py.

I only ran tests for test_images.py, I'll make sure to run and pass other tests as well.

These are just suggestions feel free to reject if these changes don't seem to go with the project.

@gurneesh
Copy link
Author

Building image using docker api.
Made changes to

  1. harvey/images.py
  2. test/unit/test_images.py

Copy link
Owner

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I've been wanting to start using the docker package vs the homegrown solutions here, this is probably a good first step.

A few things:

  1. Let's keep the same format of quotes throughout the doc (single quotes, please refer to the contributing guidelines
  2. Let's cleanup the comments and docstrings to better represent these changes (other comments left throughout)
  3. Can you please update the PR summary about what is changing and why?
  4. You'll also need to ensure the build passes (line and tests)
  5. You'll need to pin docker in the setup.py file under the REQUIREMENTS constant to ensure that we include this package in the build.

I need to do a bit of research about the docker package before I'll be confident with these changes, let's make the changes requested here first and I'll look into it over the coming days.

@mock.patch('subprocess.check_output')
def test_build_image(mock_subprocess, context, mock_webhook):
@mock.patch.object(ImageCollection, 'build')
def test_build_image(mock_build, context, mock_webhook):
# TODO: Mock the subprocess better to ensure it does what it's supposed to
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's kill this comment since we aren't using the subprocess anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I'll make this change

harvey/images.py Outdated

from harvey.globals import Global


class Image:
@staticmethod
def build_image(config, webhook, context=''):
def build_image(config, webhook, context=""):
"""Build a Docker image by shelling out and running Docker commands."""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the docstring since we aren't shelling out anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

harvey/images.py Outdated
Comment on lines 14 to 17
# tar = open('./docker/pullbug.tar.gz', encoding="latin-1").read()
# json = open('./harvey/build.json', 'rb').read()
# data = requests.post(Global.BASE_URL + 'build', \
# params=json, data=tar, headers=Global.TAR_HEADERS)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove these commented out lines since they were an attempt at a previous implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will make these changes in upcoming commits.

@gurneesh
Copy link
Author

Hi,
I didn't realize this earlier but stages need to timeout after a certain time. Which is quite easy to implement in case of subprocesses.
But when building image using the docker api, it is going to be impossible as there is no timeout feature provided by docker package which can stop the build process if timeout accurs. Only http timeout is possible which might stop the build process (Not useful to us).

I am thinking of using multiprocessing module, which could start a docker image build in a new process and we could timeout.
Not sure if this will sort things.

@Justintime50
Copy link
Owner

I've tried the multiprocessing module previously in this project and determined I didn't want to use it due to the expensive overhead. There should be ways we could accomplish something similar via the threading module that is already in use.

We'll definitely want a timeout function so we don't run builds for eternity.

@gurneesh
Copy link
Author

Hi,
After replying yesterday regarding the timeout issue, I tried things using the multiprocessing module.
Please look into this once.
Meanwhile, I am going to check the threading module and see if we can accomplish similar results.

Thanks

@Justintime50
Copy link
Owner

After a bit of research, it seems threads cannot be killed in Python (after some timeout for example). I'm hesitant to adopt the multiprocessing package in this project due to the overhead it brings; however, it could allow us to use the Docker package while still having a timeout but would be more CPU intensive than I think I want at this time. Whatever the solution, it'll require some planning. I've opened #49 to track the progress of this work.

@gurneesh
Copy link
Author

Hi,
I understand your concern with the multiprocessing module and threads cannot be killed so implementing the timeout feature would be an issue but it is essential as we cannot let pipelines run for an infinite amount of time.

And, other options like concurrent.futures or gevent are totally out of scope for our use case.

I'll look for other solutions where we can allow building image and then timeout when it exceeds the given limit.

@Justintime50
Copy link
Owner

Just so I have some context, are you currently using Harvey? If so, what pieces are you using it for? Meaning - are you using docker-compose workflows or are you using it to run CI tests, etc?

@Justintime50
Copy link
Owner

I've been thinking for awhile now that the scope of the project blew up larger than initially anticipated. The reason I ask how you are using Harvey at the moment is because I think I'm going to move forward with #50 which will actually remove this logic completely and return Harvey to a docker compose only deployment platform which is what I initially built it out as.

I fear the rest of the code outside of docker-compose workflows isn't well supported and as such I haven't given it the time it deserves to be really useful - plus there are other deployment and testing platforms out there such as Rancher that will do a better job at bigger tasks like that. I wanted to keep Harvey lightweight and focused on simple compose deployments. I'll stew on this for a couple more days before doing anything drastic. Would love your thoughts on any of this since you've been working on this PR (many thanks for digging in and taking a look!)

@gurneesh
Copy link
Author

gurneesh commented Sep 2, 2021

Hi,
I am using harvey for running CI tests only on some personal projects nothing big or production ready even. I came across this repository few days ago and thought that it would be great to have an easy to use CI/CD such as this one, so I decided to take a look at source and suggested some changes. I would be great to use harvey for compose deployments.

You are right that other tools like Jenkins and Rancher can be used.

Feel free to narrow the scope of project to compose only.

Thanks

@Justintime50
Copy link
Owner

@gurneesh thank you very much for your thoughts here and the work produces for this PR. I think for now I'm going to cut back on the scope of Harvey, polish it up really nicely, and then grow the scope from there potentially back to including other aspects like testing and non-compose deployments.

The beauty of open source is you have the fork and can continue making improvements to the testing framework in parallel to the improvements I'll be making on the compose deployment side of things. The next release of Harvey will not include its testing framework as it's being removed. I'd suggest either using your own local copy to continue running tests via Harvey or switching to something well supported like GitHub Actions. You can even have GitHub Actions run your tests for you and then deploy it to your server with Harvey.

Once again, I appreciate your work here and apologize I'm unable to include it at this time. If you'd like, feel free to watch or star this project to keep an eye on its future development and there may come a point where I grow the project scope out again. Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants