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

Move config local to every controller in KCM #72800

Merged
merged 4 commits into from Mar 22, 2019

Conversation

@stewart-yu
Copy link
Contributor

commented Jan 11, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
move controller's config from pkg/controller/apis/config to pkg/controller/<controller-name>/config,
and in that's way, every controller's type will be its own deserializable Kinds.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-component-base branch from ea5dec2 to a080441 Jan 11, 2019

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

This comment has been minimized.

Copy link
@sttts

sttts Jan 11, 2019

Contributor

is this the same list as before? Can we identify the main developers of each controller via git blame?

This comment has been minimized.

Copy link
@stewart-yu

stewart-yu Jan 11, 2019

Author Contributor

sure.
And little busy doing others thing now, will fix all failure later. If all done, i will ping all in slack 😉

@stewart-yu stewart-yu force-pushed the stewart-yu:stewart-component-base branch 5 times, most recently from 417ec1f to 6d25999 Jan 13, 2019

@luxas
Copy link
Member

left a comment

Thanks very much @stewart-yu!

Overall this looks ok. We have the struggle of wanting the config to be close to the controller, but still be publicly available. I think we need to move the conversions from pkg/controller/apis/config where they are now in this PR to local to the controller, using the k8s:conversion-gen-external-types tag per-controller.

We should make these configuration types decodable by marking them with the deepcopy runtime.Object & TypeMeta tags, idk if we should do that in this PR or not. Maybe you could make that change in a dedicated commit (in k8s.io/kube-controller-manager/config) so we can easily rip it out if we don't want to have it

The end goal here is that every controller config kind is encodable/decodable on its own. Hence every schemebuilder in every package should register what's needed for it to work, and the pkg/controller/apis/config/scheme package would just run AddToScheme() per-controller.

The OWNERS files are ok as-is for now, we can make those more suitable in a future PR.

@sttts does this sound ok?

*/

// +k8s:deepcopy-gen=package
// +k8s:conversion-gen=k8s.io/kubernetes/pkg/controller/daemon/config

This comment has been minimized.

Copy link
@luxas

luxas Jan 14, 2019

Member

shouldn't we specify here where the external types are so that the conversions are correctly generated?
right now this doesn't generate any conversions...

This comment has been minimized.

Copy link
@stewart-yu

stewart-yu Jan 16, 2019

Author Contributor

fixed

@@ -0,0 +1,38 @@
/*
Copyright 2018 The Kubernetes Authors.

This comment has been minimized.

Copy link
@luxas

luxas Jan 14, 2019

Member

2019 now ;)

This comment has been minimized.

Copy link
@stewart-yu

stewart-yu Jan 15, 2019

Author Contributor

sorry, forgot it
will fix all comments tonight

"k8s.io/apimachinery/pkg/runtime"
)

var (

This comment has been minimized.

Copy link
@luxas

luxas Jan 14, 2019

Member

I'm wondering if it would make sense to register the addKnownTypes and addDefaultingFuncs funcs to the schemebuilder by default so that every scheme & package becomes "independent" and can be added to an arbitrary scheme without any new things. The recommendeddefaults pattern would still apply but it'd also be called by a SetDefaults func...

This comment has been minimized.

Copy link
@stewart-yu

stewart-yu Jan 15, 2019

Author Contributor

If need, will add it later
The key point is that the --config not implement in KCM. we can add at same time

@luxas luxas added this to the v1.14 milestone Jan 14, 2019

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

@nikopen

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

/retest

@xmudrii

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

A quick reminder that code freeze is approaching this Friday ❄️ The PR was already approved and lgtm-ed, but due to required rebase it lost the lgtm label, so pinging for this to be checked 🙂

@liggitt liggitt removed their assignment Mar 7, 2019

@spiffxp

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/hold
v1.14 release lead here, this is a size/XXL PR that is attempting to land right before Code Freeze... are you really sure this is a good idea?

@cheftako
Copy link
Member

left a comment

/lgtm

@spiffxp

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

/milestone clear
v1.14 release lead here, given the state of our CI signal, I'm removing this from the milestone. The size of this PR makes me wary at this late stage in the game.

Please discuss in #sig-release in slack if it's urgent this land as part of v1.14. Otherwise it will land once we hit code thaw.

@k8s-ci-robot k8s-ci-robot removed this from the v1.14 milestone Mar 13, 2019

@mtaufen mtaufen referenced this pull request Mar 16, 2019

Open

Tracking issue for WG Component Standard Work #75426

2 of 17 tasks complete
@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

i'm not sure whether can we remove the /hold tag
/cc @sttts @deads2k @luxas @thockin

@k8s-ci-robot k8s-ci-robot requested review from deads2k, luxas, sttts and thockin Mar 21, 2019

@nikopen

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

1.14 got branched so this can be merged as approved above

@nikopen

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 21, 2019

/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 or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented Mar 21, 2019

/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 or /hold comment for consistent failures.

@stewart-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot k8s-ci-robot merged commit 4499275 into kubernetes:master Mar 22, 2019

17 checks passed

cla/linuxfoundation stewart-yu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

component-base automation moved this from In progress to Done Mar 22, 2019

@stewart-yu stewart-yu deleted the stewart-yu:stewart-component-base branch Mar 22, 2019

@@ -62,6 +65,44 @@ func SetDefaults_KubeControllerManagerConfiguration(obj *kubectrlmgrconfigv1alph

// Use the default RecommendedDefaultGenericControllerManagerConfiguration options
RecommendedDefaultGenericControllerManagerConfiguration(&obj.Generic)
// Use the default RecommendedDefaultHPAControllerConfiguration options
attachdetachconfigv1alpha1.RecommendedDefaultAttachDetachControllerConfiguration(&obj.AttachDetachController)

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 2, 2019

Member

these controller-specific defaults should just be normal defaulting functions so we don't have to remember to call them in every place they are used. opened #76027 to track changing these to normal defaulting functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.