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.

Sign up for GitHub

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

Jump to bottom

Kubeadm add v1beta1 config #69289

Merged
merged 4 commits into from
Oct 4, 2018
Merged

Kubeadm add v1beta1 config #69289

merged 4 commits into from
Oct 4, 2018

Conversation

fabriziopandini
Copy link
Member

@fabriziopandini fabriziopandini commented Oct 1, 2018 •
edited
Loading

What this PR does / why we need it:
This PR adds v1beta1 version of kubeadm config API.

Which issue(s) this PR fixes:
Ref: kubernetes/kubeadm#911

Special notes for your reviewer:
Sorry about the size of this PR 😧

Depends on:

Please consider latest 4 commits:

  • Add a duplicated v1beta1 API
  • Automated bump from v1alpha3 references to v1beta1
  • fix tests
  • (autogenerated)

Release note:

kubeadm: Add a `v1beta1` API.

@kubernetes/sig-cluster-lifecycle-pr-reviews
/sig cluster-lifecycle
/kind api-change
/area kubeadm

/cc @timothysc @luxas @rosti

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 1, 2018
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Oct 1, 2018
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 1, 2018
rosti
rosti reviewed Oct 1, 2018
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thank you @fabriziopandini ! At a quick first pass this looks OK. I'll do another, more thorough review tomorrow.

cmd/kubeadm/app/apis/kubeadm/fuzzer/fuzzer_test.go
// v1alpha2 -> v1alpha3 migration. As the ComponentConfigs were embedded in the structs
// earlier now have moved out it's not possible to do a lossless roundtrip "the normal way"
// When we support v1alpha3 and higher only, we can reenable this

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to be moved to #69058

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, but I missed that train, so I'm fixing it here

neolit123
neolit123 reviewed Oct 1, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

cmd/kubeadm/app/util/config/cluster_test.go Outdated
},
},
{
name: "invalid v1beta1 - No CLusterStatus in kubeadm-config ConfigMap",
Copy link
Member

Choose a reason for hiding this comment

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

super minor nit:
CLusterStatus -> ClusterStatus
(appears more than once in this file)

cmd/kubeadm/app/util/config/cluster_test.go Outdated
{
name: "invalid v1beta1 - No CLusterStatus in kubeadm-config ConfigMap",
configMap: fakeConfigMap{
name: kubeadmconstants.InitConfigurationConfigMap, // ClusterConfiguration from kubeadm-config.
Copy link
Member

Choose a reason for hiding this comment

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

the typecheck CI complains about this const missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The merger of #69275 renamed InitConfigurationConfigMap to KubeadmConfigConfigMap, thus this test file won't compile.

rosti
rosti reviewed Oct 2, 2018
View reviewed changes
cmd/kubeadm/app/util/config/cluster_test.go Outdated
{
name: "invalid v1beta1 - No CLusterStatus in kubeadm-config ConfigMap",
configMap: fakeConfigMap{
name: kubeadmconstants.InitConfigurationConfigMap, // ClusterConfiguration from kubeadm-config.
Copy link
Contributor

Choose a reason for hiding this comment

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

The merger of #69275 renamed InitConfigurationConfigMap to KubeadmConfigConfigMap, thus this test file won't compile.

cmd/kubeadm/app/util/config/testdata/defaulting/master/defaulted.yaml Outdated
@@ -115,7 +115,6 @@ evictionHard:
imagefs.available: 15%
memory.available: 100Mi
nodefs.available: 10%
nodefs.inodesFree: 5%
Copy link
Contributor

Choose a reason for hiding this comment

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

Test failure here:

--- FAIL: TestConfigFileAndDefaultsToInternalConfig (0.94s)
    --- FAIL: TestConfigFileAndDefaultsToInternalConfig/incompleteYAMLToDefaultedv1beta1 (0.00s)
        masterconfig_test.go:123: the expected and actual output differs.
                        in: testdata/defaulting/master/incomplete.yaml
                        out: testdata/defaulting/master/defaulted.yaml
                        groupversion: kubeadm.k8s.io/v1beta1
                        diff: 
                --- expected
                +++ actual
                @@ -115,6 +115,7 @@
                   imagefs.available: 15%!
                (MISSING)   memory.available: 100Mi
                   nodefs.available: 10%!
                (MISSING)+  nodefs.inodesFree: 5%!
                (MISSING) evictionPressureTransitionPeriod: 5m0s
                 failSwapOn: true
                 fileCheckFrequency: 20s
    
FAIL
FAIL    k8s.io/kubernetes/cmd/kubeadm/app/util/config   1.007s

There is probably some change in the KubeletConfig here. So whenever a change is made there we need to update this. We may want to just check if we have the right kind here. Yet, this is a job for another PR.

timothysc
timothysc reviewed Oct 2, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

Minor comments otherwise LGTM
/approve

cmd/kubeadm/app/apis/kubeadm/v1beta1/bootstraptokenstring.go
// for both validation of the practically of the API server from a joining node's point
// of view and as an authentication method for the node in the bootstrap phase of
// "kubeadm join". This token is and should be short-lived
type BootstrapTokenString struct {
Copy link
Member

Choose a reason for hiding this comment

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

So with bootstrap tokens finding a landing home this cycle do we still need this? We've been carrying this for a while now.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't mind I will iterate on this in a separated PR trying to get rid of the code for bootstrap token management that now is duplicated for each API version

cmd/kubeadm/app/apis/kubeadm/v1beta1/doc.go
limitations under the License.
*/

// +k8s:defaulter-gen=TypeMeta
Copy link
Member

Choose a reason for hiding this comment

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

You may need to pull in @neolit123 's recent changes here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

cmd/kubeadm/app/cmd/config.go
@@ -228,6 +229,7 @@ func NewCmdConfigMigrate(out io.Writer) *cobra.Command {
locally in the CLI tool without ever touching anything in the cluster.
In this version of kubeadm, the following API versions are supported:
- %s
- %s
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

This gives some info about api version supported by kubeadm command migrate...

cmd/kubeadm/app/cmd/config_test.go
@@ -231,32 +232,28 @@ func TestImagesPull(t *testing.T) {
}

func TestMigrate(t *testing.T) {
/*
TODO: refactor this to test v1alpha3 --> v1beta1 after introducing v1beta1
cfg := []byte(dedent.Dedent(`
Copy link
Member

Choose a reason for hiding this comment

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

Fine for now, but next time we can break this out as a follow-up PR.

@fabriziopandini
Copy link
Member Author

/test pull-kubernetes-integration

timothysc
timothysc approved these changes Oct 4, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2018
@timothysc timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fabriziopandini, timothysc

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

@k8s-ci-robot k8s-ci-robot merged commit 5e3dc7d into kubernetes:master Oct 4, 2018
@fabriziopandini fabriziopandini deleted the kubeadm-add-v1beta1 branch October 8, 2018 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosti rosti rosti left review comments

@neolit123 neolit123 neolit123 left review comments

@timothysc timothysc timothysc approved these changes

@luxas luxas Awaiting requested review from luxas

Assignees

@timothysc timothysc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fabriziopandini @k8s-ci-robot @timothysc @neolit123 @rosti