Skip to content
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

fix: explicit workflow invocation uses the same resource intance that reconcile api #2686

Merged
merged 3 commits into from
Feb 14, 2025
Merged

fix: explicit workflow invocation uses the same resource intance that reconcile api #2686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad67c7a
fix: explicit workflow invication uses the same resource intance that…
csviri Feb 13, 2025
26ab6af
refactor + test
csviri Feb 13, 2025
38bd137
minor fixes
csviri Feb 13, 2025
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
6 changes: 5 additions & 1 deletion ...amework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public <T> Optional<T> getSecondaryResource(Class<T> expectedType, String eventS
* If a workflow has an activation condition there can be event sources which are only
* registered if the activation condition holds, but to provide a consistent API we return an
* Optional instead of throwing an exception.
*
*
* Note that not only the resource which has an activation condition might not be registered
* but dependents which depend on it.
*/
Expand Down Expand Up @@ -116,4 +116,8 @@ public DefaultContext<P> setRetryInfo(RetryInfo retryInfo) {
this.retryInfo = retryInfo;
return this;
}

public P getPrimaryResource() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big fan of exposing this method only for tests…

Copy link

Choose a reason for hiding this comment

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

Sorry for jumping in here, but if this method is not exposed, how a main reconciliation loop will be aware that the primary resource has changed if in the middle of one reconciliation an explicit dependent resource reconciliation is requested via context.managedWorkflowAndDependentResourceContext().reconcileManagedWorkflow(); It may happen that the primary resource is updated, or either its status, and then the main reconciliation loop will not be using a most up to date version of the primary resource. Or am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@afalhambra-hivemq usually you get this as a parameter for example here for dependent resources:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

Line 254 in 1e3ac83

protected R desired(P primary, Context<P> context) {

It is not part of the interface (Context) just the impl so it is unit testable but you never access this primary resource through this method. I think @metacosm has a point I'm not big fan either, but on the other hand if there is no other way to unit test it I prefer to be just pragmatic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that's the intent here, though. Also, the primary should probably not changed during a reconciliation loop but be the target of a subsequent reconciliation because otherwise the current one might be operating on inconsistent basis (for example, some dependents might have seen the old primary version and taken decisions based on that, while others might see a new, possibly contradictory version…)

Copy link
Collaborator Author

@csviri csviri Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If you use SSA should not change, otherwise fine. We should explain that in docs maybe more thoroughly or in a separate blogpost.
Will expand that section in release blogpost (currently there is a TODO 😶‍🌫️ )

Copy link

Choose a reason for hiding this comment

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

Thanks for your replies both.
Just to clarify our current scenario here.
Currently on JOSDK 4.9.7 we have managed dependent resources where in their reconcile method we update the status of the primary resource.... then the main/primary reconcile method is called afterwards with the primary and context as a parameter. - Did I understand then that updating the status of the primary on the DR renconcile method may lead to inconsistencies?

Am just asking cause we're already planning to upgrade to JOSDK 5.0.1, and would like to use the new feature to call explicitly the reconcile method on the managed dependent resources by calling context.managedWorkflowAndDependentResourceContext().reconcileManagedWorkflow() from the main/primary reconcile method, but then of course, once the method is done and the DR are reconciled, the primary object on this main/primary reconcile method won't be up to date with potential changes from the reconcile calls on the DRs.

Copy link
Collaborator Author

@csviri csviri Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @afalhambra-hivemq ,
currently working on blogpost regarding to this, what you do should to be fine if you set:
ConfigurationService.useSSAToPatchPrimaryResource
to false and this PR change released.

For SSA, basically it is required that you just create a resources where you set fields which you have "opinion". See
https://kubernetes.io/docs/reference/using-api/server-side-apply/

But it is not mandatory to use that in JOSDK, this flag to don't use SSA is not just for making the migration doable in multiple steps, we want to keep it also for the future.

Copy link

Choose a reason for hiding this comment

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

Thank you both, I tested it and works like a charm. Even with ConfigurationService.useSSAToPatchPrimaryResource set to true.

return primaryResource;
}
}
24 changes: 12 additions & 12 deletions .../src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ private PostExecutionControl<P> handleDispatch(ExecutionScope<P> executionScope)
}

