MON-4413: support MessageVersion v2.0 for remote-write#2977
MON-4413: support MessageVersion v2.0 for remote-write#2977simonpasquier wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@simonpasquier: This pull request references MON-4413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 11 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: de809eda-88a6-4407-93c8-178995765434
📒 Files selected for processing (9)
Documentation/api.mdDocumentation/openshiftdocs/modules/remotewritespec.adocgo.modpkg/manifests/config.gopkg/manifests/config_test.gopkg/manifests/manifests.gopkg/manifests/manifests_test.gopkg/manifests/types.gotest/e2e/prometheus_test.go
| if target.MessageVersion != "" { | ||
| rwConf.MessageVersion = new(monv1.RemoteWriteMessageVersion(target.MessageVersion)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant manifest code and the enum/type definitions referenced by the review comment.
sed -n '3548,3575p' pkg/manifests/manifests.go
printf '\n--- types ---\n'
sed -n '840,915p' pkg/manifests/types.go
printf '\n--- tests ---\n'
sed -n '1440,1510p' pkg/manifests/manifests_test.go
printf '\n--- search for MessageVersion usages ---\n'
rg -n "MessageVersion" pkg/manifests -g'*.go'Repository: openshift/cluster-monitoring-operator
Length of output: 8841
Materialize the enum value before taking its address. new(monv1.RemoteWriteMessageVersion(target.MessageVersion)) does not compile, and the empty-string path leaves MessageVersion nil instead of applying the documented V1.0 default.
Suggested fix
- if target.MessageVersion != "" {
- rwConf.MessageVersion = new(monv1.RemoteWriteMessageVersion(target.MessageVersion))
- }
+ version := monv1.RemoteWriteMessageVersion1_0
+ if target.MessageVersion != "" {
+ version = monv1.RemoteWriteMessageVersion(target.MessageVersion)
+ }
+ rwConf.MessageVersion = &version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if target.MessageVersion != "" { | |
| rwConf.MessageVersion = new(monv1.RemoteWriteMessageVersion(target.MessageVersion)) | |
| version := monv1.RemoteWriteMessageVersion1_0 | |
| if target.MessageVersion != "" { | |
| version = monv1.RemoteWriteMessageVersion(target.MessageVersion) | |
| } | |
| rwConf.MessageVersion = &version |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 23e11d7b-b150-49dc-8855-70fc2112b880
📒 Files selected for processing (9)
Documentation/api.mdDocumentation/openshiftdocs/modules/remotewritespec.adocgo.modpkg/manifests/config.gopkg/manifests/config_test.gopkg/manifests/manifests.gopkg/manifests/manifests_test.gopkg/manifests/types.gotest/e2e/prometheus_test.go
✅ Files skipped from review due to trivial changes (2)
- Documentation/openshiftdocs/modules/remotewritespec.adoc
- Documentation/api.md
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- pkg/manifests/manifests.go
- pkg/manifests/config_test.go
- test/e2e/prometheus_test.go
- pkg/manifests/config.go
- pkg/manifests/types.go
| { | ||
| name: "V1.0 is passed through", | ||
| messageVersion: "V1.0", | ||
| expectedVersion: new(monv1.RemoteWriteMessageVersion1_0), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for the invalid new(...) pattern in this test file:"
rg -n 'new\(monv1\.RemoteWriteMessageVersion[12]_0\)' pkg/manifests/manifests_test.go
echo
echo "Inspecting the enum declarations to confirm these identifiers are values, not types:"
fd '^types\.go$' . | while read -r f; do
rg -n -C2 'RemoteWriteMessageVersion1_0|RemoteWriteMessageVersion2_0|type RemoteWriteMessageVersion' "$f" || true
doneRepository: openshift/cluster-monitoring-operator
Length of output: 454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate the enum declarations:"
rg -n -C3 'type RemoteWriteMessageVersion|RemoteWriteMessageVersion1_0|RemoteWriteMessageVersion2_0' pkg
echo
echo "Show the surrounding lines in the defining file:"
file=$(rg -l 'RemoteWriteMessageVersion1_0|RemoteWriteMessageVersion2_0' pkg | head -n 1)
if [ -n "${file:-}" ]; then
sed -n '1,220p' "$file" | nl -ba | sed -n '/RemoteWriteMessageVersion1_0/,+20p;/RemoteWriteMessageVersion2_0/,+20p;/type RemoteWriteMessageVersion/,+20p'
fiRepository: openshift/cluster-monitoring-operator
Length of output: 1071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search the repo for the enum type and constant declarations:"
rg -n -C2 'type RemoteWriteMessageVersion|RemoteWriteMessageVersion1_0|RemoteWriteMessageVersion2_0' .
echo
echo "If found, show the surrounding lines with plain sed:"
file=$(rg -l 'RemoteWriteMessageVersion1_0|RemoteWriteMessageVersion2_0' . | head -n 1 || true)
if [ -n "${file:-}" ]; then
echo "File: $file"
sed -n '1,260p' "$file"
fiRepository: openshift/cluster-monitoring-operator
Length of output: 14531
new(...) needs a type, not the enum value monv1.RemoteWriteMessageVersion1_0 and monv1.RemoteWriteMessageVersion2_0 are constants, so these two cases won’t compile. Use a typed local variable or ptr.To(...) instead.
There was a problem hiding this comment.
are you sure abut this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Reviewed expressions:"
rg -n -C2 'new\(monv1\.RemoteWriteMessageVersion[12]_0\)' pkg/manifests/manifests_test.go
echo
echo "Resolved dependency declarations/usages:"
rg -n -C3 'type RemoteWriteMessageVersion\b|RemoteWriteMessageVersion1_0\b|RemoteWriteMessageVersion2_0\b' \
vendor $(go env GOPATH 2>/dev/null)/pkg/mod 2>/dev/null || trueLength of output: 6246
@machine424 Yes. The dependency declaration was not present in the available sandbox paths, but the issue follows directly from Go syntax: new accepts a type, whereas RemoteWriteMessageVersion1_0 and RemoteWriteMessageVersion2_0 are enum values/constants.
These expressions therefore fail with an error equivalent to “RemoteWriteMessageVersion1_0 is not a type”:
new(monv1.RemoteWriteMessageVersion1_0)Since ptr is already imported in this file, the straightforward correction is:
expectedVersion: ptr.To(monv1.RemoteWriteMessageVersion1_0),and equivalently for RemoteWriteMessageVersion2_0.
🧬 Code Graph Analysis Results
pkg/manifests/manifests.go
-
Lines 1324-1353 —
(*Factory) SharingConfig(...)func (f *Factory) SharingConfig( promHost, amHost, thanosHost *url.URL, alertmanagerUserWorkloadHost, alertmanagerTenancyHost string, ) *v1.ConfigMap { data := map[string]string{} // Configmap keys need to include "public" to indicate that they are public values. // See https://bugzilla.redhat.com/show_bug.cgi?id=1807100. if promHost != nil { data["prometheusPublicURL"] = fmt.Sprintf("%s://%s", promHost.Scheme, promHost.Host) } if amHost != nil { data["alertmanagerPublicURL"] = fmt.Sprintf("%s://%s", amHost.Scheme, amHost.Host) } if thanosHost != nil { data["thanosPublicURL"] = fmt.Sprintf("%s://%s", thanosHost.Scheme, thanosHost.Host) } data["alertmanagerUserWorkloadHost"] = alertmanagerUserWorkloadHost data["alertmanagerTenancyHost"] = alertmanagerTenancyHost return &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: sharedConfigMap, Namespace: configManagedNamespace, }, Data: data, } }
-
Lines 2215-2265 —
(*Factory) PrometheusOperatorAdmissionWebhookDeployment()func (f *Factory) PrometheusOperatorAdmissionWebhookDeployment() (*appsv1.Deployment, error) { d, err := f.NewDeployment(f.assets.MustNewAssetSlice(AdmissionWebhookDeployment)) if err != nil { return nil, err } if len(f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.NodeSelector) > 0 { d.Spec.Template.Spec.NodeSelector = f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.NodeSelector } if len(f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.Tolerations) > 0 { d.Spec.Template.Spec.Tolerations = f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.Tolerations } if len(f.config.ClusterMonitoringConfiguration.PrometheusOperatorAdmissionWebhookConfig.TopologySpreadConstraints) > 0 { d.Spec.Template.Spec.TopologySpreadConstraints = f.config.ClusterMonitoringConfiguration.PrometheusOperatorAdmissionWebhookConfig.TopologySpreadConstraints } for i, container := range d.Spec.Template.Spec.Containers { switch container.Name { case "prometheus-operator-admission-webhook": d.Spec.Template.Spec.Containers[i].Image = f.config.Images.PrometheusOperatorAdmissionWebhook if f.config.ClusterMonitoringConfiguration.PrometheusOperatorAdmissionWebhookConfig.Resources != nil { d.Spec.Template.Spec.Containers[i].Resources = *f.config.ClusterMonitoringConfiguration.PrometheusOperatorAdmissionWebhookConfig.Resources } args := d.Spec.Template.Spec.Containers[i].Args if f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.LogLevel != "" { args = append(args, fmt.Sprintf("--log-level=%s", f.config.ClusterMonitoringConfiguration.PrometheusOperatorConfig.LogLevel)) } // The admission webhook supports only TLS versions >= 1.2. tlsVersionEnforcer := &minTLSVersionEnforcer{ atLeast: tls.VersionTLS12, inner: f.APIServerConfig, } args = f.setTLSSecurityConfigurationWithMinTLSVersion( args, PrometheusOperatorWebTLSCipherSuitesFlag, PrometheusOperatorWebTLSMinTLSVersionFlag, tlsVersionEnforcer, ) d.Spec.Template.Spec.Containers[i].Args = args } } d.Namespace = f.namespace return d, nil }
-
Lines 2391-2398 —
(*minTLSVersionEnforcer) MinTLSVersion()func (m *minTLSVersionEnforcer) MinTLSVersion() string { v := m.inner.MinTLSVersion() if crypto.TLSVersionOrDie(v) < m.atLeast { return crypto.TLSVersionToNameOrDie(m.atLeast) } return v }
-
Lines 1214-1257 —
(*Factory) ThanosRulerAlertmanagerConfigSecret()func (f *Factory) ThanosRulerAlertmanagerConfigSecret() (*v1.Secret, error) { s, err := f.NewSecret(f.assets.MustNewAssetSlice(ThanosRulerAlertmanagerConfigSecret)) if err != nil { return nil, err } var alertingConfiguration thanosAlertingConfiguration err = yaml2.Unmarshal([]byte(s.StringData["alertmanagers.yaml"]), &alertingConfiguration) if err != nil { return nil, err } if f.config.UserWorkloadConfiguration.Alertmanager.Enabled { alertingConfiguration.Alertmanagers[0].HTTPConfig.TLSConfig.ServerName = fmt.Sprintf( "%s.%s.svc", userWorkloadAlertmanagerService, f.namespaceUserWorkload, ) alertingConfiguration.Alertmanagers[0].StaticConfigs = []string{ fmt.Sprintf( "dnssrv+_web._tcp.alertmanager-operated.%s.svc", f.namespaceUserWorkload, ), } } else if !f.config.ClusterMonitoringConfiguration.AlertmanagerMainConfig.IsEnabled() { alertingConfiguration.Alertmanagers = []thanosAlertmanagerConfiguration{} } additionalConfigs, err := f.ConvertToThanosAlertmanagerConfiguration(f.config.GetThanosRulerAlertmanagerConfigs()) if err != nil { return nil, err } alertingConfiguration.Alertmanagers = append(alertingConfiguration.Alertmanagers, additionalConfigs...) b, err := yaml2.Marshal(alertingConfiguration) if err != nil { return nil, err } s.StringData["alertmanagers.yaml"] = string(b) return s, nil }
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@simonpasquier: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-techpreview |
danielmellado
left a comment
danielmellado
left a comment
There was a problem hiding this comment.
Left some comments, thanks!
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e4b61cb4-f2b1-4abf-9bcc-5d41356f4a53
📒 Files selected for processing (2)
test/e2e/framework/prometheus.gotest/e2e/prometheus_test.go
| CommonPrometheusFields: monitoringv1.CommonPrometheusFields{ | ||
| Image: image, | ||
| Replicas: &replicas, | ||
| Replicas: new(int32(1)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# This must return no matches after the fix.
rg -n 'new\(int32\(1\)\)' test/e2e/framework/prometheus.goRepository: openshift/cluster-monitoring-operator
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' test/e2e/framework/prometheus.goRepository: openshift/cluster-monitoring-operator
Length of output: 3706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
p = Path('test/e2e/framework/prometheus.go')
text = p.read_text()
print('ptr import present:', 'ptr "' in text or '\tptr' in text or re.search(r'\bptr\b', text) is not None)
print('new(int32(1)) present:', 'new(int32(1))' in text)
PYRepository: openshift/cluster-monitoring-operator
Length of output: 228
Use ptr.To(int32(1)) for Replicas new takes a type, so new(int32(1)) is invalid Go syntax. ptr.To(int32(1)) is already imported and fits here.
Proposed fix
- Replicas: new(int32(1)),
+ Replicas: ptr.To(int32(1)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Replicas: new(int32(1)), | |
| Replicas: ptr.To(int32(1)), |
There was a problem hiding this comment.
are you really sure about this??
There was a problem hiding this comment.
@machine424 Yes — the concern is valid. I should have been more precise in the wording: new takes a type, so new(int32) would be valid (but yields a zero-valued *int32). new(int32(1)) is invalid because int32(1) is a value conversion expression, not a type.
For the required value of 1, either of these is valid:
Replicas: ptr.To(int32(1)),replicas := int32(1)
Replicas: &replicas,Since ptr is already imported and used in this file, ptr.To(int32(1)) is the concise appropriate replacement.
|
/lgtm |
|
Thanks Simon ;) |
|
/retest-required |
machine424
left a comment
machine424
left a comment
There was a problem hiding this comment.
thanks, just some propositions.
| }, | ||
| { | ||
| name: "cluster monitoring: V1.0 is valid", | ||
| config: "prometheusK8s:\n remoteWrite:\n - url: http://example.com\n messageVersion: V1.0\n", |
There was a problem hiding this comment.
nit: could we format these as multiline strings instead? It would make them more readable for humans :)
| SafeAuthorization: *target.Authorization, | ||
| } | ||
| } | ||
| if target.MessageVersion != "" { |
There was a problem hiding this comment.
yes, I think CMO should enforce the default version value in case prometheus-operator starts using v2 as the default one or if prometheus changes the default value to v2.
There was a problem hiding this comment.
Right now, the operator assumes no default and delegates to Prometheus. I assume that changing the default in Prometheus will only happen in a new major version at which time we would make the proper decision anyway.
There was a problem hiding this comment.
the operator assumes no default
This isn't quite right: it needs to rely on the Prometheus default value to make some decisions anyway though it won't explicitly set the parameter in the generated configuration if unset in the resource.
There was a problem hiding this comment.
I see, but I still think that being explicit on CMO side (like we already do for other defaults) is safer and should cost nothing.
| { | ||
| name: "V1.0 is passed through", | ||
| messageVersion: "V1.0", | ||
| expectedVersion: new(monv1.RemoteWriteMessageVersion1_0), |
There was a problem hiding this comment.
are you sure abut this?
| CommonPrometheusFields: monitoringv1.CommonPrometheusFields{ | ||
| Image: image, | ||
| Replicas: &replicas, | ||
| Replicas: new(int32(1)), |
There was a problem hiding this comment.
are you really sure about this??
| module github.com/openshift/cluster-monitoring-operator | ||
|
|
||
| go 1.25.0 | ||
| go 1.26.0 |
There was a problem hiding this comment.
I assume this was needed for some new syntax or sth :)
There was a problem hiding this comment.
yep and coding assistants are confused by the new syntax as they lack 1.26 knowledge :)
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
| // WaitForPrometheusUpdate reads the current generation from the given | ||
| // Prometheus resource and polls the Kubernetes API until the resource's | ||
| // generation is incremented and it matches with the status observedGeneration. | ||
| func (f Framework) WaitForPrometheusUpdate(ctx context.Context, p *monitoringv1.Prometheus) error { |
There was a problem hiding this comment.
I assume this is only needed for a "clearer failure message" (even though the more explicit would be in the pods logs).
Otherwise the config propagation delay would be absorbed into the remoteWriteCheckMetrics 6 min wait window.
(I like the idea of WaitForPrometheusUpdate's generations comparisons, I may use it for a new test )
There was a problem hiding this comment.
correct this is to distinguish between "the CMO config couldn't be applied" (for instance a secret ref is invalid) vs. "the config is applied but it didn't produce the expected result".
machine424
left a comment
machine424
left a comment
There was a problem hiding this comment.
/lgtm
/hold
we can always follow up.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielmellado, machine424, simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@simonpasquier: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-hypershift-conformance |
2 similar comments
|
/test e2e-hypershift-conformance |
|
/test e2e-hypershift-conformance |
|
/test e2e-hypershift-conformance |
|
@simonpasquier: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test images |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-aws-ovn-techpreview |
4 participants
No description provided.