Discard watch when extension status is changed only (#3362)

#### What type of PR is this?

/kind improvement
/area core

#### What this PR does / why we need it:

This PR mainly creates two things:

1. Create JsonExtension to represent any extension and make extension modification more convenient
2. Discard watch when we detect that the extension status is changed only. It's useful to prevent infinite loop of reconciler.

#### Which issue(s) this PR fixes:

Fixes https://github.com/halo-dev/halo/issues/3273

#### Special notes for your reviewer:

Try to install test plugin from [plugin-comment-widget.zip](https://github.com/halo-dev/halo/files/10799875/plugin-comment-widget.zip) before checking out this PR. You will get a infinite reconciliation loop.

Then, stop the process and checkout this PR and see the result.

#### Does this PR introduce a user-facing change?

```release-note
None
```
pull/3379/head
John Niang 2023-02-23 18:24:12 +08:00 committed by GitHub
parent 3a1587bab5
commit ce85b98539
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 367 additions and 24 deletions

View File

@ -53,8 +53,6 @@ public class JSONExtensionConverter implements ExtensionConverter {
var scheme = schemeManager.get(gvk); var scheme = schemeManager.get(gvk);
try { try {
var data = objectMapper.writeValueAsBytes(extension);
var validation = new ValidationData<>(extension); var validation = new ValidationData<>(extension);
var extensionJsonNode = objectMapper.valueToTree(extension); var extensionJsonNode = objectMapper.valueToTree(extension);
var validator = getValidator(scheme); var validator = getValidator(scheme);
@ -68,6 +66,7 @@ public class JSONExtensionConverter implements ExtensionConverter {
var version = extension.getMetadata().getVersion(); var version = extension.getMetadata().getVersion();
var storeName = ExtensionUtil.buildStoreName(scheme, extension.getMetadata().getName()); var storeName = ExtensionUtil.buildStoreName(scheme, extension.getMetadata().getName());
var data = objectMapper.writeValueAsBytes(extensionJsonNode);
return new ExtensionStore(storeName, data, version); return new ExtensionStore(storeName, data, version);
} catch (IOException e) { } catch (IOException e) {
throw new ExtensionConvertException("Failed write Extension as bytes", e); throw new ExtensionConvertException("Failed write Extension as bytes", e);

View File

@ -0,0 +1,232 @@
package run.halo.app.extension;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.node.LongNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.databind.node.TextNode;
import java.io.IOException;
import java.time.Instant;
import java.util.Map;
import java.util.Set;
/**
* JsonExtension is representation an extension using ObjectNode. This extension is preparing for
* patching in the future.
*
* @author johnniang
*/
@JsonSerialize(using = JsonExtension.ObjectNodeExtensionSerializer.class)
@JsonDeserialize(using = JsonExtension.ObjectNodeExtensionDeSerializer.class)
class JsonExtension implements Extension {
private final ObjectMapper objectMapper;
private final ObjectNode objectNode;
public JsonExtension(ObjectMapper objectMapper) {
this(objectMapper, objectMapper.createObjectNode());
}
public JsonExtension(ObjectMapper objectMapper, ObjectNode objectNode) {
this.objectMapper = objectMapper;
this.objectNode = objectNode;
}
public JsonExtension(ObjectMapper objectMapper, Extension e) {
this(objectMapper, (ObjectNode) objectMapper.valueToTree(e));
}
@Override
public MetadataOperator getMetadata() {
var metadataNode = objectNode.get("metadata");
if (metadataNode == null) {
return null;
}
return new ObjectNodeMetadata((ObjectNode) metadataNode);
}
@Override
public String getApiVersion() {
var apiVersionNode = objectNode.get("apiVersion");
return apiVersionNode == null ? null : apiVersionNode.asText();
}
@Override
public String getKind() {
return objectNode.get("kind").asText();
}
@Override
public void setApiVersion(String apiVersion) {
objectNode.set("apiVersion", new TextNode(apiVersion));
}
@Override
public void setKind(String kind) {
objectNode.set("kind", new TextNode(kind));
}
@Override
public void setMetadata(MetadataOperator metadata) {
objectNode.set("metadata", objectMapper.valueToTree(metadata));
}
public static class ObjectNodeExtensionSerializer extends JsonSerializer<JsonExtension> {
@Override
public void serialize(JsonExtension value, JsonGenerator gen,
SerializerProvider serializers) throws IOException {
gen.writeTree(value.objectNode);
}
}
public static class ObjectNodeExtensionDeSerializer
extends JsonDeserializer<JsonExtension> {
@Override
public JsonExtension deserialize(JsonParser p, DeserializationContext ctxt)
throws IOException {
var mapper = (ObjectMapper) p.getCodec();
var treeNode = mapper.readTree(p);
return new JsonExtension(mapper, (ObjectNode) treeNode);
}
}
/**
* Get internal representation.
*
* @return internal representation
*/
public ObjectNode getInternal() {
return objectNode;
}
public MetadataOperator getMetadataOrCreate() {
var metadataNode = objectMapper.createObjectNode();
objectNode.set("metadata", metadataNode);
return new ObjectNodeMetadata(metadataNode);
}
class ObjectNodeMetadata implements MetadataOperator {
private final ObjectNode objectNode;
public ObjectNodeMetadata(ObjectNode objectNode) {
this.objectNode = objectNode;
}
@Override
public String getName() {
var nameNode = objectNode.get("name");
return objectMapper.convertValue(nameNode, String.class);
}
@Override
public String getGenerateName() {
var generateNameNode = objectNode.get("generateName");
return objectMapper.convertValue(generateNameNode, String.class);
}
@Override
public Map<String, String> getLabels() {
var labelsNode = objectNode.get("labels");
return objectMapper.convertValue(labelsNode, new TypeReference<>() {
});
}
@Override
public Map<String, String> getAnnotations() {
var annotationsNode = objectNode.get("annotations");
return objectMapper.convertValue(annotationsNode, new TypeReference<>() {
});
}
@Override
public Long getVersion() {
JsonNode versionNode = objectNode.get("version");
return objectMapper.convertValue(versionNode, Long.class);
}
@Override
public Instant getCreationTimestamp() {
return objectMapper.convertValue(objectNode.get("creationTimestamp"), Instant.class);
}
@Override
public Instant getDeletionTimestamp() {
return objectMapper.convertValue(objectNode.get("deletionTimestamp"), Instant.class);
}
@Override
public Set<String> getFinalizers() {
return objectMapper.convertValue(objectNode.get("finalizers"), new TypeReference<>() {
});
}
@Override
public void setName(String name) {
if (name != null) {
objectNode.set("name", TextNode.valueOf(name));
}
}
@Override
public void setGenerateName(String generateName) {
if (generateName != null) {
objectNode.set("generateName", TextNode.valueOf(generateName));
}
}
@Override
public void setLabels(Map<String, String> labels) {
if (labels != null) {
objectNode.set("labels", objectMapper.valueToTree(labels));
}
}
@Override
public void setAnnotations(Map<String, String> annotations) {
if (annotations != null) {
objectNode.set("annotations", objectMapper.valueToTree(annotations));
}
}
@Override
public void setVersion(Long version) {
if (version != null) {
objectNode.set("version", LongNode.valueOf(version));
}
}
@Override
public void setCreationTimestamp(Instant creationTimestamp) {
if (creationTimestamp != null) {
objectNode.set("creationTimestamp", objectMapper.valueToTree(creationTimestamp));
}
}
@Override
public void setDeletionTimestamp(Instant deletionTimestamp) {
if (deletionTimestamp != null) {
objectNode.set("deletionTimestamp", objectMapper.valueToTree(deletionTimestamp));
}
}
@Override
public void setFinalizers(Set<String> finalizers) {
if (finalizers != null) {
objectNode.set("finalizers", objectMapper.valueToTree(finalizers));
}
}
}
}

View File

@ -3,11 +3,13 @@ package run.halo.app.extension;
import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic; import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic;
import static org.springframework.util.StringUtils.hasText; import static org.springframework.util.StringUtils.hasText;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.time.Duration; import java.time.Duration;
import java.time.Instant; import java.time.Instant;
import java.util.Comparator; import java.util.Comparator;
import java.util.List; import java.util.List;
import java.util.Objects; import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Predicate; import java.util.function.Predicate;
import org.springframework.dao.DataIntegrityViolationException; import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.data.util.Predicates; import org.springframework.data.util.Predicates;
@ -29,11 +31,14 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient {
private final Watcher.WatcherComposite watchers; private final Watcher.WatcherComposite watchers;
private final ObjectMapper objectMapper;
public ReactiveExtensionClientImpl(ReactiveExtensionStoreClient client, public ReactiveExtensionClientImpl(ReactiveExtensionStoreClient client,
ExtensionConverter converter, SchemeManager schemeManager) { ExtensionConverter converter, SchemeManager schemeManager, ObjectMapper objectMapper) {
this.client = client; this.client = client;
this.converter = converter; this.converter = converter;
this.schemeManager = schemeManager; this.schemeManager = schemeManager;
this.objectMapper = objectMapper;
this.watchers = new Watcher.WatcherComposite(); this.watchers = new Watcher.WatcherComposite();
} }
@ -110,7 +115,7 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient {
if (!hasText(metadata.getGenerateName())) { if (!hasText(metadata.getGenerateName())) {
throw new IllegalArgumentException( throw new IllegalArgumentException(
"The metadata.generateName must not be blank when metadata.name is " "The metadata.generateName must not be blank when metadata.name is "
+ "blank"); + "blank");
} }
// generate name with random text // generate name with random text
metadata.setName(metadata.getGenerateName() + randomAlphabetic(5)); metadata.setName(metadata.getGenerateName() + randomAlphabetic(5));
@ -124,40 +129,57 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient {
.retryWhen(Retry.backoff(3, Duration.ofMillis(100)) .retryWhen(Retry.backoff(3, Duration.ofMillis(100))
// retry when generateName is set // retry when generateName is set
.filter(t -> t instanceof DataIntegrityViolationException .filter(t -> t instanceof DataIntegrityViolationException
&& hasText(extension.getMetadata().getGenerateName()))); && hasText(extension.getMetadata().getGenerateName())));
} }
@Override @Override
public <E extends Extension> Mono<E> update(E extension) { public <E extends Extension> Mono<E> update(E extension) {
// overwrite some fields // Refactor the atomic reference if we have a better solution.
Mono<? extends Extension> mono; final var statusChangeOnly = new AtomicBoolean(false);
if (extension instanceof Unstructured unstructured) { return getLatest(extension)
mono = get(unstructured.groupVersionKind(), extension.getMetadata().getName()); .map(old -> new JsonExtension(objectMapper, old))
} else { .flatMap(oldJsonExt -> {
mono = get(extension.getClass(), extension.getMetadata().getName()); var newJsonExt = new JsonExtension(objectMapper, extension);
} // reset some mandatory fields
return mono var oldMetadata = oldJsonExt.getMetadata();
.flatMap(old -> { var newMetadata = newJsonExt.getMetadata();
// reset some fields
var oldMetadata = old.getMetadata();
var newMetadata = extension.getMetadata();
newMetadata.setCreationTimestamp(oldMetadata.getCreationTimestamp()); newMetadata.setCreationTimestamp(oldMetadata.getCreationTimestamp());
newMetadata.setDeletionTimestamp(oldMetadata.getDeletionTimestamp()); newMetadata.setGenerateName(oldMetadata.getGenerateName());
extension.setMetadata(newMetadata);
if (Objects.equals(old, extension)) { var oldObjectNode = oldJsonExt.getInternal().deepCopy();
var newObjectNode = newJsonExt.getInternal().deepCopy();
if (Objects.equals(oldObjectNode, newObjectNode)) {
// if no data were changed, just skip updating.
return Mono.empty(); return Mono.empty();
} }
return Mono.just(extension); // check status is changed
oldObjectNode.remove("status");
newObjectNode.remove("status");
if (Objects.equals(oldObjectNode, newObjectNode)) {
statusChangeOnly.set(true);
}
return Mono.just(newJsonExt);
}) })
.map(converter::convertTo) .map(converter::convertTo)
.flatMap(extensionStore -> client.update(extensionStore.getName(), .flatMap(extensionStore -> client.update(extensionStore.getName(),
extensionStore.getVersion(), extensionStore.getVersion(),
extensionStore.getData())) extensionStore.getData()))
.map(updated -> converter.convertFrom((Class<E>) extension.getClass(), updated)) .map(updated -> converter.convertFrom((Class<E>) extension.getClass(), updated))
.doOnNext(updated -> watchers.onUpdate(extension, updated)) .doOnNext(updated -> {
if (!statusChangeOnly.get()) {
watchers.onUpdate(extension, updated);
}
})
.switchIfEmpty(Mono.defer(() -> Mono.just(extension))); .switchIfEmpty(Mono.defer(() -> Mono.just(extension)));
} }
private Mono<? extends Extension> getLatest(Extension extension) {
if (extension instanceof Unstructured unstructured) {
return get(unstructured.groupVersionKind(), unstructured.getMetadata().getName());
}
return get(extension.getClass(), extension.getMetadata().getName());
}
@Override @Override
public <E extends Extension> Mono<E> delete(E extension) { public <E extends Extension> Mono<E> delete(E extension) {
// set deletionTimestamp // set deletionTimestamp

View File

@ -0,0 +1,80 @@
package run.halo.app.extension;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.databind.node.TextNode;
import org.json.JSONException;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.skyscreamer.jsonassert.JSONAssert;
class JsonExtensionTest {
ObjectMapper objectMapper;
@BeforeEach
void setUp() {
objectMapper = JsonMapper.builder().build();
}
@Test
void serializeEmptyExt() throws JsonProcessingException, JSONException {
var ext = new JsonExtension(objectMapper);
var json = objectMapper.writeValueAsString(ext);
JSONAssert.assertEquals("{}", json, true);
}
@Test
void serializeExt() throws JsonProcessingException, JSONException {
var ext = new JsonExtension(objectMapper);
ext.setApiVersion("fake.halo.run/v1alpha");
ext.setKind("Fake");
var metadata = ext.getMetadataOrCreate();
metadata.setName("fake-name");
ext.getInternal().set("data", TextNode.valueOf("halo"));
JSONAssert.assertEquals("""
{
"apiVersion": "fake.halo.run/v1alpha",
"kind": "Fake",
"metadata": {
"name": "fake-name"
},
"data": "halo"
}""", objectMapper.writeValueAsString(ext), true);
}
@Test
void deserialize() throws JsonProcessingException {
var json = """
{
"apiVersion": "fake.halo.run/v1alpha1",
"kind": "Fake",
"metadata": {
"name": "faker"
},
"otherProperty": "otherPropertyValue"
}""";
var ext = objectMapper.readValue(json, JsonExtension.class);
assertEquals("fake.halo.run/v1alpha1", ext.getApiVersion());
assertEquals("Fake", ext.getKind());
assertNotNull(ext.getMetadata());
assertEquals("faker", ext.getMetadata().getName());
assertNull(ext.getMetadata().getVersion());
assertNull(ext.getMetadata().getFinalizers());
assertNull(ext.getMetadata().getAnnotations());
assertNull(ext.getMetadata().getLabels());
assertNull(ext.getMetadata().getGenerateName());
assertNull(ext.getMetadata().getCreationTimestamp());
assertNull(ext.getMetadata().getDeletionTimestamp());
assertEquals("otherPropertyValue", ext.getInternal().get("otherProperty").asText());
}
}

View File

@ -20,6 +20,9 @@ import static org.mockito.Mockito.when;
import static run.halo.app.extension.GroupVersionKind.fromAPIVersionAndKind; import static run.halo.app.extension.GroupVersionKind.fromAPIVersionAndKind;
import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.BeforeEach;
@ -29,6 +32,7 @@ import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks; import org.mockito.InjectMocks;
import org.mockito.Mock; import org.mockito.Mock;
import org.mockito.Spy;
import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.dao.DataIntegrityViolationException; import org.springframework.dao.DataIntegrityViolationException;
import reactor.core.Exceptions; import reactor.core.Exceptions;
@ -53,6 +57,11 @@ class ReactiveExtensionClientTest {
@Mock @Mock
SchemeManager schemeManager; SchemeManager schemeManager;
@Spy
ObjectMapper objectMapper = JsonMapper.builder()
.addModule(new JavaTimeModule())
.build();
@InjectMocks @InjectMocks
ReactiveExtensionClientImpl client; ReactiveExtensionClientImpl client;
@ -419,7 +428,7 @@ class ReactiveExtensionClientTest {
.verifyComplete(); .verifyComplete();
verify(storeClient).fetchByName(storeName); verify(storeClient).fetchByName(storeName);
verify(converter).convertTo(eq(fake)); verify(converter).convertTo(isA(JsonExtension.class));
verify(converter, times(2)).convertFrom(same(FakeExtension.class), any()); verify(converter, times(2)).convertFrom(same(FakeExtension.class), any());
verify(storeClient) verify(storeClient)
.update(eq("/registry/fake.halo.run/fakes/fake"), eq(2L), any()); .update(eq("/registry/fake.halo.run/fakes/fake"), eq(2L), any());
@ -433,6 +442,7 @@ class ReactiveExtensionClientTest {
Mono.just(createExtensionStore(storeName, 1L))); Mono.just(createExtensionStore(storeName, 1L)));
var oldFake = createFakeExtension("fake", 2L); var oldFake = createFakeExtension("fake", 2L);
when(converter.convertFrom(same(FakeExtension.class), any())).thenReturn(oldFake); when(converter.convertFrom(same(FakeExtension.class), any())).thenReturn(oldFake);
StepVerifier.create(client.update(fake)) StepVerifier.create(client.update(fake))
@ -470,7 +480,7 @@ class ReactiveExtensionClientTest {
.verifyComplete(); .verifyComplete();
verify(storeClient).fetchByName(name); verify(storeClient).fetchByName(name);
verify(converter).convertTo(eq(fake)); verify(converter).convertTo(isA(JsonExtension.class));
verify(converter, times(2)).convertFrom(same(Unstructured.class), any()); verify(converter, times(2)).convertFrom(same(Unstructured.class), any());
verify(storeClient) verify(storeClient)
.update(eq("/registry/fake.halo.run/fakes/fake"), eq(12345L), any()); .update(eq("/registry/fake.halo.run/fakes/fake"), eq(12345L), any());