From fac93022be6b8a283d55fb3b38b89dfaec52e661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Sat, 4 Jan 2025 11:33:44 +0100 Subject: [PATCH] fix: error message handling issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../reconciler/ValidationAndErrorHandler.java | 8 ++- .../glue/reconciler/glue/GlueReconciler.java | 50 +++++++++++-------- .../operator/glue/GlueTest.java | 28 ++++++++++- src/test/resources/glue/Invalid.yaml | 23 +++++++++ 4 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 src/test/resources/glue/Invalid.yaml diff --git a/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/ValidationAndErrorHandler.java b/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/ValidationAndErrorHandler.java index 6210329..43cf57d 100644 --- a/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/ValidationAndErrorHandler.java +++ b/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/ValidationAndErrorHandler.java @@ -22,6 +22,8 @@ @Singleton public class ValidationAndErrorHandler { + public static final int MAX_MESSAGE_SIZE = 150; + private static final Logger log = LoggerFactory.getLogger(ValidationAndErrorHandler.class); public static final String NON_UNIQUE_NAMES_FOUND_PREFIX = "Non unique names found: "; @@ -37,7 +39,11 @@ public class ValidationAndErrorHandler { .setErrorMessage(NON_UNIQUE_NAMES_FOUND_PREFIX + String.join(",", ex.getDuplicates())); return ErrorStatusUpdateControl.updateStatus(resource).withNoRetry(); } else { - resource.getStatus().setErrorMessage("Error during reconciliation"); + var message = e.getMessage(); + if (message.length() > MAX_MESSAGE_SIZE) { + message = message.substring(0, MAX_MESSAGE_SIZE) + "..."; + } + resource.getStatus().setErrorMessage("Error: " + message); return ErrorStatusUpdateControl.updateStatus(resource); } } diff --git a/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java b/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java index 34b72d8..a39a7bc 100644 --- a/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java +++ b/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java @@ -93,17 +93,7 @@ public UpdateControl reconcile(Glue primary, informerRegister.deRegisterInformerOnResourceFlowChange(context, primary); result.throwAggregateExceptionIfErrorsPresent(); patchRelatedResourcesStatus(context, primary); - return UpdateControl.noUpdate(); - } - - private boolean deletedGlueIfParentMarkedForDeletion(Context context, Glue primary) { - var parent = getParentRelatedResource(primary, context); - if (parent.map(HasMetadata::isMarkedForDeletion).orElse(false)) { - context.getClient().resource(primary).delete(); - return true; - } else { - return false; - } + return removeErrorMessageFromGlueStatusIfPresent(primary); } @Override @@ -128,6 +118,34 @@ public DeleteControl cleanup(Glue primary, Context context) { } } + @Override + public ErrorStatusUpdateControl updateErrorStatus(Glue resource, Context context, + Exception e) { + if (resource.getStatus() == null) { + resource.setStatus(new GlueStatus()); + } + return validationAndErrorHandler.updateStatusErrorMessage(e, resource); + } + + private boolean deletedGlueIfParentMarkedForDeletion(Context context, Glue primary) { + var parent = getParentRelatedResource(primary, context); + if (parent.map(HasMetadata::isMarkedForDeletion).orElse(false)) { + context.getClient().resource(primary).delete(); + return true; + } else { + return false; + } + } + + private UpdateControl removeErrorMessageFromGlueStatusIfPresent(Glue primary) { + if (primary.getStatus() != null && primary.getStatus().getErrorMessage() != null) { + primary.getStatus().setErrorMessage(null); + return UpdateControl.patchStatus(primary); + } else { + return UpdateControl.noUpdate(); + } + } + private void registerRelatedResourceInformers(Context context, Glue glue) { glue.getSpec().getRelatedResources().forEach(r -> { @@ -234,7 +252,6 @@ private GenericDependentResource createDependentResource(DependentResourceSpec s } } - // todo add workflow result? private void patchRelatedResourcesStatus(Context context, Glue primary) { @@ -345,15 +362,6 @@ private String parentFinalizer(String glueName) { return PARENT_GLUE_FINALIZER_PREFIX + glueName; } - @Override - public ErrorStatusUpdateControl updateErrorStatus(Glue resource, Context context, - Exception e) { - if (resource.getStatus() == null) { - resource.setStatus(new GlueStatus()); - } - return validationAndErrorHandler.updateStatusErrorMessage(e, resource); - } - public static boolean isGlueOfAGlueOperator(Glue glue) { var labelValue = glue.getMetadata().getLabels().get(GlueOperatorReconciler.FOR_GLUE_OPERATOR_LABEL_KEY); diff --git a/src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java b/src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java index cfe62b2..cdd3515 100644 --- a/src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java +++ b/src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java @@ -380,11 +380,37 @@ void simpleBulk() { }); delete(glue); - await().untilAsserted( () -> assertThat(getRelatedList(ConfigMap.class, glue.getMetadata().getName()).isEmpty())); } + @Test + void invalidGlueMessageHandling() { + var glueName = "invalid-glue"; + var glue = createGlue("/glue/Invalid.yaml"); + + await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> { + var g = get(Glue.class, glueName); + assertThat(g.getStatus()).isNotNull(); + assertThat(g.getStatus().getErrorMessage()).isNotNull(); + }); + + // fix error + glue.getSpec().getChildResources().get(1).setName("configMap2"); + glue.getMetadata().setResourceVersion(null); + update(glue); + + await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> { + var g = get(Glue.class, glueName); + assertThat(g.getStatus().getErrorMessage()).isNull(); + }); + + delete(glue); + await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> { + var g = get(Glue.class, glueName); + assertThat(g).isNull(); + }); + } private List testWorkflowList(int num) { diff --git a/src/test/resources/glue/Invalid.yaml b/src/test/resources/glue/Invalid.yaml new file mode 100644 index 0000000..919e203 --- /dev/null +++ b/src/test/resources/glue/Invalid.yaml @@ -0,0 +1,23 @@ +# Invalid GLUE, presents resources with non-unique name +apiVersion: io.javaoperatorsdk.operator.glue/v1beta1 +kind: Glue +metadata: + name: invalid-glue +spec: + childResources: + - name: configMap + resource: + apiVersion: v1 + kind: ConfigMap + metadata: + name: simple-glue-configmap1 + data: + key: "value1" + - name: configMap # invalid: duplicate name + resource: + apiVersion: v1 + kind: ConfigMap + metadata: + name: simple-glue-configmap2 + data: + key: "value2"