Skip to content

Commit

Permalink
refactor: improve responses on partial errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@pedrokiefer @cezarsa
pedrokiefer authored and cezarsa committed Jan 11, 2021
1 parent b2d9356 commit e59a26e
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 14 deletions.
4 changes: 2 additions & 2 deletions internal/pkg/rpaas/k8s.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1033,10 +1033,10 @@ func (m *k8sRpaasManager) PurgeCache(ctx context.Context, instanceName string, a
continue
}
if err = m.cacheManager.PurgeCache(podStatus.Address, args.Path, port, args.PreservePath); err != nil {
purgeErrors = multierror.Append(purgeErrors, err)
purgeErrors = multierror.Append(purgeErrors, errors.Wrapf(err, "pod %s failed", podStatus.Address))
continue
}
purgeCount += 1
purgeCount++
}
return purgeCount, purgeErrors
}
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/rpaas/k8s_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1906,7 +1906,7 @@ func Test_k8sRpaasManager_PurgeCache(t *testing.T) {
},
},
assertion: func(t *testing.T, count int, err error) {
assert.Error(t, err)
assert.EqualError(t, err, "1 error occurred:\n\t* pod 10.0.0.9 failed: some nginx error\n\n")
assert.Equal(t, 1, count)
},
},
Expand Down
11 changes: 7 additions & 4 deletions internal/purge/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/hashicorp/go-multierror"
"github.com/labstack/echo/v4"
"github.com/pkg/errors"

"github.com/tsuru/rpaas-operator/internal/pkg/rpaas"
)
Expand All @@ -28,10 +29,12 @@ func (p *purge) cachePurge(c echo.Context) error {
}

count, err := p.PurgeCache(ctx, name, args)
if err != nil {
if err != nil && count == 0 {
return err
} else if err != nil {
return c.JSON(http.StatusOK, rpaas.PurgeCacheBulkResult{Path: args.Path, InstancesPurged: count, Error: err.Error()})
}
return c.String(http.StatusOK, fmt.Sprintf("Object purged on %d servers", count))
return c.JSON(http.StatusOK, rpaas.PurgeCacheBulkResult{Path: args.Path, InstancesPurged: count})
}

