Skip to content

use new crio socket path via constant#17761

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
bparees:crio
Dec 14, 2017
Merged

use new crio socket path via constant#17761
openshift-merge-robot merged 1 commit into
openshift:masterfrom
bparees:crio

Conversation

@bparees
Copy link
Copy Markdown
Contributor

@bparees bparees commented Dec 13, 2017

No description provided.

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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

Details 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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2017
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 13, 2017
@bparees
Copy link
Copy Markdown
Contributor Author

bparees commented Dec 13, 2017

@mrunalp @runcom ptal.

@bparees
Copy link
Copy Markdown
Contributor Author

bparees commented Dec 13, 2017

@deads2k @derekwaynecarr fyi.

@bparees
Copy link
Copy Markdown
Contributor Author

bparees commented Dec 13, 2017

/test crio

@runcom
Copy link
Copy Markdown
Member

runcom commented Dec 13, 2017

LGTM

@bparees
Copy link
Copy Markdown
Contributor Author

bparees commented Dec 13, 2017

/retest

@openshift-merge-robot
Copy link
Copy Markdown
Contributor

Automatic merge from submit-queue (batch tested with PRs 17734, 17550, 17647, 17761, 17564).

@openshift-merge-robot openshift-merge-robot merged commit c6d3fa6 into openshift:master Dec 14, 2017
@smarterclayton
Copy link
Copy Markdown
Contributor

Please stop taking constant dependencies from controllers onto components that should not be required for controllers.

@bparees
Copy link
Copy Markdown
Contributor Author

bparees commented Dec 15, 2017

Please stop taking constant dependencies from controllers onto components that should not be required for controllers.

The crio team changed the value of this path between releases and we weren't using the constant and broke. Making us carry our own copy of the value is fragile.

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Dec 15, 2017

The crio team changed the value of this path between releases and we weren't using the constant and broke. Making us carry our own copy of the value is fragile.

Doing this creates a diamond dependency problem between controllers and kubelet that ends up left as debt for someone doing a rebase to try to sort out. Someone unfamiliar with the code then has to decide how to reconcile competing levels of a fast moving dependency. If you were actually speaking to cadvisor or interacted with it, this diamond is worth resolving. If you aren't, incurring that debt seems questionable.

Even with this dependency you introduced, changing a constant value will still produce problems at runtime since you're trying to match constant values across components which are allows (and expected) to skew.

@smarterclayton
Copy link
Copy Markdown
Contributor

smarterclayton commented Dec 15, 2017 via email

@bparees bparees deleted the crio branch December 15, 2017 23:08
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.

6 participants