emit warning on no liveness probe defined for pods #10363

juanvallejo · 2016-08-11T20:53:10Z

This patch iterates through pod specs, listing specs with no liveness
probes set. Any replication controllers whose deployment is fulfilled by
a DeploymentConfig are ignored.

Example

$ oc create -f pkg/api/graph/test/simple-deployment
deploymentconfig "simple-deployment" created

$ oc get all
NAME                   REVISION   DESIRED   CURRENT   TRIGGERED BY
dc/simple-deployment   1          1         0         config

NAME                     DESIRED   CURRENT   READY     AGE
rc/simple-deployment-1   0         0         0         1m

NAME                            READY     STATUS              RESTARTS
AGE
po/simple-deployment-1-deploy   0/1       ContainerCreating   0
1m

$ oc status -v
In project test on server https://10.111.123.56:8443

dc/simple-deployment deploys docker.io/openshift/deployment-example:v1
  deployment #1 pending 26 seconds ago

Warnings:
  * pod/simple-deployment-1-deploy has no liveness probe to verify pods are still running.
    try: oc set probe pod/simple-deployment-1-deploy --liveness ...
  * dc/simple-deployment has no liveness probe to verify pods are still running.
    try: oc set probe dc/simple-deployment --liveness ...

View details with 'oc describe <resource>/<name>' or list everything with 'oc get all'.

cc @fabianofranz @stevekuznetsov

juanvallejo · 2016-08-11T20:58:20Z

[test]

stevekuznetsov · 2016-08-12T13:12:49Z

pkg/deploy/graph/analysis/dc.go

