added oc status checks for hpa missing cpu target and overlapping hpas #7799

stevekuznetsov · 2016-03-04T16:53:37Z

TODO: overlapping HPAs

stevekuznetsov · 2016-03-04T16:54:24Z

pkg/api/kubegraph/analysis/hpa_test.go

+)
+
+func TestHPAMissingCPUTargetWarning(t *testing.T) {
+	g, _, err := osgraphtest.BuildGraph("./../../../api/graph/test/hpa-missing-cpu-target.yaml")


At this step the CPUUtilization is defaulted to 80 by some API machinery. It's unclear to me how to get that not to happen.

submitting to extensions/v1beta1 defaults to 80%. submitting to autoscaling/v1 does not.

unable to decode "<snip>/hpa-missing-cpu-target.yaml": no kind "HorizontalPodAutoscaler" is registered for version "autoscaling/v1"

right, origin doesn't have that group yet

Is it even currently possible for an HPA in Origin to be missing the CPU target?

Unless an admin edits one to remove it

I don't think it's possible in the API, but it will be as soon as we get the autoscaling group in the rebase :-/

@deads2k based on this discussion, unless I'm misunderstanding why you need these changes ASAP, let's postpone this PR for a while

overlapping is valuable. missing will be valuable in a week

Ah, the timeline is that close. Ok. Should I just t.Skip() the test for now? Do we want this before the rebase?

0xmichalis · 2016-03-04T17:21:34Z

@DirectXMan12 fyi

0xmichalis · 2016-03-04T17:21:59Z

also @jwforres @spadgett

stevekuznetsov · 2016-03-04T17:52:41Z

@deads2k this is ready for a look

0xmichalis · 2016-03-04T17:54:12Z

pkg/api/kubegraph/analysis/hpa.go

+	HPAOverlappingScaleRefWarning = "HPAOverlappingScaleRef"
+)
+
+func FindHPASpecsMissingCPUTargets(g osgraph.Graph, f osgraph.Namer) []osgraph.Marker {


stevekuznetsov · 2016-03-04T18:01:06Z

@Kargakis addressed comments

0xmichalis · 2016-03-04T18:03:08Z

pkg/api/kubegraph/analysis/hpa.go

+			continue
+		}
+
+		if otherHPA, ok := references[hpa.Spec.ScaleRef]; ok {


We need to warn about all hpas that may overlap and not just two.

Which HPA node will we tie the warning marker to? Or should we make a marker for each one?

Hm, I am not sure about the main node (definitely one of the younger hpas) but all should be included in the related nodes (except the main one)

Also the dc/rc should be in the related nodes too

Which HPA node will we tie the warning marker to? Or should we make a marker for each one?

RCs create a marker on each node. That seems reasonable and it allows good filtering by Node.

stevekuznetsov · 2016-03-04T18:21:19Z

@Kargakis How should we find the DC/RC/Job to put in the list of related nodes? Won't we have to get all of the DC/RC/Jobs we know about and iterate through them all to find which one is referenced in the ScaleRef? That seems pretty computationally inefficient for what we're trying to achieve.

deads2k · 2016-03-04T18:28:41Z

@Kargakis How should we find the DC/RC/Job to put in the list of related nodes? Won't we have to get all of the DC/RC/Jobs we know about and iterate through them all to find which one is referenced in the ScaleRef? That seems pretty computationally inefficient for what we're trying to achieve.

Why aren't the edges relating the hpa to the things its scaling not already in the graph?

stevekuznetsov · 2016-03-04T18:31:13Z

@deads2k good question :)

jwforres · 2016-03-04T19:48:01Z

can you include a sample of the cli output when these warnings appear

stevekuznetsov · 2016-03-04T20:05:59Z

@deads2k would appreciate a look while I write tests

deads2k · 2016-03-04T20:13:10Z

pkg/api/kubegraph/analysis/hpa.go