Context<P> context =
new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource);
new DefaultContext<>(executionScope.getRetryInfo(), controller, resourceForExecution);
if (markedForDeletion) {
return handleCleanup(resourceForExecution, originalResource, context);
} else {
Expand Down Expand Up @@ -234,29 +234,29 @@ private void updatePostExecutionControlWithReschedule(
baseControl.getScheduleDelay().ifPresent(postExecutionControl::withReSchedule);
}

private PostExecutionControl<P> handleCleanup(P resource,
private PostExecutionControl<P> handleCleanup(P resourceForExecution,
P originalResource, Context<P> context) {
if (log.isDebugEnabled()) {
log.debug(
"Executing delete for resource: {} with version: {}",
ResourceID.fromResource(resource),
getVersion(resource));
ResourceID.fromResource(resourceForExecution),
getVersion(resourceForExecution));
}
DeleteControl deleteControl = controller.cleanup(resource, context);
DeleteControl deleteControl = controller.cleanup(resourceForExecution, context);
final var useFinalizer = controller.useFinalizer();
if (useFinalizer) {
// note that we don't reschedule here even if instructed. Removing finalizer means that
// cleanup is finished, nothing left to done
// cleanup is finished, nothing left to be done
final var finalizerName = configuration().getFinalizerName();
if (deleteControl.isRemoveFinalizer() && resource.hasFinalizer(finalizerName)) {
P customResource = conflictRetryingPatch(resource, originalResource, r -> {
if (deleteControl.isRemoveFinalizer() && resourceForExecution.hasFinalizer(finalizerName)) {
P customResource = conflictRetryingPatch(resourceForExecution, originalResource, r -> {
// the operator might not be allowed to retrieve the resource on a retry, e.g. when its
// permissions are removed by deleting the namespace concurrently
if (r == null) {
log.warn(
"Could not remove finalizer on null resource: {} with version: {}",
getUID(resource),
getVersion(resource));
getUID(resourceForExecution),
getVersion(resourceForExecution));
return false;
}
return r.removeFinalizer(finalizerName);
Expand All @@ -266,8 +266,8 @@ private PostExecutionControl<P> handleCleanup(P resource,
}
log.debug(
"Skipping finalizer remove for resource: {} with version: {}. delete control: {}, uses finalizer: {}",
getUID(resource),
getVersion(resource),
getUID(resourceForExecution),
getVersion(resourceForExecution),
deleteControl,
useFinalizer);
PostExecutionControl<P> postExecutionControl = PostExecutionControl.defaultDispatch();
Expand Down
33 changes: 28 additions & 5 deletions .../test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import java.util.function.BiFunction;
import java.util.function.Supplier;

import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
Expand Down Expand Up @@ -68,6 +70,9 @@ void setup() {
}

static void initConfigService(boolean useSSA) {
initConfigService(useSSA,true);
}
static void initConfigService(boolean useSSA, boolean noCloning) {
/*
* We need this for mock reconcilers to properly generate the expected UpdateControl: without
* this, calls such as `when(reconciler.reconcile(eq(testCustomResource),
Expand All @@ -77,14 +82,18 @@ static void initConfigService(boolean useSSA) {
*/
configurationService =
ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(),
overrider -> overrider.checkingCRDAndValidateLocalModel(false)
overrider -> overrider.checkingCRDAndValidateLocalModel(false)

.withResourceCloner(new Cloner() {
@Override
public <R extends HasMetadata> R clone(R object) {
if (noCloning) {
return object;
}else {
return new KubernetesSerialization().clone(object);
}
})
.withUseSSAToPatchPrimaryResource(useSSA));
}})
.withUseSSAToPatchPrimaryResource(useSSA));
}

private <R extends HasMetadata> ReconciliationDispatcher<R> init(R customResource,
Expand Down Expand Up @@ -659,10 +668,24 @@ void reSchedulesFromErrorHandler() {
}

@Test
void addsFinalizerToPatchWithSSA() {
void reconcilerContextUsesTheSameInstanceOfResourceAsParam() {
initConfigService(false,false);

}
final ReconciliationDispatcher<TestCustomResource> dispatcher =
init(testCustomResource, reconciler, null, customResourceFacade, true);

testCustomResource.addFinalizer(DEFAULT_FINALIZER);
ArgumentCaptor<DefaultContext> contextArgumentCaptor = ArgumentCaptor.forClass(DefaultContext.class);
ArgumentCaptor<TestCustomResource> customResourceCaptor = ArgumentCaptor.forClass(TestCustomResource.class);

dispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
verify(reconciler, times(1))
.reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture());

assertThat(contextArgumentCaptor.getValue().getPrimaryResource())
.isSameAs(customResourceCaptor.getValue())
.isNotSameAs(testCustomResource);
}

private ObservedGenCustomResource createObservedGenCustomResource() {
ObservedGenCustomResource observedGenCustomResource = new ObservedGenCustomResource();
Expand Down