refactor: migrate zookeeper-operator to the redesigned base-operator-go framework - #362
Merged
Merged
Conversation
Refactor the zookeeper-operator to use the operator-go SDK's reconciler framework and the new SecretProvisioner API for CSI secret volume management. Key changes: - New controller package using reconciler.RoleGroupHandler pattern - ZkRoleGroupHandler with buildSecretProvisioner() that declaratively registers server-tls, client-tls, and quorum-tls CSI volumes - ZookeeperSecurity.ConfigSettings() now accepts SecretProvisioner for path resolution instead of hardcoded directory constants - Replaced old AddVolumeMounts() with provisioner Volumes()/VolumeMounts() - Added public accessors to ZookeeperSecurity for secret class info - Deleted internal/util/secret.go (replaced by framework) - Removed duplicate annotation constants from internal/constant - Updated znode controller to use new discovery and resource utilities Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard server TLS registration against empty ServerSecretClass - Cache runtime scheme in package-level var instead of per-call allocation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion
- Fix panic when TLS auth class exists without serverSecretClass by
falling back to TlsDefaultSecretClass ("tls") with log warning
- Update E2E test mount paths to match SecretProvisioner convention
(/kubedoop/mount/server-tls/ instead of /kubedoop/server_tls/)
- Replace unsafe Containers[0] index with name-based container lookup
- Consolidate duplicated JvmJmxOpts and ToProperties into util package
- Remove unused RBAC pods/exec permission, fix import ordering
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Integrate Vector sidecar injection for log collection when logging is enabled, bridge ZK log volume into Vector, and update e2e test selectors to match the new base-operator-go label conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ZkRoleGroupHandler now embeds reconciler.BaseRoleGroupHandler: the framework builds
the canonical labels, headless/client Services, StatefulSet (with data PVC) and PDB,
and the handler customizes the returned StatefulSet (start command, exec probes, TLS
CSI volumes, config/log volumes) and ConfigMap. The myid init container is injected
through the SidecarManager (StaticContainerProvider); Vector runs as a native sidecar.
Labels: adopt the framework canonical app.kubernetes.io/* descriptive labels (plus
app.kubernetes.io/name) and use product-owned zookeeper.kubedoop.dev/{cluster,role}
identity labels for all operator selectors (cluster Service, health-check pod list,
metrics Service) so they never select another product's pods.
Naming: resources follow the framework "<cluster>-<role>-<group>" convention; discovery
pod FQDNs and the main container are aligned (container named "server").
ConfigMap: logback via the framework generator with a rolling file appender so the
Vector sidecar can collect logs; storage defaults when resources.storage is omitted.
Adds ClusterServiceExtension for the cluster-wide client Service; removes the
hand-rolled StatefulSet/Service builders. Updates unit tests and e2e label/name asserts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Switch from the local `replace => ../operator-go` to the published operator-go containing the framework enhancements (PR #494), pinned to v0.12.7-0.20260623053059-70aacdb72243. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Temporary local-development dependency: builds zookeeper-operator against the local operator-go checkout for in-tandem iteration. Must be dropped (restore the plain pseudo-version require) before opening the PR, since upstream CI cannot resolve the local path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Still developing against the local `replace => ../operator-go` (the framework refactor is ongoing); this only changes zk-side code. - logging: generate logback via the framework's productlogging (reconciler.RenderContainerLogging) instead of the removed config generator. - keep the main container name "zookeeper" (referenced via a constant) with matching e2e assertions; exec the JVM so it receives SIGTERM for graceful shutdown. - extract serverRoleName / bashShell constants; resolve golangci-lint findings (unparam, staticcheck, unconvert, gofmt, lll) surfaced during the migration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Makefile pinned golangci-lint v2.8.0 (built with go1.24), incompatible with the module's go1.25.8 so `make lint` could not run; CI used the action default (unpinned), so local and CI could drift. Pin a go1.25-compatible v2.12.2 and derive the CI version from the Makefile (single source of truth). Enable goconst ignore-tests so test fixtures may repeat short literals (role names, namespaces). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… done Add a prominent comment above the replace => ../operator-go directive so it is not stripped again during future iterations. The base-operator-go refactor is still in progress and built locally; the replace must stay until it lands upstream and operator-go is pinned to a released version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ext/Vector/logging) Migrate the server role group handler onto the merged + redesigned operator-go framework, dropping the product-level workarounds: - ServiceAccount binding, the default pod/container SecurityContext, and the Vector log bridge are now framework-owned (#498 / #499 / #501); remove the manual podSpec wiring and append (not replace) main-container volume mounts. - Adopt the Vector-provider-owned shared log pipeline (operator-go #503): rename the main container via MainContainerName, declare LoggingContainers + LogVolumeSize, and render logback.xml + vector.yaml through reconciler.RenderLoggingConfigMapData. - Delete server_logging.go (buildVectorConfigMapData is framework-owned); the CR implements reconciler.VectorAggregatorProvider so the framework generates vector.yaml from spec.clusterConfig.vectorAggregatorConfigMapName. Drop ContainerLogging.OutputFile (framework derives <container>.stdout.log). Builds against local ../operator-go (feat/vector-owned-log-pipeline) pending the #503 merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
operator-go #506 makes EnableServiceLinks=false a framework default (overridable via PodOverrides). Drop zk's manual podSpec.EnableServiceLinks — which was also a latent bug: set after the base build, it clobbered any user podOverrides.spec.enableServiceLinks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
operator-go #507/#508 make the framework unconditionally mount the role-group config ConfigMap at the kubedoop-canonical KubedoopConfigDirMount (/kubedoop/mount/config). zk now consumes it instead of stripping + re-adding its own: - drop removeNamedVolume/removeNamedVolumeMount + zk's own 'config' volume/mount (framework provides it); drop the redundant 'log-config' volume (same ConfigMap, second mount) and its copy — logback.xml reaches /kubedoop/config via the config-mount copy, which is where the JVM reads it (-Dlogback.configurationFile). - delete zk's divergent internal/constant KubedoopConfigDirMount (/kubedoop/config-mount) and KubedoopLogDirMount (/kubedoop/log-mount); the start-script copy source now references the framework's canonical pkg/constant.KubedoopConfigDirMount (single source of truth). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adopt operator-go's VolumeProvider (operator-go#510): register the per-CR SecretProvisioner on buildCtx.VolumeProviders before base.BuildResources so the framework injects the TLS CSI volumes into the pod and the main container, instead of hand-appending secretProvisioner.Volumes()/VolumeMounts() in customizeStatefulSet after the base build. - zk_handler.go: append secretProvisioner to buildCtx.VolumeProviders; drop the secretProvisioner arg from the customizeStatefulSet call. - server_statefulset.go: remove the manual podSpec.Volumes / main.VolumeMounts appends and the now-unused secretProvisioner param + opgosecurity import; refresh the doc comment. secretProvisioner is still built per-CR and passed to buildConfigMap (keystore paths), so nothing else changes. Build/vet/test/lint (golangci-lint v2.12.2) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The main container start script carried a leftover `ls /kubedoop/ > /dev/null 2>&1 || true` — a no-op with no output or side effect, left behind when the old Vector shutdown-file / trap / wait block was replaced by `exec`. Remove it; the start script now just copies the config, echoes, and execs zkServer.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serverCnxnFactory=NettyServerCnxnFactory and authProvider.x509 were emitted only in the quorum-TLS branch of ConfigSettings, gated on quorumSecretClass. But both are required by ZooKeeper for ANY TLS: client/server TLS is unusable with the default NIO connection factory, and X509 client-cert auth needs the X509 provider. A client-TLS-only config — a TLS AuthenticationClass with no `tls` block, so quorumSecretClass is empty while TLSEnabled() is true — therefore opened the secure client port with a keystore and clientAuth=need but WITHOUT Netty or the X509 provider, so TLS never worked. (Pre-existing since before the framework refactor; the common path is unaffected because quorumSecretClass defaults to "tls" whenever a tls block is present.) Move both settings to a common block emitted whenever TLSEnabled() || quorum TLS. Add internal unit tests for ConfigSettings covering no-TLS / server-only / quorum-only / server+quorum (the security package previously had zero coverage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a ZookeeperZnode was being deleted but its referenced ZookeeperCluster no longer existed (e.g. the cluster was deleted first), Reconcile returned early with a 10s requeue from the getClusterInstance error path — before the finalizer logic ran. The delete finalizer could therefore never complete, so the ZookeeperZnode was stuck forever, requeueing every 10s. - getClusterInstance now returns the original API error instead of collapsing every failure into an opaque sentinel, so the caller can tell "not found" from a transient error. - On not-found: if the znode is being deleted, drop the delete finalizer directly (the ensemble is gone, there is nothing to remove from ZooKeeper); otherwise keep the existing "wait for the cluster to appear" requeue. Transient API errors now propagate (requeue with backoff) instead of being silently swallowed. - Add a unit test (fake client) for the deleting-znode + missing-cluster case; the znodecontroller package previously had no tests. Pre-existing since before the framework refactor. Build/vet/test/lint (v2.12.2) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The base-operator-go framework work has all merged upstream (operator-go PRs #498/#499/#501/#503/#506/#507/#508/#510), so the local `replace => ../operator-go` is no longer needed. Pin to the operator-go main commit that carries the full refactor (v0.12.7-0.20260705104121-d90fc0400ed4, commit d90fc04) via a pseudo- version so CI resolves the dependency from the public module proxy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-pin to operator-go main (v0.12.7-0.20260705162431-27c59c1877a3, commit 27c59c1) which merges the ClusterOperation pause-gate ordering fix (#512, closes #511) and the ResourceName comment correction (#515). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ions
The base-operator-go refactor deleted the old zookeepercluster_controller.go along
with its kubebuilder RBAC markers, and the ZookeeperCluster controller — now wired
through operator-go's GenericReconciler — was left with none. The generated manager
ClusterRole therefore lacked list/watch on the operator's OWN ZookeeperCluster CRD as
well as create/manage on statefulsets, poddisruptionbudgets, serviceaccounts and
pods/exec. At runtime the manager could not sync its informer caches and crash-looped
("failed to wait for caches to sync"), so no cluster was ever reconciled.
Add an rbac.go marker file in internal/controller (lll is excluded there, matching the
znode controller markers) covering: zookeeperclusters(+status,+finalizers),
apps/statefulsets, policy/poddisruptionbudgets, core/serviceaccounts (full — the
framework provisions the SA), core/pods/exec (service health check), and
authentication.kubedoop.dev/authenticationclasses (TLS auth resolution). Regenerate
config/rbac/role.yaml. Verified in a kind e2e cluster: the operator now reconciles the
StatefulSet, ConfigMap and Services.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The tls-override-pdb test's final step exec'd into container 'server', a stale reference from before the main container was renamed to 'zookeeper' (the cp step two lines above already used 'zookeeper'). Align it so kubectl exec targets the real container. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
resolveImage produced a bare "<repo>/zookeeper:<productVersion>" tag whenever the CR did not set image.kubedoopVersion (the default path and the no-image path). But the kubedoop ZooKeeper product images are ONLY published as "<productVersion>-kubedoop<v>" (e.g. 3.9.3-kubedoop0.0.0-dev) — a bare "3.9.3" tag does not exist — so every default cluster failed to pull its image (ErrImagePull / NotFound), leaving all pods stuck in Init. The pre-refactor operator built the tag via operator-go's util.NewImage, defaulting the kubedoop version to the operator's own build version. Restore that: default kubedoopVersion to version.BuildVersion (injected via -ldflags, e.g. "0.0.0-dev") and always emit the "-kubedoop<version>" suffix, while still honoring image.custom / repo / productVersion / kubedoopVersion overrides. Verified in a kind e2e cluster: the server pod now pulls quay.io/zncdatadev/zookeeper: 3.9.3-kubedoop0.0.0-dev and reaches 1/1 Running. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The base-operator-go refactor dropped cluster-level discovery: the GenericReconciler only builds role-group-scoped resources, and the ZookeeperZnode controller only creates per-znode discovery ConfigMaps, so a ZookeeperCluster no longer published a discovery ConfigMap named after itself. Clients (and the znode discovery assertion) that expect to connect to the whole ensemble at the root znode without creating a ZookeeperZnode had nothing to read. Pre-refactor, the cluster reconcile owned this ConfigMap. Restore it in ClusterServiceExtension.PostReconcile (which already owns cluster-scope resources and runs after the role groups' pods/endpoints exist): build a ClusterInternal discovery ConfigMap named "<cluster>" (root znode "/") always, plus an ExternalUnstable "<cluster>-nodeport" ConfigMap for the external-unstable listener class — mirroring the per-znode discovery the ZookeeperZnode controller emits. Both are owned by the cluster and share the common discoverer. Verified: the znode e2e (test/e2e/znode) now passes end to end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ck layout
Re-pin operator-go to main (v0.12.7-0.20260707111227-2291143ff3ca), picking up 10
merged commits including several that resolve e2e findings:
- #523 restores the v0.12.6 stable Vector log pipeline: the framework no longer emits
the invalid VRL `string!(.message) ?? ""` (Vector E651 crash-loop), and the logback
file appender now writes /kubedoop/log/<container>/<container>.log4j.xml (XMLLayout)
for Vector edge parsing. Update the server logback default test to the new file path
and refresh the stale comment.
- #519 adds MetricsServiceBuilder.WithTargetPortName; call it with MetricsPortName so
the metrics Service targets the container port by name ("metrics") — matching the
observability e2e assertion and staying valid if the numeric port changes.
Build/vet/test/lint green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bump operator-go to pick up the stopped-reconcile-all fix (zncdatadev/ operator-go#529): stopped is now a replica modifier that reconciles every resource at 0 replicas instead of a reconcile short-circuit, so a cluster created stopped and config changes made while stopped are handled correctly. Rewrite the cluster-operation chainsaw test to exercise the full lifecycle: install -> 1, stopped -> 0, restart -> 1, then two pause cases proving the reconciliationPaused freeze semantics -- requesting stopped WHILE paused has no effect (stays 1), and unpausing applies the previously-frozen stop (-> 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-pins operator-go to the merged main (0994784) that emits one PodDisruptionBudget per role instead of per role group. The tls-override-pdb e2e asserts the role-level PDB "test-zk-server" (expectedPods=3 across both role groups), which this framework version now produces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
generateServerList numbered the zoo.cfg "server.N" entries from clusterConfig.minServerId (server.<minServerId+ordinal>), but the prepare init container wrote each pod's myid file as "MYID_OFFSET + ordinal" with MYID_OFFSET hardcoded to "1". The two only agreed at the default minServerId=1; any other value produced myid files that matched no server.N entry, so no node could join the quorum and the ensemble never formed. (The pre-refactor code used the same offset on both sides, so this was a regression introduced when the server list started honoring minServerId.) - Add resolveMinServerID(cr) as the single source of truth for the myid base, clamping values below 1 to 1 (ZooKeeper myid must be >= 1; minServerId: 0 would otherwise yield the invalid server.0). - generateServerList and the prepare init container's MYID_OFFSET both derive from it, so the on-disk myid always matches the server.N id for that ordinal. - Drop the dead MYID_OFFSET env from the main container (only the prepare init container writes myid; the main container never reads it). Unit tests assert resolveMinServerID's defaulting/clamping and that the prepare container's MYID_OFFSET equals the lowest server.N id for every minServerId (mutation-verified: the test fails if MYID_OFFSET is hardcoded). Adds a min-server-id chainsaw e2e (minServerId=5, 3 replicas) that asserts the quorum forms and that the myid files and zoo.cfg server.N ids are 5/6/7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…actor The refactor stopped calling the old DefaultServerConfig/MergeDefaultConfig path, so the ZooKeeper-specific defaults it injected were lost and the base-operator-go framework does not supply them (it applies resources/affinity/gracefulShutdown only when the merged role group config already carries them). A cluster without an explicit resources/affinity block therefore got: - no CPU/memory requests or limits, and an unbounded JVM heap (ZK_SERVER_HEAP was only set when memory was specified) — OOM risk; - no pod anti-affinity, so ensemble members could be co-scheduled onto one node, defeating ZooKeeper's availability model; - a 30s (k8s default) graceful-shutdown window instead of the previous 120s. Extend the existing storage defaulting into ensureServerConfigDefaults, which fills CPU (100m/200m), memory (512Mi, which also drives ZK_SERVER_HEAP), a weight-70 preferred pod anti-affinity keyed on the instance/component labels, and a 120s gracefulShutdownTimeout. Values are written into the role group config the framework reads, with field-level precedence group > role > default, so user values at either level win — and because the framework only reads the role-group config, folding the role-level value in here is also what finally makes role-level config take effect. Unit tests cover the minimal-cluster defaults, user overrides winning, role->group folding, and heap derivation. Adds a config-defaults chainsaw e2e asserting a bare cluster gets the anti-affinity, 120s grace, CPU/memory requests+limits and heap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
generateServerList only emitted server.N entries for the current role group's own pods, so each group's zoo.cfg described a separate quorum — while the discovery ConfigMap and health check aggregate pods across all groups into one connection string and one quorum calculation. A cluster with more than one server role group therefore formed N disjoint ensembles that discovery advertised as a single one, and every group reused MYID_OFFSET=1 so myids collided across groups. Make the server role a single ensemble spanning every role group: - serverGroupBaseIDs assigns each group a non-overlapping myid range (groups ordered by name, each base = minServerId + running total of prior groups' replicas), so every pod's myid is unique ensemble-wide. - generateServerList now walks all role groups and emits the full server.N list (identical in every group's zoo.cfg), with each entry pointing at that group's own headless Service. - The prepare init container's MYID_OFFSET is the pod's group base, so the on-disk myid matches the server.N id for that pod. - The standalone/ensemble switch is keyed on the total desired member count across groups (serverEnsembleSize), not the current group's replica count. Replica counts come from the desired spec (default 1 when unset), independent of stopped/paused scaling, so the quorum config is preserved while a StatefulSet is scaled to zero. Single-group clusters are unchanged (base = minServerId). Unit tests cover base assignment, the cross-group server list and FQDNs, ensemble size, and the per-group myid/server.N agreement. Adds a multi-rolegroup chainsaw e2e that asserts the spanning server list, unique myids, and — the real proof — that a znode written through one group is readable through the other. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…covery labels Follow-up cleanup after the default-restoration and single-ensemble fixes: - Remove internal/common/role_config.go and affinity.go (and the fully commented-out role_config_test.go). Since the handler builds its own role group defaults, these old DefaultServerConfig/MergeDefaultConfig/affinity-builder paths were entirely unreferenced dead code that misleadingly looked like the config source of truth. - getExpectedReplicas now delegates to serverEnsembleSize, so the health check's quorum math shares the one place that applies the "unset replicas -> 1 per group" rule and always matches the generated server.N list. - Discovery ConfigMap labels: "app.kubernetes.io/name" now carries the product (zookeeper) and the cluster name moves to "app.kubernetes.io/instance", matching the recommended-label semantics (previously "name" held the instance and "instance" was absent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…default
The e2e caught that a minimal cluster still got terminationGracePeriodSeconds=30
despite the default-restoration change. Root cause: the base-operator-go CRD marks
gracefulShutdownTimeout with a kubebuilder default of "30s", so the API server
auto-injects servers.config={gracefulShutdownTimeout:"30s"} even when the user
omits the block. ensureServerConfigDefaults then folded that injected "30s" in as
if the user had chosen it, so ZooKeeper's 120s product default never applied
(resources and affinity have no CRD default, which is why only grace was affected).
Treat the framework's platform default "30s" as "unset" when resolving the
graceful-shutdown window, so ZooKeeper's 120s default applies for a bare cluster
while any other explicit value (group or role level) is still honored.
Verified against a live minimal cluster: terminationGracePeriodSeconds is now 120.
Unit tests cover the CRD-injected-30s-at-role-level case and an explicit non-default
role value.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…etries) The cross-group read used the plaintext client port 2181, but ZooKeeper is TLS-by-default (serverSecretClass defaults to "tls"), so the client port is 2282 with portUnification (which accepts plaintext). Point zkCli at 2282, and retry the write/read since zkCli opens a fresh session per call and can hit a transient ConnectionLoss — distinguishing that from a genuine split ensemble, which returns a stable "Node does not exist". Verified end-to-end: the znode written through the default group is read back through the secondary group (cross-group read OK). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zkCli opens a fresh session with a TLS handshake on the unified client port for every call, which can take longer than the previous 2-retry window under load. Raise the unsecure-connection retries and wrap the secure-connection check in the same retry loop so a transient ConnectionLoss during session setup no longer fails the test; a genuine failure still surfaces after the retries are exhausted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ers (#531) Syncs operator-go to main (966aad8), which adds per-role overrides for MainContainerName and LoggingContainers (#531) and a test-only FakeRecorder buffer fix (#532). The per-role change is backward compatible — ZooKeeper has a single "server" role and keeps setting the global MainContainerName and LoggingContainers fields, so its behavior is unchanged. Build, unit tests and lint pass against the new module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…restarts CI's chainsaw run failed the multi-rolegroup "verify server list" step: the script read zoo.cfg and the myid files via kubectl exec under `set -euo pipefail` with no retry, so it failed spuriously when it raced the one-time pod restart that happens while a fresh multi-node ensemble forms. Wrap the checks (and the equivalent min-server-id myid check) in a retry loop that tolerates a transient exec failure and only fails after the values are stably wrong. Since install (availableReplicas) already proves the ensemble formed, the assertions themselves are sound — they just needed to wait out the restart. Verified: multi-rolegroup passes with the retries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chainsaw executes inline script content via /bin/sh, ignoring the #!/bin/bash shebang. On the Linux CI runner /bin/sh is dash, which does not support `set -o pipefail` and aborts immediately with exit status 2 — before any check runs — which is why the multi-rolegroup verification failed only in CI. Locally /bin/sh is bash (pipefail supported), so it passed and the failure could not be reproduced. Replace `set -uo/-euo pipefail` with the POSIX `set -u`/`set -eu` (matching the passing observability test); the checks already handle command failures explicitly with `|| return 1`, so pipefail was redundant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ssed by 62d06aa 62d06aa ("drop dead default code, unify quorum count, fix discovery labels") described these two changes in its message, but the `git add` that staged them aborted (a pathspec for an already-removed file made the whole command fatal), so only the dead-code deletions were committed. Commit the intended source changes: - Discovery ConfigMap labels: app.kubernetes.io/name now carries the product (zookeeper) and the cluster name moves to app.kubernetes.io/instance. - getExpectedReplicas delegates to serverEnsembleSize so the health check's quorum math shares the single "unset replicas -> 1 per group" rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-pins operator-go to main (4d5d74f), which sorts the container env vars built from EnvVars before appending them (#534). Without it, the map-iteration order was non-deterministic, so any cluster with envOverrides re-rendered a different StatefulSet every reconcile — an endless CreateOrUpdate loop that recreated the pods continuously and kept them from ever reaching readiness (which is why the tls-override-pdb e2e, whose cluster sets envOverrides, could not stabilize). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… test Earlier commits weakened the new e2e scripts to chase a CI failure (dropping set -e / -o pipefail and wrapping the checks in retry loops). The real cause was an operator-go reconcile-churn bug (fixed in #534), not the scripts, so restore min-server-id, config-defaults and multi-rolegroup to their strict, straightforward form (set -euo pipefail, no retries) — the tests are a gate and must stay strict. Also fix the one genuine staleness in the pre-existing test_tls.sh: it addressed the peer via <pod>.<resource> DNS, but the base-operator-go framework names the StatefulSet's governing headless Service <resource>-headless (the discovery ConfigMap advertises the same FQDN), so the per-pod name needs the -headless suffix. Verified: the full tls-override-pdb suite (install, PDB, env/config overrides, TLS handshake) passes end to end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion The new min-server-id/config-defaults/multi-rolegroup scripts used `set -euo pipefail`, a form that appears nowhere else in the suite — every existing e2e script uses `set -e` (or no `set`). chainsaw runs script content through /bin/sh, whose `-o pipefail` support is not portable, so those three scripts aborted immediately with exit status 2 under CI while the `set -e` scripts (e.g. logging, observability) ran fine. Align with the established `set -e` convention; the checks already surface failures directly (grep/`[ ]` as the last command in each pipeline), so pipefail was never needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI's chart-e2e fails the logging test with the StatefulSet reporting status.replicas=0 (no pods created), which does not reproduce locally and which the current chainsaw output does not explain (it dumps neither pod state nor operator logs). Add a global catch to the chainsaw config that, on any test failure and before cleanup deletes the namespace, dumps the failing namespace's StatefulSets (spec+status), pods, `describe pods` (scheduling/creation events), the namespace events, and the operator log. This makes the next CI failure self-diagnosing. The catch uses only POSIX sh so it runs under the CI shell. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The refactor onto base-operator-go runs the Vector log agent as a native sidecar: an init container with restartPolicy=Always and a readinessProbe. That requires the SidecarContainers feature gate, which is only on by default from k8s 1.29. chart-e2e still created its kind cluster at 1.26.15 (the pre-refactor default, unchanged since zncdatadev#347), where the API server drops the gated init-container restartPolicy but keeps the readinessProbe. Pod validation then rejects every server pod with spec.initContainers[1].readinessProbe: Forbidden: may not be set for init containers so the StatefulSet creates zero pods, status.replicas stays 0, and the logging test's assert times out at 300s. The failure did not reproduce locally only because the local kind node happened to be 1.35. Bump KIND_K8S_VERSION to 1.35.0 (native sidecars GA) and document 1.29 as the hard floor. The e2e suite was validated end-to-end on 1.35 locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move from 1.35.0 to 1.36.1, the highest published kindest/node image, so chart-e2e runs on the newest k8s the tests support. The 1.29 native-sidecar floor is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the missing authenticationclasses rule and drop grants the operator no longer has markers for: secrets, clusterroles, and write verbs on rolebindings. Chart rules now match config/rbac/role.yaml exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation Bumps operator-go from #534 (4d5d74f) to current main (0d765be), which lands the v0.13.0-unreleased standardization pass, and adapts zookeeper-operator to its breaking changes: - commons resource fields (CPU Min/Max, Memory Limit, Storage Capacity) and RoleGroupConfigSpec.GracefulShutdownTimeout became pointers so "unset" is representable. Construct them with ptr.To and nil-guard Capacity.IsZero. - gracefulShutdownTimeout is no longer defaulted by the CRD, so a nil pointer now means "unset". Drop the isUnsetGrace hack that treated the auto-injected "30s" as unset: an explicit "30s" is now honored, and ZooKeeper's 120s product default applies only when the value is nil at both the group and role level. - BaseRoleGroupHandler.ExtraLabels/ExtraAnnotations were removed (#555). Publish app.kubernetes.io/name through buildCtx.ClusterLabels, the per-reconcile map the framework copies onto every built resource and pod. The cluster Service selector keys on the product identity labels, not this one, so nothing else changes. ProductName is intentionally left unset: setting it would switch image resolution to spec.image, which cannot reproduce ZooKeeper's always -kubedoop-suffixed default tag. - The extension registry is now per-CR-type. Create it with NewExtensionRegistry[*ZookeeperCluster], re-type ClusterServiceExtension to that CR (dropping the ClusterInterface type-assertion dances), and pass it in GenericReconcilerConfig.ExtensionRegistry. Regenerate the CRD (which loses the removed default: markers) and sync the chart. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… liveness The base-operator-go migration gave the server container a liveness probe that TCP-checks the client port with a ~30s budget (initialDelay 10s + 3x10s). ZooKeeper 3.9.3 does not bind that port until ~25s into JVM startup, and under the default 200m CPU limit on a loaded node it slips past the budget: three liveness failures then SIGTERM the container (exit 143) into a CrashLoop before it ever serves, so the quorum never forms and the StatefulSet never becomes available. Pre-migration main had no liveness probe, so a slow start there only delayed readiness. The full-suite e2e run (contended node) exposed this. Add a startup probe running the same ruok check with a generous budget (30 x 10s = 5m). The kubelet suspends the liveness and readiness probes until the startup probe first succeeds, so a slow first start is no longer fatal while the liveness probe still guards steady-state hangs. Verified by re-running the logging e2e case against the rebuilt image: the servers start and the StatefulSet becomes available. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o v0.13.0 Two follow-up cleanups the operator-go bump enables: - Remove the manual SetProductImage call in registerServerContainers. operator-go #536 now propagates the product image to the framework-constructed Vector sidecar inside base.BuildResources, so the hand call is redundant; the function no longer needs to return an error. - Stop defaulting the data PVC capacity by hand. The framework builds the VolumeClaimTemplate with StorageResource.GetCapacity(), which applies the 10Gi DefaultStorageCapacity when unset, so ensureServerConfigDefaults only has to guarantee Resources.Storage is non-nil (the framework builds a PVC only then). Drops the defaultStorageCapacity constant and the IsZero re-defaulting. GracefulShutdownTimeout keeps its own defaulting: the framework's GetGracefulShutdownTimeout returns the 30s platform default, but ZooKeeper needs its longer 120s product default, so that helper does not apply. Verified against the rebuilt image: the full chainsaw e2e suite passes (logging, config-defaults, min-server-id, multi-rolegroup, cluster-operation, metrics, tls-override-pdb, znode; delete-rolegroup skipped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bumps operator-go from 0d765be to deb6578, and adapts to the four breaking changes plus the two seams that were added for this operator. ServiceAccountName is gone from GenericReconcilerConfig (#616). The framework now derives the workload ServiceAccount per CR as "<kind>-<cluster>" and owns it, which also fixes the failure the static name caused: every cluster of a product in one namespace resolved to the SAME ServiceAccount, so the second one to reconcile failed forever and deleting the first garbage-collected the SA out from under the other's running pods. WorkloadRBACRules stays unset — the ZooKeeper workload needs no Kubernetes API permissions, and a nil hook is what keeps the operator from having to grant itself cluster-wide access to roles and rolebindings. ProductName no longer decides whether spec.image is read (#581), so ZooKeeper can finally set it: the new ImageDefaults supplies whatever spec.image leaves empty, re-evaluated every reconcile, which is what the old API could not express and why this operator hand-rolled resolveImage. That function is deleted and resolution now goes through the same ImageSpec.ResolveImage the framework uses for the main container, so the prepare init container cannot drift from it, and an unresolvable spec.image is reported instead of being silently replaced by the default version. Setting ProductName also publishes app.kubernetes.io/name — no longer hand-written into ClusterLabels — and app.kubernetes.io/version, which this operator was not emitting at all. The primary container is customized through buildCtx.MainContainerCustomizer (#585) rather than by editing the built StatefulSet. The old code reached for podSpec.Containers[0], a position the framework never promised and that any sidecar provider inserting a container earlier would have silently broken. Precedence is unchanged: the hook runs after the framework applies envOverrides (so appending them after ours keeps the user's last word) and before podOverrides are strategic-merged. Per-CR ports move to buildCtx.ContainerPorts/ServicePorts (#582). One handler instance serves every ZookeeperCluster, so writing them to its fields let concurrent reconciles of different clusters overwrite each other. CRDs are regenerated: logging levels lose `default: INFO` (#573), the CRD default that made a role group's empty console block override the role's threshold. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The readiness probe ran with periodSeconds and timeoutSeconds both at 1 while the server container carries the default 200m CPU limit. The check forks bash, opens a TCP connection to the client port and greps the reply — normally ~100ms, but under that limit the container is CFS-throttled in the large majority of scheduling periods (measured: 78% of periods, with probe latencies reaching 1297ms), so the command regularly overran its one-second timeout even though ZooKeeper was serving and had formed a quorum. Three such samples in a row took a healthy server out of the Service endpoints, and with replicas flapping the StatefulSet never reported them all available: the logging e2e case failed its availability assert with both pods Running, 0 restarts and 1/2 ready. Raise the period and timeout to 5s, which covers the throttled tail. The failure threshold stays at 3, so a genuinely unresponsive server still leaves the endpoints within 15s. This is the same class of defect as the startup probe added in 7ff68fb, on the other probe: an exec probe budget that assumed an unthrottled container. Verified by re-running the full chainsaw suite, which now passes end to end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The operator has never held any permission on core Events, so every event the framework records — resources created and updated, a role group that failed to build, the ImmutableFieldIgnored warning explaining that a StatefulSet field the spec asks for cannot be applied — was rejected by the API server: events is forbidden: User "system:serviceaccount:zookeeper-operator:..." cannot create resource "events" in API group "" in the namespace "..." The information the operator meant to surface on the CR was therefore invisible to `kubectl describe`, leaving the operator log as the only place a user could learn why their cluster was not converging. Grant create and patch: repeated events are aggregated onto the existing object rather than written anew, so create alone still fails on the second occurrence. The helm chart's ClusterRole is maintained by hand, so it gets the same rule; the rendered chart role and the generated config/rbac/role.yaml are identical again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… dead merge
Two simplifications that follow from what the framework already does, with no
change to the rendered resources.
The default pod anti-affinity is now reconciler.DefaultAntiAffinity +
EncodeAffinity instead of a hand-built corev1.Affinity marshalled by hand. The
helper takes the same inputs and emits the same structure — the existing
assertions on weight 70, app.kubernetes.io/{instance,component} and the hostname
topology key pass untouched, which is the equivalence check. An encoding failure
is now propagated rather than swallowed into "no affinity at all": silently
dropping a scheduling constraint is worse than failing, because the pods land on
nodes the constraint existed to avoid.
ensureServerConfigDefaults no longer folds the role level itself. GenericReconciler
already hands the handler a role group spec whose Config is
MergeRoleGroupConfig(roleSpec.Config, groupSpec.Config), so every roleCfg/roleRes
branch here was unreachable: cfg.Resources.CPU == nil already implies the role set
none either. The doc comment claimed the opposite ("the framework itself only reads
the role-group config"), which is how the duplicate survived. What remains is what
the framework genuinely does not do: fill the fields nobody set.
The unit tests that drove this logic through RoleSpec were exercising the deleted
reimplementation, not the real contract, so they now feed RoleGroupSpec.Config —
the shape a handler is actually handed — and say so. Role-level config keeps its
end-to-end coverage in the tls-override-pdb case, which sets config, envOverrides
and configOverrides at the role level and asserts them.
Verified with the full chainsaw suite: all eight runnable cases pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…taining it The chart's ClusterRole duplicated the rules controller-gen writes into config/rbac/role.yaml from the kubebuilder markers, and nothing kept the copy in step. Adding a marker updated the kustomize deployment and silently left the chart behind — and the chart is what chart-e2e, the release and every user actually deploy. This repo has now fixed that same drift twice: 2ea40dd, and 867fd76, where a missing core/events rule meant every event the framework emitted was rejected in the helm deployment, invisibly, because event emission is best-effort. Fixing one hole twice is a missing-automation signal, not a diligence problem. helm-rbac-sync splices the generated rules into the chart template. Only the rules are generated: the document's name and labels come from helm helpers and its body is wrapped in an `{{- if .Values.serviceAccount.create -}}` guard, so it cannot be replaced wholesale, and a YAML-aware tool cannot parse a template in place. Splicing by line range keeps that chart-owned wrapper intact and needs nothing beyond sed, since `rules:` is the last top-level key of role.yaml. The CI gate is extended rather than duplicated. check-crds-sync already proved the files under config/ were current; it now regenerates the chart's copies too, so the artifacts that actually ship are the ones being verified — which also closes the same gap for the chart's CRDs. Verified three ways: the target is idempotent (byte-identical output on this repo), it is corrective (adding a marker without syncing is now caught by the gate, and the sync produces the missing rule), and the workflow still exposes the check-crds-sync job the two downstream jobs depend on. Reference implementation for zncdatadev#371, which other operator repos can copy: the chart layout is identical across them, and 0 of 14 have this automation today. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dler state Bumps operator-go to e8a9495 and adopts the declare / fold / derive split it introduces (#632), which answers the three issues this operator filed: the data PVC no longer reports an unchanged template as a dropped change (#627), the operator-side permissions the framework consumes are now documented (#629), and the config-defaults seam collapsed into one entry point with the CR in scope (#631). DeclareRoles replaces nine handler fields and four per-call assignments on the build context. Everything the server role is made of — container name, ports, entrypoint, the three probes, the data volume, log producers, log volume size, config defaults and static env — is now one statement produced per pass with the CR in hand. That is what makes it static data: the client port moves when the CR enables TLS and the anti-affinity selector names the cluster, and both are read from THIS cluster rather than written into handler state the next cluster would inherit. ResolveRoleGroup carries what cannot be declared beside the defaults that feed it. ZK_SERVER_HEAP is a function of the memory limit that survives the fold, so it is contributed after it, which is exactly the derived-value path we could not express before. ensureServerConfigDefaults is gone. Its content is declared as ConfigDefaults and folded by the framework under the CR's role and role group levels, which also retires a defect of the hand-rolled version: it tested `Resources.CPU == nil` as a whole, so a user who set only cpu.max silently lost the default cpu.min. The framework folds per leaf, so the default now survives. The primary container is no longer customized after the build — MainContainerCustomizer is removed upstream. The entrypoint carries its script inline in Command, because arguments are the user's channel through cliOverrides and a product writing them would delete what the user wrote. DefaultAntiAffinity is removed upstream; the default is now built from PreferredAffinityTerm and RoleSelectorLabels, which is the composable form that lets a product express more than one weighted term. Image resolution moves to GenericReconcilerConfig.ImageResolution, and the myid init container takes the reference the framework already resolved for the main container so the two cannot drift. Static gates pass: build, vet, unit tests, golangci-lint, and the CRDs regenerate unchanged. The rendered workload is NOT yet verified — every one of these moved the channel a value travels through, so the e2e suite is the gate and runs next. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The only conflict was the check-crds-sync step, where both sides had widened the same gate along different axes: - this branch widened WHAT is regenerated, so the chart's own copies of the CRDs and of the operator ClusterRole are verified too (they are what chart-e2e and the release actually deploy); - main (zncdatadev#365) widened WHAT COUNTS as drift, staging first so a newly generated but untracked file is caught, which a plain `git diff` ignores. Both are kept: regenerate everything, then compare the staged tree. Everything else merged cleanly, including main's chainsaw bump to v0.2.14 and its --set product_version wiring (zncdatadev#364), alongside this branch's helm-rbac-sync target and the 1.36.1 kind node. Verified after the merge: the gate command reports no drift, the operator builds, and all five unit test packages pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both conflicts were the golangci-lint version, where the two sides had fixed the same defect — CI installed the newest release and drifted from `make lint` — in different ways. main (zncdatadev#372) pinned the literal version in the workflow, with a comment asking the reader to keep it in sync with the Makefile. This branch derives it from the Makefile in a preceding step instead. The derivation is kept: it removes the duplicate main's own comment describes, which is the same reasoning that put the chart's ClusterRole under `make helm-rbac-sync` rather than under a review convention. The version itself is taken from main. v2.12.1 is the baseline zncdatadev#372 aligned to and what hive, hdfs and trino all pin; this branch's v2.12.2 would have left zookeeper as the only operator on a different linter. `make lint` reports no issues under v2.12.1. Verified after the merge: no generated-file drift, the operator builds, and all five unit test packages pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch tracked operator-go through pseudo-versions while the declare / fold / derive work was landing upstream. v0.13.0 is now tagged, so the dependency moves onto the release. It is a pin change and nothing else: the tag sits one commit past the pseudo-version this branch already used, and that commit only writes the CHANGELOG — the diff touches no Go file, so no adaptation is possible or needed. Verified: build, vet, all five unit test packages, golangci-lint (0 issues), and the generated files regenerate unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates zookeeper-operator onto the redesigned operator-go framework, moving product-generic
concerns (ServiceAccount, security context, config mount, Vector logging, CSI volume injection)
into the framework and keeping only ZooKeeper-specific logic in the operator. Consumes the merged
operator-go work (PRs #498/#499/#501/#503/#506/#507/#508/#510).
Depends on operator-go pinned via a pseudo-version of
main(v0.12.7-0.20260705104121-d90fc0400ed4);the previous local
replace => ../operator-gohas been dropped.What moved to the framework (removed from zk)
customizeStatefulSet).per-role-group override via
podOverrides./kubedoop/mount/config; zk dropped its divergent mount constants and its own config/log-configvolumes.
vector.yaml;zk declares one
ContainerLoggingproducer.VolumeProvider(zk registers itsper-CR
SecretProvisioneron the build context instead of hand-appending volumes/mounts).ZooKeeper-specific fixes included
serverCnxnFactory=NettyServerCnxnFactoryandauthProvider.x509are now emittedwhenever ANY TLS is enabled, not only for quorum TLS — a client-TLS-only config previously lost
them and TLS never came up. Adds unit tests for
ConfigSettings(package had none).ZookeeperZnodeno longer gets stuck behind its delete finalizer when thereferenced
ZookeeperClusteris already gone; transient API errors now propagate instead ofbeing swallowed. Adds the first znodecontroller unit test.
execs the JVM (direct SIGTERM / graceful shutdown) and relies on theframework's native Vector sidecar for shutdown ordering, replacing the old background + trap +
wait + shutdown-file dance; single config-copy source; dropped a vestigial no-op line.
zookeeperfor backward compatibility.Testing
Unit tests + lint green (
go test ./..., golangci-lint v2.12.2). e2e not yet run in CI.Notes / follow-ups
changeit(pre-existing); to be revisited from thesecret-operator side.
client.portUnification(always on when TLS is enabled) — worth an e2e sanity check with a client-cert auth class.
🤖 Generated with Claude Code