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: Added pprof and variable in makefile #1091

Closed
wants to merge 14 commits into from

Conversation

MdSahil-oss
Copy link

@MdSahil-oss MdSahil-oss commented Feb 3, 2023

Signed-off-by: MdSahil-oss Mohdssahil1@gmail.com

Purpose of PR?:

Fixes #1059

Does this PR introduce a breaking change?
no
If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Bug fix. Fixes Publish a debug image with reach stable/latest release. #1059
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Commit has unit tests
  • Commit has integration tests

Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
@MdSahil-oss
Copy link
Author

@achrefbensaad I've added pprof support. can you please review it ? also Let me know how can I perform testing locally using minikube? confused about it.

@achrefbensaad
Copy link
Member

Hi @MdSahil-oss, Sure will do. Yes you can use minikube to test locally.

@MdSahil-oss
Copy link
Author

@achrefbensaad Thanks for letting me know but what's the procedure to test my changes using minikube ?

@achrefbensaad
Copy link
Member

you can use this method.
1- use build_kubearmor.sh to generate a docker image, make sure to set the REPO env variable with the entended image name or it will create an image with the name kubearmor/kubearmor.
2- push the image to a container registry
3- using karmor cli, run karmor install -i <your-image-name>.

Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
…unning

Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
@MdSahil-oss
Copy link
Author

@achrefbensaad I tested pprof It's working fine according to me in this PR. Please review the flags that I've added

@MdSahil-oss
Copy link
Author

And confused about how to generate pprof enabled images because Dockerfile is just compiling the code with make command. Should I create a new Dockerfile which will create pprof enabled image when executing build_kubearmor.sh command

@achrefbensaad
Copy link
Member

Hi @MdSahil-oss , there is no need to create a new dockerfile, you cna modify the ci to build and push two images, one with pprof enabled and one without. Also can you please implement the rest of API's mentioned on the issue.

@MdSahil-oss
Copy link
Author

@achrefbensaad APIs like:
pprof.Handler("heap") -> /debug/pprof/heap
pprof.Handler("block") -> /debug/pprof/block
pprof.Handler("goroutine") -> /debug/pprof/goroutine
pprof.Handler("threadcreate") ->/debug/pprof/threadcreate
By default accessible that's why I didn't implement these.

@achrefbensaad
Copy link
Member

Ah ok, Ill test the PR. In the meantime, can you please rebase on top of main to resolve the gosec error.

@MdSahil-oss
Copy link
Author

@achrefbensaad The flow of generating pprof enabled images I can see is:

  • First CI will trigger build_kubearmor.sh with a tag <state/version>-debug as an argument.
  • By checking tag as an argument conditionally build_kubearmor.sh will call dockerfiles to create images.
  • But if we want to create pprof enabled images then ENABLE_PPROF must be set to true by ENABLE_PPROF=true make command
    As far as I can see dockerfiles is the only file which runs make command so i don't see anyother way to create pprof enabled images without creating a new dockerfile. If you have anyother method to do so then let me know will be glad to hear it :)

@achrefbensaad
Copy link
Member

you can pass the value of ENABLE_PPROF to the dockerfile and then pass it down to the makefile using env vars.
This is a link that shows how this can be achieved https://stackoverflow.com/questions/30494050/how-do-i-pass-environment-variables-to-docker-containers

@MdSahil-oss
Copy link
Author

@achrefbensaad this PR needs to be reviewed now, Let me know if you find anything to be changed.

@MdSahil-oss
Copy link
Author

@achrefbensaad I've updated the PR.

@achrefbensaad
Copy link
Member

Lets start by fixing the points above, then we will fix the other parts

@MdSahil-oss
Copy link
Author

@achrefbensaad I think I have fixed all the points that you mentioned :)

@achrefbensaad
Copy link
Member

Hi @MdSahil-oss , yes you have addressed the points I raised earlier, but some new issues were introduced also, please read the reviews above my comment.
Also. The CI should not generate only debug images (PPROF_ENABLED=true), we need to keep the normal release also (PPROF_ENABLED=false).

@MdSahil-oss
Copy link
Author

@achrefbensaad I think I have resolved all the reviews you did above.

Copy link
Member

@Ankurk99 Ankurk99 left a comment

Choose a reason for hiding this comment

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

@MdSahil-oss Please address the unresolved commits.

Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <Mohdssahil1@gmail.com>
@MdSahil-oss
Copy link
Author

MdSahil-oss commented Feb 13, 2023

@achrefbensaad @Ankurk99 Now the value of ENABLE_PPROF is handed down from the CI to this script, to the docker file , to the make file as per your suggestion

@MdSahil-oss
Copy link
Author

MdSahil-oss commented Feb 13, 2023

And I kept ENABLE_PPROF=false for pprof disabled images.

@MdSahil-oss MdSahil-oss requested review from Ankurk99 and removed request for achrefbensaad February 13, 2023 18:01
@MdSahil-oss MdSahil-oss requested review from achrefbensaad and removed request for Ankurk99 February 25, 2023 08:38
@MdSahil-oss MdSahil-oss changed the title feat: Added pprof and variable in makefile (WIP) feat: Added pprof and variable in makefile Feb 26, 2023
Copy link
Member

@daemon1024 daemon1024 left a comment

Choose a reason for hiding this comment

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

@MdSahil-oss Sorry that we didn't follow up on this PR. The changes LGTM, can you please rebase your PR to resolve conflicts. Thank You.

@PrimalPimmy
Copy link
Member

Fixed in #1495 , closing this PR

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.

Publish a debug image with reach stable/latest release.
5 participants