From 75417ba3e477d052b53d1d420760fcc5fa94efba Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Thu, 5 May 2016 15:57:25 -0700 Subject: [PATCH 1/3] Retry fetching http urls passed to '-f' in kubectl. Closes #24089. --- pkg/kubectl/resource/visitor.go | 58 ++++++++++++++++--- pkg/kubectl/resource/visitor_test.go | 83 ++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 pkg/kubectl/resource/visitor_test.go diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index be138b210e6..4654d80cda6 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/pkg/kubectl/resource/visitor.go @@ -24,6 +24,7 @@ import ( "net/url" "os" "path/filepath" + "time" "k8s.io/kubernetes/pkg/api/meta" "k8s.io/kubernetes/pkg/api/unversioned" @@ -224,19 +225,62 @@ type URLVisitor struct { } func (v *URLVisitor) Visit(fn VisitorFunc) error { - res, err := http.Get(v.URL.String()) + body, err := v.readHttpWithRetries(httpgetImpl, time.Second, v.URL.String()) if err != nil { return err } - defer res.Body.Close() - if res.StatusCode != 200 { - return fmt.Errorf("unable to read URL %q, server reported %d %s", v.URL, res.StatusCode, res.Status) - } - - v.StreamVisitor.Reader = res.Body + defer body.Close() + v.StreamVisitor.Reader = body return v.StreamVisitor.Visit(fn) } +// readHttpWithRetries tries to http.Get the v.URL 3 times before giving up. +func (v *URLVisitor) readHttpWithRetries(get httpget, duration time.Duration, u string) (io.ReadCloser, error) { + var err error + var body io.ReadCloser + for i := 0; i < 3; i++ { + var statusCode int + var status string + if i > 0 { + time.Sleep(duration) + } + + // Try to get the URL + statusCode, status, body, err = get(u) + + // Retry Errors + if err != nil { + continue + } + + // Error - Set the error condition from the StatusCode + if statusCode != 200 { + err = fmt.Errorf("unable to read URL %q, server reported %d %s", u, statusCode, status) + } + + if statusCode >= 500 && statusCode < 600 { + // Retry 500's + continue + } else { + // Don't retry other StatusCodes + break + } + } + return body, err +} + +// httpget Defines function to retrieve a url and return the results. Exists for unit test stubbing. +type httpget func(url string) (int, string, io.ReadCloser, error) + +// httpgetImpl Implements a function to retrieve a url and return the results. +func httpgetImpl(url string) (int, string, io.ReadCloser, error) { + resp, err := http.Get(url) + if err != nil { + return 0, "", nil, err + } + return resp.StatusCode, resp.Status, resp.Body, nil +} + // DecoratedVisitor will invoke the decorators in order prior to invoking the visitor function // passed to Visit. An error will terminate the visit. type DecoratedVisitor struct { diff --git a/pkg/kubectl/resource/visitor_test.go b/pkg/kubectl/resource/visitor_test.go new file mode 100644 index 00000000000..b025b775b08 --- /dev/null +++ b/pkg/kubectl/resource/visitor_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package resource + +import ( + "bytes" + "fmt" + "io" + "io/ioutil" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestVisitorHttpGet(t *testing.T) { + instance := &URLVisitor{} + + // Test retries on errors + i := 0 + expectedErr := fmt.Errorf("Failed to get http") + actualBytes, actualErr := instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + assert.Equal(t, "hello", url) + i++ + if i > 2 { + return 0, "", nil, expectedErr + } + return 0, "", nil, fmt.Errorf("Unexpected error") + }, 0, "hello") + assert.Equal(t, expectedErr, actualErr) + assert.Nil(t, actualBytes) + assert.Equal(t, 3, i) + + // Test that 500s are retried. + i = 0 + actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + assert.Equal(t, "hello", url) + i++ + return 501, "Status", nil, nil + }, 0, "hello") + assert.Error(t, actualErr) + assert.Nil(t, actualBytes) + assert.Equal(t, 3, i) + + // Test that 300s are not retried + i = 0 + actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + assert.Equal(t, "hello", url) + i++ + return 300, "Status", nil, nil + }, 0, "hello") + assert.Error(t, actualErr) + assert.Nil(t, actualBytes) + assert.Equal(t, 1, i) + + // Test Success + i = 0 + b := bytes.Buffer{} + actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + assert.Equal(t, "hello", url) + i++ + if i > 1 { + return 200, "Status", ioutil.NopCloser(&b), nil + } + return 501, "Status", nil, nil + }, 0, "hello") + assert.Nil(t, actualErr) + assert.NotNil(t, actualBytes) + assert.Equal(t, 2, i) +} From 3d08c73767a1f49f034ebb35cd6e91bb0dd4ed45 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Mon, 9 May 2016 15:57:08 -0700 Subject: [PATCH 2/3] Address PR Comments 1 --- pkg/kubectl/resource/visitor.go | 4 ++-- pkg/kubectl/resource/visitor_test.go | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index 4654d80cda6..e29513dbd0d 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/pkg/kubectl/resource/visitor.go @@ -225,7 +225,7 @@ type URLVisitor struct { } func (v *URLVisitor) Visit(fn VisitorFunc) error { - body, err := v.readHttpWithRetries(httpgetImpl, time.Second, v.URL.String()) + body, err := readHttpWithRetries(httpgetImpl, time.Second, v.URL.String()) if err != nil { return err } @@ -235,7 +235,7 @@ func (v *URLVisitor) Visit(fn VisitorFunc) error { } // readHttpWithRetries tries to http.Get the v.URL 3 times before giving up. -func (v *URLVisitor) readHttpWithRetries(get httpget, duration time.Duration, u string) (io.ReadCloser, error) { +func readHttpWithRetries(get httpget, duration time.Duration, u string) (io.ReadCloser, error) { var err error var body io.ReadCloser for i := 0; i < 3; i++ { diff --git a/pkg/kubectl/resource/visitor_test.go b/pkg/kubectl/resource/visitor_test.go index b025b775b08..c69231e9ff3 100644 --- a/pkg/kubectl/resource/visitor_test.go +++ b/pkg/kubectl/resource/visitor_test.go @@ -27,12 +27,10 @@ import ( ) func TestVisitorHttpGet(t *testing.T) { - instance := &URLVisitor{} - // Test retries on errors i := 0 expectedErr := fmt.Errorf("Failed to get http") - actualBytes, actualErr := instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + actualBytes, actualErr := readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { assert.Equal(t, "hello", url) i++ if i > 2 { @@ -46,7 +44,7 @@ func TestVisitorHttpGet(t *testing.T) { // Test that 500s are retried. i = 0 - actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { assert.Equal(t, "hello", url) i++ return 501, "Status", nil, nil @@ -57,7 +55,7 @@ func TestVisitorHttpGet(t *testing.T) { // Test that 300s are not retried i = 0 - actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { assert.Equal(t, "hello", url) i++ return 300, "Status", nil, nil @@ -69,7 +67,7 @@ func TestVisitorHttpGet(t *testing.T) { // Test Success i = 0 b := bytes.Buffer{} - actualBytes, actualErr = instance.readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { assert.Equal(t, "hello", url) i++ if i > 1 { From b9f9d130d766b353e900f3be4aafe899af8ae803 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Mon, 9 May 2016 16:19:39 -0700 Subject: [PATCH 3/3] Address PR comments 2 --- pkg/kubectl/resource/builder.go | 11 ++++++---- pkg/kubectl/resource/visitor.go | 12 +++++++---- pkg/kubectl/resource/visitor_test.go | 31 +++++++++++++++++++++++----- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/pkg/kubectl/resource/builder.go b/pkg/kubectl/resource/builder.go index 0a40e7b21ba..8853be8a9f5 100644 --- a/pkg/kubectl/resource/builder.go +++ b/pkg/kubectl/resource/builder.go @@ -36,6 +36,8 @@ import ( var FileExtensions = []string{".json", ".yaml", ".yml"} var InputExtensions = append(FileExtensions, "stdin") +const defaultHttpGetAttempts int = 3 + // Builder provides convenience functions for taking arguments and parameters // from the command line and converting them to a list of resources to iterate // over using the Visitor interface. @@ -109,7 +111,7 @@ func (b *Builder) FilenameParam(enforceNamespace, recursive bool, paths ...strin b.errs = append(b.errs, fmt.Errorf("the URL passed to filename %q is not valid: %v", s, err)) continue } - b.URL(url) + b.URL(defaultHttpGetAttempts, url) default: b.Path(recursive, s) } @@ -123,11 +125,12 @@ func (b *Builder) FilenameParam(enforceNamespace, recursive bool, paths ...strin } // URL accepts a number of URLs directly. -func (b *Builder) URL(urls ...*url.URL) *Builder { +func (b *Builder) URL(httpAttemptCount int, urls ...*url.URL) *Builder { for _, u := range urls { b.paths = append(b.paths, &URLVisitor{ - URL: u, - StreamVisitor: NewStreamVisitor(nil, b.mapper, u.String(), b.schema), + URL: u, + StreamVisitor: NewStreamVisitor(nil, b.mapper, u.String(), b.schema), + HttpAttemptCount: httpAttemptCount, }) } return b diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index e29513dbd0d..288bfa85aca 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/pkg/kubectl/resource/visitor.go @@ -222,10 +222,11 @@ func ValidateSchema(data []byte, schema validation.Schema) error { type URLVisitor struct { URL *url.URL *StreamVisitor + HttpAttemptCount int } func (v *URLVisitor) Visit(fn VisitorFunc) error { - body, err := readHttpWithRetries(httpgetImpl, time.Second, v.URL.String()) + body, err := readHttpWithRetries(httpgetImpl, time.Second, v.URL.String(), v.HttpAttemptCount) if err != nil { return err } @@ -234,11 +235,14 @@ func (v *URLVisitor) Visit(fn VisitorFunc) error { return v.StreamVisitor.Visit(fn) } -// readHttpWithRetries tries to http.Get the v.URL 3 times before giving up. -func readHttpWithRetries(get httpget, duration time.Duration, u string) (io.ReadCloser, error) { +// readHttpWithRetries tries to http.Get the v.URL retries times before giving up. +func readHttpWithRetries(get httpget, duration time.Duration, u string, attempts int) (io.ReadCloser, error) { var err error var body io.ReadCloser - for i := 0; i < 3; i++ { + if attempts <= 0 { + return nil, fmt.Errorf("http attempts must be greater than 0, was %d", attempts) + } + for i := 0; i < attempts; i++ { var statusCode int var status string if i > 0 { diff --git a/pkg/kubectl/resource/visitor_test.go b/pkg/kubectl/resource/visitor_test.go index c69231e9ff3..e781c90c249 100644 --- a/pkg/kubectl/resource/visitor_test.go +++ b/pkg/kubectl/resource/visitor_test.go @@ -37,7 +37,7 @@ func TestVisitorHttpGet(t *testing.T) { return 0, "", nil, expectedErr } return 0, "", nil, fmt.Errorf("Unexpected error") - }, 0, "hello") + }, 0, "hello", 3) assert.Equal(t, expectedErr, actualErr) assert.Nil(t, actualBytes) assert.Equal(t, 3, i) @@ -48,7 +48,7 @@ func TestVisitorHttpGet(t *testing.T) { assert.Equal(t, "hello", url) i++ return 501, "Status", nil, nil - }, 0, "hello") + }, 0, "hello", 3) assert.Error(t, actualErr) assert.Nil(t, actualBytes) assert.Equal(t, 3, i) @@ -59,14 +59,35 @@ func TestVisitorHttpGet(t *testing.T) { assert.Equal(t, "hello", url) i++ return 300, "Status", nil, nil - }, 0, "hello") + }, 0, "hello", 3) assert.Error(t, actualErr) assert.Nil(t, actualBytes) assert.Equal(t, 1, i) - // Test Success + // Test attempt count is respected + i = 0 + actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + assert.Equal(t, "hello", url) + i++ + return 501, "Status", nil, nil + }, 0, "hello", 1) + assert.Error(t, actualErr) + assert.Nil(t, actualBytes) + assert.Equal(t, 1, i) + + // Test attempts less than 1 results in an error i = 0 b := bytes.Buffer{} + actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { + return 200, "Status", ioutil.NopCloser(&b), nil + }, 0, "hello", 0) + assert.Error(t, actualErr) + assert.Nil(t, actualBytes) + assert.Equal(t, 0, i) + + // Test Success + i = 0 + b = bytes.Buffer{} actualBytes, actualErr = readHttpWithRetries(func(url string) (int, string, io.ReadCloser, error) { assert.Equal(t, "hello", url) i++ @@ -74,7 +95,7 @@ func TestVisitorHttpGet(t *testing.T) { return 200, "Status", ioutil.NopCloser(&b), nil } return 501, "Status", nil, nil - }, 0, "hello") + }, 0, "hello", 3) assert.Nil(t, actualErr) assert.NotNil(t, actualBytes) assert.Equal(t, 2, i)