Skip to content

Monitoring API: remove maximum retention days prometheus #2851

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.

Sign up for GitHub

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

Open
marioferh wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Monitoring API: remove maximum retention days prometheus #2851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a821031
Use durationInHours for Prometheus retention and tune limits
marioferh May 28, 2026
0486146
Use Prometheus Operator retention strings in ClusterMonitoring API
marioferh May 29, 2026
13d2859
Clarify retention duration semantics and fix tombstone comments
marioferh May 29, 2026
86b8e12
Fix retention API schema compatibility and validation tests
marioferh May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,27 +390,6 @@ tests:
transitKey: my-key
expectedError: "appRole config is required when authentication type is AppRole"

# transitMount required field validation
- name: Should reject Vault KMS config without transitMount
initial: |
apiVersion: config.openshift.io/v1
kind: APIServer
spec:
encryption:
type: KMS
kms:
type: Vault
vault:
kmsPluginImage: registry.example.com/vault-plugin@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
vaultAddress: https://vault.example.com:8200
authentication:
type: AppRole
appRole:
secret:
name: vault-approle
transitKey: my-key
expectedError: "Required value"

# kmsPluginImage validation tests
- name: Should reject kmsPluginImage with tag instead of digest
initial: |
Expand Down
14 changes: 9 additions & 5 deletions config/v1/types_kmsencryption.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,18 +193,22 @@ type VaultKMSPluginConfig struct {

// transitMount specifies the mount path of the Vault Transit engine.
//
// The transit mount must be between 1 and 1024 characters, cannot start or
// end with a forward slash, cannot contain consecutive forward slashes, and
// must only contain RFC 3986 unreserved characters (alphanumeric, hyphen,
// period, underscore, tilde) and forward slashes as path separators.
// When omitted, this means the user has no opinion and the platform is left
// to choose a reasonable default. These defaults are subject to change over time.
// The current default is "transit".
//
// The transit mount must be between 1 and 1024 characters when specified, cannot start or
// end with a forward slash, cannot contain consecutive forward slashes, and must only contain
// RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward
// slashes as path separators.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:XValidation:rule="!self.startsWith('/')",message="transitMount cannot start with a forward slash"
// +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="transitMount cannot end with a forward slash"
// +kubebuilder:validation:XValidation:rule="!self.contains('//')",message="transitMount cannot contain consecutive forward slashes"
// +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9._~/-]+$')",message="transitMount must only contain RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward slashes"
// +required
// +optional
TransitMount string `json:"transitMount,omitempty"`

// transitKey specifies the name of the encryption key in Vault's Transit engine.
Expand Down
13 changes: 8 additions & 5 deletions ...zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ spec:
description: |-
transitMount specifies the mount path of the Vault Transit engine.

The transit mount must be between 1 and 1024 characters, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and
must only contain RFC 3986 unreserved characters (alphanumeric, hyphen,
period, underscore, tilde) and forward slashes as path separators.
When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. These defaults are subject to change over time.
The current default is "transit".

The transit mount must be between 1 and 1024 characters when specified, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and must only contain
RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward
slashes as path separators.
maxLength: 1024
minLength: 1
type: string
Expand Down Expand Up @@ -419,7 +423,6 @@ spec:
- authentication
- kmsPluginImage
- transitKey
- transitMount
- vaultAddress
type: object
required:
Expand Down
13 changes: 8 additions & 5 deletions ...enerated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ spec:
description: |-
transitMount specifies the mount path of the Vault Transit engine.

The transit mount must be between 1 and 1024 characters, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and
must only contain RFC 3986 unreserved characters (alphanumeric, hyphen,
period, underscore, tilde) and forward slashes as path separators.
When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. These defaults are subject to change over time.
The current default is "transit".

The transit mount must be between 1 and 1024 characters when specified, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and must only contain
RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward
slashes as path separators.
maxLength: 1024
minLength: 1
type: string
Expand Down Expand Up @@ -419,7 +423,6 @@ spec:
- authentication
- kmsPluginImage
- transitKey
- transitMount
- vaultAddress
type: object
required:
Expand Down
13 changes: 8 additions & 5 deletions ...nerated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ spec:
description: |-
transitMount specifies the mount path of the Vault Transit engine.

The transit mount must be between 1 and 1024 characters, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and
must only contain RFC 3986 unreserved characters (alphanumeric, hyphen,
period, underscore, tilde) and forward slashes as path separators.
When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. These defaults are subject to change over time.
The current default is "transit".

The transit mount must be between 1 and 1024 characters when specified, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and must only contain
RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward
slashes as path separators.
maxLength: 1024
minLength: 1
type: string
Expand Down Expand Up @@ -419,7 +423,6 @@ spec:
- authentication
- kmsPluginImage
- transitKey
- transitMount
- vaultAddress
type: object
required:
Expand Down
13 changes: 8 additions & 5 deletions ...zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ spec:
description: |-
transitMount specifies the mount path of the Vault Transit engine.

The transit mount must be between 1 and 1024 characters, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and
must only contain RFC 3986 unreserved characters (alphanumeric, hyphen,
period, underscore, tilde) and forward slashes as path separators.
When omitted, this means the user has no opinion and the platform is left
to choose a reasonable default. These defaults are subject to change over time.
The current default is "transit".

The transit mount must be between 1 and 1024 characters when specified, cannot start or
end with a forward slash, cannot contain consecutive forward slashes, and must only contain
RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward
slashes as path separators.
maxLength: 1024
minLength: 1
type: string
Expand Down Expand Up @@ -419,7 +423,6 @@ spec:
- authentication
- kmsPluginImage
- transitKey
- transitMount
- vaultAddress
type: object
required:
Expand Down
2 changes: 1 addition & 1 deletion config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

84 changes: 84 additions & 0 deletions config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,90 @@ tests:
request: "100m"
limit: "0"
expectedError: 'spec.nodeExporterConfig.resources[0].limit: Invalid value: "": limit must be a positive, non-zero quantity'
- name: Should accept prometheusConfig retention duration
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
duration: "15h"
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
duration: "15h"
- name: Should accept prometheusConfig retention size
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
size: "500MiB"
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
size: "500MiB"
- name: Should reject invalid prometheusConfig retention duration
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
duration: "15days"
expectedError: 'spec.prometheusConfig.retention.duration: Invalid value: "string": must be a valid Prometheus duration string'
- name: Should reject invalid prometheusConfig retention size
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
size: "500Mi"
expectedError: 'spec.prometheusConfig.retention.size: Invalid value: "string": must be a valid Prometheus byte-size string'
- name: Should reject prometheusConfig retention with both durationInDays and duration
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
durationInDays: 15
duration: "15d"
expectedError: 'durationInDays and duration cannot both be set'
- name: Should reject prometheusConfig retention with both sizeInGiB and size
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
prometheusConfig:
retention:
sizeInGiB: 500
size: "500GiB"
expectedError: 'sizeInGiB and size cannot both be set'
onUpdate:
- name: Should reject updating TelemeterClientConfig to empty object
initial: |
Expand Down
40 changes: 39 additions & 1 deletion config/v1alpha1/types_cluster_monitoring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,7 @@ type PrometheusConfig struct {
// +kubebuilder:validation:MinItems=1
Resources []ContainerResource `json:"resources,omitempty"`
// retention configures how long Prometheus retains metrics data and how much storage it can use.
// When omitted, the platform chooses reasonable defaults (currently 15 days retention, no size limit).
// When omitted, the platform chooses reasonable defaults (currently 15d retention, no size limit).
// +optional
Retention Retention `json:"retention,omitempty,omitzero"`
// tolerations defines tolerations for the pods.
Expand Down Expand Up @@ -2234,8 +2234,11 @@ type SecretKeySelector struct {

// Retention configures how long Prometheus retains metrics data and how much storage it can use.
// +kubebuilder:validation:MinProperties=1
// +kubebuilder:validation:XValidation:rule="!has(self.durationInDays) || !has(self.duration)",message="durationInDays and duration cannot both be set"
// +kubebuilder:validation:XValidation:rule="!has(self.sizeInGiB) || !has(self.size)",message="sizeInGiB and size cannot both be set"
type Retention struct {
// durationInDays specifies how many days Prometheus will retain metrics data.
// Deprecated: use duration instead. durationInDays and duration cannot both be set.
// Prometheus automatically deletes data older than this duration.
// When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.
// The default value is 15.
Expand All @@ -2245,8 +2248,10 @@ type Retention struct {
// +kubebuilder:validation:Maximum=365
// +optional
DurationInDays int32 `json:"durationInDays,omitempty"`

// sizeInGiB specifies the maximum storage size in gibibytes (GiB) that Prometheus
// can use for data blocks and the write-ahead log (WAL).
// Deprecated: use size instead. sizeInGiB and size cannot both be set.
// When the limit is reached, Prometheus will delete oldest data first.
// When omitted, no size limit is enforced and Prometheus uses available PersistentVolume capacity.
// Minimum value is 1 GiB.
Expand All @@ -2255,6 +2260,39 @@ type Retention struct {
// +kubebuilder:validation:Maximum=16384
// +optional
SizeInGiB int32 `json:"sizeInGiB,omitempty"`

// duration is an optional field that specifies how long Prometheus retains metrics data.
// Valid values are Prometheus duration strings composed of non-negative integer components
// with unit suffixes y, w, d, h, m, s, or ms (for example, "15d", "24h", "15h").
// Single-unit forms such as "15d" or "24h" are recommended over composite durations with
// zero-valued components (for example, "0y5d"), which are redundant but valid upstream.
// Must be at least 1 character and at most 64 characters.
// When set to "0", time-based retention is disabled. Other zero-duration forms such as "0d",
// "0h", or "0y" are semantically equivalent in Prometheus parsing, but "0" is the canonical
// documented form for disabling time-based retention.
// Prometheus automatically deletes data older than this duration.
// When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.
// The current default value is `15d`.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=64
// +kubebuilder:validation:XValidation:rule=`self.matches('^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$')`,message="must be a valid Prometheus duration string"
Copy link
Copy Markdown
Contributor

@everettraven everettraven May 29, 2026

Choose a reason for hiding this comment

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

Is setting this field to 0 any different than setting it to 0y or 0d?

Should we require that if you are not disabling retention all of your units must specify a value greater than 0?

For example, 0y10m5d1h30m doesn't really make sense to have the 0y to me.

Copy link
Copy Markdown
Contributor Author

@marioferh marioferh May 29, 2026

Choose a reason for hiding this comment

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

"0", "0d", and "0y" are semantically equivalent (zero duration) in Prometheus parsing. We intentionally use the same validation pattern as the Prometheus Operator retention field so CMO can pass values through unchanged.

Upstream does not reject zero-valued components like 0y in composite durations, so we are not adding stricter admission rules here. We document "0" as the canonical way to disable time-based retention and recommend single-unit forms like "15d" or "24h" in field comments, but validation matches upstream.

Note: composite durations must follow the fixed unit order (y→w→d→h→m→s→ms), so values like 0y10m5d are invalid; valid examples with zero components include 0y5d1h30m or 0d15h.

Copy link
Copy Markdown
Contributor Author

@marioferh marioferh May 29, 2026

Choose a reason for hiding this comment

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

"0", "0d", and "0y" are semantically equivalent (zero duration) in Prometheus parsing. I intentionally use the same validation pattern as the Prometheus Operator retention field so CMO can pass values through unchanged.
I can add a comment if it is neccesary

// +optional
Duration string `json:"duration,omitempty"`

// size is an optional field that specifies the maximum storage size that Prometheus
// can use for data blocks and the write-ahead log (WAL).
// Valid values are Prometheus byte-size strings with an optional decimal prefix and a
// unit suffix B, KB, MB, GB, TB, EB, PB, or their binary equivalents KiB, MiB, GiB, TiB, EiB, PiB
// (for example, "500MiB", "10GiB").
// Must be at least 1 character and at most 32 characters.
// When set to "0", no size limit is enforced.
// When the limit is reached, Prometheus deletes oldest data first.
// When omitted, no size limit is enforced and Prometheus uses available PersistentVolume capacity.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=32
// +kubebuilder:validation:XValidation:rule=`self.matches('^(0|([0-9]*[.])?[0-9]+((K|M|G|T|E|P)i?)?B)$')`,message="must be a valid Prometheus byte-size string"
Copy link
Copy Markdown
Contributor

@everettraven everettraven May 29, 2026

Choose a reason for hiding this comment

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

Similar question here, if you aren't setting this to 0, should you be required to specify a non-zero value for a unit?

Copy link
Copy Markdown
Contributor Author

@marioferh marioferh May 29, 2026

Choose a reason for hiding this comment

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

Prometheus Operator’s ByteSize pattern admits 500B ,the scale part is optional, you just need the trailing B. I copied that on purpose so retention.size can go straight through to retentionSize on the Prometheus CR without conversion.
If it is needed to be more strict I can change the CEL expression even if diverge from the other CR

// +optional
Size string `json:"size,omitempty"`
}

// RelabelAction defines the action to perform in a relabeling rule.
Expand Down
Loading