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

do ugly things to wire a cloudconfig file #14444

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jun 1, 2017

Might fix https://bugzilla.redhat.com/show_bug.cgi?id=1454601

This punches through a couple layers to build an options object twice in order to plumb a value through an admission wiring step before admission can logically be completed to hopefully make an admission plugin work.

@eparis

@deads2k
Copy link
Contributor Author

deads2k commented Jun 1, 2017

[test]

No tests for this, so it probably works.

// note: we are passing a combined quota registry here...
kubePluginInitializer := kadmission.NewPluginInitializer(privilegedLoopbackKubeClientsetInternal, internalkubeInformerFactory, nil, nil, quotaRegistry)
kubePluginInitializer := kadmission.NewPluginInitializer(privilegedLoopbackKubeClientsetInternal, internalkubeInformerFactory, authorizer, cloudConfig, quotaRegistry)
originAdmission, kubeAdmission, err := buildAdmissionChains(options, privilegedLoopbackKubeClientsetInternal, pluginInitializer, kubePluginInitializer)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we passing authorizer here now?

@gnufied
Copy link
Member

gnufied commented Jun 1, 2017

I have tested this to work on a multizone GCE cluster. Also added a card for writing a e2e test - https://trello.com/c/Ul2Ldjjn/508-write-an-e2e-test-for-multizone-fix

@liggitt
Copy link
Contributor

liggitt commented Jun 2, 2017

is this what the upstream admission PR will improve? are you planning to pick that, or carry this until we get to 1.7?

nevermind, that was the controller refactor

@deads2k deads2k force-pushed the api-03-cloduconfig branch from de30259 to ee1db3b Compare June 2, 2017 13:10
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to ee1db3b

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1906/) (Base Commit: 5f2f3f4)

@deads2k
Copy link
Contributor Author

deads2k commented Jun 2, 2017

is this what the upstream admission PR will improve? are you planning to pick that, or carry this until we get to 1.7?

Well, I thought we had a controller problem, not an admission one. I think we need to refactor the upstream admission plugin to use runtime information instead of depending on files on disk that deep into the init structure, but that would be 1.8 I think.

@deads2k
Copy link
Contributor Author

deads2k commented Jun 5, 2017

There was a request to merge before the sprint opened, so I'll take that as lgtm. Doing it now [merge]

@gnufied
Copy link
Member

gnufied commented Jun 6, 2017

Turns out we already have an e2e test for statically provisioned PVs - kubernetes/kubernetes#42829 . Although, it was added kind of recently.

@deads2k
Copy link
Contributor Author

deads2k commented Jun 7, 2017

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to ee1db3b

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 7, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/918/) (Base Commit: 748b1ab) (Image: devenv-rhel7_6327)

@openshift-bot openshift-bot merged commit e935d8e into openshift:master Jun 7, 2017
@deads2k deads2k deleted the api-03-cloduconfig branch August 3, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants