WIP : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address. #19286

RobertKielty · 2020-09-21T10:09:10Z

Some WIP on CI Signal Alert Policy enforcement

CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address.

Fixes #18599

dims · 2020-09-23T20:33:16Z

/uncc

RobertKielty · 2020-10-23T08:58:30Z

/retest

k8s-ci-robot · 2020-10-23T13:27:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RobertKielty
To complete the pull request process, please assign dims after the PR has been reviewed.
You can assign the PR to them by writing /assign @dims in a comment when ready.

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

Needs approval from an approver in each of these files:

config/OWNERS

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

k8s-ci-robot · 2020-10-23T13:41:41Z

@RobertKielty: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-test-infra-bazel	`63e3353`	link	`/test pull-test-infra-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

RobertKielty · 2020-10-23T13:51:37Z

@spiffxp @BenTheElder @kubernetes/ci-signal when y'all get a chance can you review this?

I just fixed up the PR so that it is go fmt'd properly 🙄 apols for my numptyness!

Can't remember why I marked this as WIP?

Is this change useful/correct as-is or do we need to do more on this?

The following run (on my feature branch) looks like ...

Test output in config_tests.txt on the below local invocation is available for review here

$ bazel test //config/tests/testgrids:go_default_test > config_tests.txt
Loading: 
Loading: 0 packages loaded
Analyzing: target //config/tests/testgrids:go_default_test (0 packages loaded, 0 targets configured)
INFO: Analyzed target //config/tests/testgrids:go_default_test (1 packages loaded, 1230 targets configured).

INFO: Found 1 test target...
[0 / 1] [Prepa] BazelWorkspaceStatusAction stable-status.txt
[1,270 / 1,271] Testing //config/tests/testgrids:go_default_test; 1s linux-sandbox
FAIL: //config/tests/testgrids:go_default_test (see /home/rkielty/.cache/bazel/_bazel_rkielty/47d03368e29206548a86533381c34b95/execroot/io_k8s_test_infra/bazel-out/k8-fastbuild/testlogs/config/tests/testgrids/go_default_test/test.log)
[1,270 / 1,271] 1 / 1 tests, 1 failed; Testing //config/tests/testgrids:go_default_test; 7s linux-sandbox
INFO: From Testing //config/tests/testgrids:go_default_test:
Target //config/tests/testgrids:go_default_test up-to-date:
  bazel-bin/config/tests/testgrids/linux_amd64_race_stripped/go_default_test
INFO: Elapsed time: 13.867s, Critical Path: 8.16s
INFO: 1 process: 1 linux-sandbox.
INFO: Build completed, 1 test FAILED, 2 total actions
INFO: Build completed, 1 test FAILED, 2 total actions

No need for any of ^^^ that you can see the tests run on this PR.

RobertKielty · 2020-10-23T13:54:31Z

You can see the test in action on this PR of course :)

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/19286/pull-test-infra-bazel/1319631568897052672

spiffxp

Some suggestions

spiffxp · 2020-11-03T18:55:09Z

config/tests/testgrids/config_test.go

+// WIP See policy description on GH Issue #18599
+func TestJobsMustHaveTestGridAlertsSetup(t *testing.T) {


So the policy in #18599 is for all prow.k8s.io jobs, which is a subset of jobs that have results on testgrid.k8s.io:

I would rename this to something like TestKubernetesProwInstanceJobsMustHaveTestgridAlerts

you could also consider folding this into TestKubernetesProwInstanceJobsMustHaveMatchingTestgridEntries above

iterating over dashboard -> tab is fine, but you should skip tabs whose testgroup isn't known to come from the kubernetes prow instance (see prowGcsPrefix usage above)

spiffxp · 2020-11-03T18:55:39Z

config/tests/testgrids/config_test.go

+	for _, dashboard := range cfg.Dashboards {
+		for _, dashboardtab := range dashboard.DashboardTab {
+			if dashboardtab.GetAlertOptions() == nil {
+				t.Errorf("FAIL : %s : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address.", dashboardtab.GetName())


We should also consider how to do progressive enforcement. Adding a test that fails everything right away is going to block unrelated work.

for previous policies, I named the tests Should instead of Must, and did t.Logf instead of t.Errorf (you can also see something like this in TestReleaseBlockingJobsMustHaveTestgridDescriptions below

we could also consider using a list of legacy exemptions, then setting a goal of driving that down to zero (see usage of noPresubmitsInTestgridPrefixes above)

Dashboard tab names are not unique. Consider instead logging dashboard/tab (see TestReleaseBlockingJobsMustHaveTestgridDescriptions for an example)

spiffxp · 2020-11-03T19:03:17Z

config/tests/testgrids/config_test.go

+// WIP subtest version - no real advantage ideally I would have liked subtests
+// to register as counted failures. AFAIK no idiomatic golang test way of
+// getting subtest stats.


I'm not 100% clear on this either, don't have time to dig in. However I feel like you could gather stats and log them if you'd like ("x / y failing policy") I also think test-case-per-dashboard means hundreds or thousands of tests, which probably wouldn't make sense unless each test covered all policies.

fejta-bot · 2021-02-02T20:55:16Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

RobertKielty · 2021-02-17T06:46:38Z

/remove-lifecycle stale

fejta-bot · 2021-05-18T07:10:23Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

fejta-bot · 2021-06-17T07:33:08Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

fejta-bot · 2021-07-17T08:05:36Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

k8s-ci-robot · 2021-07-17T08:05:41Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

WIP adds test policy violation - no alert on DashboardTab

031b7e5

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 21, 2020

k8s-ci-robot requested review from dims and spiffxp September 21, 2020 10:09

k8s-ci-robot added area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 21, 2020

k8s-ci-robot removed the request for review from dims September 23, 2020 20:33

fixes go fmt error

63e3353

spiffxp suggested changes Nov 3, 2020

View reviewed changes

k8s-ci-robot assigned spiffxp Nov 3, 2020

spiffxp mentioned this pull request Nov 3, 2020

Kubernetes CI Policy: all prow.k8s.io jobs must have testgrid alerts configured #18599

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 2, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 17, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 18, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 17, 2021

k8s-ci-robot closed this Jul 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address. #19286

WIP : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address. #19286

RobertKielty commented Sep 21, 2020 •

edited

Loading

dims commented Sep 23, 2020

RobertKielty commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

RobertKielty commented Oct 23, 2020 •

edited

Loading

RobertKielty commented Oct 23, 2020

spiffxp left a comment

spiffxp Nov 3, 2020

spiffxp Nov 3, 2020 •

edited

Loading

spiffxp Nov 3, 2020

spiffxp Nov 3, 2020

fejta-bot commented Feb 2, 2021

RobertKielty commented Feb 17, 2021

fejta-bot commented May 18, 2021

fejta-bot commented Jun 17, 2021

fejta-bot commented Jul 17, 2021

k8s-ci-robot commented Jul 17, 2021

		// WIP See policy description on GH Issue #18599
		func TestJobsMustHaveTestGridAlertsSetup(t *testing.T) {

WIP : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address. #19286

WIP : CI Signal Policy Violation: A DashboardTab config MUST CONTAIN an alert contact email address. #19286

Conversation

RobertKielty commented Sep 21, 2020 • edited Loading

dims commented Sep 23, 2020

RobertKielty commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

RobertKielty commented Oct 23, 2020 • edited Loading

RobertKielty commented Oct 23, 2020

spiffxp left a comment

Choose a reason for hiding this comment

spiffxp Nov 3, 2020

Choose a reason for hiding this comment

spiffxp Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

spiffxp Nov 3, 2020

Choose a reason for hiding this comment

spiffxp Nov 3, 2020

Choose a reason for hiding this comment

fejta-bot commented Feb 2, 2021

RobertKielty commented Feb 17, 2021

fejta-bot commented May 18, 2021

fejta-bot commented Jun 17, 2021

fejta-bot commented Jul 17, 2021

k8s-ci-robot commented Jul 17, 2021

RobertKielty commented Sep 21, 2020 •

edited

Loading

RobertKielty commented Oct 23, 2020 •

edited

Loading

spiffxp Nov 3, 2020 •

edited

Loading