Add events on app CRs #121

tomahawk28 · 2021-04-28T17:21:35Z

Toward https://github.com/giantswarm/giantswarm/issues/16981

There was a discussion between @fiunchinho on this, https://gigantic.slack.com/archives/C01P7P3BS2C/p1619601080015300
and I think it would be okay with app-admission-controller to emit events since,

it would be the single endpoint to validate app CRs.
It will emit events when it spots the differences between the specs.

WDYT?

Checklist

Update changelog in CHANGELOG.md.

tomahawk28 · 2021-04-28T17:45:49Z

Tested in ginger

Events:
  Type    Reason      Age   From                      Message
  ----    ------      ----  ----                      -------
  Normal  AppUpdated  54s   app-admission-controller  version has been changed to `1.1.1`
  Normal  AppUpdated  2s    app-admission-controller  version has been changed to `1.1.0`

tomahawk28 · 2021-04-29T09:12:48Z

Events:
  Type    Reason      Age    From                      Message
  ----    ------      ----   ----                      -------
  Normal  AppUpdated  3m2s   app-admission-controller  version has been changed to `1.1.0`
  Normal  AppUpdated  2m49s  app-admission-controller  version has been changed to `1.1.1`
  Normal  AppUpdated  1s     app-admission-controller  appConfigMap has been resetted

resetted is wrong, right?

tomahawk28 · 2021-04-29T09:13:32Z

pkg/app/validate_app.go

