Modify 'restore-from-backup.sh' to work in multinode etcd cluster. #56692
Conversation
k8s-ci-robot
added
release-note-none
cluster/restore-from-backup.sh
Outdated
| @@ -101,7 +104,7 @@ wait_for_cluster_healthy() { | |||
| # Wait until etcd and apiserver pods are down. | |||
| wait_for_etcd_and_apiserver_down() { | |||
| for i in $(seq 120); do | |||
| etcd=$(docker ps | grep etcd | grep -v etcd-empty-dir | grep -v etcd-monitor | wc -l) | |||
| etcd=$(docker ps | grep -e etcd-server-gke -e etcd-server-events-gke | wc -l) | |||
There was a problem hiding this comment.
This looks like it's making the script gke specific. If it only works on GKE, why have it in the k/k repository?
There was a problem hiding this comment.
Yeah. Initially this script was supposed to be generic, but over time it appeared some GKE/GCE-specific things may be needed. But when I'm looking into it now, maybe that's the only one.
Maybe add a TODO to make it work for all platforms.
There was a problem hiding this comment.
The reason for the change is that it doesn't work in GKE right now: there is a plenty of containers with etcd in name (not only etcd-empty-dir, etcd-monitor) and this waits forever.
Will try to solve this issue in some other way.
| if [ "$?" -ne "0" ]; then | ||
| echo "Docker container didn't started correctly" | ||
| exit 1 | ||
| fi | ||
| echo "Prepare container exit code: $(docker wait ${image})" |
There was a problem hiding this comment.
I removed '-d' from docker run so that stdout/stderr goes directly to console. It's a way easier to see if it worked and debug if it failed.
So 'docker wait' is not longer needed (if command fails, the if above will catch it)
|
PTAL |
cluster/restore-from-backup.sh
Outdated
| @@ -101,7 +104,7 @@ wait_for_cluster_healthy() { | |||
| # Wait until etcd and apiserver pods are down. | |||
| wait_for_etcd_and_apiserver_down() { | |||
| for i in $(seq 120); do | |||
| etcd=$(docker ps | grep etcd | grep -v etcd-empty-dir | grep -v etcd-monitor | wc -l) | |||
| etcd=$(docker ps | grep -e etcd-server -e etcd-server-events | wc -l) | |||
There was a problem hiding this comment.
Is the "-e etcd-server-events" needed?
I think that "-e etcd-server" already matches "etcd-server-events", right?
@roberthbailey - this naming is consistent with what is in cluster/saltbase/salt/manifest, so I don't think it's GKE/GKE-specific now.
There was a problem hiding this comment.
Hmm.. Good point -- in fact etcd-server-events is not needed.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mborsz, wojtek-t Associated issue requirement bypassed by: wojtek-t The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
wojtek-t
added
status/approved-for-milestone
priority/critical-urgent
wojtek-t
added
priority/critical-urgent
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete @mborsz @roberthbailey @wojtek-t Action required: This pull request requires label changes. kind: Must specify exactly one of |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
This PR modifies cluster/restore-from-backup.sh to work in multinode etcd cluster.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: