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

Allow shareable resources for admission control plugins. #26709

Merged
merged 3 commits into from
Jul 22, 2016

Conversation

hodovska
Copy link

@hodovska hodovska commented Jun 2, 2016

Changes allow admission control plugins to share resources. This is done via new PluginInitialization structure. The structure can be extended for other resources, for now it is an shared informer for namespace plugins (NamespiceLifecycle, NamespaceAutoProvisioning, NamespaceExists).

If a plugins needs some kind of shared resource e.g. client, the client shall be added to PluginInitializer and Wants methods implemented to every plugin which will use it.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 2, 2016
@ncdc
Copy link
Member

ncdc commented Jun 2, 2016

@hodovska could you please fill in the description with some information about what this does?

@hodovska hodovska force-pushed the master branch 2 times, most recently from 0457113 to 5016de4 Compare June 6, 2016 08:35
@ncdc
Copy link
Member

ncdc commented Jun 6, 2016

FYI @smarterclayton

// Validator should be implemented by admission plugins that can validate themselves
// after initialization has happened.
type Validator interface {
Validate() error
Copy link
Member

Choose a reason for hiding this comment

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

Godoc on the function too, please

@hodovska hodovska force-pushed the master branch 2 times, most recently from 9601abc to 6300d70 Compare June 8, 2016 13:07
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2016
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. kind/old-docs size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2016
@googlebot
Copy link

CLAs look good, thanks!

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2016
@hodovska hodovska force-pushed the master branch 2 times, most recently from 5aeea93 to 30db68d Compare July 20, 2016 11:43
@@ -119,8 +121,13 @@ func Run(s *genericoptions.ServerRunOptions) error {
if err != nil {
glog.Errorf("Failed to create clientset: %v", err)
}
admissionController := admission.NewFromPlugins(client, admissionControlPluginNames, s.AdmissionControlConfigFile)
sharedInformers := informers.NewSharedInformerFactory(client, 10*time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

spawn an issue to make this somethign on struct that we can pass in.

Copy link
Member

Choose a reason for hiding this comment

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

You mean on ServerRunOptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean on ServerRunOptions?

Hrm, now I remember why we're unable to use these paths to build the server downstream. We have come in a layer deeper to provide a completely separate admission chain.

@deads2k
Copy link
Contributor

deads2k commented Jul 20, 2016

I must be missing where you're calling InformerFactory.Start. Are you sure the admission plugins are being tested?

If @ncdc sees it, this looks like a good starting point to me. I'd like to see followup issues.

@ncdc
Copy link
Member

ncdc commented Jul 20, 2016

@deads2k you're right - it looks like it's only called in the unit tests. We just need to do it after the calls to informers.NewSharedInformerFactory, right? @hodovska could you please make sure to do this?

@deads2k
Copy link
Contributor

deads2k commented Jul 20, 2016

You should start the informers just befor you launch the server

@ncdc
Copy link
Member

ncdc commented Jul 21, 2016

@k8s-bot e2e test this issue: #29308

@ncdc
Copy link
Member

ncdc commented Jul 21, 2016

@deads2k Start() calls added

@@ -59,7 +60,7 @@ func (p *provision) Admit(a admission.Attributes) (err error) {
},
Status: api.NamespaceStatus{},
}
_, exists, err := p.store.Get(namespace)
_, exists, err := p.informerFactory.Namespaces().Informer().GetStore().Get(namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Followup issue to wait for the cache to sync. This is a pre-existing bug.

@ncdc ncdc removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 22, 2016
@deads2k
Copy link
Contributor

deads2k commented Jul 22, 2016

It's a good starting point (functionally equivalent), lets merge it and follow up with issues and fixes for:

  • refactor the CreateSharedXXXIndexInformer and lazy informerfactory gets so that one calls the other.
  • allow .Start to be called multiple times
  • wait for caches to sync in admission plugins before admitting (check the openshift clusterquota admission for an example)
  • update the admission plugins to hold the values they need from the informerfactory when initialized
  • update the controllers to use this interface.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2016
@ncdc
Copy link
Member

ncdc commented Jul 22, 2016

Not sure why the e2e is listed as failed because the gs bucket only shows started.json and no artifacts. @ixdy @spxtr @pwittrock ?

@k8s-bot e2e test this issue: #IGNORE

@ncdc
Copy link
Member

ncdc commented Jul 22, 2016

GKE smoke e2e failure is #27462 (again!)

@ncdc
Copy link
Member

ncdc commented Jul 22, 2016

@k8s-bot e2e test this issue: #27462

@ncdc
Copy link
Member

ncdc commented Jul 22, 2016

@k8s-bot e2e test this issue: #29308

@k8s-bot
Copy link

k8s-bot commented Jul 22, 2016

GCE e2e build/test passed for commit 037d116.

@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 Jul 22, 2016

GCE e2e build/test passed for commit 037d116.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants