Repository navigation
Add BYOC and Worker defaults and runtime validation - #3554
Conversation
shuheiktgw
added
enhancement
b078035 to
e59e574
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e59e5749b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An HPA with minReplicas above maxReplicas was only rejected by the API server. Check that both bounds are positive and ordered. The validators are meant to run on defaulted specs so that a single explicit bound is also checked against the default of the other. Restrict the memory limit requirement to the Indexer and Searcher, which size the Quickwit node configuration from it, so defaulted Workers without limits remain valid.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aac3c3532
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Worker API docs claimed a memory limit is required for buffer sizing, but neither the Worker nor the BYOC pipeline reads it; only the Indexer and Searcher size their Quickwit configuration from it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb7da580e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if component.Resources == nil && resources != nil { | ||
| component.Resources = resources.DeepCopy() |
There was a problem hiding this comment.
When autoscaling is enabled with omitted metrics but the user supplies any non-nil partial resources object without requests.cpu, this branch preserves that object while applyAutoscalingDefaults injects a CPU-utilization metric. Kubernetes cannot calculate utilization for a pod whose relevant container has no CPU request, so the HPA remains unable to scale; the standalone Worker defaulting path has the same interaction. Merge the default CPU request when selecting this metric, or reject/default this combination.
Useful? React with 👍 / 👎.
| if component.Storage == nil { | ||
| component.Storage = &datadoghqv1alpha1.DatadogBYOCClusterStorageSpec{ | ||
| EmptyDir: &corev1.EmptyDirVolumeSource{}, | ||
| } |
There was a problem hiding this comment.
When a standalone Worker selects storage.volumeClaimTemplate: {}, this condition skips storage defaulting entirely, and no later Worker code supplies an access mode or storage request. ValidateStatefulComponent accepts the template because exactly one storage type is present, but the resulting PVC is rejected for lacking its required access modes and requested capacity; apply the same ReadWriteOnce/capacity defaults used for BYOC pipeline claims, or validate that an opted-in Worker claim is complete.
Useful? React with 👍 / 👎.
|
|
||
| func applyComponentDefaults(component *datadoghqv1alpha1.DatadogBYOCClusterComponentSpec, replicas int32, resources *corev1.ResourceRequirements) { | ||
| if component.Replicas == nil { | ||
| component.Replicas = new(replicas) |
There was a problem hiding this comment.
should this be set if autoscaling is also configured?
There was a problem hiding this comment.
Thanks! The replicas are ignored when building the actual STS if HPA is set in a later step 🙏
| claimSpec.Resources.Requests = corev1.ResourceList{} | ||
| } | ||
| if _, found := claimSpec.Resources.Requests[corev1.ResourceStorage]; !found { | ||
| claimSpec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("30Gi") |
There was a problem hiding this comment.
A default storage size is requested even in the case the PVC storage has a smaller limits configured. This will be rejected later by k8s so it should be ok.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5dcb67912
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if image.Digest != nil && *image.Digest == "" { | ||
| errs = append(errs, field.Required(path.Child("digest"), "digest must be non-empty when release is omitted")) |
There was a problem hiding this comment.
When a complete override supplies a nonempty malformed digest such as digest: nope, this validator accepts it, requiresResolution treats the image as complete, and image resolution produces an unusable repository@nope reference; the standalone Worker validator has the same gap. Despite the earlier thread's claimed schema fix, the fresh evidence in the reviewed tree is that both generated CRDs still define digest as an unconstrained string, so validate it as a SHA-256 digest here (and for Worker images), similarly to validateDigest in the resolver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5810fb0ddf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if autoscaling.MinReplicas != nil && autoscaling.MaxReplicas != nil && *autoscaling.MinReplicas > *autoscaling.MaxReplicas { | ||
| errs = append(errs, field.Invalid(path.Child("maxReplicas"), *autoscaling.MaxReplicas, fmt.Sprintf("must be greater than or equal to minReplicas (%d)", *autoscaling.MinReplicas))) | ||
| } | ||
| return errs |
There was a problem hiding this comment.
When a nonempty metric such as metrics: [{type: Resource}] is supplied, defaulting preserves it and this validator checks only replica bounds. The generated CRD requires type but does not require the corresponding source or prevent multiple source objects, so the parent resource is admitted while the resulting HorizontalPodAutoscaler is rejected; validate that each metric has exactly the source matching its type (and a valid target) for both BYOC and standalone Workers.
Useful? React with 👍 / 👎.
| if searcher.Storage == nil { | ||
| searcher.Storage = &datadoghqv1alpha1.DatadogBYOCClusterStorageSpec{ | ||
| EmptyDir: &corev1.EmptyDirVolumeSource{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
When a Searcher selects storage.volumeClaimTemplate: {}, Storage is non-nil so this branch leaves the claim empty, unlike the Indexer and pipeline paths that add ReadWriteOnce and a storage request. The shared validator accepts it because exactly one storage type is selected, but Kubernetes rejects the generated PVC for lacking access modes and requested capacity; default the selected template's required fields or reject an incomplete claim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2af8d41033
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if component == nil { | ||
| return nil | ||
| } | ||
| return validatePodDisruptionBudget(component.PodDisruptionBudget, path.Child("podDisruptionBudget")) |
There was a problem hiding this comment.
When autoscaling is omitted and a BYOC component or standalone Worker specifies replicas: -1, the generated CRDs admit it because the field has no minimum, defaulting preserves it, and this validator accepts it because it only checks the disruption budget. The resulting Deployment or StatefulSet is rejected by Kubernetes, so reject negative replica counts here or add a Minimum=0 schema constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e9c6bd25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if image.Tag != nil && image.Digest != nil { | ||
| errs = append(errs, field.Forbidden(path, "tag and digest are mutually exclusive")) |
There was a problem hiding this comment.
When a complete override uses a nonempty but invalid value such as repository: "registry.example.com/UPPER" or tag: "bad/tag", this validator accepts it and requiresResolution considers the image complete. The resolver then copies these strings verbatim into the resulting image reference, so the managed Pod is rejected rather than the parent resource receiving an InvalidConfiguration error. Apply the same repository/tag parsing used by validateReleaseImage to user-supplied overrides (and to standalone Worker images).
Useful? React with 👍 / 👎.
3 participants
What does this PR do?
Adds the defaults and runtime validation packages for
DatadogBYOCClusterandDatadogObservabilityPipelinesWorker:internal/controller/datadogbyoccluster/defaultsinternal/controller/datadogbyoccluster/validationinternal/controller/datadogobservabilitypipelinesworker/defaultsinternal/controller/datadogobservabilitypipelinesworker/validationNo reconciler calls these packages yet. The upcoming controller PRs will apply defaults first and then validate the defaulted spec before image resolution, so that explicit values are also checked against the defaults they are combined with. Failures will be reported as
InvalidConfiguration.Motivation
These checks run in the reconciler instead of CEL/XValidation so that the CRDs keep working on Kubernetes versions older than 1.25. See #3501 (comment).
Checklist
enhancementqa/skip-qalabel