BUILD-187: Detect if image trigger ID was cleared #150

adambkaplan · 2020-11-18T15:39:07Z

Detect if the lastTriggeredImageID was cleared in a BuildConfig.
If an update cleared the last triggered image, record a warning event.

A metric and associated alert were considered to better inform users
that deprecated behavior is being triggered. However, this was rejected
because the metric would require unbound time series cardinality.

The behavior that triggers builds off of spec will be removed in BUILD-188 [1].

[1] https://issues.redhat.com/browse/BUILD-188

ingvagabund · 2020-11-30T14:02:45Z

/test e2e-aws

adambkaplan

@openshift/openshift-team-monitoring I discussed this a bit with @lilic on slack.

We want to inform cluster admins that some action on the cluster is triggering behavior that will be deprecated in the future. Design initially calls for an Alert to be fired, driven by the metric here. This metric started as a simple counter, but per our discussion a Gauge was determined to be a better fit. To appropriately track if this behavior is being triggered or not, however, we need to use a vector with namespace and name as labels. This creates an unbound time series (m namespaces * n BuildConfig objects per namespace).

One alternative we discussed is foregoing the alert completely, and rely only on the event.

Are we abusing the alert system in this case?

adambkaplan · 2020-12-01T16:04:45Z

pkg/build/metrics/prometheus/metrics.go

+// MarkImageChangeTriggerCleared sets the openshift_build_image_change_trigger_cleared gague value.
+// A value of 1 indicates the LastTriggeredImageID field on a BuildConfig has been reset to empty.
+func MarkImageChangeTriggerCleared(namespace, name string, cleared bool) {
+	value := float64(0)
+	if cleared {
+		value = float64(1)
+	}
+	triggerClearedCounter.WithLabelValues(namespace, name).Set(value)
+}
+


Here we set the metric value to 0 or 1 - eventually every BuildConfig in the cluster will have a value set.

adambkaplan · 2020-12-01T16:05:55Z

pkg/build/controller/buildconfig/buildconfig_controller.go

