allow failed discovery on initial quota controller start #67433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
sig/api-machinery
k8s-ci-robot
added
cncf-cla: yes
liggitt
reviewed
Aug 15, 2018
| @@ -161,10 +161,11 @@ func NewResourceQuotaController(options *ResourceQuotaControllerOptions) (*Resou | |||
|
|
|||
| rq.quotaMonitor = qm | |||
|
|
|||
| // do initial quota monitor setup | |||
| // do initial quota monitor setup. If we have a discovery failure here, it's ok. We'll discover more resources when | |||
| // a later sync happens. | |||
| resources, err := GetQuotableResources(options.DiscoveryFunc) | |||
| if err != nil { | |||
There was a problem hiding this comment.
should this only tolerate IsGroupDiscoveryFailedError errors?
liggitt
reviewed
Aug 15, 2018
| @@ -421,7 +422,9 @@ func (rq *ResourceQuotaController) Sync(discoveryFunc NamespacedResourcesFunc, p | |||
| // Something has changed, so track the new state and perform a sync. | |||
| oldResources := make(map[schema.GroupVersionResource]struct{}) | |||
| wait.Until(func() { | |||
| // Get the current resource list from discovery. | |||
| // Get the current resource list from discovery. A failure here will (should?) prevent updates to the sync list. | |||
There was a problem hiding this comment.
Is tolerating partial discovery on start but not on resync what we want? I expected them to match. ErrGroupDiscoveryFailed errors expose which group/versions failed, if we want to use that info
There was a problem hiding this comment.
even without inspecting the details of the error, we could do something like this as a simple start. letting the quota controller continue running without using available resource info from discovery doesn't seem great.
// Get the current resource list from discovery.
newResources, err := GetQuotableResources(discoveryFunc)
if err != nil {
utilruntime.HandleError(err)
if discovery.IsGroupDiscoveryFailedError(err) && len(newResources) > 0 {
// In partial discovery cases, don't remove any existing informers, just add new ones
for k, v := range oldResources {
newResources[k] = v
}
} else {
// short circuit in non-discovery error cases or if discovery returned zero resources
return
}
}
This was referenced Aug 15, 2018
|
/assign @yliaog |
deads2k
force-pushed
the
controller-02-quotadiscovery
branch
from
dbbfb7d
to
4c8e9de
Compare
|
@liggitt comments addressed. |
|
/retest |
k8s-ci-robot
added
release-note-none
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, yliaog 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 |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
or @deads2k: cherry-pick to |
k8s-github-robot
pushed a commit
that referenced
this pull request
Aug 21, 2018
…2-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #66932 #67433: Include unavailable API services in discovery response, allow failed discovery on initial quota controller start Cherry pick of #66932 on release-1.10. #66932: Include unavailable API services in discovery response #67433: allow failed discovery on initial quota controller start ```release-note kube-apiserver now includes all registered API groups in discovery, including registered extension API group/versions for unavailable extension API servers. kube-controller-manager can now start the quota controller when discovery results can only be partially determined. ```
k8s-github-robot
pushed a commit
that referenced
this pull request
Aug 21, 2018
…2-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #66932 #67433: Include unavailable API services in discovery response, allow failed discovery on initial quota controller start Cherry pick of #66932 #67433 on release-1.11. #66932: Include unavailable API services in discovery response #67433: allow failed discovery on initial quota controller start ```release-note kube-apiserver now includes all registered API groups in discovery, including registered extension API group/versions for unavailable extension API servers. kube-controller-manager can now start the quota controller when discovery results can only be partially determined. ```
k8s-ci-robot
added
release-note
k8s-github-robot
pushed a commit
that referenced
this pull request
Aug 31, 2018
…33-upstream-release-1.9 Automatic merge from submit-queue. allow failed discovery on initial quota controller start Cherry pick of #67433 on release-1.9. #67433: allow failed discovery on initial quota controller start kube-controller-manager can now start the quota controller when discovery results can only be partially determined. ```release-note NONE ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #65005
Aggregated API servers now correctly provide 503s on discovery endpoints for groups that cannot be reached. This means that the kube-controller-manager process is now sensitive to discovery failures in the quota controller. This change allows discovery failures in the initial quota replenishment controller resource discovery.
@liggitt suspects similar races exist to those he found GC last release, but this pull doesn't make that better or worse.
@kubernetes/sig-api-machinery-bugs