Repository: shipwright-io/build
Pull request #775 (2021-05-07 18:40:32)

Mitigate context deadline exceeded issue
# Changes

With the default implementation of `repo.List`, the timeout seems to be rather
short and there are intermittent context deadline exceeded errors when running
the unit tests under load, or when repeating them over and over again.

Mitigate the error by switching to the list function that accepts a context. It
was added recently and privides the possibility to define a different timeout.

Refactor related code to make it more compact by removing superfluous returns.

In order to be able to test and reproduce it, I used Ginkgo with the `-untilItFails`
and it started to be flaky after about 20 to 25 repetitions. With the code updates,
I was able to run it without issues for over 100 times, before canceling the test run.

# Submitter Checklist

- [ ] Includes tests if functionality changed/was added
- [ ] Includes docs if changes are user-facing
- [X] [Set a kind label on this PR](https://prow.k8s.io/command-help#kind)  
- [X] Release notes block has been filled in, or marked NONE

See [the contributor guide](https://github.com/shipwright-io/build/blob/master/CONTRIBUTING.md)
for details on coding conventions, github and prow interactions, and the code review process.

# Release Notes

```release-note
Fixed issue with Git validation by using the existing context with its timeout to be used for Git validations actions as well. Since this timeout is greater than the default timeout, it is less likely to fail unexpectedly in high load situations.
```




Patch:
diff --git a/pkg/git/git.go b/pkg/git/git.go
index 1e1134c6..28942ebd 100644
--- a/pkg/git/git.go
+++ b/pkg/git/git.go
@@ -5,6 +5,7 @@
 package git
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/go-git/go-git/v5/config"
@@ -26,8 +27,7 @@ const (
 // Note: We have an upcoming PR for the Build Status, where we
 // intend to define a single Status.Reason in the form of 'remoteRepositoryUnreachable',
 // where the Status.Message will contain the longer text, like 'invalid source url
-func ValidateGitURLExists(urlPath string) error {
-
+func ValidateGitURLExists(ctx context.Context, urlPath string) error {
 	endpoint, err := transport.NewEndpoint(urlPath)
 	if err != nil {
 		return err
@@ -39,20 +39,26 @@ func ValidateGitURLExists(urlPath string) error {
 			Name: defaultRemote,
 			URLs: []string{urlPath},
 		})
-		// Note: When the urlPath is an valid public path, however, this path doesn't exist, func will return `authentication required`, this maybe mislead
-		// So convert this error message to `repository not found`
-		_, err := repo.List(&gogitv5.ListOptions{})
-		if err != nil && err == transport.ErrAuthenticationRequired {
-			return fmt.Errorf("remote repository unreachable")
-		} else if err != nil {
+
+		if _, err := repo.ListContext(ctx, &gogitv5.ListOptions{}); err != nil {
+			// Note: When the urlPath is an valid public path, however, this
+			// path doesn't exist, func will return `authentication required`,
+			// this is maybe misleading. So convert this error message to:
+			// `remote repository unreachable`
+			if err == transport.ErrAuthenticationRequired {
+				return fmt.Errorf("remote repository unreachable")
+			}
+
 			return err
 		}
+
 	case fileProtocol:
 		return fmt.Errorf("invalid source url")
+
 	case gitProtocol:
 		return fmt.Errorf("the source url requires authentication")
-	default:
-		return nil
+
 	}
+
 	return nil
 }
diff --git a/pkg/validate/sourceurl.go b/pkg/validate/sourceurl.go
index 5a5cabd8..7cf10025 100644
--- a/pkg/validate/sourceurl.go
+++ b/pkg/validate/sourceurl.go
@@ -28,14 +28,14 @@ func (s SourceURLRef) ValidatePath(ctx context.Context) error {
 	if s.Build.Spec.Source.Credentials == nil {
 		switch s.Build.GetAnnotations()[build.AnnotationBuildVerifyRepository] {
 		case "true":
-			err := git.ValidateGitURLExists(s.Build.Spec.Source.URL)
-			if err != nil {
+			if err := git.ValidateGitURLExists(ctx, s.Build.Spec.Source.URL); err != nil {
 				s.MarkBuildStatus(s.Build, build.RemoteRepositoryUnreachable, err.Error())
+				return err
 			}
-			return err
+
 		case "", "false":
 			ctxlog.Info(ctx, fmt.Sprintf("the annotation %s is set to %s, nothing to do", build.AnnotationBuildVerifyRepository, s.Build.GetAnnotations()[build.AnnotationBuildVerifyRepository]), namespace, s.Build.Namespace, name, s.Build.Name)
-			return nil
+
 		default:
 			var annoErr = fmt.Errorf("the annotation %s was not properly defined, supported values are true or false", build.AnnotationBuildVerifyRepository)
 			ctxlog.Error(ctx, annoErr, namespace, s.Build.Namespace, name, s.Build.Name)
@@ -43,6 +43,7 @@ func (s SourceURLRef) ValidatePath(ctx context.Context) error {
 			return annoErr
 		}
 	}
+
 	return nil
 }
 


Test patch:
diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go
index 6c0fddb3..9fb3fd1f 100644
--- a/pkg/git/git_test.go
+++ b/pkg/git/git_test.go
@@ -5,6 +5,7 @@
 package git_test
 
 import (
+	"context"
 	"errors"
 
 	. "github.com/onsi/ginkgo"
@@ -18,7 +19,7 @@ var _ = Describe("Git", func() {
 
 	DescribeTable("the source url validation errors",
 		func(url string, expected types.GomegaMatcher) {
-			Expect(git.ValidateGitURLExists(url)).To(expected)
+			Expect(git.ValidateGitURLExists(context.TODO(), url)).To(expected)
 		},
 		Entry("Check remote https public repository", "https://github.com/shipwright-io/build", BeNil()),
 		Entry("Check remote fake https public repository", "https://github.com/shipwright-io/build-fake", Equal(errors.New("remote repository unreachable"))),