WIP: cmd/setup-etcd-environment: add etcd pivot logic #1015
Conversation
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Would be nice to avoid the term "pivot" for this since it's highly associated here with the |
not an issue, will change this. |
beekhof
left a comment
beekhof
left a comment
There was a problem hiding this comment.
Looks sane to me with the possible improvement of the scaling-lock variable naming.
| duration := 10 * time.Second | ||
| // wait forever for success and retry every duration interval | ||
| wait.PollInfinite(duration, func() (bool, error) { | ||
| result, err := client.CoreV1().ConfigMaps("openshift-etcd").Get("scaling-lock", metav1.GetOptions{}) |
There was a problem hiding this comment.
At least in this usage, "scaling-lock" seems misnamed.
Perhaps "known-members"?
There was a problem hiding this comment.
yeah, this configmap originally was a distributed lock similar to the leader election process. But I did not find that to be necessary will consider renaming.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: beekhof, hexfusion The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| } | ||
| var e EtcdScaling | ||
| if runOpts.pivot { | ||
| clientConfig, err := rest.InClusterConfig() |
There was a problem hiding this comment.
Does this mean that it requires k8s networking to be up to talk to apiservers... That would be difficult in recovery scenarios
There was a problem hiding this comment.
This functionality is part of cluster bootstrap and relies on the operator so networking would be assumed. DR would require standing up a single node etcd cluster then scaling additional members via operator.
There was a problem hiding this comment.
But I will consider how to handle this for that single node. thanks.
kikisdeliveryservice
left a comment
kikisdeliveryservice
left a comment
There was a problem hiding this comment.
just a few questions
| wait.PollInfinite(duration, func() (bool, error) { | ||
| result, err := client.CoreV1().ConfigMaps("openshift-etcd").Get("scaling-lock", metav1.GetOptions{}) | ||
| if err != nil { | ||
| klog.Errorf("error creating client %v", err) |
There was a problem hiding this comment.
wondering why you used klog here?
There was a problem hiding this comment.
I see we have glog will drop.
| klog.Errorf("could not find self in scaling-lock") | ||
| return false, nil | ||
| } | ||
| members := e.Members |
There was a problem hiding this comment.
do we need this placeholder var? would using e.Members directly cause an issue?
There was a problem hiding this comment.
good point will adjust
| if etcdName == "" { | ||
| return fmt.Errorf("environment variable ETCD_NAME has no value") | ||
| } | ||
| var e EtcdScaling |
There was a problem hiding this comment.
could you use a more descriptive name than e, it's hard to remember what this stands for as i read thru below.
|
@hexfusion: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close Work is continued in #1221 |
6 participants
This PR adds logic required to scale up during install via openshift-etcd-operator. More details soon..
required by: openshift/cluster-etcd-operator#19