Skip to content

Kubeadm add v1beta1 config #69289

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
Merged
k8s-ci-robot merged 4 commits into from
Oct 4, 2018
Merged

Kubeadm add v1beta1 config #69289

k8s-ci-robot 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 requested a review from luxas October 1, 2018 15:43
@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 Outdated
Copy link
Contributor

@rosti rosti Oct 1, 2018

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

@fabriziopandini fabriziopandini Oct 2, 2018

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
Copy link
Member

@neolit123 neolit123 Oct 1, 2018

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
Copy link
Member

@neolit123 neolit123 Oct 1, 2018

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

@rosti rosti Oct 2, 2018

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
Copy link
Contributor

@rosti rosti Oct 2, 2018

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
Copy link
Contributor

@rosti rosti Oct 2, 2018

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
Contributor

@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 Outdated
Copy link
Contributor

@timothysc timothysc Oct 2, 2018

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

@fabriziopandini fabriziopandini Oct 4, 2018

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 Outdated
Copy link
Contributor

@timothysc timothysc Oct 2, 2018

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

@fabriziopandini fabriziopandini Oct 4, 2018

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 Outdated
Copy link
Contributor

@timothysc timothysc Oct 2, 2018

Choose a reason for hiding this comment

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

?

Copy link
Member Author

@fabriziopandini fabriziopandini Oct 4, 2018

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 Outdated
Copy link
Contributor

@timothysc timothysc Oct 2, 2018

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

fabriziopandini commented Oct 4, 2018

/test pull-kubernetes-integration

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

@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

k8s-ci-robot commented Oct 4, 2018

[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

Details 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

@luxas luxas Awaiting requested review from luxas

3 more reviewers

@neolit123 neolit123 neolit123 left review comments

@rosti rosti rosti left review comments

@timothysc timothysc timothysc approved these changes

Reviewers whose approvals may not affect merge requirements

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