Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pkg): fallback at using git client when github API files limits is reached #146

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cmd/status-reconciler/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ func main() {
logrus.WithError(err).Fatal("Error getting GitHub client.")
}

gitClient, err := o.github.GitClientFactory("", &o.config.InRepoConfigCacheDirBase, o.dryRun, false)
if err != nil {
logrus.WithError(err).Fatal("Error getting Git client.")
}

prowJobClient, err := o.kubernetes.ProwJobClient(configAgent.Config().ProwJobNamespace, o.dryRun)
if err != nil {
logrus.WithError(err).Fatal("Error getting kube client.")
Expand All @@ -139,7 +144,7 @@ func main() {
logrus.WithError(err).Fatal("Cannot create opener")
}

c := statusreconciler.NewController(o.continueOnError, o.getDenyList(), o.getDenyListAll(), opener, o.config, o.statusURI, prowJobClient, githubClient, pluginAgent)
c := statusreconciler.NewController(o.continueOnError, o.getDenyList(), o.getDenyListAll(), opener, o.config, o.statusURI, prowJobClient, gitClient, githubClient, pluginAgent)
interrupts.Run(func(ctx context.Context) {
c.Run(ctx)
})
Expand Down
25 changes: 22 additions & 3 deletions pkg/config/jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"net/url"
"regexp"
"sigs.k8s.io/prow/pkg/git/v2"
"strings"
"time"

Expand Down Expand Up @@ -525,7 +526,8 @@ type githubClient interface {
// NewGitHubDeferredChangedFilesProvider uses a closure to lazily retrieve the file changes only if they are needed.
// We only have to fetch the changes if there is at least one RunIfChanged/SkipIfOnlyChanged job that is not being
// force run (due to a `/retest` after a failure or because it is explicitly triggered with `/test foo`).
func NewGitHubDeferredChangedFilesProvider(client githubClient, org, repo string, num int) ChangedFilesProvider {
func NewGitHubDeferredChangedFilesProvider(gc git.ClientFactory, client githubClient, org, repo string, num int,
baseSHA, headSHA string) ChangedFilesProvider {
var changedFiles []string
return func() ([]string, error) {
// Fetch the changed files from github at most once.
Expand All @@ -534,8 +536,25 @@ func NewGitHubDeferredChangedFilesProvider(client githubClient, org, repo string
if err != nil {
return nil, fmt.Errorf("error getting pull request changes: %w", err)
}
for _, change := range changes {
changedFiles = append(changedFiles, change.Filename)

// Fallback to use gitClient since github API truncated the response
if len(changes) == github.ChangesFilesLimit && gc != nil {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested here: kubernetes/test-infra#30615 (comment) this is now a fallback, that is only executed when:

  • gitclient is not nil
  • length of changes is exactly 3000

Of course, this means that if unluckily number of changes is exactly 3000, we would run the git client flow for nothing.

repoClient, err := gc.ClientFor(org, repo)
if err == nil {
// Use git client since Github PushEvent is limited to 3000 keys:
// https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files
files, err := repoClient.Diff(headSHA, baseSHA)
if err == nil {
changedFiles = files
}
}
}

// in all failure cases, return truncated files
if len(changedFiles) == 0 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the gitclient approach fails, return the truncated list of files.

for _, change := range changes {
changedFiles = append(changedFiles, change.Filename)
}
}
}
return changedFiles, nil
Expand Down
4 changes: 4 additions & 0 deletions pkg/github/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ const (

// DefaultGraphQLEndpoint is the default GitHub GraphQL API endpoint.
DefaultGraphQLEndpoint = "https://api.github.com/graphql"

// ChangesFilesLimit is the limit to the files changed pushed by the github rest API.
// See https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files.
ChangesFilesLimit = 3000
)

var (
Expand Down
2 changes: 1 addition & 1 deletion pkg/plugins/skip/skip.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func handle(gc githubClient, log *logrus.Entry, e *github.GenericCommentEvent, c
}
statuses := combinedStatus.Statuses

filteredPresubmits, err := trigger.FilterPresubmits(honorOkToTest, gc, e.Body, pr, presubmits, log)
filteredPresubmits, err := trigger.FilterPresubmits(honorOkToTest, gitClient, gc, e.Body, pr, presubmits, log)
if err != nil {
resp := fmt.Sprintf("Cannot get combined status for PR #%d in %s/%s: %v", number, org, repo, err)
log.Warn(resp)
Expand Down
16 changes: 9 additions & 7 deletions pkg/plugins/trigger/generic-comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package trigger

import (
"fmt"
"sigs.k8s.io/prow/pkg/git/v2"

"github.com/sirupsen/logrus"
"sigs.k8s.io/prow/pkg/kube"
Expand Down Expand Up @@ -126,12 +127,12 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo
return err
}

toTest, err := FilterPresubmits(HonorOkToTest(trigger), c.GitHubClient, gc.Body, pr, presubmits, c.Logger)
toTest, err := FilterPresubmits(HonorOkToTest(trigger), c.GitClient, c.GitHubClient, gc.Body, pr, presubmits, c.Logger)
if err != nil {
return err
}
if needsHelp, note := pjutil.ShouldRespondWithHelp(gc.Body, len(toTest)); needsHelp {
return addHelpComment(c.GitHubClient, gc.Body, org, repo, pr.Base.Ref, pr.Number, presubmits, gc.HTMLURL, commentAuthor, note, c.Logger)
return addHelpComment(c.GitClient, c.GitHubClient, gc.Body, org, repo, pr, presubmits, gc.HTMLURL, commentAuthor, note, c.Logger)
}
// we want to be able to track re-tests separately from the general body of tests
additionalLabels := map[string]string{}
Expand Down Expand Up @@ -195,7 +196,7 @@ type GitHubClient interface {
// If a comment that we get matches more than one of the above patterns, we
// consider the set of matching presubmits the union of the results from the
// matching cases.
func FilterPresubmits(honorOkToTest bool, gitHubClient GitHubClient, body string, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, error) {
func FilterPresubmits(honorOkToTest bool, gc git.ClientFactory, gitHubClient GitHubClient, body string, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, error) {
org, repo, sha := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Head.SHA

contextGetter := func() (sets.Set[string], sets.Set[string], error) {
Expand All @@ -212,8 +213,8 @@ func FilterPresubmits(honorOkToTest bool, gitHubClient GitHubClient, body string
return nil, err
}

number, branch := pr.Number, pr.Base.Ref
changes := config.NewGitHubDeferredChangedFilesProvider(gitHubClient, org, repo, number)
number, branch, baseSHA, headSHA := pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA
changes := config.NewGitHubDeferredChangedFilesProvider(gc, gitHubClient, org, repo, number, baseSHA, headSHA)
return pjutil.FilterPresubmits(filter, changes, branch, presubmits, logger)
}

Expand All @@ -231,8 +232,9 @@ func getContexts(combinedStatus *github.CombinedStatus) (sets.Set[string], sets.
return failedContexts, allContexts
}

func addHelpComment(githubClient githubClient, body, org, repo, branch string, number int, presubmits []config.Presubmit, HTMLURL, user, note string, logger *logrus.Entry) error {
changes := config.NewGitHubDeferredChangedFilesProvider(githubClient, org, repo, number)
func addHelpComment(gc git.ClientFactory, githubClient githubClient, body, org, repo string, pr *github.PullRequest, presubmits []config.Presubmit, HTMLURL, user, note string, logger *logrus.Entry) error {
number, branch, baseSHA, headSHA := pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA
changes := config.NewGitHubDeferredChangedFilesProvider(gc, githubClient, org, repo, number, baseSHA, headSHA)
testAllNames, optionalJobsCommands, requiredJobsCommands, err := pjutil.AvailablePresubmits(changes, branch, presubmits, logger)
if err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions pkg/plugins/trigger/pull-request.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,8 @@ func buildAllButDrafts(c Client, pr *github.PullRequest, eventGUID string, baseS

// buildAll ensures that all builds that should run and will be required are built
func buildAll(c Client, pr *github.PullRequest, eventGUID string, baseSHA string, presubmits []config.Presubmit) error {
org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref
changes := config.NewGitHubDeferredChangedFilesProvider(c.GitHubClient, org, repo, number)
org, repo, number, branch, headSHA := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref, pr.Head.SHA
changes := config.NewGitHubDeferredChangedFilesProvider(c.GitClient, c.GitHubClient, org, repo, number, baseSHA, headSHA)
toTest, err := pjutil.FilterPresubmits(pjutil.NewTestAllFilter(), changes, branch, presubmits, c.Logger)
if err != nil {
return err
Expand Down
46 changes: 32 additions & 14 deletions pkg/plugins/trigger/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,46 @@ import (

prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
"sigs.k8s.io/prow/pkg/config"
"sigs.k8s.io/prow/pkg/git/v2"
"sigs.k8s.io/prow/pkg/github"
"sigs.k8s.io/prow/pkg/pjutil"
)

func listPushEventChanges(pe github.PushEvent) config.ChangedFilesProvider {
func listPushEventChanges(gc git.ClientFactory, pe github.PushEvent) config.ChangedFilesProvider {
var changedFiles []string
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the only actual "breaking change", ie: cache changedFiles just like we do in pull-request.go, to avoid a git client diff call for each configured post submit when we are in the fallback case.

return func() ([]string, error) {
changed := make(map[string]bool)
for _, commit := range pe.Commits {
for _, added := range commit.Added {
changed[added] = true
if changedFiles == nil {
// Fallback to use PushEvent
changes := make(map[string]bool)
for _, commit := range pe.Commits {
for _, added := range commit.Added {
changes[added] = true
}
for _, removed := range commit.Removed {
changes[removed] = true
}
for _, modified := range commit.Modified {
changes[modified] = true
}
}
for _, removed := range commit.Removed {
changed[removed] = true
if len(changes) == github.ChangesFilesLimit && gc != nil {
repoClient, err := gc.ClientFor(pe.Repo.Owner.Name, pe.Repo.Name)
if err == nil {
// Use git client since Github PushEvent is limited to 3000 keys:
// https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files
files, err := repoClient.Diff(pe.After, pe.Before)
if err == nil {
changedFiles = files
}
}
}
for _, modified := range commit.Modified {
changed[modified] = true

if len(changedFiles) == 0 {
for file := range changes {
changedFiles = append(changedFiles, file)
}
}
}
var changedFiles []string
for file := range changed {
changedFiles = append(changedFiles, file)
}
return changedFiles, nil
}
}
Expand Down Expand Up @@ -74,7 +92,7 @@ func handlePE(c Client, pe github.PushEvent) error {
postsubmits := getPostsubmits(c.Logger, c.GitClient, c.Config, org+"/"+repo, shaGetter)

for _, j := range postsubmits {
if shouldRun, err := j.ShouldRun(pe.Branch(), listPushEventChanges(pe)); err != nil {
if shouldRun, err := j.ShouldRun(pe.Branch(), listPushEventChanges(c.GitClient, pe)); err != nil {
return err
} else if !shouldRun {
continue
Expand Down
9 changes: 6 additions & 3 deletions pkg/statusreconciler/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package statusreconciler
import (
"context"
"fmt"
"sigs.k8s.io/prow/pkg/git/v2"
"strings"
"time"

Expand All @@ -38,7 +39,7 @@ import (
)

// NewController constructs a new controller to reconcile stauses on config change
func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitDenylistAll sets.Set[string], opener io.Opener, configOpts configflagutil.ConfigOptions, statusURI string, prowJobClient prowv1.ProwJobInterface, githubClient github.Client, pluginAgent *plugins.ConfigAgent) *Controller {
func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitDenylistAll sets.Set[string], opener io.Opener, configOpts configflagutil.ConfigOptions, statusURI string, prowJobClient prowv1.ProwJobInterface, gc git.ClientFactory, githubClient github.Client, pluginAgent *plugins.ConfigAgent) *Controller {
sc := &statusController{
logger: logrus.WithField("client", "statusController"),
opener: opener,
Expand All @@ -57,6 +58,7 @@ func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitD
pluginAgent: pluginAgent,
},
githubClient: githubClient,
gitClient: gc,
statusMigrator: &gitHubMigrator{
githubClient: githubClient,
continueOnError: continueOnError,
Expand Down Expand Up @@ -151,6 +153,7 @@ type Controller struct {
addedPresubmitDenylistAll sets.Set[string]
prowJobTriggerer prowJobTriggerer
githubClient githubClient
gitClient git.ClientFactory
statusMigrator statusMigrator
trustedChecker trustedChecker
statusClient statusClient
Expand Down Expand Up @@ -247,8 +250,8 @@ func (c *Controller) triggerNewPresubmits(addedPresubmits map[string][]config.Pr
filter := pjutil.NewArbitraryFilter(func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) {
return true, false, true
}, "inline-filter")
org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref
changes := config.NewGitHubDeferredChangedFilesProvider(c.githubClient, org, repo, number)
org, repo, number, branch, baseSHA, headSHA := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA
changes := config.NewGitHubDeferredChangedFilesProvider(c.gitClient, c.githubClient, org, repo, number, baseSHA, headSHA)
logger := log.WithFields(logrus.Fields{"org": org, "repo": repo, "number": number, "branch": branch})
toTrigger, err := pjutil.FilterPresubmits(filter, changes, branch, presubmits, logger)
if err != nil {
Expand Down