Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPSTREAM: 52092: Fix resource quota controller panic (Drop in 1.8) #16241

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

ironcladlou
Copy link
Contributor

@ironcladlou ironcladlou commented Sep 8, 2017

The pod evaluator used by the resource quota controller made direct
calls to an unsafe pod conversion function which mutates the pod
argument. With multiple resource quota controller workers, concurrent
processing of the same pod from a shared informer can result in a panic
when the conversion code attempts to write to a map field in the pod.

Swap out the direct conversion function call to Scheme.ConvertToVersion,
which copies the input before conversion.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486416
/xref kubernetes/kubernetes#52092

The pod evaluator used by the resource quota controller made direct
calls to an unsafe pod conversion function which mutates the pod
argument. With multiple resource quota controller workers, concurrent
processing of the same pod from a shared informer can result in a panic
when the conversion code attempts to write to a map field in the pod.

Swap out the direct conversion function call to Scheme.ConvertToVersion,
which copies the input before conversion.
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 8, 2017
@deads2k
Copy link
Contributor

deads2k commented Sep 8, 2017

The copy on every pod inspection is heavy, but usually beats crashing. We should probably fix the conversion function at some point instead of forcing a copy.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ironcladlou

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2017
@ironcladlou
Copy link
Contributor Author

@deads2k

We should probably fix the conversion function at some point instead of forcing a copy.

The mutations are gone in 1.8, which is why it's safe to just drop the patch entirely during a rebase.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit d223c6a into openshift:master Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants