Bugfix-Docker is consuming too much resource (PR for Devlop Branch) #2828
Conversation
WalkthroughThis pull request focuses on optimizing Docker configurations and resource utilization for the Talawa API project. The changes span multiple configuration files including Dockerfiles, docker-compose files, and package.json. The primary objectives include improving resource efficiency, updating Node.js versions, and refining the deployment setup across development and production environments. The modifications aim to reduce resource consumption and streamline the containerization process. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
compose.yaml (1)
114-114: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML best practices.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 114-114: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(1 hunks)Dockerfile.dev(0 hunks)Dockerfile.prod(0 hunks)compose.yaml(7 hunks)docker-compose.prod.yaml(0 hunks)
💤 Files with no reviewable changes (3)
- Dockerfile.dev
- Dockerfile.prod
- docker-compose.prod.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
compose.yaml
[error] 114-114: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
Dockerfile (2)
33-37: LGTM! Production configuration looks goodThe port exposure and production command configuration align well with the PR's resource optimization objectives.
31-31: Review security implications of copying certs directoryCopying the
certsdirectory into the Docker image might expose sensitive data. Consider:
- Mounting certificates as secrets or volumes
- Ensuring no sensitive keys are baked into the image
Let's check if the certs directory contains sensitive files:
✅ Verification successful
From the search results, I can see that:
- The certs directory exists and is used for SSL certificates
- The certificates are generated during development using the
generate:ssl-private-keynpm script- The certificates are used in
src/index.tsfor HTTPS server setup- The
.gitignorefile listscert.pem,key.pem, andcsr.pemLet me verify the actual certificate files:
Certificate handling is properly secured
The review concern about sensitive certificate data is unwarranted because:
- The certs directory only contains
.gitkeepand no sensitive files are committed- Certificate files (
cert.pem,key.pem,csr.pem) are properly excluded via.gitignore- Development certificates are generated locally via npm script
- The Dockerfile copy is safe as it only copies certificates from the build context, which won't contain sensitive files due to
.gitignore🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for common sensitive file patterns in certs directory fd . certs -t f -x file {} | grep -i "key\|private\|secret"Length of output: 63
Script:
#!/bin/bash # Let's first check if the certs directory exists and list its contents fd . -t d -d 1 | grep -i "certs" fd . certs -t f 2>/dev/null || echo "Directory not found or empty" # Let's also search for any references to certs in the codebase rg -i "certs?" -A 2 -B 2Length of output: 78083
Script:
#!/bin/bash # List all files in certs directory fd . certs -t f -x ls -la {} # Check if any sensitive files are committed git ls-files certs/Length of output: 69
compose.yaml (1)
Line range hint
60-84: LGTM! Well-configured API serviceThe service configuration shows good practices:
- Appropriate resource limits
- Proper service dependencies with health checks
- Optimized volume mounting
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2828 +/- ##
===========================================
- Coverage 98.17% 97.68% -0.50%
===========================================
Files 184 364 +180
Lines 10767 18563 +7796
Branches 835 2677 +1842
===========================================
+ Hits 10571 18133 +7562
- Misses 186 425 +239
+ Partials 10 5 -5 ☔ View full report in Codecov by Sentry. |
palisadoes
added
the
ignore-sensitive-files-pr
|
|
@palisadoes i am creating two script two dynamically make difference between development dependcy and production dependecy so where i have to place it . will i create a separate folder for this . Some packages are mention in dev but also used in production so in package.json we have move it as production dependicies so then remove the development dependcy. Will i create a separate issue for this. |
|
see these screenshots of production dependecy that are required but listed in dev dependency . So when we run the command npm prune --production then it removes as this command removes devDependency from nodemodules . As a result production enviroment didnot start.Now by the script we can dynamically detect the difference and installed this dependecy as our prodution dependcy so after prune it will not deleted and we can start the server with out any issue. npm prune is neccessary as it optimizes our packages by removing devDependecy by removing unWanted nodeModules that are not required to start the production server.It will be useful to reduce the image size around 200 mb now our talwa-api container size is 516mb so after this it will decrease to around 300-350mb which is requird for this pr.
|
|
@palisadoes No it is not related to dev part . For production we have to run npm prune --production this command will delete the nodemodules which are not required for our production to run the app. These nodemodules are only required for our development process . But in some modules listed in development dependcies are used in production enviroment for example cls-bluebird this is listed under dev dependencies . So when we use this command prune it removes this type of package which is required for our production. So for our use case we have to find this type of packages which is required for our production workflow to work . Once we detect these we can add it to our prodution dependecy. So when we use prune command as this command will delete the dev depnedcies so we will not face issue like nodemoules is not found . Some more packages like which is not required for our production examples are "@types/cors": "^2.8.17", |
|
@palisadoes yes i have open feature for your second question also .So i am waiting for your response what i have to do for this after npm prune run successfully i will update the docker file and i make commit again to this pr . Aftter this pr merged then i will update the documentation . Because these are interRealted now i changed the file to compose.yaml so like if you suggest any different file name the command i will update according to name like (docker compose -f compose.yaml up --build ) if u changed the name it should be (docker compose -f suggestedName.yaml up --build ) like this. And this dependecy optimize will help with if we want to run the server with out docker also in production. |
|
|
@PurnenduMIshra129th this is not an area of strength for me. Please work with @adithyanotfound as he figured out the steps to make this work. I'd like to get this merged because it's a good feature for the |
|
@adithyanotfound ok this is a good idea. I just asked this in slack . |
|
@PurnenduMIshra129th I think you should proceed as follows:
|
|
@adithyanotfound ok will proceed your suggestion |
|
Using a single Dockerfile for both dev and prod feels like trying to teach a bird to swim it might work, but it’s definitely not what it was made for. Dev and prod each have their own unique needs. |
|
@varshith257 do you want both dev and prod images ? Then i create it no problem. |
|
Yes keep them |
…kerfile.dev,Dockerfile.prod
…script for analyze depenency
|
@palisadoes @varshith257 previously i have some work out there so i am not active past 1-2 days . wIll make this issue fixed by today so that it can be merged. |
PurnenduMIshra129th
dismissed stale reviews from varshith257 and coderabbitai[bot]
via
c21adcc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docker-compose.dev.yaml (1)
116-116: Add newline at end of file.Add a newline character at the end of the file to comply with YAML best practices.
talawa-network: +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 116-116: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.dev.yaml(7 hunks)package.json(3 hunks)tests/resolvers/Query/getVolunteerRanks.spec.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
docker-compose.dev.yaml (2)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2828
File: docker-compose.dev.yaml:29-30
Timestamp: 2025-01-19T21:07:15.642Z
Learning: In development Docker environments for Talawa API, Redis port (6379) should remain accessible to other services without localhost binding to allow proper inter-service communication and debugging.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2828
File: docker-compose.dev.yaml:18-19
Timestamp: 2025-01-19T21:06:57.200Z
Learning: In docker-compose.dev.yaml, MongoDB port is bound to all interfaces (27017:27017) instead of localhost (127.0.0.1:27017:27017) to avoid port conflicts in development environment and enable container-to-container communication.
🪛 YAMLlint (1.35.1)
docker-compose.dev.yaml
[error] 116-116: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Testing Application (22.x)
🔇 Additional comments (11)
tests/resolvers/Query/getVolunteerRanks.spec.ts (3)
40-45: LGTM! Test expectations updated correctly.The updated test expectations for volunteer ranks are consistent with the implementation.
61-64: LGTM! Weekly volunteer rank test is accurate.The test case correctly verifies the weekly volunteer hours calculation.
81-83: LGTM! Monthly and yearly volunteer rank tests are consistent.Both monthly and yearly test cases correctly expect 6 hours for the top volunteer.
Also applies to: 98-100
docker-compose.dev.yaml (5)
14-17: Resource limits for MongoDB look appropriate.The 1G memory and 1.0 CPU limits are suitable for development environment considering the complex data model with 40+ schemas and resource-intensive operations.
29-36: Good choice of Redis image and resource limits.Using the Alpine-based Redis image reduces the container size. The resource limits (512M memory, 0.5 CPU) are appropriate for development.
52-56: Resource limits for MinIO are well-balanced.The 512M memory and 0.5 CPU limits are suitable for development usage of MinIO.
82-86: Appropriate resource allocation for the API service.The 1G memory and 1.0 CPU limits for the API service are well-balanced for development needs.
102-106: Good resource optimization for Caddy.The lightweight limits (256M memory, 0.25 CPU) are appropriate for the reverse proxy service in development.
package.json (3)
10-12: Improved MinIO check and startup process.The updated scripts properly handle MinIO initialization by ensuring the build is complete before checking MinIO installation.
25-25: Robust prepare script with fallback.The prepare script now gracefully handles cases where husky is not installed, which is beneficial for Docker builds.
106-108: Verify CLS package compatibility.The addition of both
cls-bluebirdandcls-hookedmight lead to conflicts. Ensure they are compatible and necessary.✅ Verification successful
Both CLS packages are compatible and properly used
The packages serve complementary purposes:
cls-hookedmanages the CLS namespacescls-bluebirdprovides Bluebird promise integration with CLSThe codebase correctly initializes and uses both packages without conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential conflicts or issues with CLS packages echo "=== Checking for CLS usage patterns ===" rg "cls-bluebird|cls-hooked" -A 5 echo -e "\n=== Checking package compatibility ===" npm info cls-bluebird peerDependencies npm info cls-hooked peerDependenciesLength of output: 5024
|
@palisadoes @varshith257 review once . I have fixed this test error |
|
|
@palisadoes I think we need some benchmark results like pulling changes from this PR vs upstream. Like in optimizations it will actually helps of benchmarking them to know what optimised of now and don't result in error prone before we merge. Changes looks good but I need benchmark results of docker image sizes of both dev and prod and CPU Utilisation (which is main reason behind this PR) |
|
@varshith257 @palisadoes now we won't see any difference between docker dev and prod image size .It sizes is now similar to upstream . As we are reverting back the use of slim node images . In prod after using that npm prune in my case image size is reduced around 200MB means initially (before 516MB and after 300MB) for dev images size after using slim node image it is reduced like 600MB(initially 1.3 GB or its equivalent after 556MB). But now we are not using due to stability as by your suggestion .But difference we can see in resource utilization.Also in development image we can't use multistage build as it is not required. |
|
@palisadoes @varshith257 i have used one tool for load test and after giving load on server observing the performance on linux using top command . link to the package https://www.npmjs.com/package/artillery . Give a read about the package. It is very useful and we can set limit how many virtual user we want to create and how much time he will make request to server and what should be the rate of request per second . And when the load test ends it will give the server performance like how much request is success and how much is failure.We can define separate graphql queries and shuffel them to create real world scenerio. |
|
Yes we never have reached for load testing as focused on dev env till today. As we are deploying on Cloud Instance soon it may good for load testing |
|
Please load test before we merge this |
|
@palisadoes are u telling me to share the load test configuration file here |
|
The results |
|
@palisadoes ok i will share |
|
@palisadoes @varshith257 this is the configuration file of my configuration file steps are pretty easy for this -download the file Any way this is the results @palisadoes |
|
@varshith257 Are these stats acceptable? |
|
@palisadoes The results indicates severe performance issues in the login API. See the results only 3 out of 30 requests were successful and even most requests timed out (ETIMEDOUT) and API response time is too high (~8-9s) leading to server overload maybe database queries or concurrency limits or API inefficiencies are likely cause this type of behaviours And don't know what's it ran against dev or prod env? We need to always load test and benchmark in every migrations we do so that we can catch early the issues and fix them @PurnenduMIshra129th Can you open an issue with this results and also make sure this results are against prod env |
|
|
@palisadoes If we consider whole this API as a mono repo instance, yes we can introduce a workflow for running the load test of @PurnenduMIshra129th done makes sense which can catch early of which PR introducing them once we fixed the issues in the develop branch. But note that we need to benchmark on prod env and don;t know is this load test done on it. If not we load test there and need to fix this issues and introduce the workflow what we mentioning. Every app user can expect a smooth app running so our API Performance should be top notch without compromising the performance issue and there are several optimisation methods todo so
Yes
In MVP stage every project load test their app/website server performance and fix them before launching Btw I don't know exact what we are in but last year we though atleast by summer 2025 we have a MVP with beta version. Is it on the track or any deviations ? |
|
@palisadoes @varshith257 i have tested this against dev enviroment . Yes i can open an issue for it. |
palisadoes
merged commit 8ab6bdd
into
PalisadoesFoundation:develop
…alisadoesFoundation#2828) * some changs * updated docker.prod * updated Dockerfile.prod and dev * created single compose file for prod and removed development dockercompose * merged branch with upstream * again added the docke-compose.dev.yaml , docker-compose.prod.yaml,Dockerfile.dev,Dockerfile.prod * changed package.json, changed start command with minio,commented the script for analyze depenency * changed according to suggestion * changed the jq installation to debian syntax * changed command of minio start * updated script of minio and removed unnecessary imports in 2nd stage of image and some suggestion * suggestion from ai * Update docker-compose.dev.yaml line 19 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.dev.yaml line 35 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.dev.yaml line55 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.prod.yaml line 16 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.prod.yaml line 34 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.prod.yaml line 54 Co-authored-by: Vamshi Maskuri <[email protected]> * Update docker-compose.prod.yaml line 84 Co-authored-by: Vamshi Maskuri <[email protected]> * updated by suggestion * updated dockerFiles * added npm install in prod * modify test expectation for getVolunteerRank.spec.ts componennt --------- Co-authored-by: Vamshi Maskuri <[email protected]>
What kind of change does this PR introduce?
bugfix
Issue Number:
Fixes #2730
Did you add tests for your changes?
No
Snapshots/Videos:
Before prod container is not running.
Before container is using total avaiable ram and used full cpu because no resource constraint is there .If we increase load cpu will be used more than 100%.
Before Load average is increase rapidly after we start the container and make some I/O task and DB related task.
Updated-Now container will use specified Ram and cpu.
Before all the files are copied to final image and dockerignore not worked properly.Now only our required files will go to final image.Because root directory is mounted in volume section which causes overwriting of dockerignore.
Updated- Image size is reduced as i have used alpline based images for availble services like redis and caddy.Mongo and MinIO is not availble in docker hub.
Updated - Now load average will not go rapidly . Because by specifing limit it will reduce memory swapping in low spec devices.
Updated-Prod container is now running.
If relevant, did you update the documentation?
No
Summary
Summary
This pull request introduces several optimizations and improvements to the Docker build and deployment process for the application. The main objective of this change is to enhance performance, reduce the size of the final Docker image, and improve runtime efficiency.
Key changes include:
buildfolder,package.json,node_modules, and necessary certificates. It is not working previously.npm run prod) instead of the development build and disabling unnecessary watchers.4.Now there is single compose file and Dockerfile.Deleted the dokcer-compose.dev.yaml and docker-compose.prod.yaml ,Docker.prod,Docker.dev .
5.Only mounts the node_modules volumes not required to mount whole root directory.
6.Changes the command to start the application.
7.Initially .dockerignore is not working properly it copies all files including dockerignore content.
Does this PR introduce a breaking change?
No
Other information
1.We have to use npm run prune --production to remove devlopment dependecy .It will reduce aroun 200MB size of our image. But now it is not implemented due some misconfiguration in package.json. After finding which are not used in production we can use it .It is very effective.
2.Currently mongo images size is greater than all of our image around 900MB . which is increasing the image size around 60%. Currently no alpline image is available for mongo.Also it consumes more ram than all of our services but here i have implemented ram constraints so it will not exceed after specifc limit.
3.I have to update the documentation after taking neccessary changes from mentors and CodeRabbits.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
Infrastructure Updates
Dependency Management
cls-bluebirdpackage.Performance Improvements
New Services
Service Renaming
mongodbtomongoandtalawa-api-prod-containertotalawa-api-prodin Docker configurations.Script Modifications
package.jsonfor improved functionality and error handling.Test Updates
volunteerRanksto align with new data expectations.