use upstream default of etcd_snapshot_count

[rptaylor]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Match the upstream default of etcd_snapshot_count decided by the etcd community, which was updated in v3.2 but not reflected in Kubespray. This way kubespray will match the expectations people have of default etcd behaviour in other kubernetes distributions.

Which issue(s) this PR fixes:
Fixes #9018

Special notes for your reviewer:
See https://etcd.io/docs/v3.4/op-guide/maintenance/

[k8s-ci-robot]

Hi @rptaylor. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

[yankay]

/ok-to-test

[yankay]

The kubernetes default value is 10k not 100k :
refer to :

• https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/etcd/local.go#L260
• etcdserver: increase snapshot to 100,000 etcd-io/etcd#7160

So keep the 10k value is resonable :-)

[rptaylor]

@yankay Okay so there's more background to this:

• it was proposed to increase to 100K in kubeadm
• it was even proposed to revert the etcd default back to 10K

Either would have resolved the discrepancy between k8s and etcd but neither went forward. It could still be changed in the future though.

That being the case, wouldn't the best course of action for Kubespray be to not define it at all? It is not a good pattern for Kubespray to have a default which overrides the kubeadm/etcd defaults. Three default values (kubeadm, etcd, kubespray) for this parameter is too many. The use of etcd_snapshot_count is already wrapped in {% if etcd_snapshot_count is defined %} so it would only be necessary to comment out here: https://github.com/kubernetes-sigs/kubespray/blob/master/roles/kubespray-defaults/defaults/main.yaml#L616

Then the choice of kubeadm/etcd for the default value will take precedence instead of Kubespray having to care/decide what it should be, and of course users can still override it if needed.

[yankay]

Thanks @rptaylor

I guess the etcd_snapshot_count could be changed after the Kubernetes change :-)

And the comments give us a good idea :-)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rptaylor
Once this PR has been reviewed and has the lgtm label, please ask for approval from yankay. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rptaylor]

There should not be a separate kubespray default that overrides the kubeadm default, but as long as Kubespray supports both kubeadm-based and host-based etcd deployment I guess it will be tricky to apply the right default in both approaches. So there is not a good way to resolve the divergence between the kubeadm default and the etcd default.