Skip to content

feat: patching with update control #1147

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

Merged
merged 4 commits into from
Apr 7, 2022
Merged

feat: patching with update control #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9395187
feat: patching in update control
csviri Apr 7, 2022
3fc299a
fix: patchin status
csviri Apr 7, 2022
1ca630c
fix: add unit tests
csviri Apr 7, 2022
1cb6c3c
feat: add integration test for patch
csviri Apr 7, 2022
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
30 changes: 25 additions & 5 deletions ...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/UpdateControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,17 @@ public class UpdateControl<T extends HasMetadata> extends BaseControl<UpdateCont
private final T resource;
private final boolean updateStatus;
private final boolean updateResource;
private final boolean patch;

private UpdateControl(
T resource, boolean updateStatus, boolean updateResource) {
T resource, boolean updateStatus, boolean updateResource, boolean patch) {
if ((updateResource || updateStatus) && resource == null) {
throw new IllegalArgumentException("CustomResource cannot be null in case of update");
}
this.resource = resource;
this.updateStatus = updateStatus;
this.updateResource = updateResource;
this.patch = patch;
}

/**
Expand All @@ -28,11 +30,19 @@ private UpdateControl(
* @return initialized update control
*/
public static <T extends HasMetadata> UpdateControl<T> updateResource(T customResource) {
return new UpdateControl<>(customResource, false, true);
return new UpdateControl<>(customResource, false, true, false);
}

public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customResource) {
return new UpdateControl<>(customResource, true, false);
return new UpdateControl<>(customResource, true, false, false);
}

public static <T extends HasMetadata> UpdateControl<T> patchResource(T customResource) {
return new UpdateControl<>(customResource, false, true, true);
}

public static <T extends HasMetadata> UpdateControl<T> patchStatus(T customResource) {
return new UpdateControl<>(customResource, true, false, true);
}

/**
Expand All @@ -45,11 +55,17 @@ public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customReso
*/
public static <T extends HasMetadata> UpdateControl<T> updateResourceAndStatus(
T customResource) {
return new UpdateControl<>(customResource, true, true);
return new UpdateControl<>(customResource, true, true, false);
}

public static <T extends HasMetadata> UpdateControl<T> patchResourceAndStatus(
T customResource) {
return new UpdateControl<>(customResource, true, true, true);
}


public static <T extends HasMetadata> UpdateControl<T> noUpdate() {
return new UpdateControl<>(null, false, false);
return new UpdateControl<>(null, false, false, false);
}

public T getResource() {
Expand All @@ -64,6 +80,10 @@ public boolean isUpdateResource() {
return updateResource;
}

public boolean isPatch() {
return patch;
}

public boolean isNoUpdate() {
return !updateResource && !updateStatus;
}
Expand Down
70 changes: 48 additions & 22 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 @@ -133,22 +133,28 @@ private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionSc
UpdateControl<R> updateControl = controller.reconcile(resourceForExecution, context);
R updatedCustomResource = null;
if (updateControl.isUpdateResourceAndStatus()) {
updatedCustomResource = updateCustomResource(updateControl.getResource());
updatedCustomResource =
updateCustomResource(updateControl.getResource(), updateControl.isPatch());
updateControl
.getResource()
.getMetadata()
.setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion());
updatedCustomResource = updateStatusGenerationAware(updateControl.getResource());
updatedCustomResource =
updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch());
} else if (updateControl.isUpdateStatus()) {
updatedCustomResource = updateStatusGenerationAware(updateControl.getResource());
updatedCustomResource =
updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch());
} else if (updateControl.isUpdateResource()) {
updatedCustomResource = updateCustomResource(updateControl.getResource());
updatedCustomResource =
updateCustomResource(updateControl.getResource(), updateControl.isPatch());
if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
updatedCustomResource = updateStatusGenerationAware(originalResource);
updatedCustomResource =
updateStatusGenerationAware(originalResource, updateControl.isPatch());
}
} else if (updateControl.isNoUpdate()
&& shouldUpdateObservedGenerationAutomatically(resourceForExecution)) {
updatedCustomResource = updateStatusGenerationAware(originalResource);
updatedCustomResource =
updateStatusGenerationAware(originalResource, updateControl.isPatch());
}
return createPostExecutionControl(updatedCustomResource, updateControl);
}
Expand Down Expand Up @@ -197,9 +203,13 @@ private boolean isErrorStatusHandlerPresent() {
return controller.getReconciler() instanceof ErrorStatusHandler;
}

