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

pass down runtime-config through apiserver chain #57228

Merged
merged 2 commits into from Jan 23, 2018

Conversation

@hzxuzhonghu
Member

hzxuzhonghu commented Dec 15, 2017

What this PR does / why we need it:

kube-apiserver is actually a delegation chain of aggregator+kube+apiextensions. Let's pass down runtime-config through the chain, each layer removing the groups it knows about.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57189

Special notes for your reviewer:

make a new pkg k8s.io/apiserver/pkg/server/resourceconfig, and

  1. move resourceconfig related code to it.
  2. abstract generic used function MergeAPIResourceConfigs, put it here.

Release note:

NONE

@hzxuzhonghu hzxuzhonghu changed the title from Runtime config to pass down runtime-config through apiserver chain Dec 15, 2017

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Dec 15, 2017

/assign @sttts
/cc @dixudx

@k8s-ci-robot k8s-ci-robot requested a review from dixudx Dec 15, 2017

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Dec 15, 2017

/ok-to-test

@@ -89,6 +89,7 @@ func mergeAPIResourceConfigs(defaultAPIResourceConfig *serverstorage.ResourceCon
} else if allAPIFlagValue == "true" {
resourceConfig.EnableVersions(legacyscheme.Registry.RegisteredGroupVersions()...)
}
delete(overrides, "api/all")

This comment has been minimized.

@dixudx

dixudx Dec 15, 2017

Member

Why removing it from overrides? Same as below.
No need to do that IMO.

overrides will be used in below group/version/resource as well.

Also it's quite dangerous to remove a element from the list, especially during iteration.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 15, 2017

Member

Why removing it from overrides? Same as below.

Only pass runtime-config not used by previous apiservers.

overrides will be used in below group/version/resource as well.
only delete that used.

I search https://stackoverflow.com/questions/23229975/is-it-safe-to-remove-selected-keys-from-golang-map-within-a-range-loop
and find it is safe, in java it is unsafe .

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 15, 2017

Member

overrides will be used in below group/version/resource as well.

does not matter, only delete which used already.

This comment has been minimized.

@liggitt

liggitt Dec 15, 2017

Member

if you delete it here, only the ones registered into the legacy scheme will be enabled... don't we need to leave this in place for delegates to see that all are desired as well?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 16, 2017

Member

@liggitt You mean this api/all also allow enable all registered into other registries other than legacy one? If so, this should be reserved.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 18, 2017

Member

I found, we can not delete this here. It is called twice in kube-apiserver.

@@ -131,13 +135,25 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
GenericAPIServer: genericServer,
}
// TODO: move this to a another file

This comment has been minimized.

@dixudx

dixudx Dec 15, 2017

Member

Why not do it now? Any blockers?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 15, 2017

Member

Currently, only one group version in the apiserver, I am not sure whether we need more groups in the future.

