Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new version of DatadogPodAutoscaler CRD #1621

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbartosik
Copy link

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Write there any instructions and details you may have to test your PR.

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@jbartosik jbartosik changed the title Cascl 120 new ddas crd Add new version of DatadogPodAutoscaler CRD Jan 13, 2025
@jbartosik jbartosik added enhancement New feature or request qa/skip-qa labels Jan 13, 2025
@jbartosik jbartosik force-pushed the CASCL-120-new-ddas-crd branch 3 times, most recently from 3c1e64d to 2f562af Compare January 13, 2025 15:55
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.28%. Comparing base (a839ab9) to head (fb5a478).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1621   +/-   ##
=======================================
  Coverage   49.28%   49.28%           
=======================================
  Files         216      216           
  Lines       20662    20662           
=======================================
  Hits        10184    10184           
  Misses       9942     9942           
  Partials      536      536           
Flag Coverage Δ
unittests 49.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a839ab9...fb5a478. Read the comment docs.

@jbartosik jbartosik force-pushed the CASCL-120-new-ddas-crd branch 3 times, most recently from 8d7bcb7 to 32b049f Compare January 13, 2025 16:14
`operator-sdk create api --group datadoghq --kind DatadogPodAutoscaler --version v2alpha1`
Similar to v1alpah1, with the following changes:
- `spec.policy` renamed to `spec.actuation`
- `spec.policy.applyMode` replaced with `spec.actuation.mode` (allowed values
   `Apply` and `Preview`)
- `spec.policy.upscale.match` and `spec.policy.donwscale.match` removed
- `spec.policy.targets` renamed to `spec.policy.objectives`
- `sepc.policy.contraints.containers[].limits` removed
@jbartosik jbartosik force-pushed the CASCL-120-new-ddas-crd branch from 32b049f to fb5a478 Compare January 13, 2025 16:15
@jbartosik jbartosik marked this pull request as ready for review January 13, 2025 16:33
@jbartosik jbartosik requested a review from a team as a code owner January 13, 2025 16:33
@levan-m levan-m requested a review from a team January 13, 2025 16:37
domain: com
group: datadoghq
kind: DatadogPodAutoscaler
path: github.com/DataDog/datadog-operator/api/datadoghq/v2alpha1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll expose a v2 for GA, so that version will probably v1beta1 or v1alpha2

// It's also possible to selectively deactivate upscale, downscale or update actions thanks to the `Upscale`, `Downscale` and `Update` fields.
// +optional
// +kubebuilder:default=All
Mode DatadogPodAutoscalerActuationMode `json:"applyMode"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Mode DatadogPodAutoscalerActuationMode `json:"applyMode"`
Mode DatadogPodAutoscalerActuationMode `json:"mode"`

Update *DatadogPodAutoscalerUpdatePolicy `json:"update,omitempty"`

// Upscale defines the policy to scale up the target resource.
Upscale *DatadogPodAutoscalerScalingPolicy `json:"upscale,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't there a proposal to change to scaleUp and scaleDown?

type DatadogPodAutoscalerActuation struct {
// Mode determines recommendations that should be applied by the controller:
// - All: Apply all recommendations (regular and manual).
// - Manual: Apply only manual recommendations (recommendations manually validated by user in the Datadog app).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Type DatadogPodAutoscalerTargetValueType `json:"type"`

// Absolute defines the absolute value of the target (for instance 500 millicores).
Absolute *resource.Quantity `json:"absolute,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we need to keep Absolute for future cases but we need to disallow it for multi-dim and vertical scaling.

kind: Kustomization# For each CRD, "Editor" and "Viewer" roles are scaffolded by
# default, aiding admins in cluster management. Those roles are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed as we don't ship the CRD in the operator

@@ -0,0 +1,9 @@
apiVersion: datadoghq.com/v2alpha1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be removed as we don't ship the CRD with the operator

// Actuation defines how recommendations should be applied.
// +optional
// +kubebuilder:default={}
Actuation *DatadogPodAutoscalerActuation `json:"actuation,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not a big fan of presenting the actuation name to customers but LGTM if we don't find anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request qa/skip-qa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants