Skip to content

Commit

Permalink
fix #4638 using patch instead of update
Browse files Browse the repository at this point in the history
  • Loading branch information
@shawkins @manusa
shawkins authored and manusa committed Feb 28, 2023
1 parent 4c263a5 commit 8d52ae2
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 44 deletions.
2 changes: 1 addition & 1 deletion ...api/src/main/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ private void release(LeaderElectionRecord current) {
try {
ZonedDateTime now = now();
final LeaderElectionRecord newLeaderElectionRecord = new LeaderElectionRecord(
null,
"",
Duration.ofSeconds(1),
now,
now,
Expand Down
9 changes: 2 additions & 7 deletions ...java/io/fabric8/kubernetes/client/extended/leaderelection/resourcelock/ConfigMapLock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,12 @@ protected LeaderElectionRecord toRecord(ConfigMap resource) {
return null;
}
})
.map(record -> {
record.setVersion(resource.getMetadata().getResourceVersion());
return record;
})
.orElse(null);
}

@Override
protected ConfigMap toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta, ConfigMap current) {
ConfigMapBuilder builder = Optional.ofNullable(current).map(ConfigMapBuilder::new).orElse(new ConfigMapBuilder());
return builder.withMetadata(meta).editMetadata()
protected ConfigMap toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta) {
return new ConfigMapBuilder().withMetadata(meta).editMetadata()
.addToAnnotations(LEADER_ELECTION_RECORD_ANNOTATION_KEY, Serialization.asJson(leaderElectionRecord))
.endMetadata()
.build();
Expand Down
21 changes: 8 additions & 13 deletions ...ain/java/io/fabric8/kubernetes/client/extended/leaderelection/resourcelock/LeaseLock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,8 @@ protected Class<Lease> getKind() {
}

@Override
protected Lease toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta, Lease current) {
LeaseBuilder builder = Optional.ofNullable(current).map(LeaseBuilder::new).orElse(new LeaseBuilder());
return builder.withMetadata(meta)
protected Lease toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta) {
return new LeaseBuilder().withMetadata(meta)
.withNewSpec()
.withHolderIdentity(leaderElectionRecord.getHolderIdentity())
.withLeaseDurationSeconds((int) leaderElectionRecord.getLeaseDuration().get(ChronoUnit.SECONDS))
Expand All @@ -50,16 +49,12 @@ protected Lease toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta

@Override
protected LeaderElectionRecord toRecord(Lease resource) {
return Optional.ofNullable(resource.getSpec()).map(spec -> {
final LeaderElectionRecord ret = new LeaderElectionRecord(
spec.getHolderIdentity(),
Duration.ofSeconds(spec.getLeaseDurationSeconds()),
spec.getAcquireTime(),
spec.getRenewTime(),
Optional.ofNullable(spec.getLeaseTransitions()).orElse(0));
ret.setVersion(resource.getMetadata().getResourceVersion());
return ret;
}).orElse(null);
return Optional.ofNullable(resource.getSpec()).map(spec -> new LeaderElectionRecord(
spec.getHolderIdentity(),
Duration.ofSeconds(spec.getLeaseDurationSeconds()),
spec.getAcquireTime(),
spec.getRenewTime(),
Optional.ofNullable(spec.getLeaseTransitions()).orElse(0))).orElse(null);
}

}
26 changes: 16 additions & 10 deletions .../java/io/fabric8/kubernetes/client/extended/leaderelection/resourcelock/ResourceLock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.base.PatchContext;
import io.fabric8.kubernetes.client.dsl.base.PatchType;

import java.io.Serializable;
import java.util.Objects;
import java.util.Optional;

