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

Add a 'kubeadm config print-default' command #63969

Merged
merged 3 commits into from May 18, 2018

Conversation

@luxas
Copy link
Member

luxas commented May 17, 2018

What this PR does / why we need it:
Improves the UX around creating config files.

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 kubernetes/kubeadm#829

Special notes for your reviewer:

Release note:

kubeadm: A `kubeadm config print-default` command has now been added that you can use as a starting point when writing your own kubeadm configuration files

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

@timothysc
Copy link
Member

timothysc left a comment

minor nits and a question.

apiObject := masterConfig
cmd := &cobra.Command{
Use: "print-default",
Short: "Print the default values for a kubeadm configuration object.",

This comment has been minimized.

@timothysc

timothysc May 17, 2018

Member

Prints out the default value for the kubeadm configuration object.

Use: "print-default",
Short: "Print the default values for a kubeadm configuration object.",
Long: fmt.Sprintf(dedent.Dedent(`
This command prints by default the default MasterConfiguration object that is used for 'kubeadm init' and 'kubeadm upgrade'.

This comment has been minimized.

@timothysc

timothysc May 17, 2018

Member

I thought we were going to add in the node configuration object at 1st then in the future split it out from our conversation yesterday.

This comment has been minimized.

@timothysc

timothysc May 17, 2018

Member

"This command prints the default MasterConfiguration object used by 'kubeadm init' and 'kubeadm upgrade'.

return cmd
}

func getDefaultAPIObjectBytes(apiObject string) ([]byte, error) {

This comment has been minimized.

@timothysc

timothysc May 17, 2018

Member

Wouldn't something like this be more generally useful under config as an exported f(n)?

This comment has been minimized.

@timothysc

timothysc May 17, 2018

Member

Not a blocker, b/c we can shuffle later.

cmd.AddCommand(NewCmdConfigUpload(out, &kubeConfigFile))
cmd.AddCommand(NewCmdConfigView(out, &kubeConfigFile))
cmd.AddCommand(NewCmdConfigImages(out))
return cmd
}

// NewCmdConfigṔrintDefault returns cobra.Command for "kubeadm config print-default" command
func NewCmdConfigrintDefault(out io.Writer) *cobra.Command {

This comment has been minimized.

@mattkelly

mattkelly May 17, 2018

Contributor

You used here in Print but I think you wanted a P

@@ -129,24 +126,24 @@ func NewCmdJoin(out io.Writer) *cobra.Command {
}

// NewValidJoin validates the command line that are passed to the cobra command
func NewValidJoin(cfg *kubeadmapiv1alpha2.NodeConfiguration, args []string, skipPreFlight bool, cfgPath, featureGatesString string, ignorePreflightErrors []string) (*Join, error) {
func NewValidJoin(cmd *cobra.Command, cfg *kubeadmapiv1alpha2.NodeConfiguration, args []string, skipPreFlight bool, cfgPath, featureGatesString string, ignorePreflightErrors []string) (*Join, error) {

This comment has been minimized.

@neolit123

neolit123 May 17, 2018

Member

join_test.go needs a little change:

cmd := NewCmdJoin(&out)
join, err := NewValidJoin(cmd, cfg, tc.args, tc.skipPreFlight, tc.cfgPath, tc.featureGatesString, tc.ignorePreflightErrors)
//                         ^^ passing cmd as argument now
return err
}
return validation.ValidateNodeConfiguration(j.cfg).ToAggregate()
return &Join{cfg: internalcfg}, nil

This comment has been minimized.

@neolit123

neolit123 May 17, 2018

Member

welp, there is also this issue in join_test.go:

invalid operation: join (variable of type *Join) has no field or method Validate

possibly this and the related tests has to be removed:
https://github.com/luxas/kubernetes/blob/469532c49917bbecaf9d9b29ff5da5a4aa57209c/cmd/kubeadm/app/cmd/join_test.go#L177-L181

@chuckha
Copy link
Member

chuckha left a comment

This looks good to me, just one question:

This is printing the internal version of the config, shouldn't it be printing the external version of the config instead? Users should be defining the api extension version of the config, not the internal one.

@luxas luxas force-pushed the luxas:kubeadm_config_print_defaults branch from e8ebbf8 to c4f75f3 May 17, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 17, 2018

This is printing the internal version of the config, shouldn't it be printing the external version of the config instead? Users should be defining the api extension version of the config, not the internal one.

@chuckha This is printing the external version of the config 😎. The line doing the magic is this one:

kubeadmutil.MarshalToYamlForCodecs(internalcfg, kubeadmapiv1alpha2.SchemeGroupVersion, kubeadmscheme.Codecs)

Telling the API machinery framework to serialize it for the external v1alpha2 GroupVersion.
Not obvious I know, but kinda cool 😉

luxas added some commits May 17, 2018

@luxas luxas force-pushed the luxas:kubeadm_config_print_defaults branch from c4f75f3 to 33fc0e8 May 17, 2018

@chuckha

This comment has been minimized.

Copy link
Member

chuckha commented May 17, 2018

ahhh gotcha, very nice!

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 17, 2018

/retest

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 17, 2018

/test pull-kubernetes-verify
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-kubemark-e2e-gce-big

@timothysc
Copy link
Member

timothysc left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 17, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 17, 2018

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

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented May 17, 2018

@luxas
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63969/pull-kubernetes-verify/91406/

Chuck was getting this due to not generating the docs locally and including all files(?) i think it's the same PY stacktrace.
/cc @chuckha

@k8s-ci-robot k8s-ci-robot requested a review from chuckha May 17, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 17, 2018

New changes are detected. LGTM label has been removed.

@luxas luxas force-pushed the luxas:kubeadm_config_print_defaults branch from 33fc0e8 to 00390b6 May 17, 2018

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 17, 2018

Yup, that's right @neolit123, thanks! Ran ./hack/update-generated-docs.sh now.
Readding lgtm/approved.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 18, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 18, 2018

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

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 18, 2018

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

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 18, 2018

/retest

1 similar comment
@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented May 18, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 18, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 18, 2018

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

@k8s-github-robot k8s-github-robot merged commit d159857 into kubernetes:master May 18, 2018

11 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation luxas authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 18, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu 00390b6 link /test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

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.