Add Dockerfile #106

Samyak2 · 2021-05-06T13:49:53Z

Description

~~It isn't working yet, do not merge pls~~
It works now!

Fixes #101

Added a Dockerfile based on the golang image
~~grofer is installed using go get (not from local source, I think?)~~ grofer is installed using the local current directory.
HOST_PROC, HOST_SYS, etc. is set for gopsutil
The whole root directory of the host has to be mounted as read only for this to work. A few other docker flags are also required.
See updated README for instructions

~~What does not work:~~

grofer just freezes on launch, while taking 100% CPU. It has to be stopped using docker stop (ctrl-c does not work). Only --help, about seem to work (proc freezes too). I tried setting HOST_SYS, etc. the issue still exists. I'm opening this draft PR to see if we can fix this.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Checklist:

I have read the contribution guidelines and followed it as far as possible.
I have performed a self-review of my own code (if applicable)
I have commented my code, particularly in hard-to-understand areas (if applicable)
I have run go fmt on my code (reference)
I have made corresponding changes to the documentation (if applicable)
My changes generate no new warnings
Any dependent and pending changes have been merged and published

Gituser143 · 2021-05-06T14:37:43Z

I just tried this out but used golang as image and my system just shut down xD

Edit: @Samyak2 any reason for picking buster?

Samyak2 · 2021-05-06T15:24:34Z

I just tried this out but used golang as image and my system just shut down xD

Oof, that's bad. Does buster work?

Edit: @Samyak2 any reason for picking buster?

