feat: user can set PDB by himself #1093
Conversation
|
Warning Rate limit exceeded@Rory-Z has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThe pull request introduces new fields Changes
Sequence DiagramsequenceDiagram
participant Operator
participant CRD
participant PDB
participant Pods
Operator->>CRD: Read MinAvailable/MaxUnavailable
CRD-->>Operator: Return configuration
Operator->>PDB: Generate PodDisruptionBudget
PDB-->>Pods: Apply availability constraints
Pods-->>Operator: Maintain desired availability
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/en_US/reference/v2beta1-reference.md (1)
111-112: Documentation could be enhanced with examplesThe documentation for
minAvailableandmaxUnavailablefields is accurate but would benefit from practical examples showing how to use these fields in YAML configurations.Consider adding examples like:
# Example 1: Using minAvailable spec: coreTemplate: spec: minAvailable: 1 # Ensure at least 1 pod is available # Example 2: Using maxUnavailable spec: coreTemplate: spec: maxUnavailable: "50%" # Allow up to 50% pods to be unavailabledeploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml (1)
9813-9823: Add descriptions and validation for PDB fieldsThe implementation of
maxUnavailableandminAvailablefields looks good, matching Kubernetes PodDisruptionBudget spec. Consider these enhancements:
- Add field descriptions to improve API documentation:
maxUnavailable: + description: "An eviction is allowed if at most \"maxUnavailable\" pods selected by \"selector\" are unavailable after the eviction. Can be absolute number or percentage." anyOf: - type: integer - type: string x-kubernetes-int-or-string: true minAvailable: + description: "An eviction is allowed if at least \"minAvailable\" pods selected by \"selector\" remain available after the eviction. Can be absolute number or percentage." anyOf: - type: integer - type: string default: 1 x-kubernetes-int-or-string: true
- Consider adding OpenAPI v3 validation rules to prevent setting both fields simultaneously and validate against replicas:
x-kubernetes-validations: - rule: "self.maxUnavailable == null || self.minAvailable == null" message: "maxUnavailable and minAvailable are mutually exclusive" - rule: "self.maxUnavailable == null || self.maxUnavailable <= self.replicas" message: "maxUnavailable must not exceed replicas"Also applies to: 13378-13388
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apis/apps/v2beta1/emqx_types.go(2 hunks)apis/apps/v2beta1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/apps.emqx.io_emqxes.yaml(2 hunks)controllers/apps/v2beta1/add_pdb.go(1 hunks)deploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml(2 hunks)docs/en_US/reference/v2beta1-reference.md(2 hunks)docs/zh_CN/reference/v2beta1-reference.md(2 hunks)
🔇 Additional comments (12)
controllers/apps/v2beta1/add_pdb.go (2)
79-83: Double-check mutual exclusivity for MinAvailable and MaxUnavailable.
This block correctly sets one field to nil if the other is specified. Just verify that your controller logic and CR validations also prevent both from being set simultaneously in an ambiguous way.
92-96: Replicant PDB mirrors the core logic.
The code block for the replicant node’s PDB is consistent with the core node’s approach. It also correctly toggles MinAvailable and MaxUnavailable. This seems correct.
apis/apps/v2beta1/emqx_types.go (3)
22-22: No concerns with intstr import.
The addition of "k8s.io/apimachinery/pkg/util/intstr" is necessary to handle the new IntOrString fields for MinAvailable and MaxUnavailable.
202-202: Replicas default assignment appears correct.
Defining a default of 2 is sensible and consistent with the existing design; no issues found.
209-219: Ensure MaxUnavailable and MinAvailable are truly exclusive.
Declaring both fields as pointers to intstr.IntOrString is fine, but verify that your validation logic enforces mutual exclusivity. Otherwise, unexpected PDB behavior might occur if both fields are non-nil.
apis/apps/v2beta1/zz_generated.deepcopy.go (2)
27-27: Import for intstr recognized.
Including "k8s.io/apimachinery/pkg/util/intstr" aligns with the usage of IntOrString.
251-260: DeepCopy logic for new fields.
The additional code correctly ensures that MinAvailable and MaxUnavailable pointers are deeply copied. This is crucial for CRD operations; well done.
docs/zh_CN/reference/v2beta1-reference.md (2)
111-111: Documentation of minAvailable is consistent with the code changes.
Thank you for adding a clear explanation in the reference documentation.
112-112: Documentation of maxUnavailable is accurately described.
This aligns with the new field in the CRD and helps users understand how to configure PDB.
docs/en_US/reference/v2beta1-reference.md (1)
235-236: Verify consistent implementation across templates
The documentation for PDB fields in EMQXReplicantTemplateSpec matches the CoreTemplateSpec. Please ensure that the implementation handles these fields consistently for both core and replicant nodes.
✅ Verification successful
Let me gather more information to verify the implementation consistency across templates.
Let me search for the core template spec to compare the implementations.
Let me check how the PDB fields are used in the controller implementation.
Based on the gathered information, I can now provide the final response:
PDB fields are properly implemented across templates
The implementation in add_pdb.go correctly handles both minAvailable and maxUnavailable fields for both core and replicant templates:
- Core template: Uses the fields from
CoreTemplate.Spec - Replicant template: Uses the fields from
ReplicantTemplate.Spec - Maintains mutual exclusivity by clearing
minAvailablewhenmaxUnavailableis set - Both templates inherit from
EMQXReplicantTemplateSpec, ensuring consistent field definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of PDB fields
# Check for PDB field handling in controller code
rg -A 5 "minAvailable|maxUnavailable" --type go
Length of output: 1605
Script:
#!/bin/bash
# Search for PDB-related struct definitions and fields in both core and replicant templates
ast-grep --pattern 'type $_ struct {
$$$
MinAvailable $_
$$$
}'
ast-grep --pattern 'type $_ struct {
$$$
MaxUnavailable $_
$$$
}'
# Search for PDB creation/handling logic
rg -A 10 "NewPodDisruptionBudget|CreatePodDisruptionBudget" --type go
Length of output: 28389
Script:
#!/bin/bash
# Search for CoreTemplateSpec definition
ast-grep --pattern 'type EMQXCoreTemplateSpec struct {
$$$
}'
# Search for PDB-related code in controllers
rg -A 5 "PodDisruptionBudget" --type go
Length of output: 5618
Script:
#!/bin/bash
# Search for the actual usage of minAvailable and maxUnavailable in PDB generation
rg -B 5 -A 10 "MinAvailable|MaxUnavailable" controllers/apps/v2beta1/add_pdb.go
Length of output: 1753
config/crd/bases/apps.emqx.io_emqxes.yaml (1)
9800-9810: LGTM: Schema changes for PDB fields are well-defined
The CRD schema correctly defines:
- Both integer and string formats for flexible specification
- Default value of 1 for minAvailable
- Proper Kubernetes int-or-string type annotation
deploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml (1)
9813-9823: LGTM on PDB field implementation
The implementation correctly:
- Uses IntOrString type to support both absolute numbers and percentages
- Provides a safe default of 1 for minAvailable
- Maintains consistency with Kubernetes PDB spec
Also applies to: 13378-13388
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
- Coverage 76.33% 76.17% -0.16%
==========================================
Files 69 69
Lines 6250 6251 +1
==========================================
- Hits 4771 4762 -9
- Misses 1242 1249 +7
- Partials 237 240 +3 ☔ View full report in Codecov by Sentry. |
Rory-Z
force-pushed
the
feat/user-can-set-pdb
branch
2 times, most recently
from
57ec4d5 to
53b8947
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/crd/bases/apps.emqx.io_emqxes.yaml (1)
Line range hint
209-223: Consider adding documentation for PDB configuration best practicesWhile the implementation is solid, consider adding documentation that covers:
- Recommended values for different deployment sizes
- Impact of different settings on cluster availability
- Examples of common configurations
- Migration guide for existing deployments
This would help users make informed decisions when configuring these values.
deploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml (2)
9813-9829: Consider extracting common PDB field definitions to reduce duplication.The
maxUnavailableandminAvailablefields are duplicated in bothcoreTemplate.specandreplicantTemplate.spec. Consider extracting these field definitions into a shared schema to improve maintainability.# Define a shared schema x-pdb-fields: &pdbFields maxUnavailable: anyOf: - type: integer - type: string x-kubernetes-int-or-string: true x-kubernetes-validations: - message: Cannot be set when minAvailable is specified rule: 'has(self.minAvailable) ?false : true' minAvailable: anyOf: - type: integer - type: string default: 1 x-kubernetes-int-or-string: true x-kubernetes-validations: - message: Cannot be set when maxUnavailable is specified rule: 'has(self.maxUnavailable) ?false : true' # Use the shared schema spec: coreTemplate: spec: <<: *pdbFields replicantTemplate: spec: <<: *pdbFieldsAlso applies to: 13384-13400
9819-9820: Enhance validation messages for better clarity.The current validation messages could be more descriptive to help users understand the constraint better.
- message: This is a mutually exclusive setting with 'minAvailable' + message: maxUnavailable cannot be set when minAvailable is specified. These fields are mutually exclusive in PodDisruptionBudget. - message: This is a mutually exclusive setting with 'maxUnavailable' + message: minAvailable cannot be set when maxUnavailable is specified. These fields are mutually exclusive in PodDisruptionBudget.Also applies to: 9828-9829, 13390-13391, 13399-13400
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apis/apps/v2beta1/emqx_types.go(2 hunks)apis/apps/v2beta1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/apps.emqx.io_emqxes.yaml(2 hunks)controllers/apps/v2beta1/add_pdb.go(1 hunks)deploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml(2 hunks)docs/en_US/reference/v2beta1-reference.md(2 hunks)docs/zh_CN/reference/v2beta1-reference.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apis/apps/v2beta1/zz_generated.deepcopy.go
- docs/zh_CN/reference/v2beta1-reference.md
- controllers/apps/v2beta1/add_pdb.go
- docs/en_US/reference/v2beta1-reference.md
🔇 Additional comments (4)
apis/apps/v2beta1/emqx_types.go (2)
209-216: LGTM: Well-implemented MinAvailable field with proper validation
The implementation follows Kubernetes PodDisruptionBudget spec pattern and includes:
- Clear documentation of the field's purpose
- Proper type using IntOrString for flexibility
- Default value of 1 for safe defaults
- Validation rule to ensure mutual exclusivity with MaxUnavailable
217-223: LGTM: Well-implemented MaxUnavailable field with proper validation
The implementation follows Kubernetes PodDisruptionBudget spec pattern and includes:
- Clear documentation of the field's purpose
- Proper type using IntOrString for flexibility
- Validation rule to ensure mutual exclusivity with MinAvailable
config/crd/bases/apps.emqx.io_emqxes.yaml (1)
13371-13387: LGTM: CRD schema properly reflects the PDB configuration
The CRD schema correctly implements:
- Both MinAvailable and MaxUnavailable fields with proper types
- Mutual exclusivity validation rules
- Default value for MinAvailable
- Consistent documentation with the Go types
deploy/charts/emqx-operator/templates/crd.emqxes.apps.emqx.io.yaml (1)
9813-9829: LGTM on the PDB field implementation.
The implementation of maxUnavailable and minAvailable fields follows Kubernetes PodDisruptionBudget specification correctly:
- Proper use of x-kubernetes-int-or-string for supporting both integer and percentage values
- Correct mutual exclusivity validation
- Sensible default of 1 for minAvailable
Also applies to: 13384-13400
Rory-Z
force-pushed
the
feat/user-can-set-pdb
branch
4 times, most recently
from
028f59d to
ea84b0d
Compare
Signed-off-by: Rory Z <[email protected]>
Rory-Z
force-pushed
the
feat/user-can-set-pdb
branch
from
ea84b0d to
aff9217
Compare
Fix #1092
Summary by CodeRabbit
New Features
minAvailableandmaxUnavailablefields for enhanced pod availability management during updates in EMQX resources.Documentation
minAvailableandmaxUnavailablein the API documentation forEMQXCoreTemplateSpecandEMQXReplicantTemplateSpec.