+func FindDeploymentConfigLivenessWarnings(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker {
+	markers := []osgraph.Marker{}
+
+Node:


Named points almost always mean the code should be factored differently. Factor our your one large method into smaller methods to reduce cyclomatic complexity and as a result the need for this will go away.

Named points almost always mean the code should be factored differently. Factor our your one large method into smaller methods to reduce cyclomatic complexity and as a result the need for this will go away.

@stevekuznetsov is right.

fabianofranz · 2016-08-12T15:32:05Z

cc @deads2k

deads2k · 2016-08-12T20:13:00Z

pkg/deploy/graph/analysis/dc.go

+			// All of the containers in the deployment config lack a readiness probe
+			markers = append(markers, osgraph.Marker{
+				Node:     uncastDcNode,
+				Severity: osgraph.WarningSeverity,


@fabianofranz I'll defer to you about info vs warning.

I vote for info.

deads2k · 2016-08-12T20:13:11Z

Needs a test.

juanvallejo · 2016-08-15T21:13:06Z

Needs a test.

Will add a unit test

This patch currently lists all pods without a --liveness probe, including replicationcontroller pods created by deploymentconfigs. It would be ideal to only list deploymentconfig pods in those cases.

Current output:

  * rc/mysql-master-1 has no liveness probe to verify pods are still running.
    try: oc set probe rc/mysql-master-1 --liveness ...
  * rc/mysql-slave-1 has no liveness probe to verify pods are still running.
    try: oc set probe rc/mysql-slave-1 --liveness ...
  * rc/node-1 has no liveness probe to verify pods are still running.
    try: oc set probe rc/node-1 --liveness ...
  * rc/ruby-1 has no liveness probe to verify pods are still running.
    try: oc set probe rc/ruby-1 --liveness ...
  * pod/mysql-slave-1-deploy has no liveness probe to verify pods are still running.
    try: oc set probe pod/mysql-slave-1-deploy --liveness ...
  * pod/ruby-1-nec4y has no liveness probe to verify pods are still running.
    try: oc set probe pod/ruby-1-nec4y --liveness ...
  * dc/mysql-master has no liveness probe to verify pods are still running.
    try: oc set probe dc/mysql-master --liveness ...
  * dc/mysql-slave has no liveness probe to verify pods are still running.
    try: oc set probe dc/mysql-slave --liveness ...
  * dc/node has no liveness probe to verify pods are still running.
    try: oc set probe dc/node --liveness ...
  * dc/ruby has no liveness probe to verify pods are still running.
    try: oc set probe dc/ruby --liveness ...

Any feedback / help with this is welcome

stevekuznetsov · 2016-08-16T12:48:40Z

pkg/api/kubegraph/analysis/podspec.go

@@ -48,6 +50,39 @@ func FindUnmountableSecrets(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker {
 	return markers
 }

+// FindMissingLivenessProbes inspects all PodSpecs for missing liveness probes


// and generates a list of deduped markers.... ?

deads2k · 2016-08-16T15:05:30Z

This patch currently lists all pods without a --liveness probe, including replicationcontroller pods created by deploymentconfigs. It would be ideal to only list deploymentconfig pods in those cases.

Not just non-ideal. Any advice you give for fixing up an RC that's managed by a DC is bad advise. users shouldn't modify those resources.

juanvallejo · 2016-10-12T17:59:36Z

@fabianofranz @deads2k Went ahead updated this PR to ignore rc's owned by deployment configs. Also added tests. PTAL

fabianofranz · 2016-10-13T21:09:24Z

LGTM, @deads2k anything else?

deads2k · 2016-10-14T12:11:28Z

pkg/api/kubegraph/analysis/podspec.go

+func FindMissingLivenessProbes(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker {
+	markers := []osgraph.Marker{}
+	appendedNodes := sets.NewString()
+	ignoredNodes := findDeploymentEdgeKinds(g)


seems fragile.

deads2k · 2016-10-14T12:12:10Z

LGTM, @deads2k anything else?

The graph bits look a little fragile, but if you're committed to fixing them up later I'll let it go.

juanvallejo · 2016-10-14T13:46:09Z

@deads2k

The graph bits look a little fragile, but if you're committed to fixing them up later I'll let it go.

Definitely. Do you have any suggestions for improving it in this PR?

juanvallejo · 2016-10-17T21:54:18Z

@fabianofranz @deads2k Added a ControllerRefEdgeKind and appended it to the edge between deploymentconfigs and replicationcontrollers for now, any thoughts on this implementation? d75004e

stevekuznetsov · 2016-10-18T13:41:21Z

pkg/deploy/graph/edges.go

@@ -73,6 +75,7 @@ func AddDeploymentEdges(g osgraph.MutableUniqueGraph, node *deploygraph.Deployme
 			}
 			if BelongsToDeploymentConfig(node.DeploymentConfig, rcNode.ReplicationController) {
 				g.AddEdge(node, rcNode, DeploymentEdgeKind)
+				g.AddEdge(node, rcNode, ControllerRefEdgeKind)


The comments I had for you in person remain -- enforcing this weakly by co-locating the two edge creations instead of programmatically by subtyping ControllerRefEdgeKind with DeploymentEdgeKind is not ideal.

@stevekuznetsov I will go ahead and open those changes in a new PR, unless it's better to add it in a new commit here? Thanks for the feedback on this btw

I am not sure that closing out that issue in this fast/incomplete way is the correct first step. I would implement the structure necessary to do it first, then come back to this PR. Will defer my opinion to @deads / @fabianofranz

Slightly different deads summoned :]
cc @deads2k

@stevekuznetsov

I am not sure that closing out that issue in this fast/incomplete way is the correct first step. I would implement the structure necessary to do it first, then come back to this PR

Sounds good, I'll go ahead and propose this change and begin working on a separate PR for it

deads2k · 2016-10-18T17:41:14Z

pkg/deploy/graph/edges.go

@@ -22,6 +22,8 @@ const (
 	DeploymentEdgeKind = "Deployment"
 	// VolumeClaimEdgeKind goes from DeploymentConfigs to PersistentVolumeClaims indicating a request for persistent storage.
 	VolumeClaimEdgeKind = "VolumeClaim"
+	// ControllerRefEdgeKind goes from a controller node to its controlled child-node
+	ControllerRefEdgeKind = "ControllerRef"


isn't this the same as the "ManagedByController" edge from AddManagedByControllerPodEdges? See if you can find the PR, but I'm pretty sure we discussed this as generic there.

@deads2k Found this PR #9972. Since this edge kind exists, would I just be filtering the graph by ManagedByControllerEdgeKind here?

juanvallejo · 2016-10-18T21:56:15Z

@deads2k or @csrwng PTAL 6807a7a

deads2k · 2016-10-21T18:13:43Z

pkg/api/kubegraph/analysis/podspec.go

+
+// CheckForLivenessProbes iterates through all of the containers in a podSpecNode until it finds one
+// with a liveness probe set. The list of nodes whose containers have no liveness probe set is returned.
+func CheckForLivenessProbes(podSpecNode *kubegraph.PodSpecNode) []*kubegraph.PodSpecNode {


make this a bool check.

deads2k · 2016-10-21T18:14:04Z

pkg/api/kubegraph/analysis/podspec.go

+
+	for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) {
+		podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode)
+		podsWithoutLivenessProbes := CheckForLivenessProbes(podSpecNode)


bad name, this is a bool. Skip from here if there's no issue.

deads2k · 2016-10-21T18:14:37Z

pkg/api/kubegraph/analysis/podspec.go

+		topLevelString := f.ResourceName(topLevelNode)
+
+		// prevent duplicate markers
+		if appendedNodes.Has(topLevelString) {


deads2k · 2016-10-21T18:15:33Z

pkg/api/kubegraph/analysis/podspec.go

+	for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) {
+		podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode)
+		podsWithoutLivenessProbes := CheckForLivenessProbes(podSpecNode)
+


Check here to see if you

deads2k · 2016-10-21T18:16:00Z

pkg/api/kubegraph/analysis/podspec.go

+		topLevelString := f.ResourceName(topLevelNode)
+
+		// prevent duplicate markers
+		if appendedNodes.Has(topLevelString) {


check here to see if you have a controllerref edge (whatever its called). continue if you do.

deads2k · 2016-10-21T18:16:28Z

pkg/api/kubegraph/analysis/podspec.go

+func FindMissingLivenessProbes(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker {
+	markers := []osgraph.Marker{}
+	appendedNodes := sets.NewString()
+	ignoredNodes := findNodesManagedByController(g)


this isn't needed.

deads2k · 2016-10-21T18:16:43Z

pkg/api/kubegraph/analysis/podspec.go

+// FindMissingLivenessProbes inspects all PodSpecs for missing liveness probes and generates a list of non-duplicate markers
+func FindMissingLivenessProbes(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker {
+	markers := []osgraph.Marker{}
+	appendedNodes := sets.NewString()


unnecessary.

deads2k · 2016-10-21T18:17:04Z

pkg/api/kubegraph/analysis/podspec.go

+		}
+
+		appendedNodes.Insert(topLevelString)
+		for _, node := range podsWithoutLivenessProbes {


the name node sucks so much.

deads2k · 2016-10-21T18:18:24Z

pkg/api/kubegraph/analysis/podspec.go

+
+		appendedNodes.Insert(topLevelString)
+		for _, node := range podsWithoutLivenessProbes {
+			markers = append(markers, osgraph.Marker{


At this point, you either attach the marker to the toplevelcontainingnode or to the podspec node (see which renders right). If they both render correctly, attach to the podspecnode.

deads2k · 2016-10-21T18:19:47Z

pkg/api/kubegraph/analysis/podspec.go

+	appendedNodes := sets.NewString()
+	ignoredNodes := findNodesManagedByController(g)
+
+	for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) {


in psuedo code:

if hasliveness(podspec){ continue } topContainingNode = .... if topcontainerNode has controllerref edge{ continue } addmarker

juanvallejo · 2016-10-24T22:04:06Z

@deads2k Thanks for the feedback! Review comments addressed 9ed2553

cc @fabianofranz

juanvallejo · 2016-10-25T14:03:30Z

networking check flaked on #11452 re[test]

deads2k · 2016-10-26T13:58:24Z

pkg/api/kubegraph/analysis/podspec.go

+		podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode)
+		topLevelNode := osgraph.GetTopLevelContainerNode(g, podSpecNode)
+
+		if hasLivenessProbe(podSpecNode) {


this check is cheap, run it first.

deads2k · 2016-10-26T13:58:39Z

pkg/api/kubegraph/analysis/podspec.go

+
+	for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) {
+		podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode)
+		topLevelNode := osgraph.GetTopLevelContainerNode(g, podSpecNode)


move after the "is this node ok" check

deads2k · 2016-10-26T13:58:55Z

pkg/api/kubegraph/analysis/podspec.go

+		if hasLivenessProbe(podSpecNode) {
+			continue
+		}
+		if hasControllerRefEdge(g, topLevelNode) {


add a comment here explaining the reasoning.

deads2k · 2016-10-26T13:59:15Z

pkg/api/kubegraph/analysis/podspec.go

+
+		topLevelString := f.ResourceName(topLevelNode)
+		markers = append(markers, osgraph.Marker{
+			Node: podSpecNode,


add the toplevel node as related.

deads2k · 2016-10-26T14:00:06Z

pkg/api/kubegraph/analysis/podspec.go

+}
+
+// hasControllerRefEdge returns true if a given node contains a "ManagedByController" edge between itself and any of its "To" nodes.
+func hasControllerRefEdge(g osgraph.Graph, node graph.Node) bool {


Don't we have a cleaner way to do this? List outbound edges and check them directly?

deads2k · 2016-10-26T14:03:31Z

minor comments. lgtm otherwise.

juanvallejo · 2016-10-26T15:16:20Z

@deads2k Thanks for the feedback, review comments addressed

deads2k · 2016-10-26T15:19:48Z

lgtm. needs a squash.

This patch iterates through pod specs, listing specs with no liveness probes set. Ignores podspec nodes with a controlling edge kind.

juanvallejo · 2016-10-26T20:22:11Z

conformance test flaked on #11114 re[test]

openshift-bot · 2016-10-26T20:29:30Z

Evaluated for origin test up to 914b5c9

openshift-bot · 2016-10-26T21:51:53Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10701/) (Base Commit: 8bb33be)

juanvallejo · 2016-10-27T15:13:32Z

@deads2k

lgtm. needs a squash.

Done!

fabianofranz · 2016-10-27T19:26:36Z

[merge]

openshift-bot · 2016-10-27T19:29:31Z

Evaluated for origin merge up to 914b5c9

openshift-bot · 2016-10-27T22:37:38Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10767/) (Base Commit: 0403599) (Image: devenv-rhel7_5261)

stevekuznetsov reviewed Aug 12, 2016
View reviewed changes

deads2k reviewed Aug 12, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from 2ecad3d to f5d49b6 Compare August 15, 2016 19:57

stevekuznetsov reviewed Aug 16, 2016
View reviewed changes

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch 3 times, most recently from d5417ff to d24e4a0 Compare October 12, 2016 17:57

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from d24e4a0 to 612b301 Compare October 12, 2016 19:29

deads2k reviewed Oct 14, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from 612b301 to d75004e Compare October 17, 2016 20:38

stevekuznetsov reviewed Oct 18, 2016

View reviewed changes

deads2k reviewed Oct 18, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from 298b191 to 6807a7a Compare October 18, 2016 22:06

deads2k reviewed Oct 21, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from 9ed2553 to 92c5fb5 Compare October 25, 2016 20:40

deads2k reviewed Oct 26, 2016

View reviewed changes

add oc status warning for missing liveness probe

914b5c9

This patch iterates through pod specs, listing specs with no liveness probes set. Ignores podspec nodes with a controlling edge kind.

juanvallejo force-pushed the jvallejo_emit-warning-no-liveness-probe branch from d43f32f to 914b5c9 Compare October 26, 2016 16:54

openshift-bot merged commit 5494a7e into openshift:master Oct 28, 2016

juanvallejo deleted the jvallejo_emit-warning-no-liveness-probe branch October 28, 2016 14:15

emit warning on no liveness probe defined for pods #10363

emit warning on no liveness probe defined for pods #10363

Conversation

juanvallejo commented Aug 11, 2016 • edited Loading

juanvallejo commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Aug 12, 2016

juanvallejo commented Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

deads2k commented Aug 16, 2016 • edited Loading

juanvallejo commented Oct 12, 2016

fabianofranz commented Oct 13, 2016

Choose a reason for hiding this comment

deads2k commented Oct 14, 2016

juanvallejo commented Oct 14, 2016

juanvallejo commented Oct 17, 2016

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

juanvallejo commented Oct 18, 2016 • edited Loading

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

juanvallejo commented Oct 24, 2016

juanvallejo commented Oct 25, 2016

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

deads2k commented Oct 26, 2016

juanvallejo commented Oct 26, 2016

deads2k commented Oct 26, 2016

juanvallejo commented Oct 26, 2016

openshift-bot commented Oct 26, 2016

openshift-bot commented Oct 26, 2016

juanvallejo commented Oct 27, 2016

fabianofranz commented Oct 27, 2016

openshift-bot commented Oct 27, 2016

openshift-bot commented Oct 27, 2016 • edited Loading

juanvallejo commented Aug 11, 2016 •

edited

Loading

juanvallejo commented Aug 15, 2016 •

edited

Loading

deads2k commented Aug 16, 2016 •

edited

Loading

juanvallejo commented Oct 18, 2016 •

edited

Loading

openshift-bot commented Oct 27, 2016 •

edited

Loading