private R updateStatusGenerationAware(R resource) {
private R updateStatusGenerationAware(R resource, boolean patch) {
updateStatusObservedGenerationIfRequired(resource);
return customResourceFacade.updateStatus(resource);
if (patch) {
return customResourceFacade.patchStatus(resource);
} else {
return customResourceFacade.updateStatus(resource);
}
}

private boolean shouldUpdateObservedGenerationAutomatically(R resource) {
Expand Down Expand Up @@ -281,13 +291,17 @@ private void updateCustomResourceWithFinalizer(R resource) {
log.debug(
"Adding finalizer for resource: {} version: {}", getUID(resource), getVersion(resource));
resource.addFinalizer(configuration().getFinalizerName());
replace(resource);
customResourceFacade.replaceResourceWithLock(resource);
}

private R updateCustomResource(R resource) {
private R updateCustomResource(R resource, boolean patch) {
log.debug("Updating resource: {} with version: {}", getUID(resource), getVersion(resource));
log.trace("Resource before update: {}", resource);
return replace(resource);
if (patch) {
return customResourceFacade.patchResource(resource);
} else {
return customResourceFacade.replaceResourceWithLock(resource);
}
}

private R removeFinalizer(R resource) {
Expand All @@ -296,16 +310,9 @@ private R removeFinalizer(R resource) {
getUID(resource),
getVersion(resource));
resource.removeFinalizer(configuration().getFinalizerName());
return customResourceFacade.replaceWithLock(resource);
return customResourceFacade.replaceResourceWithLock(resource);
}

private R replace(R resource) {
log.debug(
"Trying to replace resource {}, version: {}",
getName(resource),
resource.getMetadata().getResourceVersion());
return customResourceFacade.replaceWithLock(resource);
}

ControllerConfiguration<R> configuration() {
return controller.getConfiguration();
Expand All @@ -321,6 +328,25 @@ public CustomResourceFacade(
this.resourceOperation = resourceOperation;
}

public R replaceResourceWithLock(R resource) {
log.debug(
"Trying to replace resource {}, version: {}",
getName(resource),
resource.getMetadata().getResourceVersion());
return resourceOperation
.inNamespace(resource.getMetadata().getNamespace())
.withName(getName(resource))
.lockResourceVersion(resource.getMetadata().getResourceVersion())
.replace(resource);
}

public R patchResource(R resource) {
return resourceOperation
.inNamespace(resource.getMetadata().getNamespace())
.withName(getName(resource))
.patch(resource);
}

public R updateStatus(R resource) {
log.trace("Updating status for resource: {}", resource);
return resourceOperation
Expand All @@ -329,12 +355,12 @@ public R updateStatus(R resource) {
.replaceStatus(resource);
}

public R replaceWithLock(R resource) {
public R patchStatus(R resource) {
log.trace("Updating status for resource: {}", resource);
return resourceOperation
.inNamespace(resource.getMetadata().getNamespace())
.withName(getName(resource))
.lockResourceVersion(resource.getMetadata().getResourceVersion())
.replace(resource);
.patchStatus(resource);
}
}
}
48 changes: 38 additions & 10 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 @@ -120,7 +120,7 @@ void addFinalizerOnNewResource() throws Exception {
verify(reconciler, never())
.reconcile(ArgumentMatchers.eq(testCustomResource), any());
verify(customResourceFacade, times(1))
.replaceWithLock(
.replaceResourceWithLock(
argThat(testCustomResource -> testCustomResource.hasFinalizer(DEFAULT_FINALIZER)));
assertThat(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)).isTrue();
}
Expand All @@ -142,22 +142,50 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() throws Exception {
reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, times(1)).updateStatus(testCustomResource);
verify(customResourceFacade, never()).replaceWithLock(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
}

@Test
void updatesBothResourceAndStatusIfFinalizerSet() throws Exception {
testCustomResource.addFinalizer(DEFAULT_FINALIZER);

reconciler.reconcile = (r, c) -> UpdateControl.updateResourceAndStatus(testCustomResource);
when(customResourceFacade.replaceWithLock(testCustomResource)).thenReturn(testCustomResource);
when(customResourceFacade.replaceResourceWithLock(testCustomResource))
.thenReturn(testCustomResource);

reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, times(1)).replaceWithLock(testCustomResource);
verify(customResourceFacade, times(1)).replaceResourceWithLock(testCustomResource);
verify(customResourceFacade, times(1)).updateStatus(testCustomResource);
}


@Test
void patchesResource() {
testCustomResource.addFinalizer(DEFAULT_FINALIZER);

reconciler.reconcile = (r, c) -> UpdateControl.patchResource(testCustomResource);

reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, times(1)).patchResource(testCustomResource);
verify(customResourceFacade, never()).updateStatus(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
}

@Test
void patchesStatus() {
testCustomResource.addFinalizer(DEFAULT_FINALIZER);

reconciler.reconcile = (r, c) -> UpdateControl.patchStatus(testCustomResource);

reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, times(1)).patchStatus(testCustomResource);
verify(customResourceFacade, never()).updateStatus(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
}

@Test
void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() throws Exception {
testCustomResource.addFinalizer(DEFAULT_FINALIZER);
Expand Down Expand Up @@ -216,7 +244,7 @@ void removesDefaultFinalizerOnDeleteIfSet() {
reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

assertEquals(0, testCustomResource.getMetadata().getFinalizers().size());
verify(customResourceFacade, times(1)).replaceWithLock(any());
verify(customResourceFacade, times(1)).replaceResourceWithLock(any());
}

@Test
Expand All @@ -229,7 +257,7 @@ void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() {
reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

assertEquals(1, testCustomResource.getMetadata().getFinalizers().size());
verify(customResourceFacade, never()).replaceWithLock(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
}

@Test
Expand All @@ -239,7 +267,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() throws Exce
reconciler.reconcile = (r, c) -> UpdateControl.noUpdate();

reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
verify(customResourceFacade, never()).replaceWithLock(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
verify(customResourceFacade, never()).updateStatus(testCustomResource);
}

Expand All @@ -252,7 +280,7 @@ void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() throws
reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

assertEquals(1, testCustomResource.getMetadata().getFinalizers().size());
verify(customResourceFacade, times(1)).replaceWithLock(any());
verify(customResourceFacade, times(1)).replaceResourceWithLock(any());
}

@Test
Expand All @@ -262,7 +290,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() {

reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));

verify(customResourceFacade, never()).replaceWithLock(any());
verify(customResourceFacade, never()).replaceResourceWithLock(any());
verify(reconciler, never()).cleanup(eq(testCustomResource), any());
}

Expand Down Expand Up @@ -388,7 +416,7 @@ void updateObservedGenerationOnCustomResourceUpdate() throws Exception {
when(config.isGenerationAware()).thenReturn(true);
when(reconciler.reconcile(any(), any()))
.thenReturn(UpdateControl.updateResource(observedGenResource));
when(facade.replaceWithLock(any())).thenReturn(observedGenResource);
when(facade.replaceResourceWithLock(any())).thenReturn(observedGenResource);
when(facade.updateStatus(observedGenResource)).thenReturn(observedGenResource);
var dispatcher = init(observedGenResource, reconciler, config, facade, true);

Expand Down
12 changes: 12 additions & 0 deletions operator-framework/src/test/java/io/javaoperatorsdk/operator/ControllerExecutionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.awaitility.Awaitility.await;

class ControllerExecutionIT {

@RegisterExtension
OperatorExtension operator =
OperatorExtension.builder().withReconciler(new TestReconciler(true)).build();
Expand All @@ -32,6 +33,17 @@ void configMapGetsCreatedForTestCustomResource() {
assertThat(TestUtils.getNumberOfExecutions(operator)).isEqualTo(2);
}

@Test
void patchesStatusForTestCustomResource() {
operator.getControllerOfType(TestReconciler.class).setPatchStatus(true);
operator.getControllerOfType(TestReconciler.class).setUpdateStatus(true);

TestCustomResource resource = TestUtils.testCustomResource();
operator.create(TestCustomResource.class, resource);

awaitStatusUpdated();
}

@Test
void eventIsSkippedChangedOnMetadataOnlyUpdate() {
operator.getControllerOfType(TestReconciler.class).setUpdateStatus(false);
Expand Down
19 changes: 17 additions & 2 deletions ...tor-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,21 @@ public class TestReconciler
private final AtomicInteger numberOfExecutions = new AtomicInteger(0);
private final AtomicInteger numberOfCleanupExecutions = new AtomicInteger(0);
private KubernetesClient kubernetesClient;
private boolean updateStatus;
private volatile boolean updateStatus;
private volatile boolean patchStatus;

public TestReconciler(boolean updateStatus) {
this(updateStatus, false);
}

public TestReconciler(boolean updateStatus, boolean patchStatus) {
this.updateStatus = updateStatus;
this.patchStatus = patchStatus;
}

public TestReconciler setPatchStatus(boolean patchStatus) {
this.patchStatus = patchStatus;
return this;
}

public void setUpdateStatus(boolean updateStatus) {
Expand Down Expand Up @@ -122,7 +133,11 @@ public UpdateControl<TestCustomResource> reconcile(
}
resource.getStatus().setConfigMapStatus("ConfigMap Ready");
}
return UpdateControl.updateStatus(resource);
if (patchStatus) {
return UpdateControl.patchStatus(resource);
} else {
return UpdateControl.updateStatus(resource);
}
}

private Map<String, String> configMapData(TestCustomResource resource) {
Expand Down