[3.3] AutoOps: Ignore deprecated ES clusters (#9008)#9013
[3.3] AutoOps: Ignore deprecated ES clusters (#9008)#9013naemono merged 1 commit intoelastic:3.3from
Conversation
* AutoOps: Ignore deprecate ES clusters --------- Signed-off-by: Michael Montgomery <mmontg1@gmail.com> (cherry picked from commit a9d7a46)
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
naemono
requested review from
barkbay,
Copilot,
kvalliyurnatt,
moukoublen,
pebrc,
pkoutsovasilis and
rhr323
There was a problem hiding this comment.
Pull request overview
This is a backport from main to branch 3.3 that adds logic to skip deprecated Elasticsearch cluster versions in the AutoOps controller. When an ES cluster with a deprecated version (7.0.0 to 7.16.0) matches the AutoOps policy selector, it will be logged and ignored rather than having AutoOps agent deployments created for it.
Key Changes
- Added version parsing and validation to check if ES clusters have deprecated versions before reconciling AutoOps resources
- Improved log messages by removing redundant context (es_namespace and es_name) that's already in the logger
- Added test coverage for the deprecated version scenario and improved existing test fixtures with version fields
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controller/autoops/reconcile.go | Added deprecated version check logic to skip ES clusters with versions 7.0.0-7.16.0, and cleaned up log messages to remove redundant context |
| pkg/controller/autoops/reconcile_test.go | Added test case for deprecated ES version handling, added missing version fields to existing test fixtures, and improved test assertion error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // No error means the version is within the deprecated range, so we skip the cluster. | ||
| // We do not adjust the status to indicate this issue at this time, as the status object | ||
| // does not currently support a status per-cluster. | ||
| if version.DeprecatedVersions.WithinRange(esVersion) == nil { | ||
| log.Info("Skipping ES cluster because of deprecated version", "version", es.Spec.Version) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Deprecated ES clusters are included in the accessibleClusters list passed to cleanupOrphanedResourcesForPolicy, which prevents their associated resources from being cleaned up. However, these clusters are then skipped during reconciliation (line 150 continue), so no resources are actually created or maintained for them. This creates an inconsistency where old resources for deprecated clusters may remain in the cluster without being managed or cleaned up. Consider filtering deprecated clusters before passing to cleanupOrphanedResourcesForPolicy.
| // We do not adjust the status to indicate this issue at this time, as the status object | ||
| // does not currently support a status per-cluster. | ||
| if version.DeprecatedVersions.WithinRange(esVersion) == nil { | ||
| log.Info("Skipping ES cluster because of deprecated version", "version", es.Spec.Version) |
There was a problem hiding this comment.
When all matching ES clusters are deprecated, the status is left with Resources counted but no Phase set. This creates an ambiguous state where Resources=1, Ready=0, Errors=0, but Phase="". Consider setting an appropriate phase (or documenting this as expected behavior) to make the status more meaningful to users. For comparison, when resources exist but are not ready, the phase is explicitly set to ResourcesNotReadyPhase.
| // We do not adjust the status to indicate this issue at this time, as the status object | |
| // does not currently support a status per-cluster. | |
| if version.DeprecatedVersions.WithinRange(esVersion) == nil { | |
| log.Info("Skipping ES cluster because of deprecated version", "version", es.Spec.Version) | |
| // We do not adjust the status to indicate this issue at this time on a per-cluster basis, | |
| // but we do update the overall phase to indicate that resources are not in a usable state. | |
| if version.DeprecatedVersions.WithinRange(esVersion) == nil { | |
| log.Info("Skipping ES cluster because of deprecated version", "version", es.Spec.Version) | |
| state.UpdateWithPhase(autoopsv1alpha1.ResourcesNotReadyPhase) |
There was a problem hiding this comment.
this issue will likely be addressed when the extended status reporting issue is resolved/tackled
Backport
This will backport the following commits from
mainto3.3:Questions ?
Please refer to the Backport tool documentation