Fix: kubernetes and etcd-operator issues - #119

…4986)

## What this PR does

The tenant etcd chart spreads members with `maxSkew: 1` on
`kubernetes.io/hostname` and `whenUnsatisfiable: ScheduleAnyway`, the
default since #119. `ScheduleAnyway` only adds to the score of a node
and other scores can outweigh it, so two members of one etcd can still
start on the same node, where a node-local volume then keeps them. This
PR makes the default `DoNotSchedule` with `nodeTaintsPolicy: Ignore`.
The branch for `_cluster.scheduling.globalAppTopologySpreadConstraints`
is untouched, so a platform that sets its own policy renders exactly as
before, and the soft pod anti-affinity from #3962 stays.

Seen on a three-node cluster running v1.6.4, whose `etcd-cluster.yaml`
and etcd-operator v0.5.5 are the same as in v1.6.5. A new tenant etcd
got the Pods of its first two members on one node, created 26 seconds
apart, and the third on another node 18 seconds later. The Pods carry
the `ScheduleAnyway` constraint and were scheduled by
`linstor-scheduler`, whose profile adds the LINSTOR extender's
prioritize score to the same sum with weight 5. The data volumes are on
a node-local StorageClass with `WaitForFirstConsumer` and
`allowRemoteVolumeAccess: "false"`, so the two members cannot leave that
node, and losing it takes the quorum. A second tenant etcd created there
the next day came up the same way. On a test cluster four tenant etcds
created within one week each had two members on one node, and we moved
one member of each by hand.