+		if len(hpas) > 1 {
+			for i, node := range hpas {
+				others := append(hpas[:i], hpas[i:]...)
+				relatedNodes := append(g.SuccessorNodesByEdgeKind(node, kubegraph.ReferencedReplicationControllerEdgeKind), g.SuccessorNodesByEdgeKind(node, kubegraph.ReferencedDeplotmentConfigEdgeKind)...)


0xmichalis · 2016-03-23T16:14:44Z

pkg/api/kubegraph/analysis/hpa.go

+		)
+
+		if len(scaledObjects) < 1 {
+			markers = append(markers, createMissingScaleRefMarker(node, nil, namer))


return here explicitly

return here explicitly

I don't think this returns. It checks the next one, right?

Added a continue

deads2k · 2016-03-23T17:38:57Z

pkg/api/kubegraph/analysis/hpa.go

+//  - for every resulting edge in the new sub-graph, create an edge in the reverse direction
+//  - find the shortest paths between all HPA nodes in the graph
+//  - shortest paths connecting two horizontal pod autoscalers are used to create markers for the graph
+func FindOverlappingHPAs(graph osgraph.Graph, namer osgraph.Namer) []osgraph.Marker {


Nice, tight, and readable

deads2k · 2016-03-23T18:16:05Z

Relatively minor comments overall. Not sure I want something this big in 1.2 though.

stevekuznetsov · 2016-03-23T18:50:30Z

[test]me

stevekuznetsov · 2016-03-30T14:38:22Z

@deads2k we have green

stevekuznetsov · 2016-05-02T13:44:45Z

@deads2k ping

stevekuznetsov · 2016-05-02T15:17:47Z

@deads2k updated not to embed the HPA.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

deads2k · 2016-05-02T15:24:27Z

[merge]

openshift-bot · 2016-05-02T15:25:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5782/) (Image: devenv-rhel7_4078)

openshift-bot · 2016-05-02T15:25:21Z

Evaluated for origin merge up to bd7e128

openshift-bot · 2016-05-02T15:40:22Z

Evaluated for origin test up to bd7e128

openshift-bot · 2016-05-02T17:08:35Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3529/)

stevekuznetsov reviewed Mar 4, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch from fae5124 to 194a96c Compare March 4, 2016 17:15

stevekuznetsov force-pushed the skuznets/status-hpa branch from 194a96c to 35722bc Compare March 4, 2016 17:52

stevekuznetsov changed the title ~~[wip] added oc status checks for hpa missing cpu target and overlapping hpas~~ added oc status checks for hpa missing cpu target and overlapping hpas Mar 4, 2016

0xmichalis reviewed Mar 4, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch from 35722bc to 094d6a5 Compare March 4, 2016 18:00

0xmichalis reviewed Mar 4, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch 2 times, most recently from bb4b33c to 793e5d5 Compare March 4, 2016 18:19

stevekuznetsov force-pushed the skuznets/status-hpa branch from 793e5d5 to 32f57f7 Compare March 4, 2016 18:24

stevekuznetsov force-pushed the skuznets/status-hpa branch from 32f57f7 to 106188c Compare March 4, 2016 18:31

stevekuznetsov force-pushed the skuznets/status-hpa branch from 106188c to 2faf973 Compare March 4, 2016 20:05

stevekuznetsov force-pushed the skuznets/status-hpa branch from 2faf973 to 9dfc112 Compare March 4, 2016 20:10

deads2k reviewed Mar 4, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch 2 times, most recently from 65ea64e to 8413c12 Compare March 9, 2016 16:14

0xmichalis reviewed Mar 23, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch from f4b62ec to ce2d3ca Compare March 23, 2016 17:05

deads2k reviewed Mar 23, 2016
View reviewed changes

stevekuznetsov force-pushed the skuznets/status-hpa branch from ce2d3ca to 96b0d51 Compare March 23, 2016 18:27

stevekuznetsov force-pushed the skuznets/status-hpa branch 2 times, most recently from 2bbb994 to 9a45db1 Compare March 30, 2016 13:05

deads2k added this to the 1.2.x milestone Apr 18, 2016

stevekuznetsov force-pushed the skuznets/status-hpa branch from 9a45db1 to fb91fbf Compare April 18, 2016 15:12

stevekuznetsov force-pushed the skuznets/status-hpa branch from fb91fbf to de2b87f Compare May 2, 2016 15:17

added oc status check for hpa missing cpu request

bd7e128

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/status-hpa branch from de2b87f to bd7e128 Compare May 2, 2016 15:22

openshift-bot merged commit 737ca2a into openshift:master May 2, 2016

stevekuznetsov deleted the skuznets/status-hpa branch June 15, 2016 18:10

added oc status checks for hpa missing cpu target and overlapping hpas #7799

added oc status checks for hpa missing cpu target and overlapping hpas #7799

Conversation

stevekuznetsov commented Mar 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Mar 4, 2016

0xmichalis commented Mar 4, 2016

stevekuznetsov commented Mar 4, 2016

Choose a reason for hiding this comment

stevekuznetsov commented Mar 4, 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

stevekuznetsov commented Mar 4, 2016

deads2k commented Mar 4, 2016

stevekuznetsov commented Mar 4, 2016

jwforres commented Mar 4, 2016

stevekuznetsov commented Mar 4, 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 Mar 23, 2016

stevekuznetsov commented Mar 23, 2016

stevekuznetsov commented Mar 30, 2016

stevekuznetsov commented May 2, 2016

stevekuznetsov commented May 2, 2016

deads2k commented May 2, 2016

openshift-bot commented May 2, 2016 • edited

openshift-bot commented May 2, 2016

openshift-bot commented May 2, 2016

openshift-bot commented May 2, 2016

openshift-bot commented May 2, 2016 •

edited