Adds events for route #358

yanweiguo · 2018-03-13T23:09:04Z

Adds following events:

Route:
- Create service completes/fails
- Create Ingress completes/fails
- Create Istio route rule complets/fails
- Update route traffic targets completes/fails
- Receive invalid configuration(route label already set to another route)

Sample output:

$ kubectl describe route/route-example

...
Events:
  Type     Reason          Age                 From              Message
  ----     ------          ----                ----              -------
  Normal   Created         4m                 route-controller  Created service 'route-example-service'
  Normal   Created         4m                 route-controller  Created Ingress 'route-example-ela-ingress'
  Warning  CreationFailed  4m                 route-controller  Failed to create Istio route rule 'route-example-istio': routerules.config.istio.io "route-example-istio" already exists
  Normal   Created         4m                 route-controller  Created Istio route rule 'route-example-istio'
  Warning  UpdateFailed    4m                 route-controller  Failed to update Istio route rule 'route-example-istio': Operation cannot be fulfilled on routerules.config.istio.io "route-example-istio": the object has been modified; please apply your changes to the latest version and try again
  Normal   Updated         2m (x6 over 4m)  route-controller  Updated status for route 'route-example'
  Normal   Updated         4m (x8 over 4m)   route-controller  Updated Istio route rule 'route-example-istio'

Also improves the tests for controllers:

Adds the missing events tests for revision and configuration
Adds the missing tests for revision becomes ready upon its endpoints become ready
Adds/Refactors "NoMoreEvent" tests following the test structure in Do not update configuration is LatestReadyRevisionName is up to date #329(instead of using sleep)

Closes #8 #12

yanweiguo · 2018-03-13T23:27:38Z

/retest

mattmoor · 2018-03-14T13:43:00Z

pkg/controller/route/controller.go

@@ -367,7 +353,7 @@ func (c *Controller) syncTrafficTargets(route *v1alpha1.Route) (*v1alpha1.Route,
 	}

 	// Then create the actual route rules.
-	glog.Infof("Creating istio route rules")
+	glog.Infof("Creating Istio route rules")


nit: no format args

mattmoor · 2018-03-14T13:44:26Z

pkg/controller/route/controller.go

