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

Add docker image #235

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add docker image #235

wants to merge 9 commits into from

Conversation

plantroon
Copy link

Apparently there was a similar PR/suggestion in the past but it was withdrawn. I am reviving that effort.

I wrote a minimal distroless (there's only python) and rootless Dockerfile. I successfully use this in my more elaborate setup with rssbridge and chadburn scheduling. The docker-compose example in the README.rst is taken from my setup.

This PR contains a github automation for building a docker image for ghcr.io.

I will implement any necessary changes if requested.

@plantroon
Copy link
Author

This would close #218 for example

Can anyone approve this please? :) I am happy to work on any adjustments as requested.

Copy link
Contributor

@auouymous auouymous left a comment

Choose a reason for hiding this comment

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

@amiryal Do you know anything about docker?


on:
schedule:
- cron: '43 23 * * *'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this build an image every day, even if no changes have been made?

Copy link
Author

Choose a reason for hiding this comment

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

I will bring this in line with the other python workflow, to run on push. Yes the schedule was stupid and I forgot to remove it from here.

README.rst Outdated
entrypoint: "/venv/bin/python3 -c 'import threading; threading.Event().wait()'"
labels:
chadburn.enabled: "true"
chadburn.job-exec.rss2email.schedule: "@every 5m"
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be suggesting users run r2e more than once per day.

Copy link
Author

Choose a reason for hiding this comment

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

agreed, I will bring this in line with cron example

README.rst Outdated
@@ -53,6 +53,8 @@ A docker package exists for this project::
Then it can be used like::

$ docker run -v ./data:/data -v ./config:/config rss2email list

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace on blank line.

Copy link
Author

Choose a reason for hiding this comment

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

Will fix. Got a better setup now so that I won't miss these anymore

README.rst Outdated
@@ -65,9 +72,11 @@ docker-compose.yaml::yaml
chadburn.job-exec.rss2email.schedule: "@every 5m"
chadburn.job-exec.rss2email.command: "r2e run"

The example docker-compose snippet sets up chadburn (or ofelia) job scheduler to run r2e periodically. For the labels to be picked up, it is required that the container keeps running which is ensured with the entrypoint. Python's Event().wait() was used as there is no shell and this method should use less cpu than sleeping or looping (with sleep). You can of course abandon this and just call r2e from any other job scheduler as this entrypoint only circumvents chadburn/ofelia limitations.
The example docker-compose snippet sets up chadburn (or ofelia) job scheduler to run r2e periodically.
Copy link
Contributor

Choose a reason for hiding this comment

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

End of line whitespace.

Copy link
Member

Choose a reason for hiding this comment

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

@plantroon
The guide should either walk the user through the entire setup (e.g., how does chadburn get installed and invoked?) or leave out invocation details for users to figure out for themselves.

Copy link
Author

@plantroon plantroon Jun 9, 2023

Choose a reason for hiding this comment

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

@auouymous Sorry, will fix

Copy link
Author

Choose a reason for hiding this comment

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

@amiryal it's a few more lines, I suggest adding it but want the feedback if it's a good idea or we should forget the whole job scheduler stuff.

services:
  chadburn:
    image: premoweb/chadburn:latest
    command: daemon
    restart: unless-stopped
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock:ro

README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
# github.repository as <account>/<repo>
IMAGE_NAME: ${{ github.repository }}


Copy link
Contributor

Choose a reason for hiding this comment

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

There are multiple instances of double line spacing in this file.

Copy link
Author

Choose a reason for hiding this comment

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

I'll bring this more in line with the other workflow too, the comments are also possibly useless and come from some other example. The actions are descriptive enough by themselves.

@plantroon
Copy link
Author

I'll squash commits when I fix this so that it doesn't mess up history too much

README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Outdated
@@ -65,9 +72,11 @@ docker-compose.yaml::yaml
chadburn.job-exec.rss2email.schedule: "@every 5m"
chadburn.job-exec.rss2email.command: "r2e run"

The example docker-compose snippet sets up chadburn (or ofelia) job scheduler to run r2e periodically. For the labels to be picked up, it is required that the container keeps running which is ensured with the entrypoint. Python's Event().wait() was used as there is no shell and this method should use less cpu than sleeping or looping (with sleep). You can of course abandon this and just call r2e from any other job scheduler as this entrypoint only circumvents chadburn/ofelia limitations.
The example docker-compose snippet sets up chadburn (or ofelia) job scheduler to run r2e periodically.
Copy link
Member

Choose a reason for hiding this comment

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

@plantroon
The guide should either walk the user through the entire setup (e.g., how does chadburn get installed and invoked?) or leave out invocation details for users to figure out for themselves.

README.rst Outdated

The example docker-compose snippet sets up chadburn (or ofelia) job scheduler to run r2e periodically.

Some background for entrypoint override: For the labels to be picked up, it is required that the container keeps running which is ensured with the entrypoint. Python's Event().wait() was used as there is no shell and this method should use less cpu than sleeping or looping (with sleep). You can of course abandon this and just call r2e from any other job scheduler as this entrypoint only circumvents chadburn/ofelia limitations.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like too much detail for something that is outside of r2e. In future iterations of chadburn this workaround will be made redundant, but it will remain here until someone cares enough to update. Also, it seems a little excessive for something to be kept running indefinitely just to wake up a couple of times per day. The CPU usage while running may be close to 0%, but RAM is also a resource, and some cloud platforms might charge you for every second of run time.

Copy link
Author

Choose a reason for hiding this comment

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

The CPU usage here should be lower than any sleep command available.

I don't think it's possible for Chadburn or Ofelia to do exec on a stopped service, though other job schedulers operating outside of Docker might be able to.

I originally had this set up to run via docker run in cron, but opted to go for a cleaner docker-centric setup where I have everything in one place.

Copy link
Author

Choose a reason for hiding this comment

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

@amiryal In future iterations of chadburn this workaround will be made redundant - is this related to the job-run functionality? I was not aware of that coming :)

README.rst Outdated
volumes:
- ./config:/config
- ./data:/data
entrypoint: "/venv/bin/python3 -c 'import threading; threading.Event().wait()'"
Copy link
Member

Choose a reason for hiding this comment

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

@plantroon
A later paragraph mentions there is no shell in the container, but this looks like a shell command line to me. Educate me here — does docker-compose parse this line and call exec with the parsed parameters?

@auouymous
To answer your opening question, no, I do not have much hands-on experience with Docker.

Copy link
Author

@plantroon plantroon Jun 9, 2023

Choose a reason for hiding this comment

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

@amiryal yes, this is executed directly by Docker (no need for compose, specifying docker run --entrypoint=... would yield the same result) even without a shell available in the container.

The rationale behind this is for it to keep running as a service so that a job scheduler can pick it up and exec r2e run, which to my understanding runs the same way as when you would call docker exec.

Copy link
Author

Choose a reason for hiding this comment

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

@amiryal I understand the confusion now. I specified it in the shell command form, therefore it runs through a shell - and even distroless images have sh binary in them by default, but I thought they didn't.

I will change this to use the exec command form to make it cleaner.

@plantroon plantroon requested a review from amiryal June 9, 2023 18:04
Copy link
Author

@plantroon plantroon left a comment

Choose a reason for hiding this comment

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

Fixed all your concerns. I additionally requested @amiryal to take another look at the chadburn part.

@plantroon plantroon requested a review from auouymous June 9, 2023 19:12
@Centro1993
Copy link

Great Stuff, would love to see this added!

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

4 participants