-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[Playground] Healthcheck was added #24227
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
Conversation
|
R: @rshamunov |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
1 similar comment
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
rshamunov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
LGTM |
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| name: playground-java | ||
| spec: | ||
| maxReplicas: 10 | ||
| minReplicas: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one configured with 2 while the others 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tested Java backend. For normal autoscaling at least +1 pod required
| - containerPort: 8080 | ||
| livenessProbe: | ||
| httpGet: | ||
| path: /liveness | ||
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the 8080 to be acquired from a helm variable to populate ports.containerPort, livenessProbe.httpGet.port, and readinessProbe.httpGet.port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| memory: "50Mi" | ||
| cpu: "250m" | ||
| limits: | ||
| cpu: "500" | ||
| memory: "500Mi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was the values 50Mi, 250m, 500, and 500Mi determined? I wouldn't know myself and not a blocker for this PR. I was just curious and thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did some tests thru the Apache JMeter. These resources are enough for simple tasks, and they will be autoscaled once loading will be increased
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, can we populate the port parameter from a helm variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, can we populate the port parameter from a helm variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, can we populate the port parameter from a helm variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, can we populate the port parameter from a helm variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| port: 8080 | ||
| initialDelaySeconds: 3 | ||
| periodSeconds: 3 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /readiness | ||
| port: 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, can we populate the port parameter from a helm variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Everything was moved to the values.yaml file
| static_ip: 34.120.95.76 | ||
| redis_ip: 10.226.29.100:6379 | ||
| project_id: astst-369706 | ||
| registry: us-east1-docker.pkg.dev/astst-369706/playground-repository | ||
| static_ip_name: pg-static-ip | ||
| tag: teta | ||
| dns_name: dev-playground.online |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to hardcode these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those variables are changing automatically thru the Gradle task (if they exist) or will be created during execution. Removed for now
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloem LGTM
|
lgtm thanks! |
* Healthcheck was added * Autoscaling * License information was added * Move healthcheck vars to value.yaml * remove unused values * Resources limits were corrected Co-authored-by: Sergey Makarkin <sergey.makarkin@akvelon.com>
solves #24310
Add Healthcheck and Autoscaling in Kubernetes deployments
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.