move apiserver Configuration to k8s.io/apiserver/pkg/apis/config #66059

hanxiaoshuai · 2018-07-11T08:24:45Z

What this PR does / why we need it:
ref #2354
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 #

Special notes for your reviewer:
After the related componentconfig to be moved to staging, LeaderElectionConfiguration and DebuggingConfiguration should be clean up in pkg/apis/componentconfig
Release note:

NONE

hanxiaoshuai · 2018-07-11T08:25:21Z

/cc @luxas @sttts

hanxiaoshuai · 2018-07-11T09:58:42Z

/retest

hanxiaoshuai · 2018-07-11T11:20:03Z

/hold

luxas

Thanks for this PR!
@rosti FYI
Please add OWNERS file as in the KEP. Fix verify. Add comment to DebuggingConfiguration. Lowercase the EnableContentionProfiling comment. Maybe add validation now or in a later PR. Move/duplicate the "endpoints" string to this package.

luxas · 2018-07-11T17:57:51Z

Feel free to add these packages that need it to .golint_failures btw

neolit123 · 2018-07-11T23:55:40Z

@kubernetes/sig-api-machinery-pr-reviews

stewart-yu · 2018-07-12T00:51:01Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/types.go

+	// leaderElect enables a leader election client to gain leadership
+	// before executing the main loop. Enable this when running replicated
+	// components for high availability.
+	LeaderElect *bool `json:"leaderElect"`


IMO, we should add probuf for all var , syncing with others api struct, so that prase easy . such as https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/apis/example/v1/types.go#L60. Even that may be unnecessary now. @luxas @sttts WDYT

do we support protobuf for configs? Do we want to?

Actually, no need for now, Json enough.

hanxiaoshuai · 2018-07-12T11:30:31Z

@luxas Done, and I will add validation later if it is need.
/assign @luxas @sttts

sttts · 2018-07-12T11:58:37Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/doc.go

+
+// +k8s:deepcopy-gen=package
+// +k8s:conversion-gen=k8s.io/apiserver/pkg/apis/config
+// +k8s:openapi-gen=true


do we want this in openapi?

It seems we don't want it, I remove it

sttts · 2018-07-12T11:58:48Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/register.go

