mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 03:57:52 +09:00
Aleksei Sviridkin
42b7239198
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The opensearch suite sends its first OpenSearch API call, the index create in `make-the-cluster-permanently-yellow`, as soon as the nodes StatefulSet reports one ready replica. Two operator actions are released at that moment and neither has finished: the bootstrap pod handing over the cluster-manager role, and a rolling restart of the only data node. #4412 is the first of them hitting the create with a 503. This PR makes the step wait for both before its first call: an `error:` assert that `opensearch-test-bootstrap-0` no longer exists, then an assert that the nodes StatefulSet has settled on its update revision, in the same form the suite already uses after the listener move. Anchors below are opensearch-k8s-operator v2.8.0 with the in-tree patches applied, paths under `opensearch-operator/`. **Bootstrap handoff.** - The operator starts retiring the bootstrap pod only once the cluster is `Initialized` (`pkg/reconcilers/cluster.go:110-111`), and that flag is set as soon as every cluster-manager pool has all its replicas ready (`controllers/opensearchController.go:257`, `pkg/builders/cluster.go:1146-1164`). A Ready data node therefore starts the handoff and does not finish it. - The voting exclusion is a cluster-manager action, so it goes through the bootstrap pod and fails while that pod's security is not initialized. The operator then keeps the pod and requeues after 10s (`pkg/reconcilers/cluster.go:36`, `:186-190`). - The pod is deleted only after the bootstrap node has left the last committed voting configuration (`pkg/reconcilers/cluster.go:195-205`, `opensearch-gateway/services/os_client.go:760-771`). Once the pod object is gone, including its Terminating phase, the bootstrap process has exited and holds no vote, and the only cluster-manager-eligible node left is `opensearch-test-nodes-0`. - The assert cannot pass vacuously. The cluster forms around the bootstrap pod (`cluster.initial_master_nodes` names only that pod, `pkg/builders/cluster.go:807`), and the pod is created unconditionally while the cluster is not initialized (`pkg/reconcilers/cluster.go:112-113`). **Rolling restart on first boot.** The failing run's own operator log and StatefulSet dump show it: - `10:27:52.850` the reconcile in which `Initialized` became true. - `10:27:53.251` "Failed to exclude the bootstrap node from the voting configuration". - `10:27:53.981` "Starting rolling restart of the StatefulSet opensearch-test-nodes" (`pkg/reconcilers/rollingRestart.go:155`). - `10:27:54.079` "Couldn't proceed with rolling restart … because enabled shard allocation" (`:182`), the first step of the yellow path, after which the next 10s requeue moves on to `PreparePodForDelete` (`:195`) and deletes the pod. The StatefulSet in the same dump is at generation 2 with `currentRevision` `…744cc7b944` and `updateRevision` `…677c65d7fb`: the pod template was rewritten after `opensearch-test-nodes-0` was created, and the restart onto it waits only on `Initialized` (`rollingRestart.go:79`, `:123`). With one replica that restart takes down the node the step's requests go to, so a create that got past the handoff could still land on a terminating or restarting node. Once the StatefulSet reports `currentRevision == updateRevision` with its one replica updated and ready, the node the step talks to is on its final template and the operator has nothing pending. The suite runs one topology: one replica in a single pool with the cluster_manager, data and ingest roles, on a PVC. The bootstrap pod comes back only through the emptyDir recovery path (`pkg/reconcilers/cluster.go:452-478`), which a PVC-backed cluster never takes, and a restart that was not pending leaves the second assert true immediately. I checked the rest of the suite for the same window. Apart from this step, only the catch script (diagnostics, `|| true`) calls the OpenSearch HTTP API. `verify-endpoint-serves-the-chart-chain` makes a TLS handshake only, and it runs after the suite has already waited for the listener roll to complete, on a single node that is its own cluster-manager. Budgets: 3m for the handoff, which took about two seconds in the failing run; that leaves room for several 10s retire retries, a 30s requeue and the default 30s termination grace period the bootstrap pod runs with. 6m for the restart, shorter than the 12m the suite gives the listener roll because nothing is pulled here: the health steps requeue every 10s and the pod then restarts on an image the node already holds, where the first boot, bootstrap included, took under two and a half minutes. Residual risk, unproven: if the pod template were rewritten again after the rollout assert passed, that assert would have passed on a template that is not the final one. I found no mechanism for it. The StatefulSet builder reads no cluster status (the only status read in `pkg/builders/cluster.go` is the replica check in `AllMastersReady`), so the transition to `Initialized` cannot rewrite the template by itself; a rewrite would need a configuration or secret input to change after that point. Which input changed on first boot in the failing run I could not tell from the log, because the ControllerRevisions were not captured. What I ran: `chainsaw lint test` with Chainsaw v0.2.15, the version the e2e sandbox pins, passes on the changed suite. As a positive control, lint rejects each new operation when I rename one of its keys. `hack/select-e2e.sh` on this diff selects `opensearch` only. What I did not run: the E2E suite itself. The race is timing-dependent (one hit in six failed in-tree runs checked, per the issue), so a single green run would not prove the fix either. The argument rests on the operator code and the failing run's log cited above. Closes #4412 ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not a UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: The only file changed is `hack/e2e-chainsaw/opensearch/chainsaw-test.yaml`, which no downstream repository consumes. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Cluster health checks now wait for bootstrap completion and verify that all nodes are ready and up to date before proceeding. <!-- end of auto-generated comment: release notes by coderabbit.ai -->