OCPBUGS-60093: fix(upsert): add desired-state hash to detect spec field removals - #7713
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
muraee
changed the title
|
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:
📝 WalkthroughWalkthroughThis pull request adds a desired-state hash annotation to manifest creation and updates. Reconciliation compares stored and computed hashes, detects label removals, and falls back to normalized DeepDerivative comparison. Normalization ignores volatile metadata, status, and the hash annotation. Tests cover migration, idempotence, update detection, loop detection, and comparison filtering. Sequence Diagram(s)sequenceDiagram
participant ApplyManifest
participant Upsert
participant KubernetesAPI
ApplyManifest->>Upsert: compute and store desired-state hash
Upsert->>KubernetesAPI: create or fetch object
Upsert->>Upsert: compare hashes and normalized desired state
Upsert->>KubernetesAPI: update when reconciliation detects drift
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request explicitly references no jira issue. 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
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/AROSwift/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml`:
- Around line 6-7: The last-applied-configuration annotation on the Secret
(annotation key hypershift.openshift.io/last-applied-configuration for Secret
name azure-cloud-config) contains embedded sensitive Secret data; remove that
annotation or replace its value with a non-sensitive placeholder in the fixture
so secret material is not stored in metadata. Locate the Secret YAML containing
the hypershift.openshift.io/last-applied-configuration annotation and either
delete the entire annotation entry or replace the embedded "cloud.conf" blob
with a sanitized placeholder (e.g., "<redacted>" or an empty object) and ensure
ownerReferences/metadata remain intact; for future fixtures prefer using
Server-Side Apply/managedFields or explicit, non-sensitive test values instead
of embedding real secret contents.
There was a problem hiding this comment.
This appears to be an extremely costly update to a large fleet that is already being stressed with etcd size explosion caused by HyperShift node pools. I don't think IBM Cloud can afford having hypershift.openshift.io/last-applied-configuration added. I would like to discuss with you all at our interlock meeting next week.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
muraee
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
edede18 to
5fb21f8
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by mgencur |
openshift-ci-robot
added
the
verified
|
@mgencur: 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. |
|
@muraee: The following tests failed, say
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. |
|
/retest |
1 similar comment
|
/retest |
9e00898
into
openshift:main
|
@muraee: Jira Issue OCPBUGS-60093: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-60093 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
|
/cherry-pick release-4.22 release-4.21 release-4.20 |
|
@muraee: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked. Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}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 kubernetes-sigs/prow repository. |
|
/jira backport release-4.22 |
|
@muraee: The following backport issues have been created:
Queuing cherrypicks to the requested branches to be created after this PR merges: 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-robot: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked. Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}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 kubernetes-sigs/prow repository. |
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR #7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add E2E v2 test covering the desired-state-hash annotation introduced in support/upsert.ApplyManifest (PR openshift#7713): - DesiredStateHashAnnotationTest: asserts every managed Deployment in the control plane namespace carries a valid 64-char hex desired-state-hash annotation
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add E2E v2 test covering the desired-state-hash annotation introduced in support/upsert.ApplyManifest (PR openshift#7713): - DesiredStateHashAnnotationTest: asserts every managed Deployment in the control plane namespace carries a valid 64-char hex desired-state-hash annotation
7 participants
Summary
hypershift.openshift.io/desired-state-hashannotation containing a SHA-256 hash of the desired manifest stateDeepDerivativecannot detect (field removals, trailing slice items, zero-value clearing)DeepDerivativeas a fallback for drift detection (external modifications) and migration (objects without the annotation)Details
DeepDerivativetreats nil/empty fields as "don't care", making it impossible to detect when spec fields likenodeSelector,tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879).This replaces the earlier full-JSON
last-applied-configurationapproach which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash (~64 bytes) has none of these costs.The existing label-count removal workaround is preserved since hash-based detection cannot drive three-way metadata merges.
Test plan
toUnstructuredfield strippingMachineTemplateBuilderstest updated to account for hash annotation🤖 Generated with Claude Code
Summary by CodeRabbit