Add criSocket to kubeadm NodeConfiguration manifest #59292

JordanFaust · 2018-02-02T23:16:47Z

What this PR does / why we need it:
Adds a criSocket field to the NodeConfiguration manifest used by kubeadm. This field configures the cri socket that kubeadm uses during preflight checks.

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#679

Special notes for your reviewer:

This is a follow up PR, as requested, to #59057. The NodeConfiguration manifest now has a criSocket field that can be used when using the config manifest to join a node to the cluster.

Release note:
/area kubeadm
/assign @luxas
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

kubeadm: add criSocket field to NodeConfiguration manifiest

k8s-ci-robot · 2018-02-02T23:16:56Z

@JordanFaust: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

What this PR does / why we need it:
Adds a criSocket field to the NodeConfiguration manifest used by kubeadm. This field configures the cri socket that kubeadm uses during preflight checks.

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#679

Special notes for your reviewer:

This is a follow up PR, as requested, to #59057. The NodeConfiguration manifest now has a criSocket field that can be used when using the config manifest to join a node to the cluster.

Release note:
/area kubeadm
/assign @luxas
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
kubeadm: add criSocket field to NodeConfiguration manifiest

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.

fabriziopandini

Thanks for the PR!

/lgtm

jamiehannaford · 2018-02-04T11:24:49Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/defaults.go

@@ -147,6 +147,9 @@ func SetDefaults_NodeConfiguration(obj *NodeConfiguration) {
 	if len(obj.DiscoveryToken) == 0 && len(obj.DiscoveryFile) == 0 {
 		obj.DiscoveryToken = obj.Token
 	}
+	if obj.CRISocket == "" {
+		obj.CRISocket = DefaultCRISocket


you need to remove this since DefaultCRISocket is no longer defined

I remember it was defined when I review the code last time. obj.CRISocket should have a default value I think.

I missed adding this since I did the initial work within the same branch as my first PR. I added the DefaultCRISocket to resolve this issue.

timothysc · 2018-02-07T03:22:19Z

cmd/kubeadm/app/cmd/join.go

@@ -169,10 +168,14 @@ func AddJoinConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiext.NodeConfigurat
 		featureGatesString, "feature-gates", *featureGatesString,
 		"A set of key=value pairs that describe feature gates for various features. "+
 			"Options are:\n"+strings.Join(features.KnownFeatures(&features.InitFeatureGates), "\n"))
+	flagSet.StringVar(


Does this need to be a top level command line flag? Shouldn't this be an exception case?

timothysc

/approve per @fabianofranz comments but please squash your changes before lgtm

JordanFaust · 2018-02-09T14:49:58Z

/retest

k8s-ci-robot · 2018-02-09T14:50:14Z

@JordanFaust: you can't request testing unless you are a kubernetes member.

In response to this:

/retest

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.

xiangpengzhao · 2018-02-11T06:23:50Z

/ok-to-test
/retest

dixudx

One small nit. Others lgtm.

dixudx · 2018-02-13T05:56:09Z

cmd/kubeadm/app/apis/kubeadm/types.go

@@ -206,6 +206,8 @@ type NodeConfiguration struct {
 	TLSBootstrapToken string
 	// Token is used for both discovery and TLS bootstrapping.
 	Token string
+	// CRI socket to be used to retrieve container runtime info


Change to CRISocket is used to retrieve container runtime info? WDYT?

Same as below v1alpha1/types.go

Sounds good to me. I will update the documentation in the other PR as well.

JordanFaust · 2018-02-13T21:13:42Z

@dixudx I made the change in both PRs and squashed all of the commits. Let me know if there is anything else needed for a LGTM.

jamiehannaford · 2018-02-21T09:47:12Z

/lgtm

k8s-ci-robot · 2018-02-21T09:47:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

k8s-github-robot · 2018-02-21T09:47:57Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2018-02-21T09:48:11Z

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

k8s-ci-robot · 2018-02-21T10:14:59Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`62dddb7`	link	`/test pull-kubernetes-e2e-kops-aws`

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-github-robot · 2018-02-21T11:25:30Z

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

k8s-ci-robot assigned luxas Feb 2, 2018

k8s-ci-robot requested review from fabriziopandini and mattmoyer February 2, 2018 23:16

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 2, 2018

k8s-ci-robot assigned fabriziopandini Feb 4, 2018

fabriziopandini approved these changes Feb 4, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2018

jamiehannaford reviewed Feb 4, 2018

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2018

timothysc reviewed Feb 7, 2018

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2018

JordanFaust force-pushed the add-cri-socket-to-node-configuration branch from c5e2f41 to b70262f Compare February 7, 2018 15:56

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2018

timothysc removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 8, 2018

timothysc approved these changes Feb 8, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2018

JordanFaust force-pushed the add-cri-socket-to-node-configuration branch from 448a780 to db3d5e6 Compare February 8, 2018 15:43

dixudx reviewed Feb 13, 2018

View reviewed changes

Add criSocket to kubeadm NodeConfiguration manifest

62dddb7

JordanFaust force-pushed the add-cri-socket-to-node-configuration branch from 2da0678 to 62dddb7 Compare February 13, 2018 15:16

JordanFaust mentioned this pull request Feb 13, 2018

Add criSocket to kubeadm MasterConfiguration manifest #59057

Merged

k8s-ci-robot assigned jamiehannaford Feb 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2018

k8s-github-robot merged commit a90e43c into kubernetes:master Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add criSocket to kubeadm NodeConfiguration manifest #59292

Add criSocket to kubeadm NodeConfiguration manifest #59292

JordanFaust commented Feb 2, 2018

k8s-ci-robot commented Feb 2, 2018

fabriziopandini left a comment

jamiehannaford Feb 4, 2018

xiangpengzhao Feb 5, 2018

JordanFaust Feb 5, 2018

timothysc Feb 7, 2018

timothysc left a comment

JordanFaust commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

xiangpengzhao commented Feb 11, 2018

dixudx left a comment

dixudx Feb 13, 2018

JordanFaust Feb 13, 2018

JordanFaust commented Feb 13, 2018

jamiehannaford commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 •

edited

Loading

k8s-github-robot commented Feb 21, 2018

Add criSocket to kubeadm NodeConfiguration manifest #59292

Add criSocket to kubeadm NodeConfiguration manifest #59292

Conversation

JordanFaust commented Feb 2, 2018

k8s-ci-robot commented Feb 2, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

jamiehannaford Feb 4, 2018

Choose a reason for hiding this comment

xiangpengzhao Feb 5, 2018

Choose a reason for hiding this comment

JordanFaust Feb 5, 2018

Choose a reason for hiding this comment

timothysc Feb 7, 2018

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

JordanFaust commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

xiangpengzhao commented Feb 11, 2018

dixudx left a comment

Choose a reason for hiding this comment

dixudx Feb 13, 2018

Choose a reason for hiding this comment

JordanFaust Feb 13, 2018

Choose a reason for hiding this comment

JordanFaust commented Feb 13, 2018

jamiehannaford commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 • edited Loading

k8s-github-robot commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018 •

edited

Loading