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

feat(mem-analysis): Adding Dockerfile_with_heaptrack #1681

Merged
merged 8 commits into from
Apr 25, 2023

Conversation

Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented Apr 18, 2023

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 underneath heaptrack.

Once the container is running, it will generate periodic mem-allocation stats in the container's / folder with the name heaptrack.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 throughout heaptrack, 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 passing NIM_COMMIT=heaptrack_support to the make wakunode2 call.

How to use it

  1. Go to the nwaku root folder.
  2. Create a docker image with a command like the next:
    sudo make docker-image DOCKER_IMAGE_NAME=docker_repo:docker_tag HEAPTRACKER=1
  3. Run the docker container as usual, giving the needed wakunode2 params.
  4. Wait for some minutes until the mem-stats file gets populated.
  5. copy out the stats file: sudo docker cp 768e7de52d3c:/heaptrack.wakunode.1.gz . (replace 768.. with your docker container id.).
  6. Analyse the file with: bin/heaptrack heaptrack.wakunode.1.gz (Heaptrack should be built locally beforehand.).

Additional hints

#1682 (comment)

.dockerignore Outdated
@@ -1,5 +1,6 @@
/README.md
/Dockerfile
/Dockerfile_with_heaptrack
Copy link
Contributor

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 :)

@Ivansete-status
Copy link
Collaborator Author

I've applied @vpavlin's recommendation and now we only install libunwind (apk add libunwind). Thanks!

Copy link
Member

@vpavlin vpavlin left a 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
Copy link
Member

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

Suggested change
RUN apk add --no-cache gdb
RUN apk add --no-cache gdb libunwind

Copy link
Collaborator Author

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RUN apk add libunwind

Copy link
Contributor

@jm-clius jm-clius 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 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?

@vpavlin
Copy link
Member

vpavlin commented Apr 19, 2023

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 -d:heaptrack to the wakunode build based on @Ivansete-status comments

@jm-clius
Copy link
Contributor

If the Dockerfiles are essentially copies of each other, then that feels like a bad practice

Good point and thanks. I think proper use of ARGs here in the same Dockerfile is then the way to go.

@Ivansete-status Ivansete-status self-assigned this Apr 19, 2023
@Ivansete-status
Copy link
Collaborator Author

Actually I was preparing the next commit by adding a combination of new 'heaptrack' args to the Dockerfile plus Makefile.
I'll commit once I confirm it works.

Dockerfile Outdated

FROM alpine:edge AS nim-build

ARG NIMFLAGS
ARG MAKE_TARGET=wakunode2
ARG EXPERIMENTAL=false
ARG NIM_COMMIT_ARG
Copy link
Member

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:)

Copy link
Collaborator Author

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
Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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 !

@vpavlin vpavlin self-requested a review April 25, 2023 15:07
Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

LGTM!

@Ivansete-status Ivansete-status merged commit 9b9172a into master Apr 25, 2023
12 checks passed
@Ivansete-status Ivansete-status deleted the issue-mem-leak-1662 branch April 25, 2023 15:54
@vpavlin vpavlin mentioned this pull request May 16, 2023
1 task
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