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.

Sign up for GitHub

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

Provide a client method to bypass server side timeout on upload #20419

Merged
merged 3 commits into from
Jul 28, 2018
Merged

Provide a client method to bypass server side timeout on upload #20419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6dc1369
Provide a client method to bypass server side timeout on upload
smarterclayton Jul 25, 2018
62b5ebf
Importer integration test was an off by one
smarterclayton Jul 26, 2018
e73f7b4
Fix Go 1.10 issue with integration test
smarterclayton Jul 26, 2018
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
2 changes: 1 addition & 1 deletion pkg/image/controller/imagestream_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ func handleImageStream(
ImportPolicy: imageapi.TagImportPolicy{Insecure: insecure},
}
}
result, err := client.ImageStreamImports(stream.Namespace).Create(isi)
result, err := client.ImageStreamImports(stream.Namespace).CreateWithoutTimeout(isi)
if err != nil {
if apierrs.IsNotFound(err) && isStatusErrorKind(err, "imageStream") {
return result, ErrNotImportable
Expand Down
18 changes: 18 additions & 0 deletions ...ed/internalclientset/typed/image/internalversion/fake/fake_imagestreamimport_expansion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions pkg/image/generated/internalclientset/typed/image/internalversion/generated_expansion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions ...ge/generated/internalclientset/typed/image/internalversion/imagestreamimport_expansion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package internalversion

import (
"time"

image "github.com/openshift/origin/pkg/image/apis/image"
)

type ImageStreamImportExpansion interface {
CreateWithoutTimeout(*image.ImageStreamImport) (*image.ImageStreamImport, error)
}

// CreateWithoutTimeout imports the provided images and won't time out after 30 seconds. Use this when you must
// import a large number of images.
func (c *imageStreamImports) CreateWithoutTimeout(imageStreamImport *image.ImageStreamImport) (result *image.ImageStreamImport, err error) {
result = &image.ImageStreamImport{}
err = c.client.Post().
Namespace(c.ns).
Resource("imagestreamimports").
Body(imageStreamImport).
// this instructs the api server to allow our request to take up to an hour - chosen as a high boundary
Timeout(time.Hour).
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just make this an argument to the function? especially since the function name is currently a lie.

also where's the generator code change that yields this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we don't want end users setting arbitrary timeouts. We define what "no timeout" means behind the interface. End clients don't get to set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no generator code change. This is an expansion, which are side car methods that you write and get slotted in to the interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

An expansion that we will have to carry over when we move the image controllers to external :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@smarterclayton if there is just one place where we actually use this expansion, can't we just call the client directly with Timeout in that place? (instead of having one-time-use expansion)

Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just call the client directly with Timeout in that place?

the client doesn't expose the timeout field that i saw, so no.

Do().
Into(result)
return
}
12 changes: 6 additions & 6 deletions test/integration/imagechange_buildtrigger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,19 +302,19 @@ func runTest(t *testing.T, testname string, projectAdminClientConfig *rest.Confi
if strategy.SourceStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:"+tag {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %s\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.SourceStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %#v\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.SourceStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
}
case strategy.DockerStrategy != nil:
if strategy.DockerStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:"+tag {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %s\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.DockerStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %#v\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.DockerStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
}
case strategy.CustomStrategy != nil:
if strategy.CustomStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:"+tag {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %s\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.CustomStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\ntrigger is %#v\n", registryHostname+"/openshift/test-image-trigger:"+tag, strategy.CustomStrategy.From.Name, i, bc.Spec.Triggers[0].ImageChange)
}
}
event = <-buildWatch.ResultChan()
Expand Down Expand Up @@ -376,19 +376,19 @@ func runTest(t *testing.T, testname string, projectAdminClientConfig *rest.Confi
if strategy.SourceStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:ref-2-random" {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %s\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.SourceStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %#v\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.SourceStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
}
case strategy.DockerStrategy != nil:
if strategy.DockerStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:ref-2-random" {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %s\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.DockerStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %#v\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.DockerStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
}
case strategy.CustomStrategy != nil:
if strategy.CustomStrategy.From.Name != registryHostname+"/openshift/test-image-trigger:ref-2-random" {
i, _ := projectAdminImageClient.ImageStreams(testutil.Namespace()).Get(imageStream.Name, metav1.GetOptions{})
bc, _ := projectAdminBuildClient.BuildConfigs(testutil.Namespace()).Get(config.Name, metav1.GetOptions{})
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %s\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.CustomStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
t.Fatalf("Expected build with base image %s, got %s\n, imagerepo is %v\trigger is %#v\n", registryHostname+"/openshift/test-image-trigger:ref-2-random", strategy.CustomStrategy.From.Name, i, bc.Spec.Triggers[3].ImageChange)
}
}

Expand Down
10 changes: 8 additions & 2 deletions test/integration/imageimporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ import (
"net/url"
"reflect"
"strings"
"sync"
"testing"
"time"

"github.com/docker/distribution/registry/api/errcode"
"github.com/golang/glog"
gocontext "golang.org/x/net/context"

"k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -654,7 +656,10 @@ func TestImageStreamImportTagsFromRepository(t *testing.T) {
func TestImageStreamImportScheduled(t *testing.T) {
written := make(chan struct{}, 2)
count := 0
var lock sync.Mutex
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
lock.Lock()
defer lock.Unlock()
t.Logf("got %s %s", r.Method, r.URL.Path)
switch r.URL.Path {
case "/v2/":
Expand All @@ -663,12 +668,13 @@ func TestImageStreamImportScheduled(t *testing.T) {
case "/v2/test/image/manifests/latest", "/v2/test/image/manifests/" + etcdDigest, "/v2/test/image/manifests/" + phpDigest:
count++
t.Logf("serving %d", count)
glog.Infof("serving request %d for %s", count, r.URL.Path)
var manifest, digest string
switch count {
case 1, 2:
digest = etcdDigest
manifest = etcdManifest
case 3, 4, 5, 6:
case 3, 4, 5:
digest = phpDigest
manifest = phpManifest
default:
Expand Down Expand Up @@ -817,7 +823,7 @@ func TestImageStreamImportScheduled(t *testing.T) {
t.Fatalf("expected generation 2 for stream and spec tag: %v %#v", tagGen, change)
}
conditions := change.Status.Tags["latest"].Conditions
if len(conditions) == 0 || conditions[0].Type != imageapi.ImportSuccess || conditions[0].Generation != 3 {
if len(conditions) == 0 || conditions[0].Type != imageapi.ImportSuccess || string(conditions[0].Status) != "False" || conditions[0].Generation != 3 {
t.Fatalf("expected generation 3 for condition and import failed: %#v", conditions)
}

Expand Down