func (p *purge) cachePurgeBulk(c echo.Context) error {
Expand All @@ -57,7 +60,7 @@ func (p *purge) cachePurgeBulk(c echo.Context) error {
count, err := p.PurgeCache(ctx, name, args)
if err != nil {
status = http.StatusInternalServerError
r = rpaas.PurgeCacheBulkResult{Path: args.Path, Error: err.Error()}
r = rpaas.PurgeCacheBulkResult{Path: args.Path, InstancesPurged: count, Error: err.Error()}
} else {
r = rpaas.PurgeCacheBulkResult{Path: args.Path, InstancesPurged: count}
}
Expand All @@ -84,7 +87,7 @@ func (p *purge) PurgeCache(ctx context.Context, name string, args rpaas.PurgeCac
continue
}
if err = p.cacheManager.PurgeCache(pod.Address, args.Path, port, args.PreservePath); err != nil {
purgeErrors = multierror.Append(purgeErrors, err)
purgeErrors = multierror.Append(purgeErrors, errors.Wrapf(err, "pod %s failed", pod.Address))
continue
}
purgeCount++
Expand Down
107 changes: 101 additions & 6 deletions internal/purge/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/watch"
ktesting "k8s.io/client-go/testing"

nginxManager "github.com/tsuru/rpaas-operator/internal/pkg/rpaas/nginx"
)

func bodyContent(rsp *httptest.ResponseRecorder) string {
Expand All @@ -22,9 +24,13 @@ func bodyContent(rsp *httptest.ResponseRecorder) string {
}

type fakeCacheManager struct {
purgeCacheFunc func(host, path string, port int32, preservePath bool) error
}

func (f fakeCacheManager) PurgeCache(host, path string, port int32, preservePath bool) error {
if f.purgeCacheFunc != nil {
return f.purgeCacheFunc(host, path, port, preservePath)
}
return nil
}

Expand All @@ -43,7 +49,7 @@ func TestCachePurge(t *testing.T) {
assert.NoError(t, err)

// adds pods to watcher to ensure correct behaviour for success test cases
basePod := &apiv1.Pod{
pod1 := &apiv1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod0-sample-rpaasv2",
Labels: map[string]string{
Expand All @@ -65,7 +71,32 @@ func TestCachePurge(t *testing.T) {
ContainerStatuses: []apiv1.ContainerStatus{{Ready: true}},
},
}
watchFake.Add(basePod.DeepCopy())
watchFake.Add(pod1.DeepCopy())

pod2 := &apiv1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod1-sample-rpaasv2",
Labels: map[string]string{
defaultInstanceLabel: "sample-rpaasv2",
},
ResourceVersion: "0",
},
Spec: apiv1.PodSpec{
Containers: []apiv1.Container{
{
Ports: []apiv1.ContainerPort{
{Name: "nginx-metrics", ContainerPort: 8889},
},
},
},
},
Status: apiv1.PodStatus{
PodIP: "172.0.2.2",
ContainerStatuses: []apiv1.ContainerStatus{{Ready: true}},
},
}
watchFake.Add(pod2.DeepCopy())

// Let fake watch propagate the event
time.Sleep(100 * time.Millisecond)

Expand All @@ -75,32 +106,52 @@ func TestCachePurge(t *testing.T) {
requestBody string
expectedStatus int
expectedBody string
cacheManager fakeCacheManager
}{
{
name: "success",
instance: "sample-rpaasv2",
requestBody: `{"path":"/index.html","preserve_path":true}`,
expectedStatus: http.StatusOK,
expectedBody: "Object purged on 1 servers",
expectedBody: `{"path":"/index.html","instances_purged":2}`,
cacheManager: fakeCacheManager{},
},
{
name: "fails on some servers",
instance: "sample-rpaasv2",
requestBody: `{"path":"/index.html","preserve_path":true}`,
expectedStatus: http.StatusOK,
expectedBody: `{"path":"/index.html","instances_purged":1,"error":"1 error occurred:\n\t* pod 172.0.2.2 failed: some nginx error\n\n"}`,
cacheManager: fakeCacheManager{
purgeCacheFunc: func(host, path string, port int32, preservePath bool) error {
if host == "172.0.2.2" {
return nginxManager.NginxError{Msg: "some nginx error"}
}
return nil
},
},
},
{
name: "returns bad request if body is empty",
instance: "some-instance",
requestBody: "",
expectedStatus: http.StatusBadRequest,
expectedBody: `{"message":"Request body can't be empty"}`,
cacheManager: fakeCacheManager{},
},
{
name: "returns bad request if instance is not sent",
instance: "",
requestBody: `{"path":"/index.html","preserve_path":true}`,
expectedStatus: http.StatusBadRequest,
expectedBody: "instance is required",
cacheManager: fakeCacheManager{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
api.cacheManager = tt.cacheManager

w := httptest.NewRecorder()
r, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://%s/resources/%s/purge", api.Address, tt.instance), strings.NewReader(tt.requestBody))
Expand Down Expand Up @@ -131,7 +182,7 @@ func TestCachePurgeBulk(t *testing.T) {
assert.NoError(t, err)

// adds pods to watcher to ensure correct behaviour for success test cases
basePod := &apiv1.Pod{
pod1 := &apiv1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod0-sample-rpaasv2",
Labels: map[string]string{
Expand All @@ -153,7 +204,31 @@ func TestCachePurgeBulk(t *testing.T) {
ContainerStatuses: []apiv1.ContainerStatus{{Ready: true}},
},
}
watchFake.Add(basePod.DeepCopy())
watchFake.Add(pod1.DeepCopy())

pod2 := &apiv1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod1-sample-rpaasv2",
Labels: map[string]string{
defaultInstanceLabel: "sample-rpaasv2",
},
ResourceVersion: "0",
},
Spec: apiv1.PodSpec{
Containers: []apiv1.Container{
{
Ports: []apiv1.ContainerPort{
{Name: "nginx-metrics", ContainerPort: 8889},
},
},
},
},
Status: apiv1.PodStatus{
PodIP: "172.0.2.2",
ContainerStatuses: []apiv1.ContainerStatus{{Ready: true}},
},
}
watchFake.Add(pod2.DeepCopy())
// Let fake watch propagate the event
time.Sleep(100 * time.Millisecond)

Expand All @@ -163,32 +238,52 @@ func TestCachePurgeBulk(t *testing.T) {
requestBody string
expectedStatus int
expectedBody string
cacheManager fakeCacheManager
}{
{
name: "success",
instance: "sample-rpaasv2",
requestBody: `[{"path":"/index.html","preserve_path":true}]`,
expectedStatus: http.StatusOK,
expectedBody: `[{"path":"/index.html","instances_purged":1}]`,
expectedBody: `[{"path":"/index.html","instances_purged":2}]`,
cacheManager: fakeCacheManager{},
},
{
name: "fails on some servers",
instance: "sample-rpaasv2",
requestBody: `[{"path":"/index.html","preserve_path":true},{"path":"/other.html","preserve_path":true}]`,
expectedStatus: http.StatusInternalServerError,
expectedBody: `[{"path":"/index.html","instances_purged":2},{"path":"/other.html","instances_purged":1,"error":"1 error occurred:\n\t* pod 172.0.2.2 failed: some nginx error\n\n"}]`,
cacheManager: fakeCacheManager{
purgeCacheFunc: func(host, path string, port int32, preservePath bool) error {
if host == "172.0.2.2" && path == "/other.html" {
return nginxManager.NginxError{Msg: "some nginx error"}
}
return nil
},
},
},
{
name: "returns bad request if instance is not sent",
instance: "",
requestBody: `[{"path":"/index.html","preserve_path":true}]`,
expectedStatus: http.StatusBadRequest,
expectedBody: "instance is required",
cacheManager: fakeCacheManager{},
},
{
name: "returns bad request if arguments are not a list",
instance: "",
requestBody: `{"path":"/index.html","preserve_path":true}`,
expectedStatus: http.StatusBadRequest,
expectedBody: "instance is required",
cacheManager: fakeCacheManager{},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
api.cacheManager = tt.cacheManager

w := httptest.NewRecorder()
r, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://%s/resources/%s/purge/bulk", api.Address, tt.instance), strings.NewReader(tt.requestBody))
Expand Down
4 changes: 3 additions & 1 deletion internal/web/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ func cachePurge(c echo.Context) error {
return err
}
count, err := manager.PurgeCache(ctx, name, args)
if err != nil {
if err != nil && count == 0 {
return err
} else if err != nil {
return c.String(http.StatusOK, fmt.Sprintf("Object purged on %d servers\n%v", count, err))
}
return c.String(http.StatusOK, fmt.Sprintf("Object purged on %d servers", count))
}
Expand Down
12 changes: 12 additions & 0 deletions internal/web/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@ func Test_cachePurge(t *testing.T) {
},
},
},
{
description: "returns OK with the total number of servers where the cache was successfully purged and error where it failed",
instanceName: "my-instance",
requestBody: "path=/index.html&preserve_path=true",
expectedCode: http.StatusOK,
expectedBody: "Object purged on 2 servers\nSomething was not found",
manager: &fake.RpaasManager{
FakePurgeCache: func(instanceName string, args rpaas.PurgeCacheArgs) (int, error) {
return 2, rpaas.NotFoundError{Msg: "Something was not found"}
},
},
},
}

for _, tt := range testCases {
Expand Down

0 comments on commit e59a26e

Please sign in to comment.