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

kubeadm: Split out ClusterConfiguration from InitConfiguration #66219

Closed
wants to merge 2 commits into from

Conversation

@luxas
Copy link
Member

commented Jul 15, 2018

What this PR does / why we need it:

Splits MasterConfiguration to InitConfiguration and ClusterConfiguration as outlined in the kubeadm Config KEP. InitConfiguration holds init-only information, and ClusterConfiguration holds cluster-wide information. In the internal representation InitConfiguration wraps ClusterConfiguration as a field, but in serialized format they're different YAML documents.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/kubeadm#911
Depends on:

Special notes for your reviewer:

Release note:

kubeadm: InitConfiguration now consists of two structs: InitConfiguration and ClusterConfiguration

@kubernetes/sig-cluster-lifecycle-pr-reviews

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@luxas luxas force-pushed the luxas:kubeadm_clusterconfig branch from 2de9892 to 20bf485 Jul 17, 2018

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

@luxas can you unmark WIP and tests are failing atm. Also needs a much better description ... TODO ;-)

type InitConfiguration struct {
metav1.TypeMeta `json:",inline"`

ClusterConfiguration `json:"-"`

This comment has been minimized.

Copy link
@timothysc

timothysc Jul 17, 2018

Member

Doesn't this overload TypeMeta on struct embedding?

This comment has been minimized.

Copy link
@luxas

luxas Jul 25, 2018

Author Member

This is never marshalled, it's a different YAML document hence no TypeMeta overloading will happen in practice

decodedObjs := map[componentconfigs.RegistrationKind]runtime.Object{}
masterConfigFound := false
initConfigFound, clusterConfigFound := false, false

This comment has been minimized.

Copy link
@timothysc

timothysc Jul 17, 2018

Member

This function seems pretty brittle.

This comment has been minimized.

Copy link
@luxas

luxas Jul 25, 2018

Author Member

Yeah, I hope I made it look more robust now

continue
}

fmt.Printf("[config] WARNING: Ignored YAML document with GroupVersionKind %v\n", gvk)
}
// If InitConfiguration was found but not ClusterConfiguration, do nothing
if initConfigFound && !clusterConfigFound { }

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Jul 18, 2018

Contributor

I think this can be removed

@rosti
Copy link
Member

left a comment

Great work so far! Yet, a lot of the code still expects InitConfiguration, but really needs ClusterConfiguration.
We should probably address all such cases in this PR.
To me InitConfiguration should be marginalized to just some parts of the code needed for kubeadm init. Everything else should work with ClusterConfiguration.

@chuckha
Copy link
Member

left a comment

This is looking good. I think we could make the BytesToInternalConfig function a bit nicer, but I'm fine with that as a cleanup task later so long as we're tracking it.

// If InitConfiguration was found but not ClusterConfiguration, do nothing
if initConfigFound && !clusterConfigFound { }
// If ClusterConfiguration was found but not ClusterConfiguration, just set
if (!initConfigFound && clusterConfigFound) || (initConfigFound && clusterConfigFound) {

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 19, 2018

Member

this logic is easier read as

if clusterConfigFound {
...
}

This comment has been minimized.

Copy link
@luxas

luxas Jul 25, 2018

Author Member

Yeah, that was WIP, now fixed

continue
}

fmt.Printf("[config] WARNING: Ignored YAML document with GroupVersionKind %v\n", gvk)
}
// If InitConfiguration was found but not ClusterConfiguration, do nothing
if initConfigFound && !clusterConfigFound { }
// If ClusterConfiguration was found but not ClusterConfiguration, just set

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 19, 2018

Member

This comment doesn't make sense

This comment has been minimized.

Copy link
@luxas

luxas Jul 25, 2018

Author Member

fixed

