-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat(mem-analysis): Adding Dockerfile_with_heaptrack #1681
Conversation
.dockerignore
Outdated
@@ -1,5 +1,6 @@ | |||
/README.md | |||
/Dockerfile | |||
/Dockerfile_with_heaptrack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be removed now :)
I've applied @vpavlin's recommendation and now we only install libunwind ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested a change, but otherwise LGTM
Dockerfile
Outdated
# Debug image | ||
FROM prod AS debug | ||
|
||
RUN apk add --no-cache gdb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to only call apk once
RUN apk add --no-cache gdb | |
RUN apk add --no-cache gdb libunwind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll do that thanks !
Dockerfile
Outdated
|
||
# Add heaptrack | ||
COPY --from=heaptrack-build /heaptrack/build/ /heaptrack/build/ | ||
RUN apk add libunwind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUN apk add libunwind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! No strong opinion, but since we'll still have to add -d:heaptrack
or -d:heaptrack -d:heaptrack_inject
to the [make
command] I think we at least need another flag/ARG
for HEAPTRACK
or DEBUG
. Since this could pollute the Dockerfile, wouldn't an alternative be to create a new Dockerfile specifically for these debug builds under say /tools/nwaku-heaptrack
? cc @vpavlin here for advice - not sure what standard/best practice is here re number of Dockerfiles: opting for separate files for different use cases or a single Dockerfile, but with flags/ARGs for the different build scenarios?
If the Dockerfiles are essentially copies of each other, then that feels like a bad practice because we may forget to update one or another and then we potentially get different images, hence different behaviour of waku node. For such case it would make sense to "polute" the Dockerfile with more ARGs and instruct Makefile. If one Dockerfile is just an exetension of another (i.e. building on top of another image build) or is separate then it makes sense to have separate Dockerfiles to keep the main one as clean as possible. I was not aware we need to add |
Good point and thanks. I think proper use of ARGs here in the same Dockerfile is then the way to go. |
Actually I was preparing the next commit by adding a combination of new 'heaptrack' args to the Dockerfile plus Makefile. |
…pport' in Nim compiler
Dockerfile
Outdated
|
||
FROM alpine:edge AS nim-build | ||
|
||
ARG NIMFLAGS | ||
ARG MAKE_TARGET=wakunode2 | ||
ARG EXPERIMENTAL=false | ||
ARG NIM_COMMIT_ARG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the ARG shoul have the suffix ARG:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, I change it now. thanks :)
Makefile
Outdated
HEAPTRACKER_INJECT ?= 0 | ||
ifeq ($(HEAPTRACKER), 1) | ||
# Needed to make nimbus-build-system use the Nim's 'heaptrack_support' branch | ||
NIM_COMMIT := NIM_COMMIT=heaptrack_support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this result in NIM_COMMIT
having a value NIM_COMMIT=heaptrack_support
- i.e. trying to checkout branch NIM_COMMIT=heaptrack_support
, rather than heaptrack_support
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah that may be happening. For that reason I used the NIM_COMMIT_ARG previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've applied your suggestions and I think it now looks much better. Thanks @vpavlin !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue
#1662
Description
Adding a Dockerfile that allows to generate a Docker image with both waku and heaptrack. The generated docker containers will run a
wakunode2
instance underneathheaptrack
.Once the container is running, it will generate periodic mem-allocation stats in the container's
/
folder with the nameheaptrack.wakunode.1.gz
. If the container is restarted, then the previous allocation file is lost so it is important to copy out the file before the container restarts or crashes.Heaptrack is the tool used and it has two operational modes. In this case, the 'attach' mode makes the underlying program to crash (SYGSEGV). I tried this with a simple C program and it happens the same so it seems to be a Docker-related issue. On the other hand, when running
wakunode
throughoutheaptrack
, the stats are properly written periodically.This docker file also enforces
nimbus-build-system
to use the nim compiler that points at the heaptrack_support branch. This is achieved by passingNIM_COMMIT=heaptrack_support
to themake wakunode2
call.How to use it
sudo make docker-image DOCKER_IMAGE_NAME=docker_repo:docker_tag HEAPTRACKER=1
sudo docker cp 768e7de52d3c:/heaptrack.wakunode.1.gz .
(replace 768.. with your docker container id.).bin/heaptrack heaptrack.wakunode.1.gz
(Heaptrack should be built locally beforehand.).Additional hints
#1682 (comment)