status: Report routes that have no route port specified #5089

0xmichalis · 2015-10-12T18:36:24Z

This PR adds graph pieces for routes and makes oc status report routes with missing route ports when they expose multiport services.

@deads2k @smarterclayton @pweil- PTAL

0xmichalis · 2015-10-12T18:38:06Z

Currently:

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

service/database - 172.30.14.62:5434 -> 3306
  dc/database deploys docker.io/openshift/mysql-55-centos7:latest 
    #1 deployed 26 minutes ago - 1 pod

service/frontend - 172.30.70.12:5432 -> 8080
  dc/frontend deploys istag/origin-ruby-sample:latest <-
    builds https://github.com/openshift/ruby-hello-world.git with test/ruby-20-centos7:latest through bc/ruby-sample-build 
    #1 deployed 25 minutes ago - 2 pods

Warnings:
  route/database is routing traffic to svc/database but it doesn't have a route port specified.
  route/route-edge is routing traffic to svc/frontend but it doesn't have a route port specified.

To see more, use 'oc describe <resource>/<name>'.
You can use 'oc get all' to see a list of other objects.

0xmichalis · 2015-10-12T18:40:04Z

Ugh, this is WIP, need to add tests.

deads2k · 2015-10-12T18:40:55Z

It seems like routes should be somehow related to the services they're routing to in the main status section instead of just having warnings about them. Maybe an "exposed through route/foo" on the service line?

0xmichalis · 2015-10-12T18:42:54Z

It seems like routes should be somehow related to the services they're routing to in the main status section instead of just having warnings about them. Maybe an "exposed through route/foo" on the service line?

Sure, I just need to get around the graph code as I'm not 100% familiar with it. I'll look into this tomorrow.

0xmichalis · 2015-10-13T11:13:01Z

@deads2k how about:

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

svc/database - 172.30.45.255:5434 -> 3306
  dc/database deploys docker.io/openshift/mysql-55-centos7:latest 
    #1 deployed 57 minutes ago - 1 pod
  exposed through route/database at database-test.router.default.svc.cluster.local

svc/frontend - 172.30.182.32:5432 -> 8080
  dc/frontend deploys imagestreamtag/origin-ruby-sample:latest <-
    builds https://github.com/openshift/ruby-hello-world.git with test/ruby-20-centos7:latest through bc/ruby-sample-build 
    #1 deployed 55 minutes ago - 2 pods
  exposed through route/route-edge at www.example.com

Warnings:
  route/database is routing traffic to svc/database but it doesn't have a route port specified.
  route/route-edge is routing traffic to svc/frontend but it doesn't have a route port specified.

To see more, use 'oc describe <resource>/<name>'.
You can use 'oc get all' to see a list of other objects.

smarterclayton · 2015-10-13T12:09:18Z

There's no point to showing the full DNS name until we report status to the
route - you won't know what correct the route DNS suffix is. So for now,
say "exposed via route/name" and if a port is set, include the port.

On Oct 13, 2015, at 7:13 AM, Michail Kargakis notifications@github.com
wrote:

@deads2k https://github.com/deads2k how about:

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

svc/database - 172.30.45.255:5434 -> 3306
dc/database deploys docker.io/openshift/mysql-55-centos7:latest
#1 deployed 57 minutes ago - 1 pod
exposed through route/database at
database-test.router.default.svc.cluster.local

svc/frontend - 172.30.182.32:5432 -> 8080
dc/frontend deploys imagestreamtag/origin-ruby-sample:latest <-
builds https://github.com/openshift/ruby-hello-world.git with
test/ruby-20-centos7:latest through bc/ruby-sample-build
#1 deployed 55 minutes ago - 2 pods
exposed through route/route-edge at www.example.com

Warnings:
route/database is routing traffic to svc/database but it doesn't
have a route port specified.
route/route-edge is routing traffic to svc/frontend but it doesn't
have a route port specified.

To see more, use 'oc describe /'.
You can use 'oc get all' to see a list of other objects.

—
Reply to this email directly or view it on GitHub
#5089 (comment).

0xmichalis · 2015-10-13T12:41:27Z

So for now, say "exposed via route/name" and if a port is set, include the port.

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

svc/frontend - 172.30.182.32:5432 -> 8080
  exposed through route/route-edge

Warnings:
  route/route-edge is routing traffic to svc/frontend but it doesn't have a route port specified.

To see more, use 'oc describe <resource>/<name>'.
You can use 'oc get all' to see a list of other objects.

# Add a route port

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

