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

pckhoi · 2024-12-25T09:11:54Z

Description of the change

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

It relies on files outside of the data directory which could contain conflicting information compared to the data dir and the cluster
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
If the member was removed, the container chokes up on non-empty data dir in most cases except when recovering from a snapshot
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
It runs etcdctl member update for unclear reasons when the data dir is not empty and there is a member ID
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

fixes #16069

Additional information

Related changes in the Helm chart: bitnami/charts#31161 and bitnami/charts#31164

- 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]>

Signed-off-by: Khoi Pham <[email protected]>

…s new Signed-off-by: Khoi Pham <[email protected]>

pckhoi · 2024-12-26T02:18:43Z

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

juan131

Hi @pckhoi

Thanks so much for this amazing contribution! It'd definitely help on making the Bitnami etcd chart more stable.

I think the main concern/challenge with your changes would be providing a solution for users who may scale down the cluster via kubectl scale sts/etcd --replicas X (or via some HorizontalPodAutoscaler that may also scale down the cluster without Helm's control via hooks). Correct me if I'm wrong but this use case won't be covered, right?

juan131 · 2024-12-30T09:36:16Z

bitnami/etcd/3.5/debian-12/rootfs/opt/bitnami/scripts/etcd/preupgrade.sh

+    local -r current=$(mktemp)
+    local -r expected=$(mktemp)


Is there any reason to save on "current" and "expected" results on temporary files instead of using simple variables and save it in memory?

juan131 · 2024-12-30T09:40:53Z

bitnami/etcd/3.5/debian-12/rootfs/opt/bitnami/scripts/etcd/preupgrade.sh

+########################
+# Obtain endpoints to connect when running 'ectdctl' in a hook job
+# Globals:
+#   ETCD_*
+# Arguments:
+#   None
+# Returns:
+#   String
+########################
+etcdctl_job_endpoints() {
+    local -a endpoints=()
+    local host domain port count
+
+    # get number of endpoints from initial cluster endpoints
+    count="$(echo $ETCD_INITIAL_CLUSTER | awk -F, '{print NF}')"
+
+    # This piece of code assumes this code is executed on a K8s environment
+    # where etcd members are part of a statefulset that uses a headless service
+    # to create a unique FQDN per member. Under these circumstances, the
+    # ETCD_ADVERTISE_CLIENT_URLS env. variable is created as follows:
+    #   SCHEME://POD_NAME.HEADLESS_SVC_DOMAIN:CLIENT_PORT,SCHEME://SVC_DOMAIN:SVC_CLIENT_PORT
+    #
+    # Assuming this, we can extract the HEADLESS_SVC_DOMAIN and obtain
+    # every available endpoint
+    read -r -a advertised_array <<<"$(tr ',;' ' ' <<<"$ETCD_ADVERTISE_CLIENT_URLS")"
+    port="$(parse_uri "${advertised_array[0]}" "port")"
+
+    for i in $(seq 0 $(($count - 1))); do
+        pod_name="${MY_STS_NAME}-${i}"
+        endpoints+=("${pod_name}.${ETCD_CLUSTER_DOMAIN}:${port:-2380}")
+    done
+
+    debug "etcdctl endpoints are ${endpoints[*]}"
+    echo "${endpoints[*]}" | tr ' ' ','
+}


Isn't this function very similar to get_initial_cluster available at libetcd.sh?

juan131 · 2024-12-30T09:49:12Z

bitnami/etcd/3.5/debian-12/rootfs/opt/bitnami/scripts/libetcd.sh

+    if grep -q "the member has been permanently removed from the cluster\|ignored streaming request; ID mismatch" "$tmp_file"; then
+        info "The remote member ID is different from the local member ID"
+        ret=1
+    elif grep -q "\"error\":\"cluster ID mismatch\"" "$tmp_file"; then
+        info "The remote cluster ID is different from the local cluster ID"
+        ret=1
+    else
+        info "The member is still part of the cluster"
+    fi
+    rm -f "$tmp_file"
+    return $ret


You can simplify this:

Suggested change

if grep -q "the member has been permanently removed from the cluster\|ignored streaming request; ID mismatch" "$tmp_file"; then

info "The remote member ID is different from the local member ID"

ret=1

elif grep -q "\"error\":\"cluster ID mismatch\"" "$tmp_file"; then

info "The remote cluster ID is different from the local cluster ID"

ret=1

else

info "The member is still part of the cluster"

fi

rm -f "$tmp_file"

return $ret

trap rm -f "$tmp_file"

if grep -q "the member has been permanently removed from the cluster\|ignored streaming request; ID mismatch" "$tmp_file"; then

info "The remote member ID is different from the local member ID"

return 1

elif grep -q "\"error\":\"cluster ID mismatch\"" "$tmp_file"; then

info "The remote cluster ID is different from the local cluster ID"

return 1

fi

info "The member is still part of the cluster"

return 0

Also, wouldn't it be better to rely on some etcdctl command to check if the node is still a member instead of starting etcd in background?

juan131 · 2024-12-30T09:50:00Z

bitnami/etcd/README.md

+| `ETCD_NEW_MEMBERS_ENV_FILE`  | File containining the etcd environment to use after adding a member. | `${ETCD_DATA_DIR}/new_member_envs` |
+| `ETCD_DAEMON_USER`           | etcd system user name.                                               | `etcd`                             |
+| `ETCD_DAEMON_GROUP`          | etcd system user group.                                              | `etcd`                             |
+| `ETCD_INITIAL_CLUSTER_STATE` | Initial cluster state. Either "new" or "existing".                   | `nil`                              |

 Additionally, you can configure etcd using the upstream env variables [here](https://etcd.io/docs/v3.4/op-guide/configuration/)


"Notable Changes" section should be updated describing the introduced changes.

pckhoi added 6 commits December 20, 2024 09:19

[bitnami/etcd] Fix remove_obsolete_members function

95811a3

Signed-off-by: Khoi Pham <[email protected]>

[bitnami/etcd] is_new_etcd_cluster queries current cluster

96e784c

Signed-off-by: Khoi Pham <[email protected]>

[bitnami/etcd] Added preupgrade.sh

c3a825a

Signed-off-by: Khoi Pham <[email protected]>

[bitnami/etcd] Document changes to ETCD_INITIAL_CLUSTER_STATE

a18d711

Signed-off-by: Khoi Pham <[email protected]>

[bitnami/etcd] Use etcdctl endpoint status to check whether cluster i…

6e4d2a6

…s new Signed-off-by: Khoi Pham <[email protected]>

github-actions bot added etcd triage Triage is needed labels Dec 25, 2024

github-actions bot assigned javsalgar Dec 25, 2024

github-actions bot requested a review from javsalgar December 25, 2024 09:12

pckhoi mentioned this pull request Dec 26, 2024

[bitnami/etcd] Add pre-upgrade hook bitnami/charts#31161

Open

4 tasks

Merge branch 'bitnami:main' into main

3d7115e

carrodher added verify Execute verification workflow for these changes in-progress labels Dec 26, 2024

github-actions bot removed the triage Triage is needed label Dec 26, 2024

github-actions bot unassigned javsalgar Dec 26, 2024

github-actions bot removed the request for review from javsalgar December 26, 2024 08:06

github-actions bot assigned alvneiayu Dec 26, 2024

github-actions bot requested a review from alvneiayu December 26, 2024 08:06

carrodher mentioned this pull request Dec 27, 2024

[bitnami/common] Add "common.capabilities.job.apiVersion" template bitnami/charts#31164

Open

4 tasks

carrodher removed the request for review from alvneiayu December 27, 2024 07:36

carrodher unassigned alvneiayu Dec 27, 2024

carrodher requested review from juan131 and dgomezleon December 27, 2024 07:36

carrodher assigned juan131 and dgomezleon Dec 27, 2024

juan131 requested changes Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pckhoi commented Dec 25, 2024 •

edited by carrodher

Loading

pckhoi commented Dec 26, 2024

juan131 left a comment

juan131 Dec 30, 2024

juan131 Dec 30, 2024

juan131 Dec 30, 2024

juan131 Dec 30, 2024

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

Are you sure you want to change the base?

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

Conversation

pckhoi commented Dec 25, 2024 • edited by carrodher Loading

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

pckhoi commented Dec 26, 2024

juan131 left a comment

Choose a reason for hiding this comment

juan131 Dec 30, 2024

Choose a reason for hiding this comment

juan131 Dec 30, 2024

Choose a reason for hiding this comment

juan131 Dec 30, 2024

Choose a reason for hiding this comment

juan131 Dec 30, 2024

Choose a reason for hiding this comment

pckhoi commented Dec 25, 2024 •

edited by carrodher

Loading