+	for _, currentTrigger := range cur.Spec.Triggers {
+		if currentTrigger.ImageChange == nil {
+			continue
+		}
+		from := c.getImageChangeTriggerInputReference(cur, currentTrigger)
+		prev, found := imageLastTriggers[*from]
+		if found && len(prev) > 0 && len(currentTrigger.ImageChange.LastTriggeredImageID) == 0 {
+			return true
+		}


A lot of the logic here was lifted from the ImageChange trigger controller code.

adambkaplan · 2020-12-01T16:09:21Z

pkg/build/controller/buildconfig/buildconfig_controller.go

+			"ImageChangeTriggerCleared",
+			"spec.triggers[*].imagechange.lastTriggeredImageID was cleared, which will trigger a build. This behavior is deprecated and will be removed in a future OpenShift release.")
+	} else {
+		prometheus.MarkImageChangeTriggerCleared(bc.Namespace, bc.Name, false)


Perhaps we can be a bit more intelligent and only mark cleared to false if:

BuildConfig has a "valid" ImageChange trigger (unchanged, updates from empty -> sha, or sha -> sha)

BuildConfig previously had an ImageChange trigger, but it was removed.

In a real cluster, this would significantly cut down the time series size.

adambkaplan · 2020-12-01T16:11:26Z

pkg/build/controller/buildconfig/buildconfig_controller.go

+		c.recorder.Event(bc,
+			corev1.EventTypeWarning,
+			"ImageChangeTriggerCleared",
+			"spec.triggers[*].imagechange.lastTriggeredImageID was cleared, which will trigger a build. This behavior is deprecated and will be removed in a future OpenShift release.")


Record a warning event. This will bubble up to the web console and reference the BuildConfig.

simonpasquier

From a monitoring standpoint, I agree with @lilic that the use case is better suited for events rather than metrics given the unbounded cardinality.

adambkaplan · 2020-12-03T14:30:20Z

/hold

Will add an e2e test in origin before merging.

adambkaplan · 2020-12-03T17:45:07Z

Event as it appears in the admin console

adambkaplan · 2020-12-03T19:28:38Z

PR to verify the behavior: openshift/origin#25739

adambkaplan · 2020-12-04T19:16:48Z

/hold cancel

e2e test is in origin

gabemontero

couple of minor comments/suggestions

pkg/build/controller/buildconfig/buildconfig_controller.go

gabemontero · 2020-12-04T19:22:39Z

pkg/build/controller/buildconfig/buildconfig_controller.go

+		}
+		from := c.getImageChangeTriggerInputReference(cur, currentTrigger)
+		prev, found := imageLastTriggers[*from]
+		if found && len(prev) > 0 && len(currentTrigger.ImageChange.LastTriggeredImageID) == 0 {


do we care if the devops user set it to some non-zero "dummy" value vs. clearing it out

i.e. alert that they should not be setting it in any fashion

I think we care less about this scenario, but is something we can check. If the current is an invalid image ref, we have a pretty good idea that the image trigger controller didn't do this.

Actually, I don't think we can reliably do this other than with the empty string. "foo" is a perfectly valid pull spec - otherwise would would need to assume that the image trigger always adds the full SHA reference to this field.

the validity of the image ref was not a much a thought for me as the fact that they really should not be setting it at all IMO

that said, upon further review, it could get kludgy at best figuring out if a user set it vs. the controller set it

that, and don't feel strongly about it really anyway ... so let's move on .... an edge case at most, if a concern at all, so we can wait for it to crop up in a customer case before worrying about it at most

Detect if the lastTriggeredImageID was cleared in a BuildConfig. If an update cleared the last triggered image, record a warning event. A metric and associated alert were considered to better inform users that deprecated behavior is being triggered. However, this was rejected because the metric would require unbound time series cardinality. The behavior that triggers builds off of `spec` will be removed in BUILD-188 [1]. [1] https://issues.redhat.com/browse/BUILD-188

gabemontero · 2020-12-04T22:39:43Z

/lgtm

/retest

openshift-ci-robot · 2020-12-04T22:40:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-12-05T06:46:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-12-05T07:37:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2020

openshift-ci-robot requested review from gabemontero and soltysh November 18, 2020 15:39

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020

adambkaplan commented Dec 1, 2020

View reviewed changes

simonpasquier reviewed Dec 1, 2020

View reviewed changes

adambkaplan force-pushed the alert-imagetrigger branch from a6998c7 to 6f6d6e2 Compare December 3, 2020 14:28

adambkaplan changed the title ~~WIP - BUILD-187: Detect if image trigger ID was cleared~~ BUILD-187: Detect if image trigger ID was cleared Dec 3, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2020

gabemontero reviewed Dec 4, 2020

View reviewed changes

adambkaplan force-pushed the alert-imagetrigger branch from 6f6d6e2 to 7d133c1 Compare December 4, 2020 20:29

openshift-ci-robot assigned gabemontero Dec 4, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020

openshift-merge-robot merged commit 6290112 into openshift:master Dec 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUILD-187: Detect if image trigger ID was cleared #150

BUILD-187: Detect if image trigger ID was cleared #150

adambkaplan commented Nov 18, 2020 •

edited by openshift-ci-robot

ingvagabund commented Nov 30, 2020

adambkaplan left a comment

adambkaplan Dec 1, 2020

adambkaplan Dec 1, 2020

adambkaplan Dec 1, 2020

adambkaplan Dec 1, 2020

simonpasquier left a comment

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 4, 2020

gabemontero left a comment

gabemontero Dec 4, 2020

adambkaplan Dec 4, 2020

adambkaplan Dec 4, 2020

gabemontero Dec 4, 2020

gabemontero commented Dec 4, 2020

openshift-ci-robot commented Dec 4, 2020

openshift-bot commented Dec 5, 2020

openshift-bot commented Dec 5, 2020

BUILD-187: Detect if image trigger ID was cleared #150

BUILD-187: Detect if image trigger ID was cleared #150

Conversation

adambkaplan commented Nov 18, 2020 • edited by openshift-ci-robot

ingvagabund commented Nov 30, 2020

adambkaplan 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

simonpasquier left a comment

Choose a reason for hiding this comment

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 3, 2020

adambkaplan commented Dec 4, 2020

gabemontero 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

gabemontero commented Dec 4, 2020

openshift-ci-robot commented Dec 4, 2020

openshift-bot commented Dec 5, 2020

openshift-bot commented Dec 5, 2020

adambkaplan commented Nov 18, 2020 •

edited by openshift-ci-robot