precondition checker for reducing encryption QPS #1059
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: p0lyn0mial The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| ) | ||
|
|
||
| // PreconditionProvider determines if encryption controllers should synchronise. | ||
| type PreconditionProvider interface { |
There was a problem hiding this comment.
Java'esque name.
Interface with one func are odd. Use type Precondition func() (bool, error). Why the extra layer of abstraction?
|
|
||
| // NewPreconditionChecker determines if encryption controllers should synchronise. | ||
| // It uses the cache for gathering data to avoid sending requests to the API servers. | ||
| func NewPreconditionChecker(apiServerConfigInformer configv1informers.APIServerInformer, kubeInformersForNamespaces operatorv1helpers.KubeInformersForNamespaces, encryptionSecretSelectorString string) (*PreconditionChecker, error) { |
There was a problem hiding this comment.
NewEncryptionEnabledPrecondition
| // since rolling out a new version is a slow process, for kas it takes about ~20 min | ||
| // and the encryption controllers must wait until all servers converge on the same revision before they can act | ||
| // we have work to do if all api servers are on the same revision | ||
| if !pc.areServersOnTheSameRevision() { |
There was a problem hiding this comment.
why is same revision enough? Couldn't the revision controller be lagging?
| if !errors.IsNotFound(err) { | ||
| return false, err // unknown error | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
do we check that the lister is synched?
| return false, err // unknown error | ||
| } | ||
|
|
||
| return encryptionConfiguration == nil && len(encryptionSecrets) == 0, nil |
There was a problem hiding this comment.
do we check that the lister is synced?
| } | ||
|
|
||
| func (pc *PreconditionChecker) areServersOnTheSameRevision() bool { | ||
| // TODO |
| } | ||
|
|
||
| func (pc *PreconditionChecker) areServersOnTheSameRevision() bool { | ||
| // TODO |
…should synchronise Synchronizing encryption controllers is expensive because they pull data directly from the servers to get the most recent data. This PR provides a checker that uses the cache for gathering enough data to determine if there is some work to be done. This helps to avoid sending requests to the API servers if there is no work to do.
…controller construction
p0lyn0mial
force-pushed
the
cache-based-prechecks-for-encryption
branch
from
345e482
to
cb236f2
Compare
|
/hold for a manual test (openshift/cluster-openshift-apiserver-operator#451) |
openshift-ci-robot
added
the
do-not-merge/hold
| func (pc *preconditionChecker) encryptionNeverEnabled() (bool, error) { | ||
| apiServerConfig, err := pc.apiServerConfigLister.Get("cluster") | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { |
There was a problem hiding this comment.
nit
if apierrors.notFound(err){
return true, nil
}
if err != nil{
return false, nil
}
| if !errors.IsNotFound(err) { | ||
| return false, err // unknown error | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
Why special case this? it seems like if you have no apiserverconfig, you have no work to do because you won't be able to know whether encryption is on.
There was a problem hiding this comment.
this is exactly what I'm doing here, if there is no apiserverconfig I'm returning true to indicate encryption wasn never on
| // it hasn't been enabled when: | ||
| // the current mode is set to identity | ||
| // AND the encryption configuration in the managed namespace doesn't exists AND we don't have encryption secrets | ||
| func (pc *preconditionChecker) encryptionNeverEnabled() (bool, error) { |
There was a problem hiding this comment.
I'm having difficulty parsing the negative logic here. Can you change it to, EncryptionHasBeenEnabled?
There was a problem hiding this comment.
I could but there is a difference between never enabled and currently disabled - see TODO a few lines earlier.
|
This approach will help. I'm having trouble reading the inverted logic. It might just be me getting old and tired, but a positively named method would help me I think. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Provides a precondition checker that determines if encryption controllers should synchronize.
Synchronizing encryption controllers is expensive because they pull data directly from the servers to get the most recent data. This PR provides a checker that uses the cache for gathering enough data to determine if there is some work to be done. This helps to avoid sending requests to the API servers if there is no work to do.
the previous attempt didn't get traction - #1057