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.

Sign up for GitHub

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

Jump to bottom

Deprecate HostConfig at container start #20615

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Deprecate HostConfig at container start #20615

merged 1 commit into from
Feb 5, 2016
+60 −57

Conversation

Random-Liu
Copy link
Member

For #20454 and #20583.

This PR changes the docker manager to set HostConfig when container creating instead of starting. For StartContainer() we just pass nil into it now.

@Random-Liu Random-Liu added area/upward-api sig/node Categorizes an issue or PR as relevant to SIG Node. labels Feb 4, 2016
@Random-Liu Random-Liu added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed area/upward-api labels Feb 4, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 4, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 4, 2016

GCE e2e test build/test passed for commit 4a386f8.

yujuhong
yujuhong reviewed Feb 4, 2016
View reviewed changes
pkg/kubelet/dockertools/manager.go
podHasSELinuxLabel := pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.SELinuxOptions != nil
binds := makeMountBindings(opts.Mounts, podHasSELinuxLabel)

// The reason we create and mount the log file in here (not in kubelet) is because
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TODO comment doesn't make sense anymore. Could you update them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@yujuhong
Copy link
Contributor

yujuhong commented Feb 4, 2016

LGTM overall with nits

@Random-Liu Random-Liu force-pushed the deprecate-hostconfig-at-container-start branch from 4a386f8 to 8118092 Compare February 4, 2016 23:35
@Random-Liu
Copy link
Member Author

@yujuhong Addressed comments and squashed.

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 8118092.

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@yujuhong
Copy link
Contributor

yujuhong commented Feb 5, 2016

@dchen1107, FYI

@yujuhong yujuhong assigned dchen1107 and unassigned yujuhong Feb 5, 2016
@yujuhong yujuhong removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@yujuhong
Copy link
Contributor

yujuhong commented Feb 5, 2016

Reassign to @dchen1107 so she can take a proper look before merging.

@dchen1107
Copy link
Member

Sorry for holding the pr for last check.

LGTM. Thanks for fixing.

@dchen1107 dchen1107 added this to the v1.2 milestone Feb 5, 2016
@dchen1107 dchen1107 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 5, 2016
@k8s-github-robot
Copy link

Travis continuous integration appears to have missed, closing and re-opening to trigger it

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e build/test failed for commit 8118092.

@k8s-github-robot
Copy link

@Random-Liu
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e build/test failed for commit 8118092.

@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016
@Random-Liu
Copy link
Member Author

Link a random flaky test because the test is aborted.
@k8s-bot test this issue: #20694

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 8118092.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Feb 5, 2016

GCE e2e test build/test passed for commit 8118092.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 5, 2016
Merge pull request #20615 from Random-Liu/deprecate-hostconfig-at-con…
df0bbd4 
…tainer-start

Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit df0bbd4 into kubernetes:master Feb 5, 2016
@Random-Liu Random-Liu deleted the deprecate-hostconfig-at-container-start branch February 5, 2016 17:45
This was referenced Feb 5, 2016
Closed
Closed
Merged
Closed
Merged
@yujuhong yujuhong mentioned this pull request Mar 17, 2016
Closed
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Aug 17, 2018
@k8s-publishing-bot
Merge pull request kubernetes#20615 from ingvagabund/backport-retriev…
c235f75 
…e-node-address-logic-from-upstream

Backport retrieve node address logic from upstream

Origin-commit: 3c717d73258620724d602ae5b6db99e2edeef995
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 29, 2019
@k8s-publishing-bot
Merge pull request kubernetes#20615 from ingvagabund/backport-retriev…
4b22fce 
…e-node-address-logic-from-upstream

Backport retrieve node address logic from upstream

Origin-commit: 3c717d73258620724d602ae5b6db99e2edeef995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dchen1107 dchen1107

Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
6 participants
@Random-Liu @k8s-github-robot @k8s-bot @yujuhong @dchen1107 @googlebot