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

Add OpenShift's recycler templates to Kubernetes controller config #16139

Merged
merged 2 commits into from
Sep 20, 2017
Merged

Add OpenShift's recycler templates to Kubernetes controller config #16139

merged 2 commits into from
Sep 20, 2017

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Sep 5, 2017

When user did not specify any recycler template files, create OpenShift one and let Kubernetes use it. The template file is created in /tmp and is deleted after controller initialization (which is safe).

This removes any OpenShift recycler <carry> patches in Kubernetes.

/assign @deads2k

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2017
@jsafrane
Copy link
Contributor Author

jsafrane commented Sep 5, 2017

Split into two commits to make travis happy.

@deads2k
Copy link
Contributor

deads2k commented Sep 5, 2017

UPSTREAM: revert: format so that we can find it during the rebase

deads2k
deads2k reviewed Sep 5, 2017
View reviewed changes
pkg/cmd/server/start/start_master.go Outdated
@@ -478,6 +476,25 @@ func (m *Master) Start() error {
// continuously run the scheduler while we have the primary lease
go runEmbeddedScheduler(m.config.MasterClients.OpenShiftLoopbackKubeConfig, m.config.KubernetesMasterConfig.SchedulerConfigFile, m.config.KubernetesMasterConfig.SchedulerArguments)

// OpenShift uses a different default volume recycler template than
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to remove newPersistentVolumeRecyclerPodTemplate from pkg/cmd/server/start/start_kube_controller_manager.go, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

deads2k
deads2k reviewed Sep 5, 2017
View reviewed changes
pkg/cmd/server/kubernetes/master/master_config.go Outdated
// adds it into PersistentVolumeRecyclerConfiguration if it is not set. It
// returns name of the template file that should be deleted after controllers
// start.
func AddRecyclerTemplates(masterConfig configapi.MasterConfig, cmserver *cmapp.CMServer) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all a mutation of BuildControllerManagerServer right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is all a mutation of BuildControllerManagerServer right?

I mean, we should call it from that method and construct the one we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of. I tried it and it looked extremely ugly. If I create a temp file in BuildControllerManagerServer, I need to delete it in all error paths between BuildControllerManagerServer call and startControllers. I can't use defer because startControllers runs in a separate goroutine and Master.Start might actually finish earlier than startControllers is called.

deads2k
deads2k reviewed Sep 5, 2017
View reviewed changes
pkg/cmd/server/kubernetes/master/master_config.go Outdated
return "", err
}

f, err := ioutil.TempFile("", "openshift-recycler-template-")
Copy link
Contributor

Choose a reason for hiding this comment

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

What permissions do we end up with. We need to make sure that most people can't mutate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ioutil.TempFile leads to os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) which looks paranoid enough to me.

deads2k
deads2k reviewed Sep 5, 2017
View reviewed changes
pkg/cmd/server/kubernetes/master/master_config.go Outdated
template.Spec.Containers[0].SecurityContext = &kapiv1.SecurityContext{RunAsUser: &uid}
template.Spec.Containers[0].ImagePullPolicy = kapiv1.PullIfNotPresent

templateBytes, err := json.Marshal(template)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to go through our normal encoding path. runtime.Encode(kapi.Codecs.LegacyCodec( would do I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2017
@jsafrane jsafrane force-pushed the tmpfile-template branch 3 times, most recently from c917a57 to 3760284 Compare September 6, 2017 11:44
@jsafrane
Copy link
Contributor Author

/retest

@jsafrane
Copy link
Contributor Author

filled #16278

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2017
@jsafrane
Add OpenShift's recycler templates to Kubernetes controller config
f1e6a45 
When user did not specify any recycler template files, create OpenShift one and
let Kubernetes use it.

The template file is created in /tmp and is deleted after controller
initialization (which is safe).
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2017
@jsafrane
Copy link
Contributor Author

Rebased a lot, it's almost a new PR.

@jsafrane
Copy link
Contributor Author

Umm, controllerapp.Run() never returns so nothing removes the temporary recycler template file. And there is no way how to fix it with current architecture.

@deads2k, any ideas? Will we always stick to patch we have now?

deads2k
deads2k reviewed Sep 18, 2017
View reviewed changes
pkg/cmd/server/start/start_kube_controller_manager.go Outdated
@@ -67,10 +49,11 @@ func kubeControllerManagerAddFlags(cmserver *controlleroptions.CMServer) func(fl
}
}

func newKubeControllerManager(kubeconfigFile, saPrivateKeyFile, saRootCAFile, podEvictionTimeout string, dynamicProvisioningEnabled bool, cmdLineArgs map[string][]string) (*controlleroptions.CMServer, error) {
func newKubeControllerManager(kubeconfigFile, saPrivateKeyFile, saRootCAFile, podEvictionTimeout, recyclerImage string, dynamicProvisioningEnabled bool, cmdLineArgs map[string][]string) (*controlleroptions.CMServer, []string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mind making this return a cleanupFn func()? Doesn't have to be typed. Just keeping all the cleanup logic in one spot with the thing making it.

@deads2k
Copy link
Contributor

deads2k commented Sep 18, 2017

Sorry, this fell of my radar. One minor comment, lgtm otherwise. Feel free to tag once done.

/approve

@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 18, 2017
@jsafrane
Copy link
Contributor Author

@deads2k As I wrote earlier this PR won't work. runEmbeddedKubeControllerManager calls controllerapp.Run(controllerManager). This Run() won't ever return so any cleanup in defer is impossible. And the template file is read inside this Run().

I don't think there is a sane way out of this.

@deads2k
Copy link
Contributor

deads2k commented Sep 18, 2017

@deads2k As I wrote earlier this PR won't work. runEmbeddedKubeControllerManager calls controllerapp.Run(controllerManager). This Run() won't ever return so any cleanup in defer is impossible. And the template file is read inside this Run().

I don't think there is a sane way out of this.

Alright. I may fiddle around in here later.

/lgtm

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

Ok, I reworked the cleanup to []func() instead of []string with filenames + I added TODO to the code. PTAL

@deads2k
Copy link
Contributor

deads2k commented Sep 18, 2017

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jsafrane
Copy link
Contributor Author

/retest

1 similar comment
@jsafrane
Copy link
Contributor Author

/retest

@jsafrane
Copy link
Contributor Author

flake: #16248
/retest

I wonder if this PR makes it more reproducible.

@jsafrane
Copy link
Contributor Author

/retest
#16414
#16248

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jsafrane
Copy link
Contributor Author

/retest

1 similar comment
@0xmichalis
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16411, 16139, 16430, 16435, 15794)

@openshift-merge-robot openshift-merge-robot merged commit a3aa986 into openshift:master Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@deads2k deads2k

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jsafrane @deads2k @openshift-merge-robot @openshift-bot @0xmichalis @openshift-ci-robot