svc/frontend - 172.30.182.32:5432 -> 8080
  exposed through route/route-edge at port 5432

To see more, use 'oc describe <resource>/<name>'.
You can use 'oc get all' to see a list of other objects.

0xmichalis · 2015-10-13T14:42:21Z

[test]

openshift-bot · 2015-10-13T14:57:20Z

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

smarterclayton · 2015-10-13T14:57:57Z

"exposed pod port 5432 through route/route-edge"

Otherwise review delegated to david.

On Tue, Oct 13, 2015 at 10:42 AM, Michail Kargakis <notifications@github.com

wrote:

[test]

—
Reply to this email directly or view it on GitHub
#5089 (comment).

0xmichalis · 2015-10-13T16:00:06Z

@danmcp why jenkins is failing to execute the tests?

danmcp · 2015-10-13T17:33:57Z

@Kargakis It looks like it reran them. What didn't happen that you thought should have?

0xmichalis · 2015-10-13T19:40:48Z

This test didn't happen: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5733/console

http://pastebin.com/9ZK9Kr8i

0xmichalis · 2015-10-13T19:42:29Z

re[test] btw

danmcp · 2015-10-13T19:52:00Z

@Kargakis Ah. Yeah that's a known issue that needs to be debugged still.

0xmichalis · 2015-10-14T11:18:42Z

[test]

deads2k · 2015-10-14T13:25:04Z

pkg/route/graph/analysis/analysis.go

+
+route:
+	for _, uncastRouteNode := range g.NodesByKind(routegraph.RouteNodeKind) {
+		for _, uncastServiceNode := range g.SuccessorNodesByEdgeKind(uncastRouteNode, kubeedges.ExposedThroughServiceEdgeKind) {


This doesn't look right. The error is based on whether the route has a target port. I don't think the service is a related object. It wasn't involved in the decision at all.

This doesn't look right. The error is based on whether the route has a target port. I don't think the service is a related object. It wasn't involved in the decision at all.

Reading through the doc, it looks like this is only something we need to call out if the services themselves have more than one port exposed? That would be a reason for having services be related. Add the test to see if its actually a problem and then be more descriptive in the message.

deads2k · 2015-10-14T13:34:32Z

For congruence in the output, try: "route/route-edge exposes pod port 5432".

smarterclayton · 2015-10-14T14:40:59Z

I think it's more important to indicate the service is what is being
exposed. I'm less interested in the route name until I know why. In the
future we probably won't show the route name.

On Oct 14, 2015, at 9:34 AM, David Eads notifications@github.com wrote:

For congruence in the output, try: "route/route-edge exposes pod port 5432".

—
Reply to this email directly or view it on GitHub
#5089 (comment).

deads2k · 2015-10-14T14:45:14Z

I think it's more important to indicate the service is what is being
exposed. I'm less interested in the route name until I know why. In the
future we probably won't show the route name.

svc/frontend - 172.30.182.32:5432 -> 8080 (exposed) ? We could conditionally show the route line based on whether we have a problem?

smarterclayton · 2015-10-14T15:11:14Z

The message I suggested last indented below the service is the right thing
to be consistent with our other patterns. Putting the object type and name
first isn't a hard rule

On Wed, Oct 14, 2015 at 10:45 AM, David Eads notifications@github.com
wrote:

I think it's more important to indicate the service is what is being
exposed. I'm less interested in the route name until I know why. In the
future we probably won't show the route name.

svc/frontend - 172.30.182.32:5432 -> 8080 (exposed) ? We could
conditionally show the route line based on whether we have a problem?

—
Reply to this email directly or view it on GitHub
#5089 (comment).

deads2k · 2015-10-14T15:12:25Z

The message I suggested last indented below the service is the right thing
to be consistent with our other patterns. Putting the object type and name
first isn't a hard rule

It makes it hard to read.

0xmichalis · 2015-10-14T16:24:53Z

@deads2k comments addressed plus added warning about a missing service

[vagrant@openshiftdev sample-app]$ oc status
In project test on server https://localhost:8443

service/etcd-server - 172.30.13.65 ports 2380, 4001

service/external - 172.30.0.100 ports 443, 80->443

service/new - 172.30.26.177 ports 443, 80

Warnings:
  route/new2 is supposed to route traffic to service/new2 but service/new2 doesn't exist.

To see more, use 'oc describe <resource>/<name>'.
You can use 'oc get all' to see a list of other objects.

smarterclayton · 2015-10-14T21:58:05Z

I don't agree "exposed by route/foo on pod port 8080" is what I want to see

On Oct 14, 2015, at 11:12 AM, David Eads notifications@github.com wrote:

The message I suggested last indented below the service is the right thing
to be consistent with our other patterns. Putting the object type and name
first isn't a hard rule

It makes it hard to read.

—
Reply to this email directly or view it on GitHub
#5089 (comment).

0xmichalis · 2015-10-15T18:43:40Z

@deads2k comments addressed

smarterclayton · 2015-10-29T03:43:40Z

pkg/api/graph/graphview/service_group.go

+		}
+	}
+
+	for _, uncastServiceFulfiller := range g.PredecessorNodesByEdgeKind(serviceNode, routeedges.ExposedThroughRouteEdgeKind) {


@deads2k the names in this package are getting ridiculous.

smarterclayton · 2015-10-29T03:47:16Z

A few changes, revert service back to svc

smarterclayton · 2015-10-29T14:15:48Z

Project status is broken.

0xmichalis · 2015-10-29T14:29:56Z

I had to update resource strings in tests. PTAL

This commit adds graph pieces for routes and makes `oc status` report routes with missing route ports.

0xmichalis · 2015-10-29T15:48:11Z

@smarterclayton test added

openshift-bot · 2015-10-29T15:49:03Z

Evaluated for origin test up to e68fa2f

smarterclayton · 2015-10-29T15:50:45Z

LGTM [merge]

openshift-bot · 2015-10-29T15:57:05Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6380/) (Image: devenv-rhel7_2594)