@@ -212,3 +228,109 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
return s, nil
}
func defaultAPIResourceConfigSource() *serverstorage.ResourceConfig {

This comment has been minimized.

@dixudx

dixudx Dec 15, 2017

Member

defaultAPIResourceConfigSource and mergeAPIResourceConfigs are exactly the same with those in staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go.

Better to move to staing/src/k8s.io/apiserver/pkg/util/<xx-folder>. May create a new folder if needed.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 15, 2017

Member

yes, make sense.
@sttts, what's your opinion?

This comment has been minimized.

@sttts

sttts Dec 15, 2017

Contributor

yes, they should be at a central place in k8s.io/apiserver. pkg/util or even pkg/server/resourceconfig or something like that?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 15, 2017

Member

But have one issue, if move to staing/src/k8s.io/apiserver/pkg/util/<xx-folder>, the pkg will import serverstorage "k8s.io/apiserver/pkg/server/storage"
this may be not acceptable.

This comment has been minimized.

@sttts

sttts Dec 15, 2017

Contributor

why not into k8s.io/apiserver/pkg/server/resourceconfig?

This comment has been minimized.

@hzxuzhonghu
@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Dec 16, 2017

  1. update create pkg k8s.io/apiserver/pkg/server/resourceconfig, and move ResourceConfig related code from k8s.io/apiserver/pkg/server/storage into it.
  2. Fix some test cases.
@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Dec 18, 2017

@sttts @liggitt I think all comments addressed.

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
package storage
package resourceconfig

This comment has been minimized.

@sttts

sttts Dec 19, 2017

Contributor

do we have to move the interfaces? It would make this PR much smaller if we didn't.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 19, 2017

Member

Not a must.

@@ -181,13 +182,27 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
serviceResolver: c.ExtraConfig.ServiceResolver,
}
apiResourceConfig, err := resourceconfig.MergeAPIResourceConfigs(

This comment has been minimized.

@sttts

sttts Dec 19, 2017

Contributor

Can we turn this boilerplate code into a shared APIEnablementOptions with an ApplyTo func? And then in GenericApiServer.Complete we can fill in a default "allow all resources" APIResourceConfigSource?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 20, 2017

Member

merged apiResourceConfig currently is used locally, I do not make it shared. And I make APIEnablementOptions into RecommendedOptions.

@@ -175,6 +176,9 @@ type Config struct {
// if the client requests it via Accept-Encoding
EnableAPIResponseCompression bool
// RuntimeConfig contains the options for which resources to turn on and off.
RuntimeConfig utilflag.ConfigurationMap

This comment has been minimized.

@sttts

sttts Dec 19, 2017

Contributor

This should move into APIEnablementOptions (or the corresponding ApplyTo func) too. Here we should have a complete, merged *ResourceConfig. Flag processing belongs into the options.

@@ -134,11 +136,17 @@ func CreateServerChain(runOptions *options.ServerRunOptions, stopCh <-chan struc
if err != nil {
return nil, err
}
groups, err := resourceconfig.ParseGroups(runOptions.APIEnablement.RuntimeConfig)

This comment has been minimized.

@sttts

sttts Dec 20, 2017

Contributor

how about moving all these calls to one place? maybe easier to follow.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 20, 2017

Member

make sense.

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Dec 21, 2017

Take @sttts advice, and renew thoroughly.

}
// Merges the given defaultAPIResourceConfig with the given resourceConfigOverrides.
func mergeAPIResourceConfigs(defaultAPIResourceConfig *serverstorage.ResourceConfig, resourceConfigOverrides utilflag.ConfigurationMap) (*serverstorage.ResourceConfig, error) {

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 21, 2017

Member

move all these merge functions to resourceconfig/helpers.gp

@@ -175,6 +176,10 @@ type Config struct {
// if the client requests it via Accept-Encoding
EnableAPIResponseCompression bool
// RuntimeConfig contains the options for which groupVersion resources to turn on and off.
// This is passed from command line.
MergedResourceConfig *serverstore.ResourceConfig

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Dec 21, 2017

Member

use this to pass through apiserver chain.

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Jan 22, 2018

I see, squashed.

@sttts

This comment has been minimized.

Contributor

sttts commented Jan 22, 2018

/lgtm

@deads2k has to approve.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 22, 2018

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Jan 22, 2018

Thanks @sttts

@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 22, 2018

I'd still like to see these stay in the kube-apiserver, but @sttts convinced me that this is still an improvement.

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, hzxuzhonghu, sttts

Associated issue: #57189

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 22, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 23, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 23, 2018

Automatic merge from submit-queue (batch tested with PRs 58547, 57228, 58528, 58499, 58618). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit f0b7319 into kubernetes:master Jan 23, 2018

4 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-bazel-test
Details
pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-unit Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation hzxuzhonghu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke-gci Skipped
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 23, 2018

@hzxuzhonghu: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke-gci a008ec6 link /test pull-kubernetes-e2e-gke-gci
pull-kubernetes-e2e-kops-aws eff1f20 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce-device-plugin-gpu eff1f20 link /test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

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.

@hzxuzhonghu hzxuzhonghu deleted the hzxuzhonghu:runtime-config branch Jan 23, 2018

@krzyzacy

This comment has been minimized.

Member

krzyzacy commented Jan 25, 2018

(this PR could've break all master gke jobs given last run of the gke job on this PR https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/57228/pull-kubernetes-e2e-gke-gci/21485/ is red, and testgrid breakage diff is 2b0c7e2...1789a85)

cc @kubernetes/sig-gcp-bugs @abgworrall @caesarxuchao

k8s-merge-robot added a commit that referenced this pull request Jan 29, 2018

Merge pull request #58863 from hzxuzhonghu/runtime-config-resource-re…
…move

Automatic merge from submit-queue (batch tested with PRs 56995, 58498, 57426, 58902, 58863). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

cleanup enable/disable api resources code

**What this PR does / why we need it**:

After #57228, `runtime-config` flag has stop support enable/disable resources of a specific groupVersion,
so this pr does some clean work about this.

Mainly delete unused code in  `k8s.io/apiserver/pkg/server/storage/resource_config.go`

**Special notes for your reviewer**:
/assign @deads2k  @sttts 
**Release note**:

```release-note
NONE
```
/kind cleanup
@perotinus

This comment has been minimized.

Member

perotinus commented Mar 30, 2018

@hzxuzhonghu This PR looks like it removes the ability to disable specific resources within a group. Is that intentional? That seems like a regression in the capability of the API server.

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Mar 30, 2018

Yes, that's intended. In real scenario, I can hardly think of when want to only enable/disable specific resources in a group.

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 30, 2018

I think #57228 (comment) was the intent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment