mweinelt / kea-exporter

Export Kea Metrics in the Prometheus Exposition Format
MIT License
34 stars 17 forks source link

feat: add docker build #40

Closed M0NsTeRRR closed 8 months ago

M0NsTeRRR commented 9 months ago

I've added a Dockerfile to build the exporter. I've also added a github action that is executed when you push a new tag. The image is pushed to latest and tag on github registry. You can find an exemple on my fork for the result.

Qwiko commented 9 months ago

I like that you have added this, I was looking at doing this myself but put it off, looks good.

Some thoughts.

We can use environment variables with default MODE=http to configure the exporter. It does not make sense to use the socket mode in a Docker container. I can see this being used in a Kubernetes workflow as a sidecar container in a pod that points to localhost of another kea-dhcp container.

I am a bit rusty on how env-vars interact with ENTRYPOINT in a Dockerfile or if you need to have an entrypoint.sh script that runs in a shell to let it know all of the env-vars.

M0NsTeRRR commented 9 months ago

Hello,

Thank you for your feedback. My initial intention was to allow the user to choose between socket and http mode since there are no compatibility issues, but I can change this.

I will also use this in a Kubernetes environment but not as a sidecar, as I'm operating within a hybrid setup with virtual machines.

I'm not seeing a scenario where you would need to alter the entrypoint in this configuration. However, if you do find a need for it, I can add a default environment variable and incorporate it into the entrypoint, like so:

ENV PORT=9547
ENTRYPOINT ["kea-exporter", "-p", "$PORT"]

The -a 0.0.0.0 option can be overridden, as the Prometheus start_http_server function doesn't support dual-stack binding with ::, and in a Kubernetes environment where only IPv6 is used, you would need to override this parameter.

Regards,

Qwiko commented 9 months ago

Thats true, you could have a shared volume and run socket mode between containers. We should leave every option open for the user to choose.👍

Qwiko commented 9 months ago

What do you think about this layout @M0NsTeRRR? You can still add optional command line arguments or use env-vars at runtime. As click.option specifies the default port in cli.py so we do not actually need to specify it with the entrypoint.

FROM python:3.12-slim

WORKDIR /usr/src/app

COPY . .

RUN pip install --no-cache-dir -e .

EXPOSE 9547/tcp

ENTRYPOINT ["kea-exporter"]
M0NsTeRRR commented 9 months ago

I've added suggested change, i've removed also the /tcp as it's the default behavior for EXPOSE

Qwiko commented 9 months ago

Looks good to me! @mweinelt will need to have a look as well.

Edit @M0NsTeRRR you should add yourself as the maintainer of the image. Example from: https://docs.docker.com/engine/reference/builder/#maintainer-deprecated LABEL org.opencontainers.image.authors="SvenDowideit@home.org.au"

M0NsTeRRR commented 9 months ago

Sure, we have some more labels to add if we want to follow opencontainers specification. I also need to look about USER property as this container doesn't need root permission to run.

M0NsTeRRR commented 9 months ago

I've added default labels and the image is run with a non root user. Regarding org.opencontainers.image.authors label, I think @mweinelt should put their name here as I'm only a contributor :)

mweinelt commented 9 months ago

Thanks for working on this. Can you also update the README.md (maybe just above "Features") with information relating to the resulting image? And can we make sure the resulting image will also be built for arm64?

mweinelt commented 9 months ago

I think @mweinelt should put their name here

Feel free to add Martin Weinelt <hexa@darmstadt.ccc.de> into the metadata as needed.

M0NsTeRRR commented 9 months ago

Thanks for working on this. Can you also update the README.md (maybe just above "Features") with information relating to the resulting image?

Sure :)

And can we make sure the resulting image will also be built for arm64?

Good catch, I completely forgot ARM users. I've updated the github action.

M0NsTeRRR commented 9 months ago

We should be good. I let you review it @mweinelt

mweinelt commented 9 months ago

Needs a rebase.

M0NsTeRRR commented 9 months ago

Should be good

M0NsTeRRR commented 8 months ago

Hello, Still waiting feedback regarding the 3 conversations, so we can merge it and tag 0.6.0 :). Regards,

fleaz commented 8 months ago

Sorry for the late reply. None of the things are real dealbreakers to me, and were just little things that popped up in my head. Carry on :)

M0NsTeRRR commented 8 months ago

Hello,

No worries, thanks for your review. I was actually also waiting for feedback from @mweinelt, regarding the ongoing discussion and what he would like to do. 😁

mweinelt commented 8 months ago

I released v0.6.0 earlier today and the CI run seems to have completed successfully.

https://github.com/mweinelt/kea-exporter/actions/runs/8347134319

M0NsTeRRR commented 8 months ago

@mweinelt package is not publicy available.

docker pull ghcr.io/mweinelt/kea-exporter
Using default tag: latest
Error response from daemon: Head "https://ghcr.io/v2/mweinelt/kea-exporter/manifests/latest": unauthorized

I think you have disabled Inherit access from source repository in https://github.com/settings/packages I think you can configure it here for this specific repository if you want to keep this global setting https://github.com/mweinelt/kea-exporter/pkgs/container/kea-exporter

mweinelt commented 8 months ago

For some reason the package is private. I have inherited access from source repo enabled.

image

M0NsTeRRR commented 8 months ago

If you have changed the global settings, it's apply only to new repository I think : This setting will be applied to new Container, npm, rubygems and NuGet packages. I still have a 401

mweinelt commented 8 months ago

Wow, did not expect visibilty to be a danger zone setting.

image

M0NsTeRRR commented 8 months ago

Oh okay, lot of menu and without a simple explanation on github documentation ^^. It works now ! :)