Add an option for turning on/off compaction from apiserver in etcd3 mode #51765
Conversation
|
@mitake: GitHub didn't allow me to request PR reviews from the following users: struz. Note that only kubernetes members can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
Hi @mitake. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. I understand the commands that are listed here. |
|
Adding do-not-merge/release-note-label-needed because the release note process has not been followed. |
| @@ -50,6 +50,9 @@ type Config struct { | |||
| Copier runtime.ObjectCopier | |||
| // Transformer allows the value to be transformed prior to persisting into etcd. | |||
| Transformer value.Transformer | |||
|
|
|||
| // DoCompaction is a flag to turn on or off requesting compaction from apiserver. | |||
| DoCompaction bool | |||
There was a problem hiding this comment.
I'm fine with exposing this as an option - I've already heard about customers who would like to control it themselves too.
I'm not sure if DoCompaction is the best name though.
I think that "auto-compaction" should be default and you should be able to opt-out. That would mean I would suggest changing to:
DisableCompaction
[with default false].
There was a problem hiding this comment.
Thanks for your comment. I'll rename the flag to DisableCompaction.
There was a problem hiding this comment.
@wojtek-t BTW can I add DisableCompaction to EtcdOptions instead of storagebackend.Config? It seems that we don't have a way of configuring storagebackend.Config by command line option or other ways.
There was a problem hiding this comment.
@wojtek-t sorry I found a very simple way to do this... please ignore the above comment
80d9c09
to
fe3b759
Compare
|
@wojtek-t I renamed the field name to |
| @@ -127,6 +127,10 @@ func (s *EtcdOptions) AddFlags(fs *pflag.FlagSet) { | |||
|
|
|||
| fs.StringVar(&s.EncryptionProviderConfigFilepath, "experimental-encryption-provider-config", s.EncryptionProviderConfigFilepath, | |||
| "The file containing configuration for encryption providers to be used for storing secrets in etcd") | |||
|
|
|||
| fs.BoolVar(&s.StorageConfig.DisableCompaction, "etcd-disable-compaction", s.StorageConfig.DisableCompaction, | |||
There was a problem hiding this comment.
probably we should make it clear that if this option is enabled, user MUST compact the underlying etcd3 storage themselves.
There was a problem hiding this comment.
It makes sense. I'll update the description later. Thanks.
fe3b759
to
c9603ed
Compare
|
So given that we may want to tune this for apichunking, I'd prefer to make
this field an interval with a default value and the value 0 mean
"compaction off".
…On Fri, Sep 8, 2017 at 9:55 AM, Hitoshi Mitake ***@***.***> wrote:
@xiang90 <https://github.com/xiang90> @wojtek-t
<https://github.com/wojtek-t> I updated the description and briefly
tested it and at least it wouldn't be breaking existing things. Could you
trigger the tests? Or should I add new tests or a release note (sorry I'm
not familiar with PR merging process of k8s...)?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#51765 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p1HizaM9R1v1yoyoDk7qjXvYbzH2ks5sgUdDgaJpZM4PJuBH>
.
|
|
@smarterclayton what does "apichunking" mean (sorry I'm not familiar with k8s terminologies)? |
|
In this context apichunking means "pagination of the LIST responses". |
|
The API server specifies etcd revision to get a consistent snapshot for pagination. It can achieve isolation with it. However, if compaction compacts a revision that a client is ranging over, the client has to abort the range and start over. Controlling the compaction interval can hint the client how fast it should finish the range without getting a abort. |
5dfa49b
to
3894f6d
Compare
|
@xiang90 @wojtek-t @smarterclayton I changed the type of option from bool to duration and the name to |
|
Thank you for changing - high level this looks ok to me. |
|
/lgtm /ok-to-test |
|
It seems that |
|
@mitake - the problem with full disk is not job specific - it's about test infrastructure. We are seeing it in many different jobs on many different PRs. |
|
/lgtm |
|
@wojtek-t ah sorry I added a release note to the PR description. Should I remove it (you added release-note-none tag)? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mitake, 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 |
|
@wojtek-t and I understand the situation about the disk full issue, thanks. |
|
Actually, I think this release note makes sense - changed the label. |
|
I see, thanks for updating the label. |
|
/retest Review the full test history for this PR. |
1 similar comment
|
/retest Review the full test history for this PR. |
|
/retest |
|
/retest Review the full test history for this PR. |
1 similar comment
|
/retest Review the full test history for this PR. |
|
/test all Tests are more than 96 hours old. Re-running tests. |
|
/retest (the unit test isn't triggered yet) BTW, can this be pushed to the submit queue because v1.8 is now released? |
|
/retest |
|
/test pull-kubernetes-unit |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 51765, 53053, 52771, 52860, 53284). If you want to cherry-pick this change to another branch, please follow the instructions here. |
#59106-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #51765 #59106 upstream release 1.8 Cherry pick of #51765 #59106 on release-1.8. #51765 : Add an option for turning on/off compaction from apiserver in etcd3 mode #59106 : Expose etcd compaction time via environmental variable in GCE
…erver
What this PR does / why we need it:
This commit adds an option for controlling request of compaction to
etcd3 from apiserver. There is a situation that apiserver cannot fully
own its etcd cluster (e.g. sharing it with canal). In such a case,
apiserver should have limited access in terms of etcd's auth
functionality so it don't have a privilege to issue compaction
requests. It means that the compaction requests should be issued by
other component and apiserver's compaction requests are needless.
For such use cases, this commit adds a new flag
storagebackend.Config.DoCompaction. If the flag is true (default),
apiserver issues the compaction requests like current behaviour. If it
is false, apiserver doesn't issue the requests.
Related issue (etcd)
etcd-io/etcd#8458
/cc @xiang90 @struz
Release note: