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

emit warning on no liveness probe defined for pods #10363

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/api/kubegraph/analysis/hpa.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

osgraph "github.com/openshift/origin/pkg/api/graph"
"github.com/openshift/origin/pkg/api/kubegraph"
kubeedges "github.com/openshift/origin/pkg/api/kubegraph"
kubenodes "github.com/openshift/origin/pkg/api/kubegraph/nodes"
deploygraph "github.com/openshift/origin/pkg/deploy/graph"
deploynodes "github.com/openshift/origin/pkg/deploy/graph/nodes"
Expand Down Expand Up @@ -117,6 +118,7 @@ func FindOverlappingHPAs(graph osgraph.Graph, namer osgraph.Namer) []osgraph.Mar
edgeFilter := osgraph.EdgesOfKind(
kubegraph.ScalingEdgeKind,
deploygraph.DeploymentEdgeKind,
kubeedges.ManagedByControllerEdgeKind,
)

hpaSubGraph := graph.Subgraph(nodeFilter, edgeFilter)
Expand Down
57 changes: 55 additions & 2 deletions pkg/api/kubegraph/analysis/podspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ import (
)

const (
UnmountableSecretWarning = "UnmountableSecret"
MissingSecretWarning = "MissingSecret"
UnmountableSecretWarning = "UnmountableSecret"
MissingSecretWarning = "MissingSecret"
MissingLivenessProbeWarning = "MissingLivenessProbe"
)

// FindUnmountableSecrets inspects all PodSpecs for any Secret reference that isn't listed as mountable by the referenced ServiceAccount
Expand Down Expand Up @@ -75,6 +76,58 @@ func FindMissingSecrets(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker {
return markers
}

// 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{}

for _, uncastPodSpecNode := range g.NodesByKind(kubegraph.PodSpecNodeKind) {
Copy link
Contributor

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

podSpecNode := uncastPodSpecNode.(*kubegraph.PodSpecNode)
if hasLivenessProbe(podSpecNode) {
Copy link
Contributor

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.

continue
}

topLevelNode := osgraph.GetTopLevelContainerNode(g, podSpecNode)

// skip any podSpec nodes that are managed by other nodes.
// Liveness probes should only be applied to a controlling
// podSpec node, and not to any of its children.
if hasControllerRefEdge(g, topLevelNode) {
Copy link
Contributor

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.

continue
}

topLevelString := f.ResourceName(topLevelNode)
markers = append(markers, osgraph.Marker{
Node: podSpecNode,
RelatedNodes: []graph.Node{topLevelNode},

Severity: osgraph.WarningSeverity,
Key: MissingLivenessProbeWarning,
Message: fmt.Sprintf("%s has no liveness probe to verify pods are still running.",
topLevelString),
Suggestion: osgraph.Suggestion(fmt.Sprintf("%s %s --liveness ...", setProbeCommand, topLevelString)),
})
}

return markers
}

// hasLivenessProbe iterates through all of the containers in a podSpecNode returning true
// if at least one container has a liveness probe, or false otherwise
func hasLivenessProbe(podSpecNode *kubegraph.PodSpecNode) bool {
for _, container := range podSpecNode.PodSpec.Containers {
if container.LivenessProbe != nil {
return true
}
}
return false
}

// hasControllerRefEdge returns true if a given node contains one or more "ManagedByController" outbound edges
func hasControllerRefEdge(g osgraph.Graph, node graph.Node) bool {
Copy link
Contributor

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?

managedEdges := g.OutboundEdges(node, kubeedges.ManagedByControllerEdgeKind)
return len(managedEdges) > 0
}

// CheckForUnmountableSecrets checks to be sure that all the referenced secrets are mountable (by service account)
func CheckForUnmountableSecrets(g osgraph.Graph, podSpecNode *kubegraph.PodSpecNode) []*kubegraph.SecretNode {
saNodes := g.SuccessorNodesByNodeAndEdgeKind(podSpecNode, kubegraph.ServiceAccountNodeKind, kubeedges.ReferencedServiceAccountEdgeKind)
Expand Down
20 changes: 20 additions & 0 deletions pkg/api/kubegraph/analysis/podspec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,23 @@ func TestUnmountableSecrets(t *testing.T) {
t.Errorf("expected %v, got %v", expectedSecret2, markers)
}
}

func TestMissingLivenessProbes(t *testing.T) {
g, _, err := osgraphtest.BuildGraph("../../../api/graph/test/simple-deployment.yaml")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

kubeedges.AddAllExposedPodEdges(g)

markers := FindMissingLivenessProbes(g, osgraph.DefaultNamer, "oc set probe")
if e, a := 1, len(markers); e != a {
t.Fatalf("expected %v, got %v", e, a)
}

actualDC := osgraph.GetTopLevelContainerNode(g, markers[0].Node)
expectedDC := g.Find(osgraph.UniqueName("DeploymentConfig|/simple-deployment"))
if e, a := expectedDC.ID(), actualDC.ID(); e != a {
t.Errorf("expected %v, got %v", e, a)
}
}
3 changes: 3 additions & 0 deletions pkg/cmd/cli/describe/projectstatus.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,9 @@ func getMarkerScanners(logsCommandName, securityPolicyCommandFormat, setProbeCom
func(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker {
return deployanalysis.FindDeploymentConfigReadinessWarnings(g, f, setProbeCommandName)
},
func(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker {
return kubeanalysis.FindMissingLivenessProbes(g, f, setProbeCommandName)
},
routeanalysis.FindPortMappingIssues,
routeanalysis.FindMissingTLSTerminationType,
routeanalysis.FindPathBasedPassthroughRoutes,
Expand Down
2 changes: 2 additions & 0 deletions pkg/deploy/graph/edges.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
kapi "k8s.io/kubernetes/pkg/api"

osgraph "github.com/openshift/origin/pkg/api/graph"
kubeedges "github.com/openshift/origin/pkg/api/kubegraph"
kubegraph "github.com/openshift/origin/pkg/api/kubegraph/nodes"
deployapi "github.com/openshift/origin/pkg/deploy/api"
deploygraph "github.com/openshift/origin/pkg/deploy/graph/nodes"
Expand Down Expand Up @@ -73,6 +74,7 @@ func AddDeploymentEdges(g osgraph.MutableUniqueGraph, node *deploygraph.Deployme
}
if BelongsToDeploymentConfig(node.DeploymentConfig, rcNode.ReplicationController) {
g.AddEdge(node, rcNode, DeploymentEdgeKind)
g.AddEdge(rcNode, node, kubeedges.ManagedByControllerEdgeKind)
}
}
}
Expand Down
21 changes: 21 additions & 0 deletions test/cmd/set-liveness-probe.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/bin/bash
source "$(dirname "${BASH_SOURCE}")/../../hack/lib/init.sh"
trap os::test::junit::reconcile_output EXIT

# Cleanup cluster resources created by this test
(
set +e
oc delete all,templates --all
exit 0
) &>/dev/null


os::test::junit::declare_suite_start "cmd/set-probe-liveness"
# This test setting a liveness probe, without warning about replication controllers whose deployment depends on deployment configs
os::cmd::expect_success_and_text 'oc create -f pkg/api/graph/test/simple-deployment.yaml' 'deploymentconfig "simple-deployment" created'
os::cmd::expect_success_and_text 'oc status -v' 'dc/simple-deployment has no liveness probe'
os::cmd::expect_success_and_not_text 'oc status -v' 'rc/simple-deployment-1 has no liveness probe'
os::cmd::expect_success_and_text 'oc set probe dc/simple-deployment --liveness --get-url=http://google.com:80' 'deploymentconfig "simple-deployment" updated'
os::cmd::expect_success_and_not_text 'oc status -v' 'dc/simple-deployment has no liveness probe'
echo "set-probe-liveness: ok"
os::test::junit::declare_suite_end