Added TopologyKey to noobaa CRD for multizone support#1653
Added TopologyKey to noobaa CRD for multizone support#1653dannyzaken merged 1 commit intonoobaa:masterfrom
Conversation
|
Warning Rate limit exceeded@dannyzaken has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 16 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)
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NooBaa Controller
participant Kubernetes API
User->>NooBaa Controller: Set/Update NooBaa.Spec.TopologyKey
NooBaa Controller->>NooBaa Controller: Reconcile NooBaa resource
NooBaa Controller->>NooBaa Controller: Build Affinity via GetAffinity()
NooBaa Controller->>Kubernetes API: Update Deployment/Cluster Spec with Affinity and TopologyKey
alt TopologyKey is set and differs from default
NooBaa Controller->>Kubernetes API: Add extra TopologySpreadConstraint with TopologyKey
else TopologyKey is unset or default
NooBaa Controller->>Kubernetes API: Use default TopologySpreadConstraint
end
Kubernetes API-->>NooBaa Controller: Apply changes to pods
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
6453235 to
a65e325
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/bundle/deploy.go (1)
2414-2415: Consider adding validation and description to the topologyKey field.The
topologyKeyfield addition looks good and aligns with the PR objective for multizone support. However, consider enhancing the schema definition:topologyKey: + description: "TopologyKey used as the domain for TopologySpreadConstraint and affinity settings of NooBaa components. Examples include 'kubernetes.io/hostname' for host-level distribution or 'topology.kubernetes.io/zone' for zone-level distribution." type: string + pattern: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*(/[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"The pattern validation ensures the field follows Kubernetes label/annotation naming conventions for topology keys.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
deploy/crds/noobaa.io_noobaas.yaml(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(2 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(15 hunks)pkg/backingstore/reconciler.go(1 hunks)pkg/bundle/deploy.go(2 hunks)pkg/system/db_reconciler.go(2 hunks)pkg/system/phase2_creating.go(2 hunks)pkg/system/phase4_configuring.go(3 hunks)pkg/system/reconciler.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- pkg/system/phase2_creating.go
- pkg/system/reconciler.go
- deploy/crds/noobaa.io_noobaas.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/system/db_reconciler.go
- pkg/system/phase4_configuring.go
🧰 Additional context used
🧠 Learnings (2)
pkg/backingstore/reconciler.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: run-operator-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (6)
pkg/backingstore/reconciler.go (1)
1332-1336: LGTM: Correct adaptation to new AffinitySpec structure.The change properly adapts to the new
AffinitySpecstructure by explicitly constructing acorev1.Affinitystruct from the three standard Kubernetes affinity types. This maintains the same functionality while accommodating the addition of theTopologyKeyfield in the new affinity specification.pkg/bundle/deploy.go (1)
1426-1426: LGTM: Checksum updated correctly.The SHA256 checksum update reflects the changes made to the underlying CRD YAML content, specifically the addition of the
topologyKeyfield to the affinity specification.pkg/apis/noobaa/v1alpha1/noobaa_types.go (2)
160-160: Good design pattern for extending Kubernetes affinity configuration.The change from
*corev1.Affinityto*AffinitySpecfollows a good extension pattern that maintains the original Kubernetes affinity semantics while adding custom functionality for topology support.
245-260: Well-structured AffinitySpec design for multizone support.The new
AffinitySpecstruct effectively wraps the standard Kubernetes affinity types and adds theTopologyKeyfield for topology-aware scheduling. The field documentation clearly explains the purpose and usage.Note: The protobuf tags on lines 249, 252, and 255 may not be necessary for this custom struct, but they maintain consistency with Kubernetes API conventions.
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (2)
13-14: Import alias changes are consistent.The import alias changes (
corev1→v1and addition ofconditionsv1) are applied consistently throughout the autogenerated file.
83-112: AffinitySpec deepcopy implementation is correct.The autogenerated deepcopy methods for
AffinitySpecproperly handle:
- Nil checks for each optional affinity field (lines 86, 91, 96)
- Deep copying of nested Kubernetes affinity objects (lines 88-89, 93-94, 98-99)
- The
TopologyKeystring field is correctly copied via the*out = *inassignment on line 85
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| type: object | ||
| topologyKey: |
There was a problem hiding this comment.
Could you please update the doc doc/noobaa-crd.md.
There was a problem hiding this comment.
I will create a separate PR with changes to noobaa-crd.md, that also includes updates regarding the dbSpec
|
@naveenpaul1, I noticed now that we do not update the topologySpreadConstraints if it is already set. Since we want it to be updated if a topologyKey is supplied, I think we should always reconcile it unless the annotation is set, WDYT? |
c6a1352 to
fdf81e8
Compare
- Changed `affinity` under Noobaa spec to a new struct `AffinitySpec`, containing the same fields as corev1.Affinity - Added `TopologyKey` to AffinitySpec. This can be used for example to set `topology.kubernetes.io/zone` as the topology domain - If a topologyKey is used and is different from the default `kubernetes.io/hostname` TopologyKey, adding another TopologySpreadConstraint with the additional key. - Passing the TopologyKey to the DB cluster CR under affinity. This will be set to the podAntiAffinity of the DB pods. Signed-off-by: Danny Zaken <dannyzaken@gmail.com> updated core ContainerImageTag Signed-off-by: Danny Zaken <dannyzaken@gmail.com>
Explain the changes
affinityunder Noobaa spec to a new structAffinitySpec,containing the same fields as corev1.Affinity
TopologyKeyto AffinitySpec. This can be used for example to settopology.kubernetes.io/zoneas the topology domainkubernetes.io/hostnameTopologyKey, adding another TopologySpreadConstraint with the additional key.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements