Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bitnami/etcd] Stop relying on files for state #75906

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pckhoi
Copy link
Contributor

@pckhoi pckhoi commented Dec 25, 2024

Description of the change

The current etcd container and chart have a few major problems:

  1. It relies on files outside of the data directory which could contain conflicting information compared to the data dir and the cluster
  2. Removing the member during pre-stop hook is problematic. I guess this was added to support scaling down the cluster. If so, this logic is leaky. There are 2 cases where this breaks down:
    1. If the pod was killed for reasons other than replicas update then the next time the pod starts, it will not be able to start from the existing data dir which means it must throw away the data dir and start from scratch.
    2. If the cluster is scaled down and the PVC is retained, the next time the cluster is scaled up, the new member will encounter a non-empty data dir which it must discard
  3. If the member was removed, the container chokes up on non-empty data dir in most cases except when recovering from a snapshot
  4. It might attempt to add a new member even when an old member with the same name already exists. This is caused by relying on files for state
  5. It runs etcdctl member update for unclear reasons when the data dir is not empty and there is a member ID
  6. It relies on ETCD_INITIAL_CLUSTER_STATE to know whether the cluster is new which could be inaccurate

This PR add the following changes:

  • Add preupgrade.sh which should be run in a Helm pre-upgrade hook. When the cluster is scaled down, it detects and removes obsolete members with etcdctl member remove.
  • prestop.sh no longer removes the member from the cluster. The script is kept for backward compatibility but it does nothing now.
  • Stop storing/checking member ID from the member_id file. Instead, the remote member ID is read from the cluster with etcdctl member list, and the local member ID is checked for conflict during startup.
  • Stop storing/checking member removal state from member_removal.log. Check with etcdctl member list instead.
  • If the data dir is not empty, check if the member still belongs to the cluster (remote ID and local ID are the same). If there is a conflict, remove the data dir, remove the old member, add a new member, and start the member from scratch.
  • Remove environment variable ETCD_DISABLE_STORE_MEMBER_ID
  • Remove environment variable ETCD_DISABLE_PRESTOP
  • Environment variable ETCD_INITIAL_CLUSTER_STATE becomes read-only

Benefits

  • Not relying on files outside of the data directory means there is only a single source of truths (or only as many as there are live members in the cluster plus the data dir), which makes most operations more reliable
  • Removing obsolete members in Helm pre-upgrade hook means the etcdctl member remove command tends to be executed against a healthy cluster
  • If the pod was killed for reasons other than replica changes, it can rejoin the cluster on its own while keeping all its data intact
  • The container no longer chokes up on a non-empty data dir, even when the old member is removed

Possible drawbacks

  • If during initialization there is a network outage and the current member can't connect to other members, it will think that it must start a new cluster. That said, I don't think there is any good solution in this case except manual recovery.
  • I have not tested this set of changes outside of Helm/K8s

Applicable issues

Additional information

- Remove prestop logic (no longer removing member when container stops)
- Remove members not included in ETCD_INITIAL_CLUSTERS during startup
- Stop storing member id on a separate file, member id is checked from
  etcd data dir instead
- Stop reading member removal state off of disk, probe the cluster
  instead
- Remove old member (with the same name) if exist before adding new
  member
- If data dir is not empty, check if the member still belongs to the
  cluster. If not, remove data dir, remove member with the same name,
  and add new member
- Remove env var ETCD_DISABLE_STORE_MEMBER_ID
- Remove env var ETCD_DISABLE_PRESTOP

Signed-off-by: Khoi Pham <[email protected]>
@github-actions github-actions bot added etcd triage Triage is needed labels Dec 25, 2024
@github-actions github-actions bot requested a review from javsalgar December 25, 2024 09:12
@pckhoi
Copy link
Contributor Author

pckhoi commented Dec 26, 2024

I'm planning to open a complementary PR in the charts repo. I will try to add more tests there.

@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Dec 26, 2024
@github-actions github-actions bot removed the triage Triage is needed label Dec 26, 2024
@github-actions github-actions bot removed the request for review from javsalgar December 26, 2024 08:06
@github-actions github-actions bot requested a review from alvneiayu December 26, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
etcd in-progress verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/etcd] etcd pods are unable to join existing cluster on node drain
4 participants