+		return microerror.Maskf(parsingFailedError, "unable to parse app: %#v", err)
+	}
+
+	compareFunc := map[string]func(v1alpha1.App) string{


I think having a list of compare functions like below is much tidy.

Yes makes sense but also could be useful in other places.

What about moving to the key package in the app library? Since it's using lots of key funcs anyway.

Also offtopic but the logging with namespace/name is nice.

I think @fiunchinho did that in apptest as well. We should improve the logging in app- and chart-operator too.

@rossf7 Do you mean moving compareFunc into the app library? But here I would like to show users about events that relevant to the user interface when we hide other changes (e.g. annotation, kubeconfig changes).

Both app-operator and chart-operator are using cmp.diff from the desired chart CRs, so there are not many places we could use this list other than app-admission-controller. So I'm inclined to stay this function here only, WDYT?

OK sure if we're not going to reuse it then it's more readable to have it here IMO. 👍

But the ordering is weird here. Can we have this in alpha order?

pkg/app/validate_app.go

tomahawk28 · 2021-04-29T09:17:43Z

pkg/app/validate_app.go

+		newValue := f(app)
+		if newValue != f(oldApp) {
+			if newValue == "/" {
+				v.event.Emit(ctx, &app, "AppUpdated", "%s has been resetted", name)


Need a little help here from 🇬🇧

fiunchinho · 2021-04-29T09:33:39Z

I think it looks good! nice job

internal/recorder/recorder.go

rossf7

@tomahawk28 This is cool thanks! 🚀

There was a discussion between @fiunchinho on this, https://gigantic.slack.com/archives/C01P7P3BS2C/p1619601080015300
and I think it would be okay with app-admission-controller to emit events since,

it would be the single endpoint to validate app CRs.
It will emit events when it spots the differences between the specs.

Emitting events from the admission controller makes sense but I would expect it's only serving webhooks. Can you update the README to cover that? For describing the webhooks what about linking to the docs?

My main concern is could we have duplicate events? Also what happens when we have a failure condition, like when chart-operator / helm is blocked by a webhook?

Could this generate a lot of events and impact the management cluster?
Can we have a metric for how many events are emitted?

pkg/app/validate_app.go

rossf7 · 2021-04-29T14:06:22Z

pkg/app/validate_app.go

+		return microerror.Maskf(parsingFailedError, "unable to parse app: %#v", err)
+	}
+
+	compareFunc := map[string]func(v1alpha1.App) string{


Yes makes sense but also could be useful in other places.

What about moving to the key package in the app library? Since it's using lots of key funcs anyway.

rossf7 · 2021-04-29T14:07:23Z

pkg/app/validate_app.go

+		return microerror.Maskf(parsingFailedError, "unable to parse app: %#v", err)
+	}
+
+	compareFunc := map[string]func(v1alpha1.App) string{


Also offtopic but the logging with namespace/name is nice.

I think @fiunchinho did that in apptest as well. We should improve the logging in app- and chart-operator too.

tomahawk28 · 2021-04-29T18:42:35Z

@rossf7 I checked the kube-state-metrics and it doesn't gather metrics about event resources as default.
https://github.com/kubernetes/kube-state-metrics/blob/master/docs/cli-arguments.md

So we either need to enable it from kube-state-metrics app or gather metrics ourselves. But I doubt we need to do it actually given the fact that the event resources have a 1-hour lifetime as default.
kubernetes/kubernetes#52521

Moreover, app-admission-controller emits events right just before it would admit app CRs' changes and only if it sees the actual difference between old and new spec. (e.g. change from the .spec.version)
So I don't think the number of events would be increased by these changes in any cases.

If you still want to have a metric on these events, do you perhaps know any metrics for event resources other than kube-state-metrics?

rossf7 · 2021-04-30T08:04:04Z

If you still want to have a metric on these events, do you perhaps know any metrics for event resources other than kube-state-metrics?

@tomahawk28 That's a shame kube-state-metrics doesn't gather this. But the admission controller emits its own metrics so I think we could add our own.

https://github.com/giantswarm/app-admission-controller/blob/867987245b295d4a728a9708663b4ea520ea6efc/pkg/metrics/metrics.go

But I doubt we need to do it actually given the fact that the event resources have a 1-hour lifetime as default.

Yes, that's a good point. Let's see how it goes. We can add a custom metric later if we need one.

rossf7

@tomahawk28 Thanks for the changes. Just some final 🇬🇧 nits then this looks good.

docs/events.md

internal/recorder/recorder.go

pkg/app/validate_app.go

rossf7 · 2021-04-30T09:41:33Z

pkg/app/validate_app.go

+		return microerror.Maskf(parsingFailedError, "unable to parse app: %#v", err)
+	}
+
+	compareFunc := map[string]func(v1alpha1.App) string{


OK sure if we're not going to reuse it then it's more readable to have it here IMO. 👍

But the ordering is weird here. Can we have this in alpha order?

rossf7 · 2021-04-30T09:42:24Z

pkg/app/validate_app.go

+		}
+
+		if newValue == "/" {
+			v.event.Emit(ctx, &app, "AppUpdated", "%s has been reset", name)


It's already fixed here. 😄

Co-authored-by: Ross Fairbanks <rossf7@users.noreply.github.com>

tomahawk28 · 2021-04-30T11:48:28Z

README.md


-See [docs/tests.md](https://github.com/giantswarm/app-admission-controller/blob/master/docs/tests.md)


Just a note; I'm deleting this tests.md, since we don't have this document.

rossf7

LGTM once typos in the doc are fixed

rossf7 · 2021-04-30T11:54:51Z

docs/events.md

+At the end of each successful validation of an app CR, app-admission-controller checks the changes for the following fields and emits an event.
+
+- `.spec.version` 
+- `.sepc.catalog`


These ones still have the typo.

Jihyuk Bok added 2 commits April 28, 2021 19:20

add new internal service

d6071fa

wired

e209ab1

tomahawk28 self-assigned this Apr 28, 2021

Jihyuk Bok added 3 commits April 28, 2021 19:24

sort out imports

616063d

fix go mod

a8b9b3a

add rbac

52af0b5

reset values

ec8eda5

tomahawk28 requested a deployment to ginger April 29, 2021 09:00 Abandoned

tomahawk28 commented Apr 29, 2021

View reviewed changes

pkg/app/validate_app.go Show resolved Hide resolved

tomahawk28 marked this pull request as ready for review April 29, 2021 09:16

tomahawk28 requested a review from a team April 29, 2021 09:16

tomahawk28 commented Apr 29, 2021

View reviewed changes

fiunchinho reviewed Apr 29, 2021

View reviewed changes

internal/recorder/recorder.go Outdated Show resolved Hide resolved

Jihyuk Bok added 3 commits April 29, 2021 13:49

change the struct name

a19354a

fix typo

e625127

tidy up

3182c2d

rossf7 reviewed Apr 29, 2021

View reviewed changes

Jihyuk Bok added 2 commits April 30, 2021 10:38

add a comment before emitting

f55c9aa

add docs

7a37e46

rossf7 reviewed Apr 30, 2021

View reviewed changes

Jihyuk Bok and others added 3 commits April 30, 2021 13:17

ordering functions

7c9aa15

Apply suggestions from code review

a0abca3

Co-authored-by: Ross Fairbanks <rossf7@users.noreply.github.com>

fix an error

fa29609

tomahawk28 commented Apr 30, 2021

View reviewed changes

add CHANGELOG.md

d1892f2

rossf7 approved these changes Apr 30, 2021

View reviewed changes

update typo

98afd93

tomahawk28 enabled auto-merge (squash) April 30, 2021 12:00

tomahawk28 merged commit 4326bb7 into master Apr 30, 2021

tomahawk28 deleted the request-update-events branch April 30, 2021 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add events on app CRs #121

Add events on app CRs #121

tomahawk28 commented Apr 28, 2021 •

edited

tomahawk28 commented Apr 28, 2021

tomahawk28 commented Apr 29, 2021

tomahawk28 Apr 29, 2021

rossf7 Apr 29, 2021

rossf7 Apr 29, 2021

tomahawk28 Apr 30, 2021

rossf7 Apr 30, 2021

tomahawk28 Apr 29, 2021

fiunchinho commented Apr 29, 2021

rossf7 left a comment

rossf7 Apr 29, 2021

rossf7 Apr 29, 2021

tomahawk28 commented Apr 29, 2021 •

edited

rossf7 commented Apr 30, 2021

rossf7 left a comment

rossf7 Apr 30, 2021

rossf7 Apr 30, 2021

tomahawk28 Apr 30, 2021

rossf7 left a comment

rossf7 Apr 30, 2021


		See [docs/tests.md](https://github.com/giantswarm/app-admission-controller/blob/master/docs/tests.md)

Add events on app CRs #121

Add events on app CRs #121

Conversation

tomahawk28 commented Apr 28, 2021 • edited

Checklist

tomahawk28 commented Apr 28, 2021

tomahawk28 commented Apr 29, 2021

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

fiunchinho commented Apr 29, 2021

rossf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomahawk28 commented Apr 29, 2021 • edited

rossf7 commented Apr 30, 2021

rossf7 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

rossf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomahawk28 commented Apr 28, 2021 •

edited

tomahawk28 commented Apr 29, 2021 •

edited