+)
+
+var (
+	// TODO: move SchemeBuilder with zz_generated.deepcopy.go to k8s.io/api.


remove TODO

Done, Thanks for your review

hanxiaoshuai · 2018-07-12T12:37:38Z

/hold cancel

liggitt · 2018-07-12T13:02:25Z

staging/src/k8s.io/apiserver/pkg/apis/config/OWNERS

@@ -0,0 +1,7 @@
+approvers:


the OWNERS file from k8s.io/apiserver would probably be a more appropriate starting point for the reviewers list (and the api approvers owners file for approvals, since config is considered API... see https://docs.google.com/document/d/1OkSQngGem7xaENqaO8jzHLDSSIGh2obPUaJGAFDwTUE/edit#)

@luxas @sttts what's your opinions?

+1 for @liggitt's suggestion.

liggitt · 2018-07-12T13:05:57Z

staging/src/k8s.io/apiserver/pkg/apis/config/types.go

+)
+
+// LeaderElectionConfiguration defines the configuration of leader election
+// clients for components that can run with leader election enabled.


does this belong in apiserver? lots of things that aren't apiservers use leader election. I expected it in apimachinery

As it discussed it in doc, in apiserver part, LeaderElectionConfiguration was in it.
@luxas @sttts can you show some info?

We were undecided about this one. It seems to be higher level than apimachinery, but also has no tight connections to apiserver. I don't feel strongly with either direction. As the types are kube independent, we can technically also move it to apimachinery. I would like to see some generic ResourceLock syntax which does not reference kube.

cc @mtaufen @wojtek-t who were working on the Lease API

does this belong in apiserver? lots of things that aren't apiservers use leader election. I expected it in apimachinery

I don't see this as related to our type system in the same way as TypeMeta, ObjectMeta, Time, or List. I don't even see it as quite as fundamental as ListOptions. Something common for many bits of config, sure. Something common for all types/schemes, not really.

If I understand correctly, components can migrate to using the Lease API (it was designed to represent the same information as LeaderElectionRecord), but they still have to get related configuration from somewhere (i.e. LeaderElectionConfiguration).

So this is a question of where to put a common config struct type?

is this resolved?

Yes, I think the consensus is to put it here, in k8s.io/apiserver vs putting it in k8s.io/apimachinery which should hold even more generic types.

sttts · 2018-07-12T18:34:43Z

staging/src/k8s.io/apiserver/pkg/apis/config/types.go

+	RetryPeriod metav1.Duration
+	// resourceLock indicates the resource object type that will be used to lock
+	// during leader election cycles.
+	ResourceLock string


what are the value of this? resource plus apigroup plus version?

The default is string "endpoints": https://github.com/kubernetes/kubernetes/pull/66059/files#diff-315017a8fe6dd5515919e19a0fffcc20R43

There are two lock types endpoints and configmaps: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/leaderelection/resourcelock/interface.go

fedebongio · 2018-07-12T20:10:30Z

/cc @roycaihw

luxas · 2018-07-27T17:03:49Z

/assign @jbeda

sttts · 2018-07-27T17:12:54Z

Can we at least create the clean-up PR now already to see possible issues early? Don't like to duplicate code and post-poin the cleanups.

/hold

luxas · 2018-07-30T15:30:20Z

@sttts The PR that cleans this up/moves the usage is here: #66722.
I prefer having clean PRs like this so that one PR adds the net-new code and one PR cleans stuff up later if that is ok by you.
/hold cancel

luxas · 2018-08-04T20:04:20Z

Can we please get a top-level approval of this to get the ball rolling so we can then cleanup the types (PR #66722) ASAP, and unblock the components' types moving (e.g. #66916).

We can always change the structure later if we feel we need to, before going beta with this, but if this is gonna be stuck forever we have no chance improving anything.

jbeda · 2018-08-04T23:59:11Z

/approve

Just to get this moving.

luxas

/lgtm

Thanks @hanxiaoshuai and @jbeda!
Let's get the ball rolling here and follow-up on minor nits and changes as we go.

luxas · 2018-08-05T12:38:01Z

staging/src/k8s.io/apiserver/pkg/apis/config/types.go

+)
+
+// LeaderElectionConfiguration defines the configuration of leader election
+// clients for components that can run with leader election enabled.


Yes, I think the consensus is to put it here, in k8s.io/apiserver vs putting it in k8s.io/apimachinery which should hold even more generic types.

luxas · 2018-08-05T12:40:27Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/doc.go

+
+// +k8s:deepcopy-gen=package
+// +k8s:conversion-gen=k8s.io/apiserver/pkg/apis/config
+// +k8s:defaulter-gen=TypeMeta


As said in the other PR, we're not sure if we actually need this, but we're leaving it as-is for now

luxas · 2018-08-05T12:41:01Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/defaults.go

+		obj.ResourceLock = EndpointsResourceLock
+	}
+	if obj.LeaderElect == nil {
+		obj.LeaderElect = &booltrue


nit: in the future you can use k8s.io/utils/pointer.BoolPtr(true)

luxas · 2018-08-05T12:42:25Z

staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/defaults.go

+	kruntime "k8s.io/apimachinery/pkg/runtime"
+)
+
+func addDefaultingFuncs(scheme *kruntime.Scheme) error {


TODO: we haven't fully decided if we're gonna register the defaults like this in the scheme, but having this as it is the default way of doing things right now. We can always change stuff later.

k8s-ci-robot · 2018-08-05T12:43:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hanxiaoshuai, jbeda, luxas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [jbeda]
~~staging/OWNERS~~ [jbeda]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-08-05T13:55:24Z

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

k8s-github-robot · 2018-08-05T14:05:05Z

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

@sttts

Automatic merge from submit-queue (batch tested with PRs 67071, 66906, 66722, 67276, 67039). 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>. Remove references to the structs that have moved to their own packages **What this PR does / why we need it**: Follows-up #66058 and #66059 to remove the structs that now aren't needed in `pkg/apis/componentconfig` **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: xref kubernetes/community#2354 **Special notes for your reviewer**: This PR depends on: - [x] #67090 - [x] #67149 - [x] #67159 - [x] #67207 **Only review commit 'Remove references to the structs that have moved to their own packages' please** **Release note**: ```release-note NONE ``` /kind cleanup /assign @sttts @thockin @jbeda @liggitt

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 11, 2018

k8s-ci-robot requested review from caesarxuchao and sttts July 11, 2018 08:24

k8s-ci-robot requested a review from luxas July 11, 2018 08:25

hanxiaoshuai force-pushed the apicfg branch 2 times, most recently from d22fc3a to 49de79e Compare July 11, 2018 09:49

hanxiaoshuai force-pushed the apicfg branch from 49de79e to 78fc64a Compare July 11, 2018 11:10

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2018

luxas reviewed Jul 11, 2018

View reviewed changes

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 11, 2018

stewart-yu reviewed Jul 12, 2018

View reviewed changes

hanxiaoshuai force-pushed the apicfg branch 2 times, most recently from 5b7f20b to aabee11 Compare July 12, 2018 09:53

k8s-ci-robot assigned luxas and sttts Jul 12, 2018

sttts reviewed Jul 12, 2018

View reviewed changes

hanxiaoshuai force-pushed the apicfg branch from aabee11 to 042786f Compare July 12, 2018 12:30

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2018

liggitt reviewed Jul 12, 2018

View reviewed changes

k8s-ci-robot assigned liggitt and thockin Jul 12, 2018

sttts reviewed Jul 12, 2018

View reviewed changes

k8s-ci-robot requested a review from roycaihw July 12, 2018 20:10

hanxiaoshuai force-pushed the apicfg branch from e859e7a to 10d1342 Compare July 13, 2018 01:27

k8s-ci-robot assigned jbeda Jul 27, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2018

luxas mentioned this pull request Jul 27, 2018

Remove references to the structs that have moved to their own packages #66722

Merged

4 tasks

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2018

hanxiaoshuai added 4 commits August 5, 2018 11:43

move apiserver Configuration to k8s.io/apiserver/pkg/apis/config

0875ffe

add generated code files

2193f98

add an OWNERS file

6be5a07

update zz_generated.conversion.go file

d6caefe

hanxiaoshuai force-pushed the apicfg branch from 10d1342 to d6caefe Compare August 5, 2018 03:51

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2018

luxas approved these changes Aug 5, 2018

View reviewed changes

k8s-github-robot merged commit 7dcbdbb into kubernetes:master Aug 5, 2018

liggitt mentioned this pull request Aug 7, 2018

Moving KubeSchedulerConfiguration from ComponentConfig API types to staging repos #66916

Merged

stewart-yu mentioned this pull request Aug 12, 2018

Work tracker for moving ComponentConfig types to staging repos #67233

Closed

15 tasks

move apiserver Configuration to k8s.io/apiserver/pkg/apis/config #66059

move apiserver Configuration to k8s.io/apiserver/pkg/apis/config #66059

Conversation

hanxiaoshuai commented Jul 11, 2018

hanxiaoshuai commented Jul 11, 2018

hanxiaoshuai commented Jul 11, 2018

hanxiaoshuai commented Jul 11, 2018

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 11, 2018

neolit123 commented Jul 11, 2018

stewart-yu Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanxiaoshuai commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanxiaoshuai commented Jul 12, 2018

liggitt Jul 12, 2018 • edited

Choose a reason for hiding this comment

hanxiaoshuai Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedebongio commented Jul 12, 2018

luxas commented Jul 27, 2018

sttts commented Jul 27, 2018

luxas commented Jul 30, 2018

luxas commented Aug 4, 2018

jbeda commented Aug 4, 2018

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

stewart-yu Jul 12, 2018 •

edited

liggitt Jul 12, 2018 •

edited

hanxiaoshuai Jul 12, 2018 •

edited