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

chore: add probes and config to chaos environment #903

Merged
merged 2 commits into from
May 22, 2024

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 22, 2024 21:46
@renancloudwalk renancloudwalk enabled auto-merge (squash) May 22, 2024 21:46
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves multiple configuration changes across Docker, Kubernetes, and environment settings which require careful validation to ensure they integrate properly without causing disruptions in the deployment process.

🧪 Relevant tests

No

⚡ Possible issues

Possible Misconfiguration: The failureThreshold for the startupProbe in chaos/local-deployment.yaml is set very high (60). This could lead to prolonged downtimes in case the service fails to start properly.

Environment Variable Typo: In config/run-with-importer.env.local, RUST_BACKTRACE is followed by a comma, which might be a typo and could potentially lead to runtime errors or misconfigurations.

🔒 Security concerns

No

Code feedback:
relevant filechaos/local-deployment.yaml
suggestion      

Consider reducing the failureThreshold for the startupProbe to a lower value to avoid potential long downtimes during service startup issues. [important]

relevant linefailureThreshold: 60

relevant fileconfig/run-with-importer.env.local
suggestion      

Remove the comma after 1 in the RUST_BACKTRACE environment variable to avoid format errors. [important]

relevant lineRUST_BACKTRACE=1,

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Fix syntax error in environment variable

Correct the syntax error in RUST_BACKTRACE by removing the trailing comma which could
cause environment variable parsing issues.

config/run-with-importer.env.local [14]

-RUST_BACKTRACE=1,
+RUST_BACKTRACE=1
 
Suggestion importance[1-10]: 10

Why: Removing the trailing comma in the RUST_BACKTRACE environment variable is essential to prevent parsing issues. This suggestion addresses a clear syntax error and is critical for the correct functioning of the environment configuration.

10
Correct target ports to match container ports

Ensure that the targetPort values match the actual application ports exposed and used
within the container to avoid misrouting of traffic.

chaos/local-service.yaml [12-16]

-targetPort: 8080
+targetPort: 3000
 ...
-targetPort: 8443
+targetPort: 3000
 
Suggestion importance[1-10]: 9

Why: Ensuring that the target ports match the actual application ports is crucial to avoid misrouting of traffic, which can lead to service disruptions. This suggestion addresses a potential bug and is highly relevant.

9
Best practice
Align initial delay seconds for probes

It's recommended to align the initialDelaySeconds for livenessProbe and readinessProbe to
avoid premature readiness checks before the application is stable.

chaos/local-deployment.yaml [68-75]

 initialDelaySeconds: 15
 ...
-initialDelaySeconds: 5
+initialDelaySeconds: 15
 
Suggestion importance[1-10]: 8

Why: Aligning the initial delay seconds for liveness and readiness probes is a best practice to ensure that readiness checks do not occur prematurely, which can prevent false negatives and improve the stability of the deployment.

8
Possible issue
Reduce the failure threshold for the startup probe

Consider reducing the failureThreshold for the startupProbe to a lower value to avoid
excessive retries which might delay the detection of startup failures.

chaos/local-deployment.yaml [84]

-failureThreshold: 60
+failureThreshold: 10
 
Suggestion importance[1-10]: 7

Why: Reducing the failure threshold can help detect startup failures more quickly, but the appropriate value depends on the specific application and environment. The suggestion is contextually accurate and improves the robustness of the deployment configuration.

7

@renancloudwalk renancloudwalk merged commit 08ebce3 into main May 22, 2024
26 checks passed
@renancloudwalk renancloudwalk deleted the final-chaos-backbone branch May 22, 2024 21:53
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.

1 participant