Skip to content

fix: error message handling issues #162

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.

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 1 commit into from
Jan 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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: ";
Expand All @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,7 @@ public UpdateControl<Glue> reconcile(Glue primary,
informerRegister.deRegisterInformerOnResourceFlowChange(context, primary);
result.throwAggregateExceptionIfErrorsPresent();
patchRelatedResourcesStatus(context, primary);
return UpdateControl.noUpdate();
}

private boolean deletedGlueIfParentMarkedForDeletion(Context<Glue> 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
Expand All @@ -128,6 +118,34 @@ public DeleteControl cleanup(Glue primary, Context<Glue> context) {
}
}

@Override
public ErrorStatusUpdateControl<Glue> updateErrorStatus(Glue resource, Context<Glue> context,
Exception e) {
if (resource.getStatus() == null) {
resource.setStatus(new GlueStatus());
}
return validationAndErrorHandler.updateStatusErrorMessage(e, resource);
}

private boolean deletedGlueIfParentMarkedForDeletion(Context<Glue> 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<Glue> 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<Glue> context,
Glue glue) {
glue.getSpec().getRelatedResources().forEach(r -> {
Expand Down Expand Up @@ -234,7 +252,6 @@ private GenericDependentResource createDependentResource(DependentResourceSpec s
}
}

// todo add workflow result?
private void patchRelatedResourcesStatus(Context<Glue> context,
Glue primary) {

Expand Down Expand Up @@ -345,15 +362,6 @@ private String parentFinalizer(String glueName) {
return PARENT_GLUE_FINALIZER_PREFIX + glueName;
}

@Override
public ErrorStatusUpdateControl<Glue> updateErrorStatus(Glue resource, Context<Glue> 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);
Expand Down
28 changes: 27 additions & 1 deletion src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<Glue> testWorkflowList(int num) {
Expand Down
23 changes: 23 additions & 0 deletions src/test/resources/glue/Invalid.yaml
Original file line number Diff line number Diff line change
@@ -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"
Loading