[WIP] improve: related resource reference change#3425
Draft
csviri wants to merge 18 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves handling of secondary→primary reference changes by extending the primary-to-secondary index update semantics, adding targeted tests (unit + integration), and introducing a new sample scenario demonstrating reconciliations when a secondary’s referenced primary set changes.
Changes:
- Updated the informer eventing/indexing pipeline so updates can reconcile both newly referenced and no-longer-referenced primaries.
- Added new core tests covering narrowing/moving references across updates.
- Added a new operator-framework integration test + sample resources demonstrating subset reference changes.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/TargetStatus.java | Adds a simple status model used by the sample primary CR. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/TargetReconciler.java | Sample reconciler driven by a referencing secondary CR via an informer event source. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/TargetCustomResource.java | Sample primary custom resource type for the new scenario. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/SecondaryToPrimaryReferenceChangeIT.java | New IT demonstrating subset reference changes and expected reconciliation behavior. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigToTargetMapper.java | Secondary→primary mapper for the sample that maps config.targetNames to targets. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigSpec.java | Spec model for the sample secondary CR, including targetNames and value. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigCustomResource.java | Sample secondary custom resource type. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/PrimaryToSecondaryIndexTest.java | Adds tests verifying index updates remove obsolete primaries on update. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java | Updates mocks/verifications for new index method signatures and delete behavior. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/SecondaryToPrimaryMapper.java | Updates mapper Javadoc wording. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/PrimaryToSecondaryIndex.java | Changes index callbacks to return affected primary IDs. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/NOOPPrimaryToSecondaryIndex.java | Adapts NOOP implementation to new return types. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java | Uses index-returned primaries to drive reconciliation propagation and unions old/new mappings. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/DefaultPrimaryToSecondaryIndex.java | Tracks and removes obsolete primaries on update; returns affected primaries for reconciliation. |
| docs/content/en/docs/documentation/eventing.md | Documents secondary→primary mapping behavior (currently inconsistent with the API). |
Comment on lines
186
to
190
| private synchronized void onAddOrUpdate(ResourceAction action, R newObject, R oldObject) { | ||
| primaryToSecondaryIndex.onAddOrUpdate(newObject); | ||
| var primaryIds = primaryToSecondaryIndex.onAddOrUpdate(newObject, oldObject); | ||
| var resourceID = ResourceID.fromResource(newObject); | ||
|
|
||
| var resultEvent = temporaryResourceCache.onAddOrUpdateEvent(action, newObject, oldObject); |
Comment on lines
126
to
+134
| var resultEvent = | ||
| temporaryResourceCache.onDeleteEvent(resource, deletedFinalStateUnknown); | ||
| if (resultEvent.isEmpty()) { | ||
| return; | ||
| } | ||
| primaryToSecondaryIndex.onDelete(resource); | ||
| var primaryIds = primaryToSecondaryIndex.onDelete(resource); | ||
| if (eventAcceptedByFilter( | ||
| ResourceAction.DELETED, resource, null, deletedFinalStateUnknown)) { | ||
| propagateEvent(resource); | ||
| propagateEvent(resource, null, primaryIds); |
Comment on lines
+148
to
154
| Set<ResourceID> primaryIds = null; | ||
| if (action == ResourceAction.DELETED) { | ||
| log.debug( | ||
| "handleEvent: removing from primaryToSecondaryIndex. id={}", | ||
| ResourceID.fromResource(resource)); | ||
| primaryToSecondaryIndex.onDelete(resource); | ||
| primaryIds = primaryToSecondaryIndex.onDelete(resource); | ||
| } |
Comment on lines
+35
to
+37
| * @return set of primary resource IDs to enqueue for reconciliation; an empty set means the event | ||
| * is irrelevant and no reconciliation is triggered. This is called also the old and the new | ||
| * resources, in case of an update. |
Comment on lines
+25
to
+28
| /** | ||
| * Secondary resource that references a {@link TargetCustomResource} via {@code spec.targetName} and | ||
| * serves as input for it. | ||
| */ |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
csviri
changed the title
Comment on lines
+66
to
+68
| // The framework keeps the primary-to-secondary index up to date on reference changes, so a | ||
| // config is only associated with the target it currently references. We take the value from | ||
| // the referencing config, or fall back to the default when none references this target. |
Comment on lines
+31
to
+37
| * Maps a secondary resource to the set of primary resources that should be reconciled in | ||
| * response. | ||
| * | ||
| * @param resource the secondary resource for which an event was received | ||
| * @return set of primary resource IDs to enqueue for reconciliation; an empty set means the event | ||
| * is irrelevant and no reconciliation is triggered. This is called also the old and the new | ||
| * resources, in case of an update. |
| protected void propagateEvent(R object) { | ||
| var primaryResourceIdSet = | ||
| configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object); | ||
| void propagateEvent(R resource, R oldResource, Set<ResourceID> primaryResourceIdSet) { |
Comment on lines
+206
to
+210
| if (primaryResourceIdSet == null) { | ||
| primaryResourceIdSet = new HashSet<>(); | ||
| primaryResourceIdSet.addAll( | ||
| configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(resource)); | ||
| if (oldResource != null) { |
csviri
changed the title
…urce This might needed in some corner cases where might help with optimizations to which resource to trigger. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
csviri
force-pushed
the
strict-primary-to-secondary-index
branch
from
8170175 to
74c83aa
Compare
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Comment on lines
35
to
44
| @Override | ||
| public void onAddOrUpdate(R resource) { | ||
| // empty method because of noop implementation | ||
| public Set<ResourceID> onAddOrUpdate(R resource, R oldResource) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void onDelete(R resource) { | ||
| public Set<ResourceID> onDelete(R resource) { | ||
| // empty method because of noop implementation | ||
| return null; | ||
| } |
Comment on lines
+206
to
+214
| if (primaryResourceIdSet == null) { | ||
| primaryResourceIdSet = new HashSet<>(); | ||
| primaryResourceIdSet.addAll( | ||
| configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(resource)); | ||
| if (oldResource != null) { | ||
| primaryResourceIdSet.addAll( | ||
| configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(oldResource)); | ||
| } | ||
| } |
| protected void propagateEvent(R object) { | ||
| var primaryResourceIdSet = | ||
| configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object); | ||
| void propagateEvent(R resource, R oldResource, Set<ResourceID> primaryResourceIdSet) { |
Comment on lines
+35
to
+37
| * @return set of primary resource IDs to enqueue for reconciliation; an empty set means the event | ||
| * is irrelevant and no reconciliation is triggered. This is called also the old and the new | ||
| * resources, in case of an update. |
Contributor
|
I updated my reproducer and it works as expected (dvob/java-operator-sdk-repo-example@1ce1404). Also I like that with this approach we no longer need a special wiring. @csviri thanks a lot for your effort! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When there is a resource that references (not owner reference) one or more custom resources (see sample), and the reference changes, we want to trigger reconciliation for both old and new resources referenced.
This PR fixes it by calling also the SeondaryToPrimary mapper twice (also for old resource) on update operation.
It adjusts also
PrimaryToSecondaryIndexso when a reference changes it removed from the secondary from the old primary. That is a more strict, but correct behavior.We did not have this issue for now, since usually the reference for (typically) "config resources" that serve as an input for the reconciliation (among the spec) is usually is other way around.
Note that for this use case
PrimaryToSecondaryMapperis NOT needed, since basically this is very similar to classic owner reference scenario.TODO: