Conversation
Summary of ChangesHello @pallasathena92, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Pod Disruption Budget (PDB) controller to improve the resilience and availability of services by allowing users to define minimum availability or maximum unavailability constraints for pods during voluntary disruptions. Additionally, it refines the deployment naming convention by removing a legacy modification related to HPA references, ensuring more consistent resource identification. These changes collectively aim to bolster the stability of deployed applications. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a Pod Disruption Budget (PDB) controller to improve service availability during voluntary disruptions, which is a great addition. The implementation is well-integrated into the existing reconciliation logic. The PR also includes a cleanup of some legacy code related to HPA deployment naming from a previous migration.
My review includes a high-severity comment regarding the default PDB configuration, which could inadvertently cause downtime for single-replica services. I've also pointed out a couple of minor typos in the code comments that affect the generated API documentation. Overall, the changes are solid, and addressing the default PDB behavior would make this feature safer for all use cases.
| if componentExt.MinAvailable == nil && componentExt.MaxUnavailable == nil { | ||
| // Set maxUnavailable = 1 as default | ||
| maxUnavailable = &intstr.IntOrString{ | ||
| Type: intstr.Int, | ||
| IntVal: 1, | ||
| } | ||
| } |
There was a problem hiding this comment.
The default behavior of creating a PodDisruptionBudget with maxUnavailable: 1 can be risky for single-replica services, as it would allow the single pod to be voluntarily disrupted, causing downtime. To better ensure availability, consider not creating a PDB by default if neither minAvailable nor maxUnavailable is specified by the user. This would make PDB creation an explicit opt-in, which is safer. An alternative could be to default to maxUnavailable: 0 for single-replica deployments.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* add pdb controller * [Core] add pdb controller in ome * Update pkg/apis/ome/v1beta1/component.go --------- Co-authored-by: yifeliu <yifeng.liu@oracle.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
A Pod Disruption Budget (PDB) in Kubernetes is a resource that specifies the minimum number or percentage of pods that must remain available for an application during voluntary disruptions.
Add it to our service to ensure the availiblity and volutary disruptions.
Which issue(s) this PR fixes:
there are two mian parts of this pr:
during migration, we have add "-new" as suffix to deployment name. In current inference service behavior, the deployment name will be "xxx-engine" and "xxx-decoder".
Does this PR introduce a user-facing change?
None. There is no impact on user's experience.