Expand All @@ -40,7 +43,7 @@ public ResourceLock(String namespace, String name, String identity) {

@Override
public LeaderElectionRecord get(KubernetesClient client) {
return getResource(client).map(this::toRecord).orElse(null);
return getResource(client).map(this::toRecordInternal).orElse(null);
}

private Optional<T> getResource(KubernetesClient client) {
Expand All @@ -49,14 +52,13 @@ private Optional<T> getResource(KubernetesClient client) {

@Override
public void create(KubernetesClient client, LeaderElectionRecord leaderElectionRecord) {
client.resource(toResource(leaderElectionRecord, getObjectMeta(null), null)).create();
client.resource(toResource(leaderElectionRecord, getObjectMeta(leaderElectionRecord.getVersion()))).create();
}

@Override
public void update(KubernetesClient client, LeaderElectionRecord leaderElectionRecord) {
// this should be an edit, but we've disabled the ability for it to have optimistic locking
client.resource(getResource(client).map(r -> toResource(leaderElectionRecord, getObjectMeta(r), r))
.orElseThrow(() -> new NullPointerException())).lockResourceVersion().replace();
client.resource(toResource(leaderElectionRecord, getObjectMeta(leaderElectionRecord.getVersion())))
.patch(PatchContext.of(PatchType.STRATEGIC_MERGE));
}

/**
Expand All @@ -67,14 +69,18 @@ public void update(KubernetesClient client, LeaderElectionRecord leaderElectionR
* @param current may be null
* @return
*/
protected abstract T toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta, T current);
protected abstract T toResource(LeaderElectionRecord leaderElectionRecord, ObjectMeta meta);

protected LeaderElectionRecord toRecordInternal(T resource) {
LeaderElectionRecord result = toRecord(resource);
result.setVersion(resource.getMetadata().getResourceVersion());
return result;
}

protected abstract LeaderElectionRecord toRecord(T resource);

protected ObjectMeta getObjectMeta(T current) {
ObjectMetaBuilder builder = Optional.ofNullable(current).map(HasMetadata::getMetadata).map(ObjectMetaBuilder::new)
.orElse(new ObjectMetaBuilder());
return builder.withNamespace(namespace).withName(name).build();
protected ObjectMeta getObjectMeta(Serializable version) {
return new ObjectMetaBuilder().withNamespace(namespace).withName(name).withResourceVersion((String) version).build();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion kubernetes-examples/src/main/java/io/fabric8/kubernetes/examples/LeaderElectionExamples.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ private LeaderElector leader(String id, Function<String, Lock> lockSupplier) {
.withLeaderCallbacks(new LeaderCallbacks(
() -> System.out.printf("\r%1$s: I just became leader!!!%n", id),
() -> {
leaderReference.compareAndSet(id, null);
leaderReference.updateAndGet(s -> id.equals(s)?null:s);
System.out.printf("\r%1$s: I just lost my leadership :(%n", id);
},
leaderReference::set))
Expand Down
43 changes: 31 additions & 12 deletions kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/LeaderElectionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ public class LeaderElectionTest {
@Test
public void singleLeaderConfigMapLockCreateTest() throws Exception {
// Given
server.expect().post().withPath("/api/v1/namespaces/namespace/configmaps")
.andReturn(200, null).once();
server.expect()
.post()
.withPath("/api/v1/namespaces/namespace/configmaps")
.andReturn(200, null)
.once();
// When - Then
testAndAssertSingleLeader("lead-config-map",
new ConfigMapLock("namespace", "name", "lead-config-map"));
Expand All @@ -61,7 +64,9 @@ public void singleLeaderConfigMapLockCreateTest() throws Exception {
@Test
public void singleLeaderConfigMapLockUpdateTest() throws Exception {
// Given
server.expect().get().withPath("/api/v1/namespaces/namespace/configmaps/name")
server.expect()
.get()
.withPath("/api/v1/namespaces/namespace/configmaps/name")
.andReturn(200, new ConfigMapBuilder()
.withNewMetadata()
.withResourceVersion("1")
Expand All @@ -71,8 +76,11 @@ public void singleLeaderConfigMapLockUpdateTest() throws Exception {
.endMetadata()
.build())
.always();
server.expect().put().withPath("/api/v1/namespaces/namespace/configmaps/name")
.andReturn(200, null).once();
server.expect()
.patch()
.withPath("/api/v1/namespaces/namespace/configmaps/name")
.andReturn(200, null)
.once();
// When - Then
testAndAssertSingleLeader("lead-config-map",
new ConfigMapLock("namespace", "name", "lead-config-map"));
Expand All @@ -81,8 +89,11 @@ public void singleLeaderConfigMapLockUpdateTest() throws Exception {
@Test
public void singleLeaderLeaseLockCreateTest() throws Exception {
// Given
server.expect().post().withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases")
.andReturn(200, null).once();
server.expect()
.post()
.withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases")
.andReturn(200, null)
.once();
// When - Then
testAndAssertSingleLeader("lead-lease",
new LeaseLock("namespace", "name", "lead-lease"));
Expand All @@ -91,9 +102,13 @@ public void singleLeaderLeaseLockCreateTest() throws Exception {
@Test
public void singleLeaderLeaseLockUpdateTest() throws Exception {
// Given
server.expect().get().withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases/name")
server.expect()
.get()
.withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases/name")
.andReturn(200, new LeaseBuilder()
.withNewMetadata().withResourceVersion("1").endMetadata()
.withNewMetadata()
.withResourceVersion("1")
.endMetadata()
.withNewSpec()
.withHolderIdentity("not-lead-lease")
.withLeaseDurationSeconds(1)
Expand All @@ -103,8 +118,11 @@ public void singleLeaderLeaseLockUpdateTest() throws Exception {
.endSpec()
.build())
.always();
server.expect().put().withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases/name")
.andReturn(200, null).once();
server.expect()
.patch()
.withPath("/apis/coordination.k8s.io/v1/namespaces/namespace/leases/name")
.andReturn(200, null)
.once();
// When - Then
testAndAssertSingleLeader("lead-lease",
new LeaseLock("namespace", "name", "lead-lease"));
Expand All @@ -130,7 +148,8 @@ private void testAndAssertSingleLeader(String id, Lock lock) throws Exception {
stoppedLeading::countDown,
newLeaderRecord::set))
.build())
.build().run()));
.build()
.run()));
// Then
assertTrue(leaderLatch.await(10, TimeUnit.SECONDS));
assertEquals(id, newLeaderRecord.get());
Expand Down

0 comments on commit 8d52ae2

Please sign in to comment.