Validation: treat as error if insufficient nodes #4703

justinsb · 2018-03-18T06:44:38Z

We switch to use the rolling update logic, which gives us nodes by
InstanceGroup.

justinsb · 2018-03-18T06:46:50Z

cmd/kops/validate_cluster.go

 	}

-	// Do not use if we are running gossip


Moved into validation

what was the logic behind moving this? This seems to be an improvement of the code, not fixing the issue.

Wondering if we should put this in another PR. This needs to be tested on both Mac and Linux, since the OS behaves differently with Mac and caching dns.

We're moving the code, not changing it AFAIK.

The reason for moving it is that we want this to be part of validation, I believe, including on a rolling-update.

chrislovecnm

Overall I like it. Two big concerns

Lots of changes that are not related to the bug. I would rather have at least two PRs. One that fixes the bug or bugs, another that does improvements
You seem to have changed the schema. I love the new format, but it is a breaking change for the users. Should we move the schema into the api?

chrislovecnm · 2018-03-19T14:11:01Z

cmd/kops/validate_cluster.go

 	}

-	// Do not use if we are running gossip


what was the logic behind moving this? This seems to be an improvement of the code, not fixing the issue.

Wondering if we should put this in another PR. This needs to be tested on both Mac and Linux, since the OS behaves differently with Mac and caching dns.

chrislovecnm · 2018-03-19T14:13:02Z

cmd/kops/validate_cluster.go

 		fmt.Fprint(out, "\nValidation Failed\n")
-		fmt.Fprintf(out, "Ready Master(s) %d out of %d.\n", len(validationCluster.MastersReadyArray), validationCluster.MastersCount)


Where did you move this output to? Again seems to be code improvements / UX changes not dealing with the bug.

Should we have these changes in another PR?

It's in the failure list. The problem is that our model - particularly for nodes - is richer than a simple count, so these messages would do things like "Ready nodes 10 out of 6" etc.

Frankly as an operator that part of the ux was incredibly useful. I would prefer to not remove it

Yes - I would like to see it in the instance group grid above. It just isn't well-defined here, sadly.

chrislovecnm · 2018-03-19T14:15:45Z

pkg/validation/node_conditions.go