`DoNotSchedule` does not stop an etcd with more members than nodes from
starting when every node can run its Pods, unlike a required
anti-affinity, because the skew is counted between the nodes that exist.
kube-scheduler rejects a node only when the matching Pods on it plus the
new one exceed the node with the fewest by more than `maxSkew`
([filtering.go](https://github.com/kubernetes/kubernetes/blob/v1.34.1/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go#L334-L352)).
One node takes all three members, two nodes take two and one, and three
or more nodes take one each. A member whose only allowed node cannot
take it, for example because the storage pool there is full, now waits
Pending instead of joining another member.

`nodeTaintsPolicy: Ignore` is the Kubernetes default and is written out
on purpose. A cordoned node carries `node.kubernetes.io/unschedulable`,
and `Honor` would take it out of the count
([common.go](https://github.com/kubernetes/kubernetes/blob/v1.34.1/pkg/scheduler/framework/plugins/podtopologyspread/common.go#L50-L54)),
so a member created during a drain would start next to another member
and stay there on a node-local volume. With `Ignore` it waits Pending
until the node is back, while the other two keep quorum. The cost is
that a node no member can run on, such as a tainted control plane,
counts as an empty node too, so on a cluster with such a node each
schedulable node takes at most one member and the members beyond that do
not start. #4947 makes the same choice for the SeaweedFS masters and
checked both policies in kwok. A platform that wants the old behaviour
can set `scheduling.globalAppTopologySpreadConstraints` to
`ScheduleAnyway`, which the other charts that read it pick up as well,
and a new test case pins that the chart renders such a policy as
written.

On upgrade nothing is recreated. The etcd-operator adopts a changed
`spec.topologySpreadConstraints` into `status.observed` once the cluster
has reached its current target, or its progress deadline has passed
([etcdcluster_controller.go#L203-L214](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdcluster_controller.go#L203-L214)).
It copies the value from there into the spec of an `EtcdMember` only
when it creates one, at bootstrap
([#L446](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdcluster_controller.go#L446))
and on scale-up
([#L851](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdcluster_controller.go#L851)),
and nothing in the operator writes it into an existing `EtcdMember`. The
member controller creates a Pod only when it is missing and builds it
from that member's own copy
([etcdmember_controller.go#L499-L512](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdmember_controller.go#L499-L512),
[#L545-L549](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdmember_controller.go#L545-L549),
[#L860](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdmember_controller.go#L860)).
So running members keep `ScheduleAnyway`, and so does a member whose Pod
is evicted, deleted or replaced after a terminal phase
([#L164-L186](https://github.com/cozystack/etcd-operator/blob/v0.5.6/controllers/etcdmember_controller.go#L164-L186)),
since the new Pod is built from the same copy. Only members created
after the upgrade get `DoNotSchedule`, which means a scale-up, a
crash-loop replacement or a member whose `EtcdMember` was deleted. No
member is rolled, neither one at a time nor all at once. This matches
the field docs
([etcdcluster_types.go#L542-L549](https://github.com/cozystack/etcd-operator/blob/v0.5.6/api/v1alpha2/etcdcluster_types.go#L542-L549),
[etcdmember_types.go#L112-L116](https://github.com/cozystack/etcd-operator/blob/v0.5.6/api/v1alpha2/etcdmember_types.go#L112-L116)).
The code paths are the same in v0.5.8, and in v0.5.5, which v1.6.4 and
v1.6.5 ship, except that v0.5.5 does not yet replace a Pod left in a
terminal phase.

Members that already share a node therefore stay there. One of them
moves only when a new `EtcdMember` takes its place, and the constraint
then sends the replacement to the node without a member. That is how we
moved the members on the test cluster, with `DoNotSchedule` set on the
`EtcdCluster` by hand. The operator's docs say a scheduling change can
be applied by deleting one Pod at a time
([concepts.md#L272](https://github.com/cozystack/etcd-operator/blob/v0.5.6/docs/concepts.md#L272)),
but the recreated Pod gets the member's old copy, so that does not move
it.

The release note now declares two cases from review. In the first, an
etcd created while the cluster had fewer nodes than members has two or
three members on one node, and once a node is added a recreated Pod of
one of them stays Pending, since `maxSkew` rejects its own node and its
node-local volume rules out the others. Deleting that member's
`EtcdMember` while the other members keep quorum replaces it on a node
without a member. In v0.5.5 the member finalizer removes the member from
etcd through the remaining members before the object goes
([`handleDeletion`](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdmember_controller.go#L195-L231),
[`removeMemberFromEtcd`](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdmember_controller.go#L243-L310)),
its Pod and its `data-<member>` PVC are owned by it and deleted with it
([#L403](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdmember_controller.go#L403),
[#L490](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdmember_controller.go#L490)),
and the cluster controller waits for the deletion, then adds a member
under a new name with the constraints from `status.observed`
([etcdcluster_controller.go#L254-L312](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdcluster_controller.go#L254-L312),
[`scaleUp`](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdcluster_controller.go#L820-L851)).
Its new PVC binds where its Pod lands, and under `DoNotSchedule` only a
node without a member passes. The operator documents the same steps for
a broken member, along with the limit that `MemberRemove` cannot run
without quorum
([operations.md#L208-L231](https://github.com/cozystack/etcd-operator/blob/v0.5.5/docs/operations.md#L208-L231)),
and v0.5.6, which main and `release-1.6` ship now, has the same path.
The upstream chart of the operator installs a policy that denies
`EtcdMember` deletion without the
`etcd-operator.cozystack.io/allow-deletion=true` annotation
([member-deletion-policy.yaml](https://github.com/cozystack/etcd-operator/blob/v0.5.5/charts/etcd-operator/templates/member-deletion-policy.yaml)),
and the `etcd-operator` package here does not install it.

On a three-node cluster running v0.5.5 a tenant etcd had two members on
one node with `DoNotSchedule` in effect. We deleted the `EtcdMember` of
one of the two while the other members kept quorum. The replacement's
Pod and its new node-local PVC were scheduled on the node without a
member, the member joined, and the `EtcdCluster` stayed `Available` with
quorum the whole time, polled every 5 seconds.

In the second case `nodeTaintsPolicy: Ignore` counts a node the Pods
cannot tolerate as one with zero members, so on a cluster with such a
node and fewer schedulable nodes than members the members beyond the
schedulable nodes do not start. The way out is the soft global policy in
the platform `Package`, set before the upgrade.

```yaml
apiVersion: cozystack.io/v1alpha1
kind: Package
metadata:
  name: cozystack.cozystack-platform
spec:
  components:
    platform:
      values:
        scheduling:
          globalAppTopologySpreadConstraints: |
            topologySpreadConstraints:
            - maxSkew: 1
              topologyKey: kubernetes.io/hostname
              whenUnsatisfiable: ScheduleAnyway
```

The platform chart writes `scheduling` into `_cluster` of the
`cozystack-values` Secret
([apps.yaml#L292-L295](https://github.com/cozystack/cozystack/blob/55aad6f91456cb545601a83aaecf39118e943b04/packages/core/platform/templates/apps.yaml#L292-L295)),
the tenant chart copies `_cluster` into the Secret of each tenant
namespace
([namespace.yaml#L109-L120](https://github.com/cozystack/cozystack/blob/55aad6f91456cb545601a83aaecf39118e943b04/packages/apps/tenant/templates/namespace.yaml#L109-L120)),
and this chart renders the value as written, which the new test case
pins. Rendering the platform and tenant charts with the value above, on
main and on `release-1.6`, gives the same string in both Secrets. The
operator adopts a changed spec into `status.observed` only once all
members are Ready or the progress deadline has passed
([etcdcluster_controller.go#L196-L213](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdcluster_controller.go#L196-L213)),
and it clears the deadline when the cluster converges
([#L1448-L1456](https://github.com/cozystack/etcd-operator/blob/v0.5.5/controllers/etcdcluster_controller.go#L1448-L1456)).
A new etcd stuck short of its members adopts the policy once its first
deadline passes, and deleting the `EtcdMember` of the Pending member
then brings a replacement that starts. An etcd that had converged before
one of its replacements went Pending adopts the policy only after the
deadline is forced as the operator docs describe
([operations.md#L197-L206](https://github.com/cozystack/etcd-operator/blob/v0.5.5/docs/operations.md#L197-L206)),
which is why the release note says to set it before the upgrade.

#4817 moves this block into `_etcd.tpl` in cozy-lib without changing it,
so whichever of the two lands second has to carry the two lines over.

`tests/topology-spread_test.yaml` now pins the whole default constraint,
and a new case pins that a `ScheduleAnyway` policy from
`globalAppTopologySpreadConstraints` renders as written, without
`nodeTaintsPolicy`. The existing zone override case is unchanged.
Against the template on main the suite fails only the default case, 1
failed and 25 passed. Switching the default to `Honor`, to
`ScheduleAnyway` or to `maxSkew: 2` fails the default case, and adding
`nodeTaintsPolicy` to the override branch fails the new case.

```console
$ docker run --rm -v "$PWD":/apps -w /apps/packages/extra/etcd helmunittest/helm-unittest:4.2.3-1.1.2 .
Charts:      1 passed, 1 total
Test Suites: 9 passed, 9 total
Tests:       26 passed, 26 total
```

On main the same command gives 25 tests in 9 suites, with helm-unittest
1.1.2 as CI pins it. With default values `helm template` differs from
main in the two lines and in the comment above the affinity block, and
with a global policy set only in that comment. The docs commit changes
the `affinity` description, from which `make generate` rewrites the
README, `values.schema.json` and the `ApplicationDefinition` with
nothing but that text, and the suite stays at 26 tests. I have not
rolled this chart change out on a cluster.

The spread block and the test file are the same on release-1.6 and
release-1.7. On release-1.6 the template has no affinity block yet, so
the fix commit cherry-picks without conflicts and the docs commit has
nothing to change there. That backport is a separate PR.

### Screenshots

Not a UI change.

### Downstream repositories

One changed and one added line in
`packages/extra/etcd/templates/etcd-cluster.yaml` and its test suite,
and new text for the `affinity` description and the comment above the
affinity block. The description reaches `values.schema.json`, the README
and the `ApplicationDefinition` as text only, with no key, type or
default change, and the website rebuilds the etcd reference page from
the README on release. The website mentions
`globalAppTopologySpreadConstraints` only as a platform setting, which
stays as it is, so no trigger map entry applies.

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:

### Release note

```release-note
fix(etcd): tenant etcd members now spread over nodes with whenUnsatisfiable DoNotSchedule by default, so two members of one etcd no longer start on the same node while another node has none. Members that exist are not moved, only members created after the upgrade get the new constraint.

An etcd created while the cluster had fewer nodes than members puts two or three members on one node. Once a node is added, such a member whose Pod is recreated (drain, eviction, graceful node shutdown) stays Pending, because maxSkew rejects its own node and its node-local volume rules out the others. With two nodes grown to three the etcd runs two of three members and its PodDisruptionBudget blocks draining the other nodes. With one node grown to three a reboot of that node brings back one member of three and quorum is lost. A stranded member is replaced by deleting its EtcdMember while the other members keep quorum. Its Pod and PVC go with it, and the replacement gets a new Pod and PVC on the node without a member. Right after adding nodes, doing this for one co-located member at a time while all members are Ready avoids the case.

With nodeTaintsPolicy Ignore a node the etcd Pods cannot tolerate, such as a tainted control plane, counts as a node with zero members, so no node can take a second member. On a cluster with such a node and fewer schedulable nodes than members, a new etcd starts only as many members as there are schedulable nodes, one of three on a single schedulable node. An existing etcd there keeps its members, but a member it replaces stays Pending the same way.

Either case is avoided by setting scheduling.globalAppTopologySpreadConstraints in spec.components.platform.values of the cozystack.cozystack-platform Package to a topologySpreadConstraints entry with maxSkew 1, topologyKey kubernetes.io/hostname and whenUnsatisfiable ScheduleAnyway before the upgrade. Every app chart that reads this value applies it, two members of a new etcd can share a node again, and the operator gives it only to members created after it adopts the changed EtcdCluster spec into status.observed. Once status.observed.topologySpreadConstraints of an EtcdCluster shows ScheduleAnyway, a member of it that stays Pending is replaced by deleting its EtcdMember.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
<!-- coderabbit-ai-generated:v1 -->

## Summary by CodeRabbit

- **Bug Fixes**
- Default etcd placement now requires even spreading across hosts and
ignores node taints when evaluating placement.
- Configurable cluster-wide spreading continues to use a flexible
scheduling policy when selected.
- **Documentation**
- Clarified how affinity and topology spreading work together, when
required anti-affinity or node-local storage can leave members pending,
and that affinity changes take effect as members are replaced.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->