-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
emit warning on no liveness probe defined for pods #10363
emit warning on no liveness probe defined for pods #10363
Conversation
[test] |
func FindDeploymentConfigLivenessWarnings(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker { | ||
markers := []osgraph.Marker{} | ||
|
||
Node: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cc @deads2k |
// All of the containers in the deployment config lack a readiness probe | ||
markers = append(markers, osgraph.Marker{ | ||
Node: uncastDcNode, | ||
Severity: osgraph.WarningSeverity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabianofranz I'll defer to you about info vs warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for info.
Needs a test. |
2ecad3d
to
f5d49b6
Compare
Will add a unit test This patch currently lists all pods without a Current output:
Any feedback / help with this is welcome |
@@ -48,6 +50,39 @@ func FindUnmountableSecrets(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker { | |||
return markers | |||
} | |||
|
|||
// FindMissingLivenessProbes inspects all PodSpecs for missing liveness probes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// and generates a list of deduped markers.... ?
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. |
d5417ff
to
d24e4a0
Compare
@fabianofranz @deads2k Went ahead updated this PR to ignore rc's owned by deployment configs. Also added tests. PTAL |
d24e4a0
to
612b301
Compare
LGTM, @deads2k anything else? |
func FindMissingLivenessProbes(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker { | ||
markers := []osgraph.Marker{} | ||
appendedNodes := sets.NewString() | ||
ignoredNodes := findDeploymentEdgeKinds(g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fragile.
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? |
612b301
to
d75004e
Compare
@fabianofranz @deads2k Added a |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly different deads summoned :]
cc @deads2k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
298b191
to
6807a7a
Compare
|
||
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this a bool check.
|
||
for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) { | ||
podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode) | ||
podsWithoutLivenessProbes := CheckForLivenessProbes(podSpecNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad name, this is a bool. Skip from here if there's no issue.
topLevelString := f.ResourceName(topLevelNode) | ||
|
||
// prevent duplicate markers | ||
if appendedNodes.Has(topLevelString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) { | ||
podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode) | ||
podsWithoutLivenessProbes := CheckForLivenessProbes(podSpecNode) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check here to see if you
topLevelString := f.ResourceName(topLevelNode) | ||
|
||
// prevent duplicate markers | ||
if appendedNodes.Has(topLevelString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check here to see if you have a controllerref edge (whatever its called). continue if you do.
func FindMissingLivenessProbes(g osgraph.Graph, f osgraph.Namer, setProbeCommand string) []osgraph.Marker { | ||
markers := []osgraph.Marker{} | ||
appendedNodes := sets.NewString() | ||
ignoredNodes := findNodesManagedByController(g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't needed.
// 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary.
} | ||
|
||
appendedNodes.Insert(topLevelString) | ||
for _, node := range podsWithoutLivenessProbes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name node sucks so much.
|
||
appendedNodes.Insert(topLevelString) | ||
for _, node := range podsWithoutLivenessProbes { | ||
markers = append(markers, osgraph.Marker{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
appendedNodes := sets.NewString() | ||
ignoredNodes := findNodesManagedByController(g) | ||
|
||
for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in psuedo code:
if hasliveness(podspec){
continue
}
topContainingNode = ....
if topcontainerNode has controllerref edge{
continue
}
addmarker
networking check flaked on #11452 re[test] |
9ed2553
to
92c5fb5
Compare
podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode) | ||
topLevelNode := osgraph.GetTopLevelContainerNode(g, podSpecNode) | ||
|
||
if hasLivenessProbe(podSpecNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is cheap, run it first.
|
||
for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) { | ||
podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode) | ||
topLevelNode := osgraph.GetTopLevelContainerNode(g, podSpecNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move after the "is this node ok" check
if hasLivenessProbe(podSpecNode) { | ||
continue | ||
} | ||
if hasControllerRefEdge(g, topLevelNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment here explaining the reasoning.
|
||
topLevelString := f.ResourceName(topLevelNode) | ||
markers = append(markers, osgraph.Marker{ | ||
Node: podSpecNode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the toplevel node as related.
} | ||
|
||
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a cleaner way to do this? List outbound edges and check them directly?
minor comments. lgtm otherwise. |
@deads2k Thanks for the feedback, review comments addressed |
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.
d43f32f
to
914b5c9
Compare
conformance test flaked on #11114 re[test] |
Evaluated for origin test up to 914b5c9 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10701/) (Base Commit: 8bb33be) |
Done! |
[merge] |
Evaluated for origin merge up to 914b5c9 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10767/) (Base Commit: 0403599) (Image: devenv-rhel7_5261) |
fixes #10271
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
cc @fabianofranz @stevekuznetsov