if !masterConfigFound {
return nil, fmt.Errorf("no InitConfiguration kind was found in the YAML file")
if !initConfigFound && !clusterConfigFound {
return nil, fmt.Errorf("no InitConfiguration or ClusterConfiguration kind was found in the YAML file")

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 19, 2018

Member

English here could be more exact with "Neither InitConfiguration nor ClusterConfiguration ..."

@@ -45,7 +45,7 @@ import (
func SetInitDynamicDefaults(cfg *kubeadmapi.InitConfiguration) error {

This comment has been minimized.

Copy link
@rosti

rosti Jul 20, 2018

Member

Should probably move most of the stuff from here to SetClusterDynamicDefaults and have SetInitDynamicDefaults invoke that. This way we won't end up generating bootstrap tokens for cluster configs, that were standalone.

This comment has been minimized.

Copy link
@luxas

luxas Jul 25, 2018

Author Member

Fixed now I think

@luxas luxas force-pushed the luxas:kubeadm_clusterconfig branch from 20bf485 to 3af1f60 Jul 25, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

Yet, a lot of the code still expects InitConfiguration, but really needs ClusterConfiguration.

Exactly.

We should probably address all such cases in this PR.

No, this PR will be as scoped as possible. There are a lot of changes anyway. This can be incrementally improved later in followup PRs.

@rosti

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

@luxas understood. I can follow up with some PRs that replace InitConfiguration with ClusterConfiguration once this gets merged.

@@ -42,15 +50,20 @@ type InitConfiguration struct {

// NodeRegistration holds fields that relate to registering the new master node to the cluster
NodeRegistration NodeRegistrationOptions
}

func (ic InitConfiguration) Fuzz(c fuzz.Continue) {}

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 5, 2018

Contributor

It could be useful to add a comment explaining why this line is necessary

This comment has been minimized.

Copy link
@luxas

luxas Aug 10, 2018

Author Member

removed

return nil
}
/*

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 5, 2018

Contributor

cleanup?

This comment has been minimized.

Copy link
@luxas

luxas Aug 10, 2018

Author Member

done

return getDefaultInitConfigBytes(constants.InitConfigurationKind)

case constants.ClusterConfigurationKind:
return getDefaultInitConfigBytes(constants.ClusterConfigurationKind)

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 5, 2018

Contributor

It sounds a little bit strange to me calling getDefaultInitConfigBytes here.
What about implementing a separated getDefaultClusterConfigBytes?

@@ -157,19 +160,29 @@ func getAllAPIObjectNames() []string {

func getDefaultedInitConfig() (*kubeadmapi.InitConfiguration, error) {
return configutil.ConfigFileAndDefaultsToInternalConfig("", &kubeadmapiv1alpha3.InitConfiguration{
API: kubeadmapiv1alpha3.API{AdvertiseAddress: "1.2.3.4"},
ClusterConfiguration: kubeadmapiv1alpha3.ClusterConfiguration{

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 5, 2018

Contributor

Move to getDefaultedClusterConfig?

@@ -50,6 +51,8 @@ func loadConfigurationBytes(client clientset.Interface, w io.Writer, logPrefix,

fmt.Fprintf(w, "[%s] Reading configuration from the cluster...\n", logPrefix)

// TODO: This code should support reading the MasterConfiguration key as well for backwards-compat
// Also, the key really should be ClusterConfiguration...

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Aug 5, 2018

Contributor

I'm fine with naming the ConfigMap kubeadm-config, but renaming the constant from InitConfigurationConfigMap to ClusterConfigurationConfigMap.
+1 for renaming the ConfigMapKey

@timothysc

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

@luxas do you think you will be able to finish this or should we try to help get this through?

luxas added some commits Aug 10, 2018

@luxas luxas force-pushed the luxas:kubeadm_clusterconfig branch from 3af1f60 to 5bdbf04 Aug 10, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-bazel-test 5bdbf04 link /test pull-kubernetes-bazel-test
pull-kubernetes-verify 5bdbf04 link /test pull-kubernetes-verify

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

@luxas: PR needs rebase.

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.

@luxas luxas changed the title [WIP] kubeadm: Split out ClusterConfiguration from InitConfiguration kubeadm: Split out ClusterConfiguration from InitConfiguration Aug 12, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

@timothysc if someone else could carry these patches it'd be great 👍. I'm probably not gonna have the capacity to finish this work wrt keeping it rebased etc. myself in the coming weeks :/

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@rosti do you want to take these?
needs a new PR.

@rosti

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

I can give my best. For me, the biggest priority here is to get this PR merged as is (with a minor cleanup of commented sections). It's already grown very big and adding more stuff to it will make things unreviewable.

Once the PR is merged, we can follow up with many smaller and more focused PRs that replace InitConfiguration with ClusterConfiguration.

@luxas @timothysc WDYT?

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

you can also bring this in the meeting as this is p0.

@rosti

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

It must be brought up to a meeting and this needs to be done this week ideally. However, it would be best to also have @luxas on the call (even for a little) in order to take the best decision there.

@dixudx

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

@rosti Great! Thanks for your help.

It must be brought up to a meeting and this needs to be done this week ideally. However, it would be best to also have @luxas on the call (even for a little) in order to take the best decision there.

We're almost done. Just need some more efforts on the renaming, rebasing and etc.

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

/hold
this PR was reissued by @rosti --> #67441

k8s-github-robot pushed a commit that referenced this pull request Aug 22, 2018

Kubernetes Submit Queue
Merge pull request #67441 from rosti/kubeadm_clusterconfig
Automatic merge from submit-queue. 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>.

[reissue] kubeadm: Split out ClusterConfiguration from InitConfiguration

As @luxas is not able to take care of #66219, I am reissuing the same change here. There are a few minor things added by me:

- The original PR is rebased on latest master.
- Some broken tests were fixed.
- Some TODOs were added.
- Run update-bazel and update-gofmt

Below is the text of the original PR by Lucas.

-----

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

Splits MasterConfiguration to InitConfiguration and ClusterConfiguration as outlined in the kubeadm Config KEP. InitConfiguration holds init-only information, and ClusterConfiguration holds cluster-wide information. In the internal representation InitConfiguration wraps ClusterConfiguration as a field, but in serialized format they're different YAML documents.

**Which issue(s) this PR fixes** (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/kubeadm#911
Depends on:

- [X] #65776
- [X] #65628
- [X] #65629
- [X] #65631
- [X] #65940
- [X] #65787
- [X] #65945
- [X] #65951
- [X] #65952

**Special notes for your reviewer:**

**Release note**:
```release-note
kubeadm: InitConfiguration now consists of two structs: InitConfiguration and ClusterConfiguration
```

@kubernetes/sig-cluster-lifecycle-pr-reviews
@luxas

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

#67441 has been merged, thanks @rosti!!

@luxas luxas closed this Aug 23, 2018

@neolit123 neolit123 referenced this pull request Sep 25, 2018

Merged

e2e refactor #68483

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.