chore: disable pdbs for dps #38204
chore: disable pdbs for dps #38204
Conversation
WalkthroughThe Changes
Poem
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
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
scripts/deploy_preview.sh (3)
Line range hint
3-31: Enhance AWS credentials security and error handlingThe AWS credentials handling needs improvement in several areas:
- Add error handling for the
aws sts assume-rolecommand- Set restrictive permissions on AWS config file
- Consider using AWS SDK environment variables instead of config file
Apply this improvement:
mkdir ~/.aws; touch ~/.aws/config +chmod 600 ~/.aws/config -sts_output="$(aws sts assume-role --role-arn "$AWS_ROLE_ARN" --role-session-name ekscisession)" +sts_output="$(aws sts assume-role --role-arn "$AWS_ROLE_ARN" --role-session-name ekscisession)" || { + echo "Failed to assume role" + exit 1 +}
Line range hint
39-54: Improve cleanup reliability and error handlingThe cleanup process needs better error handling and race condition prevention.
Apply these improvements:
if [[ -n "${RECREATE-}" ]] then - mongosh "mongodb+srv://$DB_USERNAME:$DB_PASSWORD@$DB_URL/$DBNAME?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin" --eval 'db.dropDatabase()' + mongosh "mongodb+srv://$DB_USERNAME:$DB_PASSWORD@$DB_URL/$DBNAME?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin" --eval 'db.dropDatabase()' || { + echo "Failed to drop database" + exit 1 + } - pod_name="$(kubectl get pods -n "$NAMESPACE" -o json | jq '.items[0].metadata.name' | tr -d '"')" + pod_name="$(kubectl get pods -n "$NAMESPACE" -o json | jq -r '.items[0].metadata.name')" || { + echo "No pods found in namespace $NAMESPACE" + exit 1 + }
Line range hint
102-108: Consider externalizing sensitive configurationThe script contains sensitive configuration values that should be moved to a secure configuration management system:
- Sentry DSN
- MongoDB connection string
- Customer portal URL
🧹 Nitpick comments (1)
scripts/deploy_preview.sh (1)
Line range hint
47-54: Remove or implement commented EFS access point codeThe commented code for EFS access points should either be implemented or removed to maintain code clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/deploy_preview.sh(1 hunks)
🔇 Additional comments (1)
scripts/deploy_preview.sh (1)
101-101: Verify impact of disabled pod disruption budgets
Disabling pod disruption budgets might affect application availability during node maintenance or scaling events.
Run this script to check current PDB usage:
✅ Verification successful
Let me search for PDB-related configurations in the codebase to understand the impact of disabling them.
Disabling PDBs is acceptable for preview deployments
The PDB configuration is being disabled in a preview deployment script (deploy_preview.sh), which is appropriate since:
- This is a preview environment, not production
- The StatefulSet configuration exists but preview deployments typically don't require high availability guarantees
- Resource requests are minimal (1m CPU), indicating this is a non-critical environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current PDB configuration and pod distribution
kubectl get pdb -n "$NAMESPACE" -o wide
kubectl get pods -n "$NAMESPACE" -o wide --show-labels
Length of output: 274
Script:
#!/bin/bash
# Search for PDB configurations in Helm charts and values files
fd -e yaml -e yml | xargs rg -l "podDisruptionBudget|PodDisruptionBudget"
# Check if there are any other instances of PDB configuration
rg -i "podDisruptionBudget" -A 3 -B 3
# Look for deployment configurations that might be affected
fd -e yaml -e yml | xargs rg "kind:\s+(Deployment|StatefulSet)"
Length of output: 959
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/deploy_preview.sh (1)
Line range hint
39-44: Enhance error handling in cleanup sectionThe current error handling might mask real issues. Consider:
- Adding error checks for database operations
- Logging cleanup failures
- Implementing the commented EFS access point cleanup
if [[ -n "${RECREATE-}" ]] then + echo "Starting cleanup process..." + if ! mongosh "mongodb+srv://$DB_USERNAME:$DB_PASSWORD@$DB_URL/$DBNAME?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin" --eval 'db.dropDatabase()'; then + echo "Warning: Database cleanup failed" + fi pod_name="$(kubectl get pods -n "$NAMESPACE" -o json | jq '.items[0].metadata.name' | tr -d '"')" - kubectl exec "$pod_name" -n "$NAMESPACE" -- bash -c "rm -rf /appsmith-stacks/*" + if ! kubectl exec "$pod_name" -n "$NAMESPACE" -- bash -c "rm -rf /appsmith-stacks/*"; then + echo "Warning: Failed to clean up appsmith-stacks" + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/deploy_preview.sh(1 hunks)
🔇 Additional comments (1)
scripts/deploy_preview.sh (1)
101-101: LGTM: Disabling PDBs is appropriate for preview environments
The change aligns with the deployment preview use case where high availability guarantees aren't critical.
Let's verify the PDB configuration in the Helm chart:
✅ Verification successful
Disabling PDBs is safe for preview environments
The Helm chart values file doesn't explicitly define PDB configurations, which means the --set podDisruptionBudgets.enabled=false flag will safely override any default PDB settings without side effects. This is appropriate for preview environments where high availability isn't critical.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify PDB configuration in the Helm chart
helm show values appsmith-ee/appsmith | grep -A 5 "podDisruptionBudgets"
Length of output: 167
Script:
#!/bin/bash
# Let's search for PDB-related configurations in yaml files
rg -l "podDisruptionBudgets?" --type yaml
# Check the content of any helm chart values files
fd "values.ya?ml" --type f --exec cat {}
Length of output: 10159
| --set resources.requests.cpu="1m" \ | ||
| --set resources.requests.memory="2048Mi" \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting the CPU request
The CPU request of 1m is extremely low and might cause scheduling issues. Consider setting it to a more realistic value like "100m" for better pod scheduling.
- --set resources.requests.cpu="1m" \
+ --set resources.requests.cpu="100m" \
--set resources.requests.memory="2048Mi" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --set resources.requests.cpu="1m" \ | |
| --set resources.requests.memory="2048Mi" \ | |
| --set resources.requests.cpu="100m" \ | |
| --set resources.requests.memory="2048Mi" \ |
Summary by CodeRabbit
New Features
Bug Fixes