Replaced the flake-manager, issue-cacher, and issue-sync with the clu… #2450

cjwagner · 2017-04-10T21:17:53Z

…ster-manager and issue-creator. This change replaces the munger that created issues for individual flakes with a munger that creates issues for flakes based on the clustering data on the triage page.

Design Doc:
https://docs.google.com/a/google.com/document/d/1nnAI8lCCamcluvhh10Bz8vR9moSTyez1dCchU1qnCOI/edit?usp=sharing

k8s-ci-robot · 2017-04-10T21:17:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

krzyzacy · 2017-04-10T23:02:27Z

/assign @rmmh

you'll need to sign the CLA follow the instruction from the bot first.

fejta · 2017-04-10T23:06:15Z

Is it possible to put the delete of the old munger in its own commit? 4,000 changed lines is extremely challenging to review.

Also cluster makes me think of a kubernetes cluster. Can you choose a different name than cluster-manager?

cjwagner · 2017-04-10T23:12:48Z

@k8s-bot bazel test this

cjwagner · 2017-04-11T18:06:43Z

comment

rmmh · 2017-04-11T21:30:23Z

mungegithub/mungers/issue-creator.go

+	if len(rawIssues) == 0 {
+		glog.Warningf("IssueCreator found no issues in the repo '%s/%s' authored by %s.\n", creator.config.getOrg(), creator.config.getProject(), creator.authorName)
+	}
+	//allocate 10% extra capacity since issues may be added and this list is big so copy is expensive


copying a few thousand pointers is not expensive, don't bother with this.

rmmh · 2017-04-11T21:31:18Z

mungegithub/mungers/issue-creator.go

+	if !creator.refresh() {
+		glog.Fatalf("Failed to get the list of all issues created by %s in repo '%s/%s' while initing IssueCreator.\n", creator.authorName, creator.config.getOrg(), creator.config.getProject())
+	}
+	creator.allIssues_fetchfreq, _ = time.ParseDuration(fmt.Sprintf("%dm", refreshFreqMins))


refreshFreqMins * time.Minutes

rmmh · 2017-04-11T21:33:21Z

mungegithub/mungers/issue-creator.go