No particular reason other than it being the latest one. I thought it would be good to lock it to a particular distro so that any dependencies (we don't have any yet) installed wouldn't break if the tag is updated.

Gituser143 · 2021-05-15T19:24:08Z

@Samyak2 Plis update the branch with main before continuing work as we had ignored a zero division error previously which caused quite a bit of trauma for me to catch :P

- instead of go-getting from GitHub, the binary is built from the source directory (assumed to be in the same directory as the Dockerfile) - set all `HOST_` required by gopsutil - alternatively, for faster development you can uncomment out the `# ADD grofer /go/bin/` to directly add the local `grofer` binary instead of building it inside the container everytime. If you see a GLIBC error on running the binary in the container, build it with `CGO_ENABLED=0 go build`

the `HOST_` env vars are set to `/host` and not `/rootfs`, obviously that should not have worked lol

Samyak2 · 2021-05-16T05:59:55Z

So I tried the latest version, some things works now. grofer works well - CPU, RAM and disks are correct. Network does not look correct, RX Total shows 5.7 B and the network graph does not show a spike when a speed test is running (grofer running on host does show this).

grofer proc shows only the container's processes i.e., only grofer is shown in the list. I confirmed that HOST_PROC was set in the container and also tried running it using HOST_PROC=/host/proc inside the container, the result is the same.

MadhavJivrajani · 2021-05-16T15:15:34Z

@Samyak2 I believe the grofer proc issue can be fixed by running the container with the --privileged flag like cAdvisor does it.
I haven't gotten around to investigating the network issue yet, will keep you posted.

MadhavJivrajani · 2021-05-16T18:04:26Z

Also, @Samyak2, is it possible to use golang:alpine instead of golang:1-buster?
Sizes:

Image	Size
`golang:alpine`	651mb
`golang:1-buster`	1.21gb

Gituser143 · 2021-05-16T18:28:04Z

Also, I'm a little worried about how we'll perform kill operations. The signal can't be sent from within the container to kill the host process right? Whereas this might not be an issue with the containers, which reminds me, we've got to mount the docker socket too!

MadhavJivrajani · 2021-05-16T18:38:34Z

Also, I'm a little worried about how we'll perform kill operations. The signal can't be sent from within the container to kill the host process right?

While doing docker run, you can pass a flag saying --pid=host and it should work fine.

we've got to mount the docker socket too!

Oh good point

Gituser143 · 2021-05-16T18:40:05Z

While doing docker run, you can pass a flag saying --pid=host and it should work fine.

This is so cool!

EDIT: I'm blown away, this works great xD

Samyak2 · 2021-05-17T17:18:25Z

@Samyak2 I believe the grofer proc issue can be fixed by running the container with the --privileged flag like cAdvisor does it.

Ooh yes, that fixes it.

Also, @Samyak2, is it possible to use golang:alpine instead of golang:1-buster?
Sizes:
Image Size
golang:alpine 651mb
golang:1-buster 1.21gb

@MadhavJivrajani The reason I did not choose alpine was because the dockerhub page mentions

This variant is highly experimental, and not officially supported by the Go project (see golang/go#19938 for details).

But if grofer works, it should provide a significant improvement.

Also, I'm a little worried about how we'll perform kill operations. The signal can't be sent from within the container to kill the host process right?

While doing docker run, you can pass a flag saying --pid=host and it should work fine.

This works well! Amazing

Samyak2 · 2021-05-17T17:25:27Z

Looks like alpine works, I'll switch to it.

MadhavJivrajani · 2021-05-18T05:51:03Z

Network does not look correct, RX Total shows 5.7 B and the network graph does not show a spike when a speed test is running (grofer running on host does show this).

@Samyak2 can you try this out again by passing --network=host flag while doing a docker run and see if that helps?

Samyak2 · 2021-05-18T06:19:51Z

@Samyak2 can you try this out again by passing --network=host flag while doing a docker run and see if that helps?

@MadhavJivrajani It works! The network graph is consistent between host and docker 🥳

I have a question though, what does RX and TX Total represent in the graph? It does not seem to change significantly over time, even when I run a speedtest that takes at least a few hundred megabytes (the graph does show a spike though).

this fixes wrong network stats issue

Gituser143 · 2021-05-18T07:05:17Z

RX is received and TX is transmitted, so they represent the amount of data sent and received through network.

Edit: I'm not 100% sure about this, but even with a speed test, at max a few mB gets used. It shouldn't really consume a lot of data.

Dockerfile

Samyak2 · 2021-05-18T11:17:35Z

RX is received and TX is transmitted, so they represent the amount of data sent and received through network.

Makes sense.

Edit: I'm not 100% sure about this, but even with a speed test, at max a few mB gets used. It shouldn't really consume a lot of data.

I think it consumes at least a few hundred Mbs. Considering a speed of 100Mbps and the test running for 10s, it would mean 100 * 2^20 * 10 bits or 125 MB consumed.

So I compared this with bpytop.

Before speed test, bpytop shows

Download Total: 12.5 GiB

and grofer shows

RX Total: 13.3 mB

After speed test, bpytop:

Download Total: 13.1 GiB

grofer:

RX Total: 13.9 mB

I think something is wrong here. I can open a new issue for this and move the discussion there.

Samyak2 · 2021-05-18T11:46:53Z

Also, I had another idea related to this PR. We could publish this image to the GitHub container registry and use actions to automatically publish on release.

I'll try making a workflow for this.

MadhavJivrajani · 2021-05-18T11:49:02Z

That's a great idea

MadhavJivrajani · 2021-05-23T14:33:11Z

@Samyak2 how's it going?

Samyak2 · 2021-05-24T13:35:46Z

Hi. I haven't been able to work on this over the past few days, I'll try to get the container workflow ready in a couple of days.

- publishes image to the GitHub docker registry (NOT GitHub container registry) - temporarily also includes containers for all PRs - this is only to test this in the PR - will be disabled in a later commit - testing with act fails due to the registry login step

Samyak2 · 2021-05-26T16:40:26Z

I was able to write a workflow for building and publishing Docker images to the GitHub Docker registry (NOT the container registry) using this starter workflow mentioned by @Gituser143 (thank you!).

Currently, it also publishes on PR. This was just to test the workflow. act failed at the registry login step, so I'm assuming act does not yet handle that. I'll remove that section once I confirm that the workflow works :)

Samyak2 · 2021-05-26T16:48:42Z

denied: Resource not accessible by integration

Wut. Does the docker registry need to be enabled somewhere before using it?

- moved to the GitHub container registry since it supersedes the GitHub docker registry - act still fails, need to try on actual actions

Samyak2 · 2021-05-26T17:22:50Z

In the latest commit, I changed the registry to the GitHub container registry since it's newer. This is based on RocketChat's workflow for the same.

I'm assuming this will fail too. According to this section in docker/login-action's documentation write access must be provided to this action. Do one of you @MadhavJivrajani @Gituser143 have access to this?

Edit: This time it failed because the GitHub container registry is not enabled for the organization yet.

denied: failed_precondition: Improved container support has not been enabled for 'pesos'. Learn more: https://docs.github.com/packages/getting-started-with-github-container-registry/enabling-improved-container-support

Do we go about enabling this or use the GitHub docker registry instead?

Gituser143 · 2021-05-26T17:43:04Z

Do we go about enabling this or use the GitHub docker registry instead?

Yea I feel like we should. Maybe bring it up on the slack channel once?

Samyak2 · 2021-05-29T06:46:29Z

New error now:

denied: installation not allowed to Create organization package

Perhaps we need to provide write access to this Action as described here.

Relevant thread in the GitHub community.

We could try doing this from an internal branch instead of a fork, as we won't need containers for every PR. I will direct this PR to a new, internal branch and we can test the Actions there. Is that ok?

MadhavJivrajani · 2021-05-29T06:55:08Z

We could try doing this from an internal branch instead of a fork, as we won't need containers for every PR. I will direct this PR to a new, internal branch and we can test the Actions there. Is that ok?

Sounds good 👍🏻
Thanks!

Samyak2 · 2021-06-05T12:15:28Z

I have changed the base branch to dockerfile. I'll merge this now and open a PR from that branch?

MadhavJivrajani · 2021-06-05T12:22:30Z

Thanks @Samyak2, just one thing before you proceed: you'll need to explicitly mount the docker socket for grofer container to work, that part of -v /var/run/docker.sock:/var/run/docker.sock:ro needs to be added in the README, if the command gets too long for the README, feel free to format it into multiple lines as well

Post that feel free to merge it and open a PR from the new branch 😄

Samyak2 · 2021-06-05T13:52:44Z

Ah right, I forgot about that. I'll add the docker socket and split it into multiple lines.

Gituser143 · 2021-06-05T15:14:08Z

Changes look good to me! We'll have to fix push soon though.

Samyak2 · 2021-06-05T16:57:09Z

Yes, I'll fix that from the internal dockerfile branch.

add Dockerfile with instructions in README

f472ea2

Samyak2 force-pushed the samyak-dockerfile branch from 585f370 to f472ea2 Compare May 16, 2021 05:14

Samyak2 added 2 commits May 16, 2021 11:23

dockerfile - update README to use /host instead of /rootfs

9323470

the `HOST_` env vars are set to `/host` and not `/rootfs`, obviously that should not have worked lol

Merge branch 'main' into samyak-dockerfile

f64a246

dockerfile - run with --privileged and --pid=host

fc61f94

dockerfile - switch to alpine from deb-buster

cf9d718

Merge branch 'main' into samyak-dockerfile

0763c2e

dockerfile - add --network=host flag

bcd59c3

this fixes wrong network stats issue

MadhavJivrajani reviewed May 18, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

dockerfile - update comments

0de6471

dockerfile - ghcr.io based workflow to publish img

952013c

- moved to the GitHub container registry since it supersedes the GitHub docker registry - act still fails, need to try on actual actions

Samyak2 changed the base branch from main to dockerfile May 29, 2021 08:22

Samyak2 marked this pull request as ready for review June 5, 2021 12:16

Samyak2 requested review from Gituser143 and metonymic-smokey as code owners June 5, 2021 12:16

dockerfile - add docker socket volume to instrs

4651658

Gituser143 approved these changes Jun 5, 2021

View reviewed changes

Samyak2 merged commit 94a3d2e into pesos:dockerfile Jun 5, 2021

Samyak2 mentioned this pull request Jun 5, 2021

Add Dockerfile pt. 2 #124

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Dockerfile #106

Add Dockerfile #106

Samyak2 commented May 6, 2021 •

edited

Loading

Gituser143 commented May 6, 2021 •

edited

Loading

Samyak2 commented May 6, 2021

Gituser143 commented May 15, 2021

Samyak2 commented May 16, 2021

MadhavJivrajani commented May 16, 2021 •

edited

Loading

MadhavJivrajani commented May 16, 2021

Gituser143 commented May 16, 2021

MadhavJivrajani commented May 16, 2021

Gituser143 commented May 16, 2021 •

edited

Loading

Samyak2 commented May 17, 2021

Samyak2 commented May 17, 2021

MadhavJivrajani commented May 18, 2021

Samyak2 commented May 18, 2021

Gituser143 commented May 18, 2021 •

edited

Loading

Samyak2 commented May 18, 2021

Samyak2 commented May 18, 2021

MadhavJivrajani commented May 18, 2021

MadhavJivrajani commented May 23, 2021

Samyak2 commented May 24, 2021

Samyak2 commented May 26, 2021

Samyak2 commented May 26, 2021

Samyak2 commented May 26, 2021 •

edited

Loading

Gituser143 commented May 26, 2021

Samyak2 commented May 29, 2021

MadhavJivrajani commented May 29, 2021

Samyak2 commented Jun 5, 2021

MadhavJivrajani commented Jun 5, 2021 •

edited

Loading

Samyak2 commented Jun 5, 2021

Gituser143 commented Jun 5, 2021

Samyak2 commented Jun 5, 2021

Add Dockerfile #106

Add Dockerfile #106

Conversation

Samyak2 commented May 6, 2021 • edited Loading

Description

Type of change

Checklist:

Gituser143 commented May 6, 2021 • edited Loading

Samyak2 commented May 6, 2021

Gituser143 commented May 15, 2021

Samyak2 commented May 16, 2021

MadhavJivrajani commented May 16, 2021 • edited Loading

MadhavJivrajani commented May 16, 2021

Gituser143 commented May 16, 2021

MadhavJivrajani commented May 16, 2021

Gituser143 commented May 16, 2021 • edited Loading

Samyak2 commented May 17, 2021

Samyak2 commented May 17, 2021

MadhavJivrajani commented May 18, 2021

Samyak2 commented May 18, 2021

Gituser143 commented May 18, 2021 • edited Loading

Samyak2 commented May 18, 2021

Samyak2 commented May 18, 2021

MadhavJivrajani commented May 18, 2021

MadhavJivrajani commented May 23, 2021

Samyak2 commented May 24, 2021

Samyak2 commented May 26, 2021

Samyak2 commented May 26, 2021

Samyak2 commented May 26, 2021 • edited Loading

Gituser143 commented May 26, 2021

Samyak2 commented May 29, 2021

MadhavJivrajani commented May 29, 2021

Samyak2 commented Jun 5, 2021

MadhavJivrajani commented Jun 5, 2021 • edited Loading

Samyak2 commented Jun 5, 2021

Gituser143 commented Jun 5, 2021

Samyak2 commented Jun 5, 2021

Samyak2 commented May 6, 2021 •

edited

Loading

Gituser143 commented May 6, 2021 •

edited

Loading

MadhavJivrajani commented May 16, 2021 •

edited

Loading

Gituser143 commented May 16, 2021 •

edited

Loading

Gituser143 commented May 18, 2021 •

edited

Loading

Samyak2 commented May 26, 2021 •

edited

Loading

MadhavJivrajani commented Jun 5, 2021 •

edited

Loading