From 1d158f22990575026356a0adaf584dd043f51a84 Mon Sep 17 00:00:00 2001 From: john birch-evans Date: Thu, 27 Aug 2026 13:07:10 +0100 Subject: [PATCH 1/2] Fix applied by Claude --- .../bookkeeper/BookieDecommissionUtil.java | 9 +- .../bookkeeper/PodExecBookieAdminClient.java | 36 +++- .../bookkeeper/BookieOrdinalOrderTest.java | 194 ++++++++++++++++++ 3 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java diff --git a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java index be843672..eb866a85 100644 --- a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java +++ b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java @@ -27,9 +27,16 @@ public class BookieDecommissionUtil { public static int decommissionBookies(List allBookies, int numToDecommission, BookieAdminClient bookieAdminClient) { + // allBookies must be in ascending StatefulSet ordinal order. Kubernetes removes the highest + // ordinal first, so the bookies to decommission are the ones at the end of the list. List bookiesToRemove = new ArrayList<>(); int sz = allBookies.size(); - for (int i = sz - 1; i >= sz - numToDecommission; i--) { + int count = Math.min(numToDecommission, sz); + if (count < numToDecommission) { + log.warnf("Asked to decommission %d bookies but only %d are known, limiting to %d", + numToDecommission, sz, count); + } + for (int i = sz - 1; i >= sz - count; i--) { bookiesToRemove.add(allBookies.get(i)); } return decommissionBookies(bookiesToRemove, bookieAdminClient); diff --git a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java index 48ae5547..5c7937a5 100644 --- a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java +++ b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java @@ -74,11 +74,45 @@ public PodExecBookieAdminClient(KubernetesClient client, String namespace, CRDConstants.LABEL_RESOURCESET, bookkeeperSetName)); } + /** + * Orders bookies by their StatefulSet ordinal, ascending. + * + *

Do not compare the pod names as strings. A string sort puts "bookkeeper-9" after + * "bookkeeper-12", so any caller that takes bookies from the end of the list selects the wrong + * pods as soon as the set has 10 or more replicas. Kubernetes always deletes the highest + * ordinal first, so the list order must be numeric to agree with it. + */ + static final Comparator ORDINAL_ORDER = Comparator + .comparingInt((BookieInfo b) -> podOrdinal(b.getPodResource().get().getMetadata().getName())) + .thenComparing(b -> b.getPodResource().get().getMetadata().getName()); + + /** + * Returns the StatefulSet ordinal of a pod, or {@link Integer#MAX_VALUE} if the name does not + * end in a number. An unparseable name sorts last but never throws, so a stray pod that matches + * the selector cannot break the autoscaler. + */ + static int podOrdinal(String podName) { + if (podName == null) { + return Integer.MAX_VALUE; + } + final int dash = podName.lastIndexOf('-'); + if (dash < 0 || dash == podName.length() - 1) { + log.warnf("Bookie pod name %s has no ordinal suffix, ordering it last", podName); + return Integer.MAX_VALUE; + } + try { + return Integer.parseInt(podName.substring(dash + 1)); + } catch (NumberFormatException e) { + log.warnf("Bookie pod name %s has a non-numeric ordinal suffix, ordering it last", podName); + return Integer.MAX_VALUE; + } + } + @Override public List collectBookieInfos() { this.bookieInfos = client.pods().inNamespace(namespace).withLabels(podSelector).resources() .map(pod -> getBookieInfo(pod)) - .sorted(Comparator.comparing(b -> b.podResource.get().getMetadata().getName())).toList(); + .sorted(ORDINAL_ORDER).toList(); return bookieInfos; } diff --git a/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java b/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java new file mode 100644 index 00000000..d232ccc6 --- /dev/null +++ b/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java @@ -0,0 +1,194 @@ +/* + * Copyright DataStax, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.kaap.autoscaler.bookkeeper; + +import io.fabric8.kubernetes.api.model.Pod; +import io.fabric8.kubernetes.api.model.PodBuilder; +import io.fabric8.kubernetes.client.dsl.PodResource; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +/** + * Guards the ordering of bookies used to choose which pods to decommission. + * + *

A string sort of the pod names puts "bookkeeper-9" after "bookkeeper-12", which made the + * operator decommission bookkeeper-9 while Kubernetes deleted bookkeeper-12. The set must be + * ordered numerically so that both agree. + */ +public class BookieOrdinalOrderTest { + + private static final String PREFIX = "pulsar-bookkeeper-"; + + private static BookieAdminClient.BookieInfo bookie(int ordinal) { + final String podName = PREFIX + ordinal; + final Pod pod = new PodBuilder() + .withNewMetadata() + .withName(podName) + .endMetadata() + .build(); + final PodResource podResource = Mockito.mock(PodResource.class); + Mockito.when(podResource.get()).thenReturn(pod); + return BookieAdminClient.BookieInfo.builder() + .podResource(podResource) + .bookieId(podName + ":3181") + .build(); + } + + private static List podNames(List bookies) { + return bookies.stream() + .map(b -> b.getPodResource().get().getMetadata().getName()) + .collect(Collectors.toList()); + } + + /** Builds a shuffled set of 13 bookies, so the test cannot pass on input order alone. */ + private static List thirteenBookiesUnordered() { + final List bookies = IntStream.range(0, 13) + .mapToObj(BookieOrdinalOrderTest::bookie) + .collect(Collectors.toCollection(ArrayList::new)); + Collections.shuffle(bookies); + return bookies; + } + + @Test + public void testOrdinalParsedFromPodName() { + Assertions.assertEquals(0, PodExecBookieAdminClient.podOrdinal(PREFIX + "0")); + Assertions.assertEquals(9, PodExecBookieAdminClient.podOrdinal(PREFIX + "9")); + Assertions.assertEquals(12, PodExecBookieAdminClient.podOrdinal(PREFIX + "12")); + } + + @Test + public void testUnparseableNameSortsLastAndDoesNotThrow() { + Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal(null)); + Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("no-ordinal-here")); + Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("trailing-dash-")); + Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("nodashatall")); + } + + /** + * With 13 replicas a string sort produces 0, 1, 10, 11, 12, 2 ... 9. This asserts the numeric + * order instead. + */ + @Test + public void testDoubleDigitBookiesSortNumerically() { + final List bookies = thirteenBookiesUnordered(); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + final List expected = IntStream.range(0, 13) + .mapToObj(i -> PREFIX + i) + .collect(Collectors.toList()); + Assertions.assertEquals(expected, podNames(bookies)); + } + + /** + * The regression itself: scaling 13 down to 12 must decommission bookkeeper-12, because that is + * the pod Kubernetes will delete. Before the fix this selected bookkeeper-9. + */ + @Test + public void testHighestOrdinalIsDecommissionedFirst() { + final List bookies = thirteenBookiesUnordered(); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); + final int decommissioned = BookieDecommissionUtil.decommissionBookies(bookies, 1, adminClient); + + Assertions.assertEquals(1, decommissioned); + Assertions.assertEquals(List.of(PREFIX + "12"), adminClient.getDecommissionedPodNames()); + } + + /** Scaling 13 down to 10 must take 12, 11 and 10, in that order. */ + @Test + public void testMultipleHighestOrdinalsAreDecommissionedInDescendingOrder() { + final List bookies = thirteenBookiesUnordered(); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); + final int decommissioned = BookieDecommissionUtil.decommissionBookies(bookies, 3, adminClient); + + Assertions.assertEquals(3, decommissioned); + Assertions.assertEquals( + List.of(PREFIX + "12", PREFIX + "11", PREFIX + "10"), + adminClient.getDecommissionedPodNames()); + } + + /** Asking for more bookies than exist must not overrun the list. */ + @Test + public void testDecommissionRequestLargerThanSetIsClamped() { + final List bookies = thirteenBookiesUnordered(); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); + final int decommissioned = BookieDecommissionUtil.decommissionBookies(bookies, 20, adminClient); + + Assertions.assertEquals(13, decommissioned); + Assertions.assertEquals(13, adminClient.getDecommissionedPodNames().size()); + } + + /** + * A stub that records which pods were taken through decommission and reports every step as + * successful, so the test asserts on selection rather than on recovery behaviour. + */ + private static class RecordingBookieAdminClient implements BookieAdminClient { + + private final List cookiesDeleted = new ArrayList<>(); + + List getDecommissionedPodNames() { + return cookiesDeleted; + } + + @Override + public List collectBookieInfos() { + return List.of(); + } + + @Override + public BookieStats collectBookieStats(BookieInfo bookieInfo) { + return BookieStats.builder().isWritable(true).ledgerDiskInfos(List.of()).build(); + } + + @Override + public void setReadOnly(BookieInfo bookieInfo, boolean readonly) { + } + + @Override + public void recoverAndDeleteCookieInZk(BookieInfo bookieInfo, boolean deleteCookie) { + } + + @Override + public boolean existsLedger(BookieInfo bookieInfo) { + return false; + } + + @Override + public boolean doesNotHaveUnderReplicatedLedgers() { + return true; + } + + @Override + public void triggerAudit() { + } + + @Override + public void deleteCookieOnDisk(BookieInfo bookieInfo) { + cookiesDeleted.add(bookieInfo.getPodResource().get().getMetadata().getName()); + } + } +} From 646beabb5283048221fb55169d5c5a9cbe638659 Mon Sep 17 00:00:00 2001 From: john birch-evans Date: Thu, 27 Aug 2026 15:59:43 +0100 Subject: [PATCH 2/2] Apply Claude fixes --- .../bookkeeper/BookieDecommissionUtil.java | 15 +++-- .../bookkeeper/PodExecBookieAdminClient.java | 20 ++++--- .../bookkeeper/BookieOrdinalOrderTest.java | 55 ++++++++++++++++--- 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java index eb866a85..535cc844 100644 --- a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java +++ b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieDecommissionUtil.java @@ -31,12 +31,17 @@ public static int decommissionBookies(List allBook // ordinal first, so the bookies to decommission are the ones at the end of the list. List bookiesToRemove = new ArrayList<>(); int sz = allBookies.size(); - int count = Math.min(numToDecommission, sz); - if (count < numToDecommission) { - log.warnf("Asked to decommission %d bookies but only %d are known, limiting to %d", - numToDecommission, sz, count); + if (numToDecommission > sz) { + // The pod list is smaller than the set we are scaling down, so it is incomplete. Stop + // here. Continuing would decommission every bookie we can see, including ones that must + // stay. The controller retries the reconciliation, by which time the list should be + // complete. + throw new IllegalStateException( + "Asked to decommission %d bookies but only %d are visible. The pod list looks " + .formatted(numToDecommission, sz) + + "incomplete, so no bookie will be decommissioned."); } - for (int i = sz - 1; i >= sz - count; i--) { + for (int i = sz - 1; i >= sz - numToDecommission; i--) { bookiesToRemove.add(allBookies.get(i)); } return decommissionBookies(bookiesToRemove, bookieAdminClient); diff --git a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java index 5c7937a5..87523a4e 100644 --- a/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java +++ b/operator/src/main/java/com/datastax/oss/kaap/autoscaler/bookkeeper/PodExecBookieAdminClient.java @@ -87,24 +87,28 @@ public PodExecBookieAdminClient(KubernetesClient client, String namespace, .thenComparing(b -> b.getPodResource().get().getMetadata().getName()); /** - * Returns the StatefulSet ordinal of a pod, or {@link Integer#MAX_VALUE} if the name does not - * end in a number. An unparseable name sorts last but never throws, so a stray pod that matches - * the selector cannot break the autoscaler. + * Returns the StatefulSet ordinal of a pod, or {@link Integer#MIN_VALUE} if the name does not + * end in a number. + * + *

An unparseable name never throws, so a stray pod that matches the selector cannot break the + * autoscaler. It sorts first, not last, because callers take the bookies to decommission from the + * end of the list. A pod we cannot identify must never be a candidate for decommission. */ static int podOrdinal(String podName) { if (podName == null) { - return Integer.MAX_VALUE; + log.warn("Bookie pod has no name, ordering it first"); + return Integer.MIN_VALUE; } final int dash = podName.lastIndexOf('-'); if (dash < 0 || dash == podName.length() - 1) { - log.warnf("Bookie pod name %s has no ordinal suffix, ordering it last", podName); - return Integer.MAX_VALUE; + log.warnf("Bookie pod name %s has no ordinal suffix, ordering it first", podName); + return Integer.MIN_VALUE; } try { return Integer.parseInt(podName.substring(dash + 1)); } catch (NumberFormatException e) { - log.warnf("Bookie pod name %s has a non-numeric ordinal suffix, ordering it last", podName); - return Integer.MAX_VALUE; + log.warnf("Bookie pod name %s has a non-numeric ordinal suffix, ordering it first", podName); + return Integer.MIN_VALUE; } } diff --git a/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java b/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java index d232ccc6..b5e273a6 100644 --- a/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java +++ b/operator/src/test/java/com/datastax/oss/kaap/autoscaler/bookkeeper/BookieOrdinalOrderTest.java @@ -39,7 +39,10 @@ public class BookieOrdinalOrderTest { private static final String PREFIX = "pulsar-bookkeeper-"; private static BookieAdminClient.BookieInfo bookie(int ordinal) { - final String podName = PREFIX + ordinal; + return namedBookie(PREFIX + ordinal); + } + + private static BookieAdminClient.BookieInfo namedBookie(String podName) { final Pod pod = new PodBuilder() .withNewMetadata() .withName(podName) @@ -75,12 +78,31 @@ public void testOrdinalParsedFromPodName() { Assertions.assertEquals(12, PodExecBookieAdminClient.podOrdinal(PREFIX + "12")); } + /** + * A name we cannot read must sort first, not last. Bookies are taken from the end of the list, so + * a pod we cannot identify must never become a candidate for decommission. + */ @Test - public void testUnparseableNameSortsLastAndDoesNotThrow() { - Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal(null)); - Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("no-ordinal-here")); - Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("trailing-dash-")); - Assertions.assertEquals(Integer.MAX_VALUE, PodExecBookieAdminClient.podOrdinal("nodashatall")); + public void testUnparseableNameSortsFirstAndDoesNotThrow() { + Assertions.assertEquals(Integer.MIN_VALUE, PodExecBookieAdminClient.podOrdinal(null)); + Assertions.assertEquals(Integer.MIN_VALUE, PodExecBookieAdminClient.podOrdinal("no-ordinal-here")); + Assertions.assertEquals(Integer.MIN_VALUE, PodExecBookieAdminClient.podOrdinal("trailing-dash-")); + Assertions.assertEquals(Integer.MIN_VALUE, PodExecBookieAdminClient.podOrdinal("nodashatall")); + } + + /** An unrecognised pod is never selected, even when the whole set is scaled down. */ + @Test + public void testUnparseableNameIsNeverDecommissioned() { + final List bookies = thirteenBookiesUnordered(); + bookies.add(namedBookie("stray-bookie-pod")); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + Assertions.assertEquals("stray-bookie-pod", podNames(bookies).get(0)); + + final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); + BookieDecommissionUtil.decommissionBookies(bookies, 13, adminClient); + + Assertions.assertFalse(adminClient.getDecommissionedPodNames().contains("stray-bookie-pod")); } /** @@ -129,14 +151,29 @@ public void testMultipleHighestOrdinalsAreDecommissionedInDescendingOrder() { adminClient.getDecommissionedPodNames()); } - /** Asking for more bookies than exist must not overrun the list. */ + /** + * If more bookies are requested than the pod list contains, the list is incomplete. Refuse the + * whole operation instead of decommissioning every visible bookie. The controller retries. + */ + @Test + public void testDecommissionRequestLargerThanSetIsRefused() { + final List bookies = thirteenBookiesUnordered(); + bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); + + final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); + Assertions.assertThrows(IllegalStateException.class, + () -> BookieDecommissionUtil.decommissionBookies(bookies, 20, adminClient)); + Assertions.assertTrue(adminClient.getDecommissionedPodNames().isEmpty()); + } + + /** The exact size of the set is allowed, so scaling a whole set to zero still works. */ @Test - public void testDecommissionRequestLargerThanSetIsClamped() { + public void testDecommissionRequestEqualToSetSizeIsAllowed() { final List bookies = thirteenBookiesUnordered(); bookies.sort(PodExecBookieAdminClient.ORDINAL_ORDER); final RecordingBookieAdminClient adminClient = new RecordingBookieAdminClient(); - final int decommissioned = BookieDecommissionUtil.decommissionBookies(bookies, 20, adminClient); + final int decommissioned = BookieDecommissionUtil.decommissionBookies(bookies, 13, adminClient); Assertions.assertEquals(13, decommissioned); Assertions.assertEquals(13, adminClient.getDecommissionedPodNames().size());