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

build: merge heaptrack dockerfile with production dockerfile #1682

Merged

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Apr 18, 2023

Both docker files can be consolidated into a single file by adding a debug stage and slightly modifying the Makefile.

Note
I am having local issues with my docker setup and I couldn't test them. I am pretty sure these changes should work. Feel free to test them and accept the PR.

@LNSD LNSD requested a review from Ivansete-status April 18, 2023 13:25
@LNSD LNSD self-assigned this Apr 18, 2023

# Add heaptrack
COPY --from=heaptrack-build /heaptrack/build/ /heaptrack/build/
COPY --from=heaptrack-build /usr/lib/ /usr/lib/
Copy link
Member

Choose a reason for hiding this comment

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

@Ivansete-status

This is weird, I would not blindly copy /usr/lib/ between images (especially different versions of images) as you might mess up some libraries.

I assume you are doing this because heaptrack actually drops some files in there? It would be good to know which files and only copy those rather than whole /usr/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the point. I will check it asap to have only the needed files copied.
Thanks @vpavlin !

Copy link
Member

Choose a reason for hiding this comment

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

Actually, since you are linking files from /heaptrack/build/lib/heaptrack/ using LD_LIBRARY_PATH, you might not need this at all?

Copy link
Member

Choose a reason for hiding this comment

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

Disregard, there are some libs missing when you don't include /usr/lib :)

@Ivansete-status
Copy link
Collaborator

Thanks for the PR @LNSD ! I like it :)

@vpavlin
Copy link
Member

vpavlin commented Apr 18, 2023

From #1681:

copy out the stats file: sudo docker cp 768e7de52d3c:/heaptrack.wakunode.1.gz . (replace 768.. with your docker container id.).

I would recommend using volume rather than copying the file from container - that way you make sure that the file will be persisted even if the container or machine reboots.

Something like

mkdir -p heaptrack/
docker ... --workdir /tmp/heaptrack -v $PWD/heaptrack:/tmp/heaptrack:z ...

We could also add WORKDIR /tmp/heaptrack into the Dockerfile, so that we can skip that option in docker run

Sadly it does not seem we can influence the name of the file heaptrack produces - so we cannot keep files from previous runs around easily.

The only way I was able to "simulate" this was

touch heaptrack/heaptrack.123.gz
docker run ... --workdir /tmp/heaptrack -v $PWD/heaptrack/heaptrack.123.gz:/tmp/heaptrack/heaptrack.wakunode.1.gz:z ...

But not sure how useful that is - whether it is worth the complications when running it:)

@vpavlin
Copy link
Member

vpavlin commented Apr 18, 2023

In general LGTM, I was able to run it and it produced the heaptrack file, but sadly,I was not able to open it - heaptrack gave me a qt5 related error and also the produced files were only 10bytes large, so probably no content - not sure if I did something wrong, or it just did not ran long enough?

@Ivansete-status
Copy link
Collaborator

In general LGTM, I was able to run it and it produced the heaptrack file, but sadly,I was not able to open it - heaptrack gave me a qt5 related error and also the produced files were only 10bytes large, so probably no content - not sure if I did something wrong, or it just did not ran long enough?

Yes, it didn't run long enough. heaptrack dumps mem stats regularly (I don't know how often)
Regarding the Qt issue I alse faced an issue with that due to incompatibility between qt versions. I don't remember the fix.

@LNSD
Copy link
Contributor Author

LNSD commented Apr 18, 2023

@vpavlin @Ivansete-status Please merge this PR into #1681 and continue the discussion there is a single PR open for the same changes :)

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Thanks !

@Ivansete-status Ivansete-status merged commit 104926d into issue-mem-leak-1662 Apr 18, 2023
@Ivansete-status Ivansete-status deleted the issue-mem-leak-1662-merge-dockerfiles branch April 18, 2023 15:04
Ivansete-status added a commit that referenced this pull request Apr 25, 2023
* Adding Dockerfile_with_heaptrack

* build: merge heaptrack dockerfile with production dockerfile (#1682)

* Avoid blindly copy /usr/lib/ and install only the needed libraries

* Adding heaptracker options in the Makefile

* Dockerfile simplification. (apk add libunwind)

* Dockerfile, Makefile: ++heaptrack params to nim build & 'heaptrack_support' in Nim compiler

* Dockerfile, Makefile: more convenient name for 'NIM_COMMIT' Docker arg

* Making 'NIM_COMMIT' more explicit

---------

Co-authored-by: Lorenzo Delgado <lorenzo@status.im>
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.

3 participants