@@ -389,26 +375,33 @@ func (c *Controller) syncTrafficTargets(route *v1alpha1.Route) (*v1alpha1.Route,
 	updated, err := c.updateStatus(route)
 	if err != nil {
 		glog.Warningf("Failed to update service status: %s", err)
+		c.recorder.Event(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update traffic targets")


@vaikas-google should we surface err here?

Probably, makes sense, otherwise the user knows something went wrong, but not what. We should also add details here about which route failed or succeeded. Otherwise, messages of the form (from the PR description):
Normal Updated 29s (x6 over 1m) route-controller Updated traffic targets

don't provide enough information to figure out what happened.

Added the err.

mattmoor · 2018-03-14T13:46:28Z

pkg/controller/route/controller.go

 	if err != nil {
 		if !apierrs.IsAlreadyExists(err) {
 			glog.Infof("Failed to create service: %s", err)
+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create service: %s", service.Name)


Include err here unless @vaikas-google thinks that's inappropriate.

mattmoor · 2018-03-14T13:51:39Z

pkg/controller/route/controller.go

@@ -428,8 +421,13 @@ func (c *Controller) createOrUpdateIngress(route *v1alpha1.Route, ns string) err
 			return err
 		}
 		_, createErr := ic.Create(ingress)
+		if createErr != nil {
+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %s", ingress.Name)


Include err?

mattmoor · 2018-03-14T13:52:03Z

pkg/controller/route/controller.go

@@ -428,8 +421,13 @@ func (c *Controller) createOrUpdateIngress(route *v1alpha1.Route, ns string) err
 			return err
 		}
 		_, createErr := ic.Create(ingress)
+		if createErr != nil {


Prefer: if _, err := ic.Create(); err != nil {

There are a bunch of these if you are up for changing them :)

mattmoor · 2018-03-14T13:58:01Z

pkg/controller/route/controller.go

@@ -627,15 +627,21 @@ func (c *Controller) createOrUpdateRoutes(route *v1alpha1.Route, configMap map[s
 			return nil, err
 		}
 		routeRules = MakeRouteIstioRoutes(route, ns, revisionRoutes)
-		_, createErr := routeClient.Create(routeRules)
-		return revisionRoutes, createErr
+		if _, createErr := routeClient.Create(routeRules); createErr != nil {


nit: use err

mattmoor · 2018-03-14T13:58:16Z

pkg/controller/route/controller.go

-		_, createErr := routeClient.Create(routeRules)
-		return revisionRoutes, createErr
+		if _, createErr := routeClient.Create(routeRules); createErr != nil {
+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Istio route rule: %s", routeRules.Name)


include err?

mattmoor · 2018-03-14T13:58:58Z

pkg/controller/route/controller.go

 	}

 	routeRules.Spec = MakeRouteIstioSpec(route, ns, revisionRoutes)
 	_, err = routeClient.Update(routeRules)
-	if err != nil {
+	if _, err := routeClient.Update(routeRules); err != nil {
+		c.recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update Istio route rule: %s", routeRules.Name)


include err?

mattmoor · 2018-03-14T14:01:59Z

pkg/controller/route/controller.go

 		return nil, err
 	}
+	c.recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated Istio route rule: %s", routeRules.Name)


I'd like to better understand the wisdom of the parameter in the "Updated" position.

Is this purely informational (terse) or expected to be more like an enumeration that clients can operate on programmatically? I'm trying to capture some of this for Status.Condition convention here, but am not sure what the wisdom is for events?

TBH, IDK neither. From the Event source code, it says Reason is machine understandable string that gives the reason for the transition into the object's current status.

Here are some samples of events created by k8s:

7m 7m 1 p-79de88cc-3791-4370-b6c0-5197d2cd1705-autoscaler-d7c5c6554z2rn.151be65f06790b80 Pod spec.containers{autoscaler} Normal Pulled kubelet, gke-elafros-demo-default-pool-8d4847e8-xw93 Container image "gcr.io/yanweiguo-test/ela-autoscaler@sha256:ffc20211794a1ea1298a6584797db4f5317b892c0970bd8129e65d0b68542a2a" already present on machine 7m 7m 1 p-79de88cc-3791-4370-b6c0-5197d2cd1705-autoscaler-d7c5c6554z2rn.151be65f09dc92c2 Pod spec.containers{autoscaler} Normal Created kubelet, gke-elafros-demo-default-pool-8d4847e8-xw93 Created container 7m 7m 1 p-79de88cc-3791-4370-b6c0-5197d2cd1705-autoscaler-d7c5c6554z2rn.151be65f10809f19 Pod spec.containers{autoscaler} Normal Started kubelet, gke-elafros-demo-default-pool-8d4847e8-xw93

I just followed that to use Created or Updated.

mattmoor · 2018-03-14T14:02:43Z

pkg/controller/route/controller.go

+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %s", ingress.Name)
+			return createErr
+		}
+		c.recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created Ingress: %s", ingress.Name)


Should we make this "IngressCreated"? IDK what's typical here.

vaikas · 2018-03-14T18:12:57Z

pkg/controller/route/controller.go

@@ -389,26 +375,33 @@ func (c *Controller) syncTrafficTargets(route *v1alpha1.Route) (*v1alpha1.Route,
 	updated, err := c.updateStatus(route)
 	if err != nil {
 		glog.Warningf("Failed to update service status: %s", err)
+		c.recorder.Event(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update traffic targets")


Probably, makes sense, otherwise the user knows something went wrong, but not what. We should also add details here about which route failed or succeeded. Otherwise, messages of the form (from the PR description):
Normal Updated 29s (x6 over 1m) route-controller Updated traffic targets

don't provide enough information to figure out what happened.

vaikas · 2018-03-14T18:13:56Z

pkg/controller/route/controller.go

 		return nil, err
 	}
-
+	c.recorder.Event(route, corev1.EventTypeNormal, "Updated", "Updated traffic targets")


Could you add the route name to all these messages. The other messages are nice because you can see what happened and to which object.

vaikas · 2018-03-14T18:14:13Z

pkg/controller/route/controller.go

@@ -428,8 +421,13 @@ func (c *Controller) createOrUpdateIngress(route *v1alpha1.Route, ns string) err
 			return err
 		}
 		_, createErr := ic.Create(ingress)
+		if createErr != nil {


vaikas · 2018-03-14T18:23:50Z

pkg/controller/route/controller.go

 					config.Name, ela.RouteLabelKey, routeName)
+				c.recorder.Event(route, corev1.EventTypeWarning, "InvalidConfiguration", errMsg)
+				return errors.New(errMsg)


I don't actually know what the best practices here really are. I think logs/events have different retain policies and you could maybe have different rbac rules, etc. Perhaps we should file an issue on this.
At the very least we should clean up our logs:
#361

vaikas

Looks great! Thanks for doing this, the one request is global replace of '%s' with %q for fmt. arguments.

vaikas · 2018-03-14T22:35:07Z

pkg/controller/configuration/controller.go

@@ -294,11 +294,11 @@ func (c *Controller) syncHandler(key string) error {
 		created, err := c.elaclientset.BuildV1alpha1().Builds(build.Namespace).Create(build)
 		if err != nil {
 			glog.Errorf("Failed to create Build:\n%+v\n%s", build, err)
-			c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Build: %s", build.Name)
+			c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Build '%s': %s", build.Name, err)


Instead of '%s' could you change these to %q
https://golang.org/pkg/fmt/

%q a double-quoted string safely escaped with Go syntax

Done. Good to know this. Thanks!

vaikas · 2018-03-14T22:36:00Z

pkg/controller/configuration/controller.go

 			return err
 		}
 		glog.Infof("Created Build:\n%+v", created.Name)
-		c.recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build: %s", created.Name)
+		c.recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build '%s'", created.Name)


Same here and elsewhere where you use '%s' format.

mattmoor · 2018-03-14T23:33:55Z

pkg/controller/configuration/controller.go

@@ -209,12 +209,12 @@ func (c *Controller) processNextWorkItem() bool {
 		// Run the syncHandler, passing it the namespace/name string of the
 		// Foo resource to be synced.
 		if err := c.syncHandler(key); err != nil {
-			return fmt.Errorf("error syncing '%s': %s", key, err.Error())
+			return fmt.Errorf("error syncing %q: %s", key, err.Error())


This shouldn't use err.Error(), but should instead use %v and just err.

mattmoor · 2018-03-14T23:34:31Z

pkg/controller/configuration/controller.go

@@ -294,11 +294,11 @@ func (c *Controller) syncHandler(key string) error {
 		created, err := c.elaclientset.BuildV1alpha1().Builds(build.Namespace).Create(build)
 		if err != nil {
 			glog.Errorf("Failed to create Build:\n%+v\n%s", build, err)
-			c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Build: %s", build.Name)
+			c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Build %q: %s", build.Name, err)


use %v for err

mattmoor · 2018-03-14T23:34:39Z

pkg/controller/configuration/controller.go

@@ -328,10 +328,10 @@ func (c *Controller) syncHandler(key string) error {
 	created, err := c.elaclientset.ElafrosV1alpha1().Revisions(config.Namespace).Create(rev)
 	if err != nil {
 		glog.Errorf("Failed to create Revision:\n%+v\n%s", rev, err)
-		c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision: %s", rev.Name)
+		c.recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision %q: %s", rev.Name, err)


mattmoor · 2018-03-14T23:35:20Z

pkg/controller/route/controller.go

@@ -375,10 +375,10 @@ func (c *Controller) syncTrafficTargets(route *v1alpha1.Route) (*v1alpha1.Route,
 	updated, err := c.updateStatus(route)
 	if err != nil {
 		glog.Warningf("Failed to update service status: %s", err)
-		c.recorder.Event(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update traffic targets")
+		c.recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for route '%s': %s", route.Name, err)


%q and %v

mattmoor · 2018-03-14T23:35:31Z

pkg/controller/route/controller.go

 		return nil, err
 	}
-	c.recorder.Event(route, corev1.EventTypeNormal, "Updated", "Updated traffic targets")
+	c.recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated status for route '%s'", route.Name)


mattmoor · 2018-03-14T23:44:30Z

pkg/controller/route/controller.go

 		return nil, err
 	}
-
+	c.recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated status for route '%s'", route.Name)


mattmoor · 2018-03-14T23:44:40Z

pkg/controller/route/controller.go

 		if !apierrs.IsAlreadyExists(err) {
 			glog.Infof("Failed to create service: %s", err)
+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create service '%s': %s", service.Name, err)


%q and %v

mattmoor · 2018-03-14T23:44:47Z

pkg/controller/route/controller.go

 	}
 	glog.Infof("Created service: %q", service.Name)
+	if newlyCreated {
+		c.recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created service '%s'", service.Name)


mattmoor · 2018-03-14T23:44:56Z

pkg/controller/route/controller.go

 		if !apierrs.IsNotFound(err) {
 			return err
 		}
-		_, createErr := ic.Create(ingress)
+		if _, err := ic.Create(ingress); err != nil {
+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress '%s': %s", ingress.Name, err)


%q and %v

mattmoor · 2018-03-14T23:45:03Z

pkg/controller/route/controller.go

+			c.recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress '%s': %s", ingress.Name, err)
+			return err
+		}
+		c.recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created Ingress '%s'", ingress.Name)


yanweiguo · 2018-03-15T00:29:11Z

/retest

mattmoor

thanks for bearing with my tidal wave of nits :)

I spotted one typo, but otherwise this LGTM.
-M

mattmoor · 2018-03-15T04:20:09Z

pkg/controller/revision/controller.go

@@ -277,12 +277,12 @@ func (c *Controller) processNextWorkItem() bool {
 		// Run the syncHandler, passing it the namespace/name string of the
 		// Foo resource to be synced.
 		if err := c.syncHandler(key); err != nil {
-			return fmt.Errorf("error syncing '%s': %s", key, err.Error()), controller.PromLabelValueFailure
+			return fmt.Errorf("error syncing %q: %vs", key, err), controller.PromLabelValueFailure


typo: %vs

mattmoor · 2018-03-15T04:21:55Z

pkg/controller/revision/controller_test.go

@@ -625,7 +625,7 @@ func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) {
 	// Look for the revision ready event.
 	h.OnCreate(&kubeClient.Fake, "events", func(obj runtime.Object) hooks.HookResult {
 		event := obj.(*corev1.Event)
-		expectedMessage := "Revision becomes ready upon endpoint 'test-endpoints' becoming ready"
+		expectedMessage := "Revision becomes ready upon endpoint \"test-endpoints\" becoming ready"


No change needed, but FYI: if you use `Strings "like" this` you can avoid escaping.

Good to know. Thanks.

mattmoor · 2018-03-15T04:23:50Z

/retest

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

yanweiguo added 8 commits March 13, 2018 11:04

add events

a39180a

change events in test

887111a

merge branch 'master' into add_events_for_route

b1247d9

add test for invalide configuration

5b591e7

add tests for revision and configuraiton

e6316f6

add mark revision ready tests

300a7e4

send create service event for the first time

fa32ac9

combine if

1cfe12c

google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2018

yanweiguo requested review from grantr, mattmoor and ZhiminXiang March 13, 2018 23:09

mattmoor reviewed Mar 14, 2018

View reviewed changes

vaikas reviewed Mar 14, 2018

View reviewed changes

yanweiguo added 2 commits March 14, 2018 11:57

expose err detail in events

e2f3baa

add a test and remove todo

ea4bc75

google-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 14, 2018

vaikas reviewed Mar 14, 2018

View reviewed changes

replace '%s' to %q

945832d

mattmoor suggested changes Mar 14, 2018

View reviewed changes

yanweiguo added 3 commits March 14, 2018 17:09

replace %s with %v for error

46b48ab

address some more comments

9c70979

fix tests

031b5ed

mattmoor approved these changes Mar 15, 2018

View reviewed changes

fix typo

67912d0

vaikas approved these changes Mar 15, 2018

View reviewed changes

yanweiguo merged commit 6ee4002 into knative:master Mar 15, 2018

yanweiguo deleted the add_events_for_route branch March 15, 2018 16:46

mattmoor mentioned this pull request Mar 22, 2018

Add Build Events knative/build#95

Closed

ReToCode pushed a commit to ReToCode/serving that referenced this pull request Aug 3, 2023

Remove podspec-volumes-emptydir toggle (knative#358)

35789c9

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

Adds events for route #358

Adds events for route #358

Conversation

yanweiguo commented Mar 13, 2018 • edited Loading

yanweiguo commented Mar 13, 2018

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

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

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

vaikas left a 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

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

yanweiguo commented Mar 15, 2018

mattmoor left a 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

mattmoor commented Mar 15, 2018

yanweiguo commented Mar 13, 2018 •

edited

Loading