+// It is considered ready if:
+// 1) its Ready condition is set to true
+// 2) doesn't have NetworkUnavailable condition set to true
+func isNodeReady(node *v1.Node) bool {


So a lot of this code initially was taken from upstream from the e2e testing. Did you do the same, or did you do a general rewrite?

The code that's left is the functions that were used, and then I collapsed the functions a little bit to be more readable. Logic should be unchanged.

chrislovecnm · 2018-03-19T14:18:36Z

pkg/validation/node_conditions.go

+func isNodeReady(node *v1.Node) bool {
+	nodeReadyCondition := findNodeCondition(node, v1.NodeReady)
+	if nodeReadyCondition == nil {
+		glog.Warningf("v1.NodeReady condition not set on node %s", node.Name)


Can we do more human friendly message?

How about

Node %q is not ready, no condition is set.

I know what v1.NodeReady is, but I know a lot of non programmers will not

The reason for this one is that we don't expect this to be the case: we expect there always to be a NodeReady condition. So it's not that the node is or isn't ready, it's that our assumptions aren't valid. i.e. this isn't a user-facing message, this is an "uh-oh" message for us.

chrislovecnm · 2018-03-19T14:21:31Z

pkg/validation/node_conditions.go

+			return false
+		}
+	} else {
+		glog.V(4).Infof("v1.NodeNetworkUnavailable condition not set on node %s", node.Name)


Same comment as above about the log message. You also switched from warning to info? I think all of the messages should be info. Thoughts? A warning to me is some is wrong. A new node can be so new that it does not have a node condition, correct?

This one is V(4) and just diagnostics, this isn't unexpected, but isn't really user-facing.

Missed that thanks

chrislovecnm · 2018-03-19T14:23:51Z

pkg/validation/validate_cluster.go

 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/apis/kops/util"
+	"k8s.io/kops/pkg/cloudinstances"
+	"k8s.io/kops/pkg/dns"
+	"k8s.io/kops/upup/pkg/fi/cloudup"
 )

 // ValidationCluster a cluster to validate.
 type ValidationCluster struct {


So this is a breaking change. I know people are using the yaml and json progrmatically. I wanted to have an api for this, but I think we could not decide on it. Options?

Let me ask a question. Does this change the validation output?

The yaml / json output is not currently part of our API. If we want to make it part of our API, I think the structure I'm proposing here is a step towards that - we want an arbitrary list of structured validation failures IMO. I'm not sure if the node list really belongs - it is more informational - we could consider splitting that out.

chrislovecnm · 2018-03-19T14:25:31Z

pkg/validation/validate_cluster.go

-		return validationCluster, fmt.Errorf(validationCluster.ErrorMessage)
-	}
+			// TODO: Use instance group role instead...
+			if n.Role == "master" {


Don’t we have helped metbods for this check?

I think we do if we switch to the instance group, but that would be a bigger behavioural change. The TODO to do so remains though :-)

mikesplain · 2018-03-19T19:51:03Z

@chrislovecnm asked me to give this a test and this seems to work fine for me, upgrading/rolling-over a cluster created via master.

justinsb · 2018-03-20T05:33:41Z

I think this is the last PR blocking 1.9.0-alpha.2 I've split it up into smaller PRs at @chrislovecnm 's request.

chrislovecnm · 2018-03-20T19:35:34Z

/hold

Merging others that are conprised by this PR

We switch to use the rolling update logic, which gives us nodes by InstanceGroup.

justinsb · 2018-03-21T03:31:10Z

OK split out all the low hanging fruit. I think this is last PR blocking 1.9.0-alpha.2 PTAL @mikesplain @chrislovecnm

chrislovecnm · 2018-03-21T06:30:38Z

/hold cancel
/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2018-03-21T06:32:02Z

/lgtm

k8s-ci-robot · 2018-03-21T06:32:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb

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:

~~OWNERS~~ [chrislovecnm,justinsb]

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 18, 2018

This was referenced Mar 18, 2018

Validation: fix collectComponentFailures #4701

Closed

Validation: Warn if node role is unknown #4700

Closed

Clean up timeout parse code #4699

Closed

justinsb commented Mar 18, 2018

View reviewed changes

justinsb force-pushed the int_validation_fix branch from dd1e8a1 to d7f14ce Compare March 18, 2018 06:51

justinsb requested a review from chrislovecnm March 18, 2018 06:51

justinsb assigned chrislovecnm Mar 18, 2018

justinsb force-pushed the int_validation_fix branch 2 times, most recently from 5198296 to 1de1541 Compare March 18, 2018 06:54

chrislovecnm reviewed Mar 19, 2018

View reviewed changes

justinsb added the blocks-next label Mar 20, 2018

justinsb added this to the 1.9.0 milestone Mar 20, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2018

justinsb force-pushed the int_validation_fix branch from 1de1541 to afabe66 Compare March 21, 2018 00:59

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 21, 2018

justinsb force-pushed the int_validation_fix branch 2 times, most recently from d185ae9 to af3314f Compare March 21, 2018 01:02

justinsb removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2018

Validation: treat as error if insufficient nodes

63e5133

We switch to use the rolling update logic, which gives us nodes by InstanceGroup.

justinsb force-pushed the int_validation_fix branch from af3314f to 63e5133 Compare March 21, 2018 03:28

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

k8s-ci-robot merged commit 4d2bedc into kubernetes:master Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation: treat as error if insufficient nodes #4703

Validation: treat as error if insufficient nodes #4703

justinsb commented Mar 18, 2018

justinsb Mar 18, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm left a comment

chrislovecnm Mar 19, 2018

chrislovecnm Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 21, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm Mar 19, 2018

chrislovecnm Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

chrislovecnm Mar 19, 2018

justinsb Mar 19, 2018

mikesplain commented Mar 19, 2018

justinsb commented Mar 20, 2018

chrislovecnm commented Mar 20, 2018

justinsb commented Mar 21, 2018

chrislovecnm commented Mar 21, 2018

chrislovecnm commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018

		fmt.Fprint(out, "\nValidation Failed\n")
		fmt.Fprintf(out, "Ready Master(s) %d out of %d.\n", len(validationCluster.MastersReadyArray), validationCluster.MastersCount)

Validation: treat as error if insufficient nodes #4703

Validation: treat as error if insufficient nodes #4703

Conversation

justinsb commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesplain commented Mar 19, 2018

justinsb commented Mar 20, 2018

chrislovecnm commented Mar 20, 2018

justinsb commented Mar 21, 2018

chrislovecnm commented Mar 21, 2018

chrislovecnm commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018