openshift-bot · 2015-10-29T15:57:06Z

Evaluated for origin merge up to e68fa2f

Merged by openshift-bot

0xmichalis changed the title ~~status: Report routes that have no route port specified~~ [WIP] status: Report routes that have no route port specified Oct 12, 2015

0xmichalis changed the title ~~[WIP] status: Report routes that have no route port specified~~ status: Report routes that have no route port specified Oct 13, 2015

0xmichalis closed this Oct 14, 2015

0xmichalis reopened this Oct 14, 2015

deads2k reviewed Oct 14, 2015
View reviewed changes

0xmichalis closed this Oct 22, 2015

0xmichalis reopened this Oct 22, 2015

openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015

0xmichalis closed this Oct 23, 2015

0xmichalis reopened this Oct 23, 2015

smarterclayton reviewed Oct 29, 2015
View reviewed changes

status: Report routes that have no route port specified

e68fa2f

This commit adds graph pieces for routes and makes `oc status` report routes with missing route ports.

openshift-bot pushed a commit that referenced this pull request Oct 29, 2015

Merge pull request #5089 from kargakis/routes-oc-status

c0d2376

Merged by openshift-bot

openshift-bot merged commit c0d2376 into openshift:master Oct 29, 2015

0xmichalis deleted the routes-oc-status branch October 29, 2015 22:19

status: Report routes that have no route port specified #5089

status: Report routes that have no route port specified #5089

Conversation

0xmichalis commented Oct 12, 2015

0xmichalis commented Oct 12, 2015

0xmichalis commented Oct 12, 2015

deads2k commented Oct 12, 2015

0xmichalis commented Oct 12, 2015

0xmichalis commented Oct 13, 2015

smarterclayton commented Oct 13, 2015

0xmichalis commented Oct 13, 2015

0xmichalis commented Oct 13, 2015

openshift-bot commented Oct 13, 2015

smarterclayton commented Oct 13, 2015

0xmichalis commented Oct 13, 2015

danmcp commented Oct 13, 2015

0xmichalis commented Oct 13, 2015

0xmichalis commented Oct 13, 2015

danmcp commented Oct 13, 2015

0xmichalis commented Oct 14, 2015

deads2k Oct 14, 2015

Choose a reason for hiding this comment

deads2k Oct 14, 2015

Choose a reason for hiding this comment

deads2k commented Oct 14, 2015

smarterclayton commented Oct 14, 2015

deads2k commented Oct 14, 2015

smarterclayton commented Oct 14, 2015

deads2k commented Oct 14, 2015

0xmichalis commented Oct 14, 2015

smarterclayton commented Oct 14, 2015

0xmichalis commented Oct 15, 2015

smarterclayton Oct 29, 2015

Choose a reason for hiding this comment

smarterclayton commented Oct 29, 2015

smarterclayton commented Oct 29, 2015

0xmichalis commented Oct 29, 2015

0xmichalis commented Oct 29, 2015

openshift-bot commented Oct 29, 2015

smarterclayton commented Oct 29, 2015

openshift-bot commented Oct 29, 2015

openshift-bot commented Oct 29, 2015