fix: add fields documentation in CRDs #2447
Conversation
julienmancuso
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughUpdates CRD generation in the operator Makefile and copies generated CRDs into Helm templates. Expands DynamoComponentDeployment API with a shared spec, enriched ingress configuration, new status field, and removes legacy types. Adds documentation comments to DynamoGraphDeployment. Regenerates deepcopy code reflecting removed types. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant MK as Makefile (manifests)
participant CRD as Controller-Gen
participant FS as Filesystem
participant Helm as Helm CRD Templates
Dev->>MK: make manifests
MK->>CRD: Generate CRDs (maxDescLen=100000)
CRD-->>FS: Write CRDs to config/crd/bases
MK->>FS: Copy bases/*.yaml -> ../helm/crds/templates/
FS-->>Helm: Updated CRD templates available
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
deploy/cloud/operator/api/v1alpha1/dynamographdeployment_types.go (1)
52-56: Consider documenting/validating allowed State valuesIf State has a known finite set (e.g., Pending, Reconciling, Ready, Error), consider documenting the allowed values or defining typed constants to guide users and tooling. Optional: add +kubebuilder:validation:Enum for stricter schemas if appropriate.
Would you like me to propose a set of Status state constants and an Enum schema?
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (4)
82-84: Clarify Replicas vs Autoscaling precedence in docsAvoid ambiguity by specifying how these interact. Common pattern: when Autoscaling is set, HPA drives replicas; Replicas may be ignored or used only as the initial/min value.
Proposed doc tweaks:
@@ - // Autoscaling config for this component (replica range, target utilization, etc.). + // Autoscaling config for this component (replica range, target utilization, etc.). + // When set, the autoscaler controls the number of replicas. The Spec.Replicas + // field is ignored at runtime (or used only to seed HPA minReplicas if desired). Autoscaling *Autoscaling `json:"autoscaling,omitempty"` @@ - // Replicas is the desired number of Pods for this component when autoscaling is not used. + // Replicas is the desired number of Pods for this component when autoscaling is not used. + // If both Replicas and Autoscaling are set, Autoscaling takes precedence. Replicas *int32 `json:"replicas,omitempty"`Also applies to: 107-109
64-66: Disambiguate resource-level vs pod-level metadata precedenceYou expose both component-wide Annotations/Labels (applied to Pod/Service/Ingress) and ExtraPodMetadata (pod-only). Clarify merge/precedence rules to avoid surprises when both specify Pod metadata.
Suggested wording updates:
@@ - // Annotations to add to generated Kubernetes resources for this component - // (such as Pod, Service, and Ingress when applicable). + // Annotations to add to generated Kubernetes resources for this component + // (Pod, Service, Ingress when applicable). For Pod-only annotations/labels, + // use ExtraPodMetadata. If both specify pod annotations/labels, ExtraPodMetadata + // takes precedence for Pod metadata. @@ - // ExtraPodMetadata adds labels/annotations to the created Pods. + // ExtraPodMetadata adds labels/annotations only to the created Pods. If a key + // collides with Spec.Annotations/Labels on Pods, ExtraPodMetadata wins.Also applies to: 95-101
117-137: Add light validation on ingress host fieldsOptional schema tightening can prevent invalid ingress hostnames:
- Host: consider +kubebuilder:validation:Format=hostname
- For HostPrefix/HostSuffix, consider documenting expected character set or adding a conservative Pattern/MaxLength.
If you want, I can generate conservative validation annotations that won’t block common use-cases.
212-214: Avoid false positives in IsMainComponent detectionSuffix matching on service name can misfire (e.g., “Service” matching “AnotherService”). If tag format is “pkg:ServiceName”, consider requiring the “:” delimiter in the suffix check.
- return strings.HasSuffix(s.Spec.DynamoTag, s.Spec.ServiceName) || s.Spec.ComponentType == commonconsts.ComponentTypeMain + return strings.HasSuffix(s.Spec.DynamoTag, ":"+s.Spec.ServiceName) || s.Spec.ComponentType == commonconsts.ComponentTypeMainPlease confirm all tags follow the "package:ServiceName" convention before applying.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
deploy/cloud/operator/Makefile(2 hunks)deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go(5 hunks)deploy/cloud/operator/api/v1alpha1/dynamographdeployment_types.go(2 hunks)deploy/cloud/operator/api/v1alpha1/zz_generated.deepcopy.go(0 hunks)
💤 Files with no reviewable changes (1)
- deploy/cloud/operator/api/v1alpha1/zz_generated.deepcopy.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:1178-1180
Timestamp: 2025-07-18T16:05:05.534Z
Learning: The stopSignal field under lifecycle in DynamoComponentDeployment CRDs is autogenerated due to Kubernetes library upgrades (k8s.io/api and k8s.io/apimachinery from v0.32.3 to v0.33.1), not a manual design decision by the user.
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1308-1312
Timestamp: 2025-06-11T21:29:28.650Z
Learning: User julienmancuso expects replies in English; avoid switching languages unless explicitly requested.
📚 Learning: 2025-07-18T16:04:31.771Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
Applied to files:
deploy/cloud/operator/Makefile
🧬 Code Graph Analysis (2)
deploy/cloud/operator/api/v1alpha1/dynamographdeployment_types.go (3)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (1)
DynamoComponentDeploymentOverridesSpec(56-58)deploy/cloud/operator/internal/dynamo/graph.go (1)
BackendFramework(622-622)deploy/cloud/operator/internal/controller/dynamographdeployment_controller.go (1)
State(43-43)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (2)
deploy/cloud/operator/api/dynamo/common/common.go (3)
Resources(33-36)ExtraPodMetadata(52-55)ExtraPodSpec(57-60)deploy/cloud/operator/internal/dynamo/graph.go (2)
Resources(81-86)Autoscaling(58-61)
⏰ 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). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
deploy/cloud/operator/Makefile (1)
61-62: crd:maxDescLen semantics — 0 DROPS descriptions; don't assume 0 = unlimitedShort summary: controller-gen (v0.16.x) treats
crd:maxDescLen=0as “drop all field descriptions.” To include full descriptions, omit thecrd:maxDescLenflag (default includes full text) or set a positive large value. Keepingcrd:maxDescLen=100000is valid to preserve long descriptions but may inflate CRD size — verify generated CRD sizes if that’s a concern.
- Location to check: deploy/cloud/operator/Makefile (around lines 61–62)
- Current snippet:
$(CONTROLLER_GEN) rbac:roleName=manager-role crd:maxDescLen=100000 webhook paths="./..." output:crd:artifacts:config=config/crd/bases- Action options:
- Remove the flag to preserve full descriptions:
$(CONTROLLER_GEN) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases- Or keep
crd:maxDescLen=100000if you intentionally want long-but-limited descriptions — then verify generated CRD file sizes.Likely an incorrect or invalid review comment.
deploy/cloud/operator/api/v1alpha1/dynamographdeployment_types.go (1)
32-48: Great documentation upgrades for Spec fieldsClear, actionable comments on DynamoGraph, Services, Envs, and BackendFramework. This directly supports the PR goal of richer CRD field documentation.
deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml
Show resolved
Hide resolved
deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml
Show resolved
Hide resolved
atchernych
left a comment
atchernych
left a comment
There was a problem hiding this comment.
One typo and neat (missing space). LGTM otherwise
whoisj
left a comment
whoisj
left a comment
There was a problem hiding this comment.
LGTM, thank you for doing this!
Signed-off-by: Hannah Zhang <[email protected]>
Overview:
add fields documentation in CRDs
Summary by CodeRabbit