+	}
+}
+
+func (creator *IssueCreator) refresh() bool {


We can't call this function every minute, that is prohibitively expensive. issue-cacher.go follows the correct pattern of doing the enumeration once, and then updating using changed issues using the Munge() method.

rmmh · 2017-04-11T21:39:06Z

mungegithub/mungers/issue-creator.go

+//this type wraps the github.Config type so that it can match the RepoClient interface
+type githubConfig github.Config
+
+//gets the underlying *github.Config


Throughout: these comments should be complete sentences starting with the function name, to make godoc work.

"RealConfig returns the underlying github.Config."

rmmh · 2017-04-11T21:45:15Z

I think you should use issue-cacher.go instead of deleting it. This new "refresh every minute" implementation will slow down the entire munger.

All that should be required is changing issue-cacher.go's keyFromIssue to extract cluster IDs from issue bodies when present.

fejta · 2017-04-12T05:36:49Z

I remain unconvinced that we want a monolithic munger binary verses a bunch of small focused programs that prow triggers on a periodic basis

fejta · 2017-04-12T05:37:23Z

I do not think this program needs to run more than once per day

thelinuxfoundation · 2017-04-12T17:36:01Z

bump the CLA check

rmmh · 2017-04-13T20:41:45Z

mungegithub/github/github.go

-	var assignee *string
-	if owner != "" {
-		assignee = &owner
+	if len(owners) == 0 {


This is not quite correct. You want a nil assignees pointer by default, and set it to the address of owners if it's not empty.

Looks like there are still a bunch of comments that have not been responded to.

Done. (Used empty slice since nil doesn't behave well with the github client).

rmmh · 2017-04-13T20:44:21Z

mungegithub/mungers/issue-creator_test.go

+	return matchCount == 1
+}
+
+//We also need a dummy Issue implementation


unnecessary comment

rmmh · 2017-04-13T20:44:36Z

mungegithub/mungers/issue-creator_test.go

+	priority        int //-1 indicates no priority
+}
+
+func (issue *fakeIssue) Title() string {


should be func (i *fakeIssue) Title() string { (throughout)

rmmh · 2017-04-13T20:44:44Z

mungegithub/mungers/issue-creator_test.go

+	return nil
+}
+
+//checks that exactly 1 issue in fc.issues matches the parameters and that no


Verify checks... (and end with a period)

rmmh · 2017-04-13T20:55:37Z

mungegithub/mungers/issue-creator_test.go

+}
+
+func TestIssueCreator(tt *testing.T) {
+	t = tt


just rename the function parameter

Done. (made t a field instead of a global)

rmmh · 2017-04-13T20:58:09Z

mungegithub/mungers/issue-creator_test.go

+		for i := 0; i < len(issue.Assignees); i++ {
+			assignees[i] = *issue.Assignees[i].Login
+		}
+		if !stringSlicesEqual(assignees, owners) {


use reflect.deepEqual instead

rmmh · 2017-04-13T21:15:58Z

mungegithub/mungers/mungerutil/util_test.go

@@ -111,3 +111,21 @@ func TestIssueUsersAll(t *testing.T) {
 		t.Errorf("AllUsers (%s) doesn't match expected list: %s", users.AllUsers().List(), expected)
 	}
 }
+
+func TestFetchWebFileContents(t *testing.T) {
+	goodurl := "https://storage.googleapis.com/k8s-gubernator/triage/failure_data.json"


In general, don't write tests that require external network connectivity.

You can use net/http/httptest if you really need to test this function. Otherwise I wouldn't bother-- it's pretty trivial.

rmmh · 2017-04-13T21:16:07Z

mungegithub/mungers/mungerutil/util.go

+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	} else {


remove the else { }

rmmh · 2017-04-13T21:16:50Z

mungegithub/mungers/triage-filer.go

+		glog.Fatalln("ERROR: ", err)
+	}
+	//ensure that this dict has the keys it should
+	if _, exists := buildsDict[keyColumns]; !exists {


_, ok is more idiomatic Go

rmmh · 2017-04-13T21:17:42Z

mungegithub/mungers/triage-filer.go

+	var clusters []*Cluster
+
+	//first parse the "clustered" and "builds" keys from the top level
+	topLevelDict := make(map[string]json.RawMessage)


Can you unmarshal to a struct instead of a map? You can then test each of the members for being non-empty.

rmmh · 2017-04-13T21:19:52Z

mungegithub/mungers/triage-filer.go

+	//load job and build data
+	allJobs := loadJobData(topLevelDict[keyBuilds])
+	//only consider failures that occured after this time
+	cutoffTime := time.Now().AddDate(0, 0, -1*filer.windowDays).Unix()


-filer.WindowDays

rmmh · 2017-04-13T21:21:15Z

mungegithub/mungers/triage-filer.go

+		glog.Fatalln("ERROR: ", err)
+	}
+	for _, cluster := range rawClusters {
+		clust := new(Cluster)


clust := &Cluster{filer: filer, testFailed: ... }

rmmh · 2017-04-13T21:22:26Z

mungegithub/mungers/triage-filer.go

+	topLevelDict := make(map[string]json.RawMessage)
+	err := json.Unmarshal(jsonIn, &topLevelDict)
+	if err != nil {
+		glog.Fatalln("ERROR: ", err)


Don't use Fatal in production code unless it's actually unrecoverable. The SQ going offline is very disruptive. Log an error and return nil, or make parseClusters return ([]*Cluster, err).

rmmh · 2017-04-19T22:04:13Z

mungegithub/mungers/issue-creator.go

+
+//Munge updates the IssueCreator's cache of issues if the munge object provided is an issue authored by the currently authenticated user.
+func (creator *IssueCreator) Munge(obj *github.MungeObject) {
+	//ignore pull requests


none of the comments in this function are necessary

rmmh · 2017-04-19T22:14:04Z

mungegithub/mungers/issue-creator.go

+//Initialize prepares an IssueCreator for use by other mungers.
+//This includes determining the currently authenticated user, fetching all issues created by that user
+//from github, fetching the labels that are valid for the repo, and initializing the test owner and sig data.
+func (creator *IssueCreator) Initialize(config *github.Config, feats *features.Features) error {


throughout: the method signature should be func (c *IssueCreator) or func (ic *IssueCreator). Go style prefers terse method receivers.

rmmh · 2017-04-19T22:16:13Z

mungegithub/mungers/issue-creator.go

+	authorName string
+	//allIssues is a local cache of all issues in the repo authored by the currently authenticated user.
+	//MungeObjects are keyed by issue number
+	allIssues map[int]*github.MungeObject


I think this could be map[int]*github.Issue without losing anything-- I think you only access the .Issue field of the mungeobject. That would also remove the need for the somewhat strange NewEmptyMungeObject function.

I originally did have that map type, but I switched it to MungeObject so that an Issue interface implementation can get more info about the github issues if it needs it. Specifically I was thinking that it might be useful to retrieve the comments for the issue which is a functionality provided by MungeObject. Should I switch it back?

Done. (switched back to *githubapi.Issue value type)

rmmh · 2017-04-19T22:22:20Z

mungegithub/mungers/example_ownersfile.csv

@@ -0,0 +1,19 @@
+name,owner,auto-assigned,sig


delete this file entirely, now that it's unused

rmmh · 2017-04-19T22:44:47Z

mungegithub/mungers/mungerutil/util.go

@@ -112,3 +115,20 @@ func IsValidUser(u *github.User) bool {
 func IsMungeBot(u *github.User) bool {
 	return IsValidUser(u) && *u.Login == BotName
 }
+
+//fetches raw data from a url (used to fetch json file)
+func FetchWebFileContents(url string) ([]byte, error) {


Rename to "ReadHTTP" for symmetry with bytes.ReadFile and fix the comment. You can also make submit-queue-batch.go:getJobs use this instead. :-)

rmmh · 2017-04-19T23:47:07Z

mungegithub/mungers/triage-filer.go

+	fmt.Fprintf(&buf, "Failure stats over time range '%s' to '%s'\n", cutoffTime.Format(time.RFC822), time.Now().Format(time.RFC822))
+	//totals
+	fmt.Fprintf(&buf, "%d tests failed,    %d jobs failed,    %d builds failed.\n###### Top failed tests by jobs failed:\n", clust.totalTests, clust.totalJobs, clust.totalBuilds)
+	//build top tests failed info


use a table for these? https://help.github.com/articles/organizing-information-with-tables/

rmmh · 2017-04-19T23:47:28Z

mungegithub/mungers/triage-filer.go

+	//build top jobs failed info
+	fmt.Fprintln(&buf, "###### Top failed jobs by builds failed:")
+	for _, job := range clust.topJobsFailed(clust.filer.topJobsCount) {
+		fmt.Fprintf(&buf, "%s:    %d builds\n", job, len(clust.jobsFailed[job].builds))


linking directly to an example failure for each of the failed jobs would be helpful

rmmh · 2017-04-19T23:48:25Z

mungegithub/mungers/triage-filer.go

+	if len(closedIssues) > 0 {
+		fmt.Fprintln(&buf, "###### Previously issues for this cluster:")
+		for _, closed := range closedIssues {
+			fmt.Fprintln(&buf, closed.Issue.HTMLURL)


fmt.Fprintf(&buf, "#%d ", closed.Issue.Number) should display better.

I made the changes you requested. Ready for you to review again.

rmmh · 2017-04-19T23:50:31Z

mungegithub/mungers/triage-filer.go

+
+	//now we know we need to make a new issue
+	var buf bytes.Buffer
+	fmt.Fprintf(&buf, "#### Cluster [%s] More data at: %s#%s\n", clust.ID(), clust.filer.triageURL, clust.ID())


Turn "more data" into a link: fmt.Fprintf(&buf, "#### Cluster [%s] [more data](%s#%s)\n", clust.ID(), clust.filer.triageURL, clust.ID())

rmmh · 2017-04-19T23:50:59Z

mungegithub/mungers/triage-filer.go

+	//now we know we need to make a new issue
+	var buf bytes.Buffer
+	fmt.Fprintf(&buf, "#### Cluster [%s] More data at: %s#%s\n", clust.ID(), clust.filer.triageURL, clust.ID())
+	fmt.Fprintf(&buf, "Failure stats over time range '%s' to '%s'\n", cutoffTime.Format(time.RFC822), time.Now().Format(time.RFC822))


again, the time range should be specified based on what's in the data file, not the current time.

fejta · 2017-04-20T01:56:40Z

mungegithub/submit-queue/deployment/kubernetes/configmap.yaml

-  flake-manager.test-owners-csv: /gitrepos/kubernetes/test/test_owners.csv
+  triage-filer.test-owners-csv: /gitrepos/kubernetes/test/test_owners.csv
+  triage-filer.windowdays: "5"
+  triage-filer.synccount: "10"


Can we please actually start with 3 or 5 instead of 10? I'm worried 10 issues every day will be too many. I'd much rather have too few issues than too many.

fejta · 2017-04-20T01:57:08Z

mungegithub/submit-queue/deployment/test-infra/configmap.yaml

@@ -41,7 +41,9 @@ data:
  # munger specific options.
  path-label.path-label-config: ""
  block-path.block-path-config: ""
-  flake-manager.test-owners-csv: ""
+  triage-filer.test-owners-csv: ""


I am not sure this makes sense... Is there a triage page for test-infra failures?

fejta · 2017-04-20T01:57:19Z

mungegithub/submit-queue/deployment/contrib/configmap.yaml

@@ -42,7 +42,9 @@ data:
  # munger specific options.
  path-label.path-label-config: ""
  block-path.block-path-config: ""
-  flake-manager.test-owners-csv: ""
+  triage-filer.test-owners-csv: ""


Same as test-infra comment

fejta · 2017-04-20T01:58:44Z

mungegithub/mungers/issue-creator.go

+	ListAllIssues(options *githubapi.IssueListByRepoOptions) ([]*githubapi.Issue, error)
+	NewIssue(title, body string, labels, owners []string) (*github.MungeObject, error)
+
+	getDryRun() bool


Is this the correct golang pattern? I would expect isDryRun()

fejta · 2017-04-20T02:00:56Z

mungegithub/mungers/issue-creator.go

+	creator.config = RepoClient(&cfg)
+
+	//load test owner/SIG data
+	var err error


is there a reason why you are doing this over creator.owners, err := testowner.NewReloading() and if err := creator.load(); err != nil?

Ack. (Cannot define a new variable and assign to a field in a single assignment statement in go)

fejta · 2017-04-20T02:01:48Z

mungegithub/mungers/issue-creator.go

+func (creator *IssueCreator) loadCache() error {
+	//try to get the current authenticated user
+	user, err := creator.config.GetUser("")
+	if err != nil || user == nil || user.Login == nil {


I suspect it will be more helpful to return a specific error message for each of these scenarios.

fejta · 2017-04-20T02:05:46Z

mungegithub/mungers/issue-creator.go

+	owners := issue.Owners()
+	labels := issue.Labels()
+	if prio, ok := issue.Priority(); ok {
+		labels = append(labels, "priority/P"+strconv.Itoa(prio))


I actually think these labels are deprecated. Can we just skip setting a priority? Alternatively use the new priority/random-annoying-string labels

Done. Priorities are not currently used but are now of type string instead of an int.

fejta · 2017-04-20T02:13:04Z

mungegithub/mungers/triage-filer.go

+	cmd.Flags().IntVar(&filer.topClustersCount, "triagesynccount", 10, "The number of clusters to sync issues for on github.")
+	cmd.Flags().StringVar(&filer.triageURL, "triageURL", "https://go.k8s.io/triage", "The url prefix to add before cluster hash to create a link to the triage page for a specific cluster.")
+	cmd.Flags().IntVar(&filer.windowDays, "triagewindowdays", 5, "The size of the sliding time window (in days) that is used to determine which failures to consider.")
+	cmd.Flags().IntVar(&filer.syncFreqMins, "triagesyncfreq", 60, "The frequency at which to run the TriageFiler in minutes.")


Would duration be a better fit here? https://golang.org/pkg/flag/#Duration

Also can you default this to once a day? I really want to minimize the number of issues we automatically file. Said this elsewhere but too few auto-filed issues is WAY better than too many -- since too many just overwhelms people and they give up, reducing the value of all issues.

I made the requested changes. Ready to be reviewed again.

fejta · 2017-04-25T03:17:19Z

FYI when you are ready for another round of reviews please leave a reply on each of the comments (Done, Ack, or anything else, really) to let us know to review again.

fejta

The size of this PR makes my head hurt

fejta · 2017-05-02T05:09:52Z

mungegithub/mungers/triage-filer.go

+}
+
+// AddFlags will add any requested flags to the cobra `cmd`.
+func (f *TriageFiler) AddFlags(cmd *cobra.Command, config *github.Config) {


Thoughts:

Is it important to start with these as flags rather than consts? It is cheap/trivial to release a new version.

Should all of these start with triage?

Shouldn't the words be separated with hyphens? Aka cluster-data-url &c

I changed all but the 2 most important flags to constants. I kept the triage-sync-count (the number of failure clusters to sync) and triage-window-days (the size of the sliding window) flags since we may want to mess with these frequently at first.
Both of the names start with triage and use hyphens now.

fejta · 2017-05-02T05:10:57Z

mungegithub/mungers/mungerutil/util.go

+		return nil, err
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {


This should probably automatically retry 500 level errors a few times

…ilures identified on the triage page.

…ync. These are replaced by the triage-filer.

rmmh · 2017-05-03T18:12:41Z

/lgtm

cjwagner requested a review from rmmh April 10, 2017 21:17

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 10, 2017

cjwagner force-pushed the cluster-reporter branch from 0247502 to f911658 Compare April 10, 2017 22:53

k8s-ci-robot assigned rmmh Apr 10, 2017

fejta self-assigned this Apr 10, 2017

cjwagner force-pushed the cluster-reporter branch 2 times, most recently from 9a0df2c to 2b87217 Compare April 11, 2017 00:53

cjwagner closed this Apr 11, 2017

cjwagner reopened this Apr 11, 2017

cjwagner mentioned this pull request Apr 11, 2017

Umbrella issue for the Linux Foundation (CNCF) CLA system kubernetes/kubernetes#27796

Closed

rmmh reviewed Apr 11, 2017

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 12, 2017

cjwagner force-pushed the cluster-reporter branch from 2b87217 to a1b0150 Compare April 13, 2017 00:17

rmmh reviewed Apr 13, 2017

View reviewed changes

cjwagner force-pushed the cluster-reporter branch from a1b0150 to 1693fac Compare April 14, 2017 21:59

rmmh reviewed Apr 19, 2017

View reviewed changes

fejta reviewed Apr 20, 2017

View reviewed changes

cjwagner force-pushed the cluster-reporter branch 2 times, most recently from 1a71ca5 to 1a8eef7 Compare April 21, 2017 02:15

fejta reviewed May 2, 2017

View reviewed changes

cjwagner force-pushed the cluster-reporter branch 2 times, most recently from d0c225c to ad1b242 Compare May 2, 2017 20:07

cjwagner added 2 commits May 2, 2017 13:11

Added the triage-filer which creates github issues for clusters of fa…

92850a5

…ilures identified on the triage page.

Deleted the flake-manager and the associated issue-cacher and issue-s…

9faa7fc

…ync. These are replaced by the triage-filer.

cjwagner force-pushed the cluster-reporter branch from ad1b242 to 9faa7fc Compare May 2, 2017 20:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2017

cjwagner merged commit 2c8f741 into kubernetes:master May 4, 2017

cjwagner deleted the cluster-reporter branch May 4, 2017 00:39

spiffxp mentioned this pull request Jun 29, 2017

mungebot: Flake-manager is not running on kubernetes/kubernetes #1818

Closed

Replaced the flake-manager, issue-cacher, and issue-sync with the clu… #2450

Replaced the flake-manager, issue-cacher, and issue-sync with the clu… #2450

Conversation

cjwagner commented Apr 10, 2017

k8s-ci-robot commented Apr 10, 2017

krzyzacy commented Apr 10, 2017

fejta commented Apr 10, 2017

cjwagner commented Apr 10, 2017

cjwagner commented Apr 11, 2017

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

rmmh Apr 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmmh commented Apr 11, 2017

fejta commented Apr 12, 2017

fejta commented Apr 12, 2017

thelinuxfoundation commented Apr 12, 2017

rmmh Apr 13, 2017 • edited

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

rmmh Apr 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmmh Apr 13, 2017 • edited

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

rmmh Apr 13, 2017 • edited

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

rmmh Apr 13, 2017 • edited

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

rmmh Apr 11, 2017 •

edited

rmmh Apr 13, 2017 •

edited

rmmh Apr 13, 2017 •

edited

rmmh Apr 13, 2017 •

edited

rmmh Apr 13, 2017 •

edited

rmmh Apr 13, 2017 •

edited