Configuration manager for dynamic admission control registration by caesarxuchao · Pull Request #46302 · kubernetes/kubernetes

caesarxuchao · 2017-05-23T20:21:05Z

Implementing this section of kubernetes/community#611

Adding a configuration manager that reads the ExternalAdmissionHookConfigurations and InitializerConfigurations periodically, and returns the merged configuration.

cc @smarterclayton @whitlockjc

lavalamp · 2017-05-26T18:48:14Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

Can't I just do wait.Poll instead of using this?

I think I understand now.

lavalamp · 2017-05-26T18:52:57Z

pkg/kubeapiserver/admission/configuration/external_admission_hook_manager.go

missed a name change?

lavalamp · 2017-05-26T18:56:29Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

failureThreshold int

(don't use uint just because it shouldn't be negative)

lavalamp · 2017-05-26T19:00:16Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

please check ready inside the lock or there is a race

lavalamp · 2017-05-26T19:00:41Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

I think setting in this order is probably OK but if you lock around both of them then it is obvious that there is not a problem.

lavalamp · 2017-05-26T19:01:47Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

delete this function, it is not used outside this object and easy to cause a race when calling it.

lavalamp · 2017-05-26T19:02:12Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

consistentReadPoller, maybe?

Or just poller. It's a consistent poller only if the get is consistent.

lavalamp · 2017-05-26T19:16:25Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

newConsistentReadPoller?

lavalamp · 2017-05-26T19:22:21Z

pkg/kubeapiserver/admission/configuration/initializer_manager_test.go

The later if m.invoked <= 1+m.failures+m.successes block returns.

caesarxuchao · 2017-05-26T21:07:05Z

Comments addressed. PTAL. Thanks.

lavalamp · 2017-05-26T21:42:39Z

pkg/kubeapiserver/admission/configuration/configuration_manager.go

defaultInterval, defaultFailureTheshold

lavalamp · 2017-05-26T21:45:07Z

/lgtm
/approve

k8s-github-robot · 2017-05-26T21:45:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp

Details

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubeapiserver/OWNERS~~ [lavalamp]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-05-26T22:12:41Z

/release-note-none

k8s-ci-robot · 2017-05-26T22:57:40Z

@caesarxuchao: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	27289530479b788f9d2daf5925de848d206df6b3	link	`@k8s-bot pull-kubernetes-cross test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-05-27T03:34:07Z

Automatic merge from submit-queue

smarterclayton · 2017-05-27T14:05:41Z

This is flaking in other PRs:

k8s.io/kubernetes/pkg/kubeapiserver/admission/configuration TestConfiguration 0.03s
go test -v k8s.io/kubernetes/pkg/kubeapiserver/admission/configuration -run TestConfiguration$
initializer_manager_test.go:113: case number of failures just reaches failureThreshold, expect not ready

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/36721/pull-kubernetes-unit/33132/

lavalamp · 2017-05-30T18:19:53Z

@caesarxuchao ^^^^^

caesarxuchao · 2017-05-30T20:21:25Z

It's a test issue. I'll fix the test.

caesarxuchao · 2017-05-30T21:07:53Z

Sent #46648.

liggitt · 2017-05-31T17:41:20Z

pkg/kubeapiserver/admission/configuration/initializer_manager_test.go

+
+var _ InitializerConfigurationLister = &mockLister{}
+
+func TestConfiguration(t *testing.T) {


this test is flaking (see https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/46709/pull-kubernetes-unit/33781/)

Sent #46648.

caesarxuchao self-assigned this May 23, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 23, 2017

caesarxuchao mentioned this pull request May 24, 2017

Add initializer support to admission and uninitialized filtering to rest storage #36721

Merged

caesarxuchao force-pushed the acc-configuration-manager branch from e6dd1de to 756cb83 Compare May 26, 2017 17:52

caesarxuchao changed the title ~~[WIP] Configuration manager for dynamic admission control registration~~ Configuration manager for dynamic admission control registration May 26, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels May 26, 2017

caesarxuchao force-pushed the acc-configuration-manager branch from 756cb83 to 2728953 Compare May 26, 2017 18:21

lavalamp reviewed May 26, 2017

View reviewed changes

pkg/kubeapiserver/admission/configuration/external_admission_hook_manager.go Outdated

Copy link
Copy Markdown

Contributor

lavalamp May 26, 2017

Choose a reason for hiding this comment

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

missed a name change?

lavalamp reviewed May 26, 2017

View reviewed changes

pkg/kubeapiserver/admission/configuration/configuration_manager.go Outdated

Copy link
Copy Markdown

Contributor

lavalamp May 26, 2017

Choose a reason for hiding this comment

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

newConsistentReadPoller?

lavalamp reviewed May 26, 2017

View reviewed changes

caesarxuchao force-pushed the acc-configuration-manager branch from 2728953 to 485ffae Compare May 26, 2017 21:05

caesarxuchao assigned lavalamp and unassigned caesarxuchao May 26, 2017

lavalamp reviewed May 26, 2017

View reviewed changes

pkg/kubeapiserver/admission/configuration/configuration_manager.go Outdated

Copy link
Copy Markdown

Contributor

lavalamp May 26, 2017

Choose a reason for hiding this comment

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

defaultInterval, defaultFailureTheshold

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 26, 2017

the configuration manager

d7c1a92

caesarxuchao force-pushed the acc-configuration-manager branch from 485ffae to 36a8b99 Compare May 26, 2017 22:12

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 26, 2017

caesarxuchao removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 26, 2017

generated

dce3f69

caesarxuchao force-pushed the acc-configuration-manager branch from 36a8b99 to dce3f69 Compare May 26, 2017 23:06

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 26, 2017

k8s-github-robot merged commit 94312a0 into kubernetes:master May 27, 2017

liggitt reviewed May 31, 2017

View reviewed changes

caesarxuchao deleted the acc-configuration-manager branch May 12, 2020 23:09


		var _ InitializerConfigurationLister = &mockLister{}

		func TestConfiguration(t *testing.T) {

Conversation

caesarxuchao commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp commented May 26, 2017

Uh oh!

k8s-github-robot commented May 26, 2017

Uh oh!

caesarxuchao commented May 26, 2017

Uh oh!

k8s-ci-robot commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented May 27, 2017

Uh oh!

smarterclayton commented May 27, 2017

Uh oh!

lavalamp commented May 30, 2017

Uh oh!

caesarxuchao commented May 30, 2017

Uh oh!

caesarxuchao commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

caesarxuchao commented May 23, 2017 •

edited

Loading

k8s-ci-robot commented May 26, 2017 •

edited

Loading