Replace StatefulSets with a customized controller#404
Conversation
|
The diff between this PR and the original one can be found here . |
nwnt
force-pushed
the
customized-controller
branch
3 times, most recently
from
22717d5 to
4db9881
Compare
| } | ||
| requeue = true | ||
| // Service must exist before pods start so that headless DNS resolves. | ||
| if err := createHeadlessServiceIfNotExist(ctx, logger, r.Client, s.cluster, r.Scheme); err != nil { |
There was a problem hiding this comment.
Do we still need headless service after removing statefulSet? Or is it just an improper function name?
There was a problem hiding this comment.
We still need headless service, regardless of statefulset being used or not.
There was a problem hiding this comment.
Yes, was about to reply that we need it for the FQDN hostname.subdomain.namespace.svc.cluster.local for each pod.
| func reconcileStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1alpha1.EtcdCluster, c client.Client, replicas int32, scheme *runtime.Scheme) (*appsv1.StatefulSet, error) { | ||
| // memberPodName returns the deterministic name for an etcd member pod. | ||
| // The naming convention mirrors StatefulSet so that headless-service DNS is identical. | ||
| func memberPodName(clusterName string, ordinal int) string { |
There was a problem hiding this comment.
The internal/controller/utils.go is a little overloaded. I think we need to extract all POD related stuff into a separate file, or even separate package. If we move it into separate package, we need to do some abstraction to ensure it's business agnostic.
in the first step, probably we can just move them into a separate file, and consider refactoring it in next step if possible.
There was a problem hiding this comment.
Done, move everything related into pods.go. I have also moved the related tests to pods_test.go.
|
link to #362 |
nwnt
force-pushed
the
customized-controller
branch
from
4db9881 to
17d7b0d
Compare
Based on the original [PR](etcd-io#363 (comment)) Signed-off-by: Nont <9658731+nwnt@users.noreply.github.com>
nwnt
force-pushed
the
customized-controller
branch
from
17d7b0d to
0b08a70
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt 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 |
|
It seems that the pipelines are not triggered if you raise PR against customized-controller. Let's still raise PR against main, once it's reviewed I will manually merge the PR to customized-controller. |
|
Or raise a PR to add customized-controller into https://github.com/kubernetes/test-infra/blob/master/config/jobs/etcd/etcd-operator-presubmits.yaml |
|
Both are done. |
| func nextPodOrdinal(currentOrdinals []int, expectedReplica int) int { | ||
| sort.Ints(currentOrdinals) | ||
| for i := range expectedReplica { | ||
| if i >= len(currentOrdinals) || i != currentOrdinals[i] { |
There was a problem hiding this comment.
What's the case for the i != currentOrdinals[i] ?
There was a problem hiding this comment.
It's for filling the gap if one is found. For example, if the current ordinals are {0,1,3}, when i is 2 it will see that currentOrdinals[i] is 3, so 2 is the gap and hence the next ordinal to be used.
There was a problem hiding this comment.
the current ordinal slice is always sorted before entering this loop.
There was a problem hiding this comment.
if the current ordinals are {0,1,3}
In which case we may run into this situation? If no, we'd better remove it for better readability
There was a problem hiding this comment.
Yes, we do. I added it because the logic from the original PR would fail one of the e2e tests as described here.
Here is the code from the original PR.
logger.Info("Creating pod for already-registered etcd member")
nextOrdinal := int(currentPodCount)Supposed that we have etcd-0, etcd-1, and etcd-2, if etcd-1 is deleted, the reconciliation loop logic would propose that etcd-2 should be created because the count is 2. The added function instead identifies the gap, and proposes that the first ordinal in the gap should be used. In that same scenario, 1 would be the next ordinal compared to 2 in the original PR.
There was a problem hiding this comment.
We might need to revisit this later. There are three variables that we need to consider to decide the ordinal,
- EtcdCluster.Spec.Size
- etcd member count
- real POD count
We need to differentiate two different cases
- one POD is somehow deleted or crashed
- normal scale in/out case, or upgrade case
There was a problem hiding this comment.
Yes, I agree. I came up with this just to pass the existing e2e tests and didn't mean it to be an ultimate solution for the problem. Your cases are something we want to provide as a guarantee that etcd-operator will behave in a certain way.
|
Let's try to get this PR merged later today. Overall looks good. We can consider some refactoring later when this is completely done. |
|
FYI. Before I manually merge each PR into customized-controller, I will rebase it based on main first, to ensure there is no any conflict between |
|
@nwnt I just manually merge this PR into branch |
2 participants
Based on the original PR
I have tested all the changes in my local and they passed. Will be covering the additional required changes on top of the original PR in the call today.
@ahrtr @ivanvc