Repository: puppetlabs/wash
Pull request #557 (2019-10-23 05:11:44)

Update Delete to handle long-running deletions


Patch:
diff --git a/api/client/client.go b/api/client/client.go
index 21bac8e..58bf0ce 100644
--- a/api/client/client.go
+++ b/api/client/client.go
@@ -39,7 +39,7 @@ type Client interface {
 	// A "nil" schema means that the schema's unknown.
 	Schema(path string) (*apitypes.EntrySchema, error)
 	Screenview(name string, params analytics.Params) error
-	Delete(path string) error
+	Delete(path string) (bool, error)
 }
 
 // A domainSocketClient is a wash API client.
@@ -111,25 +111,29 @@ func (c *domainSocketClient) doRequest(method, endpoint string, params url.Value
 	return nil, unmarshalErrorResp(resp)
 }
 
-func (c *domainSocketClient) getRequest(endpoint string, params url.Values, result interface{}) error {
-	respBody, err := c.doRequest(http.MethodGet, endpoint, params, nil)
+func (c *domainSocketClient) doRequestAndParseJSONBody(method, endpoint string, params url.Values, body io.Reader, result interface{}) error {
+	respBody, err := c.doRequest(method, endpoint, params, body)
 	if err != nil {
 		return err
 	}
 
 	defer func() { errz.Log(respBody.Close()) }()
-	body, err := ioutil.ReadAll(respBody)
+	respBodyBytes, err := ioutil.ReadAll(respBody)
 	if err != nil {
 		return err
 	}
 
-	if err := json.Unmarshal(body, result); err != nil {
-		return fmt.Errorf("Non-JSON body at %v: %v", endpoint, string(body))
+	if err := json.Unmarshal(respBodyBytes, result); err != nil {
+		return fmt.Errorf("Non-JSON body at %v: %v", endpoint, string(respBodyBytes))
 	}
 
 	return nil
 }
 
+func (c *domainSocketClient) getRequest(endpoint string, params url.Values, result interface{}) error {
+	return c.doRequestAndParseJSONBody(http.MethodGet, endpoint, params, nil, result)
+}
+
 // Info retrieves the information of the resource located at "path"
 func (c *domainSocketClient) Info(path string) (apitypes.Entry, error) {
 	var e apitypes.Entry
@@ -295,7 +299,8 @@ func (c *domainSocketClient) Screenview(name string, params analytics.Params) er
 }
 
 // Delete deletes the entry at "path"
-func (c *domainSocketClient) Delete(path string) error {
-	_, err := c.doRequest(http.MethodDelete, "/fs/delete", url.Values{"path": []string{path}}, nil)
-	return err
+func (c *domainSocketClient) Delete(path string) (bool, error) {
+	var deleted bool
+	err := c.doRequestAndParseJSONBody(http.MethodDelete, "/fs/delete", url.Values{"path": []string{path}}, nil, &deleted)
+	return deleted, err
 }
diff --git a/api/delete.go b/api/delete.go
index a418e5b..c4562f8 100644
--- a/api/delete.go
+++ b/api/delete.go
@@ -1,6 +1,8 @@
 package api
 
 import (
+	"encoding/json"
+	"fmt"
 	"net/http"
 
 	"github.com/puppetlabs/wash/activity"
@@ -14,7 +16,7 @@ import (
 //     Schemes: http
 //
 //     Responses:
-//       200
+//       200: bool
 //       400: errorResp
 //       404: errorResp
 //       500: errorResp
@@ -27,10 +29,14 @@ var deleteHandler handler = func(w http.ResponseWriter, r *http.Request) *errorR
 	if !plugin.DeleteAction().IsSupportedOn(entry) {
 		return unsupportedActionResponse(path, plugin.DeleteAction())
 	}
-	err := plugin.Delete(ctx, entry.(plugin.Deletable))
+	deleted, err := plugin.Delete(ctx, entry.(plugin.Deletable))
 	if err != nil {
 		return erroredActionResponse(path, plugin.DeleteAction(), err.Error())
 	}
-	activity.Record(ctx, "API: Delete %v", path)
+	activity.Record(ctx, "API: Delete %v %v", path, deleted)
+	jsonEncoder := json.NewEncoder(w)
+	if err = jsonEncoder.Encode(deleted); err != nil {
+		return unknownErrorResponse(fmt.Errorf("Could not marshal delete's result for %v: %v", path, err))
+	}
 	return nil
 }
diff --git a/cmd/delete.go b/cmd/delete.go
index 57dd99f..133f63e 100644
--- a/cmd/delete.go
+++ b/cmd/delete.go
@@ -64,13 +64,18 @@ func deleteMain(cmd *cobra.Command, args []string) exitCode {
 	for _, path := range pathsToDelete {
 		wg.Add(1)
 		go func(path string) {
-			if err := conn.Delete(path); err != nil {
-				writeMux.Lock()
-				defer writeMux.Unlock()
+			defer wg.Done()
+			deleted, err := conn.Delete(path)
+			writeMux.Lock()
+			defer writeMux.Unlock()
+			if err != nil {
 				cmdutil.ErrPrintf("%v\n", err)
 				anyFailed = true
+				return
+			}
+			if !deleted {
+				cmdutil.Printf("%v has been marked for deletion and will eventually be deleted\n", path)
 			}
-			wg.Done()
 		}(path)
 	}
 	wg.Wait()
diff --git a/docs/docs/external-plugins.md b/docs/docs/external-plugins.md
index 7368a7a..ca65ca5 100644
--- a/docs/docs/external-plugins.md
+++ b/docs/docs/external-plugins.md
@@ -11,6 +11,7 @@ title: External Plugins
     - [stream](#stream)
     - [exec](#exec)
     - [schema](#schema)
+    - [delete](#delete)
     - [Entry JSON object](#entry-json-object)
     - [Entry schema graph JSON object](#entry-schema-graph-json-object)
     - [Errors](#errors)
@@ -208,6 +209,21 @@ bash-3.2$ /path/to/myplugin.rb schema /myplugin/foo ''
 }
 ```
 
+## delete
+`<plugin_script> delete <path> <state>`
+
+When `delete` is invoked, the script must output a boolean JSON. `true` means that the entry was deleted. `false` means that the entry is marked for deletion and will eventually be deleted by the plugin's API.
+
+`delete` should ensure that both the entry and its children are removed. If the entry has any dependencies that need to be deleted, then `delete` should error.
+
+**Note:** If you anticipate `delete` taking a long time (> 30 seconds), then output `false`.
+
+### Examples
+```
+bash-3.2$ /path/to/myplugin.rb delete /myplugin/foo ''
+true
+```
+
 ## Entry JSON object
 This section describes the JSON object representing a serialized entry. An entry JSON object supports the following keys. Only the `name` and `methods` keys are required.
 
diff --git a/plugin/analyticsWrappers.go b/plugin/analyticsWrappers.go
index cb049bc..5d2b112 100644
--- a/plugin/analyticsWrappers.go
+++ b/plugin/analyticsWrappers.go
@@ -37,7 +37,7 @@ func ExecWithAnalytics(ctx context.Context, e Execable, cmd string, args []strin
 
 // DeleteWithAnalytics is a wrapper to plugin.Delete. Use it when you need to report a
 // 'Delete' invocation to analytics. Otherwise, use plugin.Delete.
-func DeleteWithAnalytics(ctx context.Context, d Deletable) error {
+func DeleteWithAnalytics(ctx context.Context, d Deletable) (bool, error) {
 	submitMethodInvocation(ctx, d, "Delete")
 	return Delete(ctx, d)
 }
diff --git a/plugin/cache.go b/plugin/cache.go
index 309991f..3bb5d3e 100644
--- a/plugin/cache.go
+++ b/plugin/cache.go
@@ -60,10 +60,23 @@ var opNameRegex = regexp.MustCompile("^[a-zA-Z]+$")
 
 const opQualifier = "^[a-zA-Z]+::"
 
-// This method exists to simplify ClearCacheFor's tests.
-// Specifically, it lets us decouple the regex's correctness
-// from the cache's implementation.
-func opKeysRegex(path string) *regexp.Regexp {
+// opKeyRegex returns a regex that matches <op>::<path>
+func opKeyRegex(op string, path string) *regexp.Regexp {
+	opRegex := "^" + regexp.QuoteMeta(op+"::")
+
+	var expr string
+	if path == "/" {
+		expr = opRegex + "/$"
+	} else {
+		expr = opRegex + "/" + regexp.QuoteMeta(strings.Trim(path, "/")) + "$"
+	}
+
+	return regexp.MustCompile(expr)
+}
+
+// This returns a regex that matches <op>::<path> and <op>::<child_path>
+// where <child_path> is a descendant of <path>.
+func allOpKeysIncludingChildrenRegex(path string) *regexp.Regexp {
 	var expr string
 	if path == "/" {
 		expr = opQualifier + "/.*"
@@ -80,7 +93,7 @@ func opKeysRegex(path string) *regexp.Regexp {
 // TODO: If path == "/", we could optimize this by calling cache.Flush(). Not important
 // right now, but may be worth considering in the future.
 func ClearCacheFor(path string) []string {
-	rx := opKeysRegex(path)
+	rx := allOpKeysIncludingChildrenRegex(path)
 	return cache.Delete(rx)
 }
 
diff --git a/plugin/externalPluginEntry.go b/plugin/externalPluginEntry.go
index 447341b..629685d 100644
--- a/plugin/externalPluginEntry.go
+++ b/plugin/externalPluginEntry.go
@@ -375,9 +375,22 @@ func (e *externalPluginEntry) Metadata(ctx context.Context) (JSONObject, error)
 	return metadata, nil
 }
 
-func (e *externalPluginEntry) Delete(ctx context.Context) error {
-	_, err := e.script.InvokeAndWait(ctx, "delete", e)
-	return err
+func (e *externalPluginEntry) Delete(ctx context.Context) (deleted bool, err error) {
+	inv, err := e.script.InvokeAndWait(ctx, "delete", e)
+	if err != nil {
+		return
+	}
+	if err = json.Unmarshal(inv.stdout.Bytes(), &deleted); err != nil {
+		err = newStdoutDecodeErr(
+			ctx,
+			"delete's result",
+			err,
+			inv,
+			"true",
+		)
+		return
+	}
+	return
 }
 
 func (e *externalPluginEntry) Stream(ctx context.Context) (io.ReadCloser, error) {
diff --git a/plugin/methodWrappers.go b/plugin/methodWrappers.go
index 137326b..079a17f 100644
--- a/plugin/methodWrappers.go
+++ b/plugin/methodWrappers.go
@@ -139,18 +139,35 @@ func Stream(ctx context.Context, s Streamable) (io.ReadCloser, error) {
 }
 
 // Delete deletes the given entry.
-func Delete(ctx context.Context, d Deletable) error {
-	if err := d.Delete(ctx); err != nil {
-		return err
+func Delete(ctx context.Context, d Deletable) (deleted bool, err error) {
+	deleted, err = d.Delete(ctx)
+	if err != nil {
+		return
 	}
+
+	// Delete was successful, so update the cache to ensure that fresh data's loaded
+	// when needed. This includes:
+	//   * Clearing the entry and its children's cache.
+	//   * Updating the parent's cached list result.
 	ClearCacheFor(d.id())
-	// Delete this entry from the parent's cached list result
 	segments := strings.Split(d.id(), "/")
 	parentID := strings.Join(segments[:len(segments)-1], "/")
+	listOpName := defaultOpCodeToNameMap[ListOp]
 	entries, _ := cache.Get(defaultOpCodeToNameMap[ListOp], parentID)
-	if entries != nil {
+	if entries == nil {
+		return
+	}
+	if deleted {
+		// The entry was deleted, so delete the entry from the parent's cached list
+		// result.
 		cname := segments[len(segments)-1]
 		entries.(*EntryMap).Delete(cname)
+	} else {
+		// The entry will eventually be deleted. However it's likely that Delete
+		// did update the entry (e.g. on VMs, Delete causes a state transition).
+		// Thus, we clear the parent's cached list result to ensure fresh data.
+		cache.Delete(opKeyRegex(listOpName, parentID))
 	}
-	return nil
+
+	return
 }
diff --git a/plugin/types.go b/plugin/types.go
index e1b58f2..20708bf 100644
--- a/plugin/types.go
+++ b/plugin/types.go
@@ -199,7 +199,12 @@ type Readable interface {
 // should ensure that it and all its children are removed. If the entry has
 // any dependencies that need to be deleted, then Delete should return an
 // error.
+//
+// If Delete returns true, then that means the entry was deleted. If Delete
+// returns false, then that means the entry is marked for deletion by the
+// plugin's API. You should return false if you anticipate delete taking a long
+// time (> 30 seconds).
 type Deletable interface {
 	Entry
-	Delete(context.Context) error
+	Delete(context.Context) (bool, error)
 }


Test patch:
diff --git a/cmd/internal/cmdtest/client.go b/cmd/internal/cmdtest/client.go
index df99e2e..305209f 100644
--- a/cmd/internal/cmdtest/client.go
+++ b/cmd/internal/cmdtest/client.go
@@ -75,7 +75,7 @@ func (c *MockClient) Screenview(name string, params analytics.Params) error {
 }
 
 // Delete mocks Client#Delete
-func (c *MockClient) Delete(path string) error {
+func (c *MockClient) Delete(path string) (bool, error) {
 	args := c.Called(path)
-	return args.Error(0)
+	return args.Get(0).(bool), args.Error(1)
 }
diff --git a/plugin/cache_test.go b/plugin/cache_test.go
index 8bede6e..e6fe479 100644
--- a/plugin/cache_test.go
+++ b/plugin/cache_test.go
@@ -77,8 +77,8 @@ func (suite *CacheTestSuite) TestOpNameRegex() {
 	suite.NotRegexp(opNameRegex, "abc  ")
 }
 
-func (suite *CacheTestSuite) TestOpKeysRegex() {
-	rx := opKeysRegex("/a")
+func (suite *CacheTestSuite) TestAllOpKeysIncludingChildrenRegex() {
+	rx := allOpKeysIncludingChildrenRegex("/a")
 
 	// Test that it matches children
 	suite.Regexp(rx, "Test::/a/b")
@@ -92,21 +92,54 @@ func (suite *CacheTestSuite) TestOpKeysRegex() {
 	suite.NotRegexp(rx, "Test::/bc/d")
 
 	// Test that it matches root, and children of root
-	rx = opKeysRegex("/")
+	rx = allOpKeysIncludingChildrenRegex("/")
 	suite.Regexp(rx, "Test::/")
 	suite.Regexp(rx, "Test::/a")
 	suite.Regexp(rx, "Test::/a/b")
 
 	// Test that it matches a path containing regex characters
-	rx = opKeysRegex("/foo*[]")
+	rx = allOpKeysIncludingChildrenRegex("/foo*[]")
 	suite.Regexp(rx, "Test::/foo*[]")
 	suite.Regexp(rx, "Test::/foo*[]/bar(")
 	suite.Regexp(rx, "Test::/foo*[]/bar(/baz)")
 }
 
+func (suite *CacheTestSuite) TestOpKeyRegex() {
+	rx := opKeyRegex("Test", "/a")
+
+	// Test that it matches <op>::<path>, that it does not match
+	// <otherOp>::<path>, and that it does not match any children.
+	suite.Regexp(rx, "Test::/a")
+	suite.NotRegexp(rx, "OtherTest::/a")
+	suite.NotRegexp(rx, "TestOther::/a")
+	suite.NotRegexp(rx, "List::/a")
+	suite.NotRegexp(rx, "Test::/a/b")
+
+	// Test that it does not match any other entries
+	suite.NotRegexp(rx, "Test::/")
+	suite.NotRegexp(rx, "Test::/ab")
+	suite.NotRegexp(rx, "Test::/bc/d")
+
+	// Repeat for root
+	rx = opKeyRegex("Test", "/")
+	suite.Regexp(rx, "Test::/")
+	suite.NotRegexp(rx, "OtherTest::/")
+	suite.NotRegexp(rx, "TestOther::/")
+	suite.NotRegexp(rx, "List::/")
+	suite.NotRegexp(rx, "Test::/a")
+
+	// Repeat for a path with regex characters
+	rx = opKeyRegex("Test", "/foo*[]")
+	suite.Regexp(rx, "Test::/foo*[]")
+	suite.NotRegexp(rx, "OtherTest::/foo*[]")
+	suite.NotRegexp(rx, "TestOther::/foo*[]")
+	suite.NotRegexp(rx, "List::/foo*[]")
+	suite.NotRegexp(rx, "Test::/foo*[]/a")
+}
+
 func (suite *CacheTestSuite) TestClearCache() {
 	path := "/a"
-	rx := opKeysRegex(path)
+	rx := allOpKeysIncludingChildrenRegex(path)
 
 	suite.cache.On("Delete", rx).Return([]string{"/a"})
 	deleted := ClearCacheFor(path)
diff --git a/plugin/externalPluginEntry_test.go b/plugin/externalPluginEntry_test.go
index 16fc9d0..2ebde12 100644
--- a/plugin/externalPluginEntry_test.go
+++ b/plugin/externalPluginEntry_test.go
@@ -699,20 +699,29 @@ func (suite *ExternalPluginEntryTestSuite) TestDelete() {
 	entry.SetTestID("/foo")
 
 	ctx := context.Background()
-	mockInvokeAndWait := func(err error) {
-		mockScript.OnInvokeAndWait(ctx, "delete", entry).Return(mockInvocation([]byte{}), err).Once()
+	mockInvokeAndWait := func(stdout []byte, err error) {
+		mockScript.OnInvokeAndWait(ctx, "delete", entry).Return(mockInvocation(stdout), err).Once()
 	}
 
 	// Test that if InvokeAndWait errors, then Delete returns its error
 	mockErr := fmt.Errorf("execution error")
-	mockInvokeAndWait(mockErr)
-	err := entry.Delete(ctx)
+	mockInvokeAndWait([]byte{}, mockErr)
+	_, err := entry.Delete(ctx)
 	suite.EqualError(mockErr, err.Error())
 
-	// Test that Delete properly deletes the entry
-	mockInvokeAndWait(nil)
-	err = entry.Delete(ctx)
-	suite.NoError(err)
+	// Test that Delete returns an error if stdout does not have the right
+	// output format
+	mockInvokeAndWait([]byte("bad format"), nil)
+	_, err = entry.Delete(ctx)
+	suite.Regexp(regexp.MustCompile("stdout"), err)
+
+	// Test that Delete properly deletes the entry and decodes the result
+	// from stdout
+	mockInvokeAndWait([]byte("true"), nil)
+	deleted, err := entry.Delete(ctx)
+	if suite.NoError(err) {
+		suite.True(deleted)
+	}
 }
 
 // TODO: Add tests for stdoutStreamer, Stream and Exec
diff --git a/plugin/methodWrappers_test.go b/plugin/methodWrappers_test.go
index ae68fcd..5574495 100644
--- a/plugin/methodWrappers_test.go
+++ b/plugin/methodWrappers_test.go
@@ -65,9 +65,9 @@ func (m *methodWrappersTestsMockEntry) List(ctx context.Context) ([]Entry, error
 	return args.Get(0).([]Entry), args.Error(1)
 }
 
-func (m *methodWrappersTestsMockEntry) Delete(ctx context.Context) error {
+func (m *methodWrappersTestsMockEntry) Delete(ctx context.Context) (bool, error) {
 	args := m.Called(ctx)
-	return args.Error(0)
+	return args.Get(0).(bool), args.Error(1)
 }
 
 func (m *methodWrappersTestsMockEntry) Open(ctx context.Context) (SizedReader, error) {
@@ -106,56 +106,80 @@ func (suite *MethodWrappersTestSuite) TestDelete_ReturnsDeleteError() {
 	e := newMethodWrappersTestsMockEntry("foo")
 
 	expectedErr := fmt.Errorf("an error")
-	e.On("Delete", ctx).Return(expectedErr)
+	e.On("Delete", ctx).Return(false, expectedErr)
 
-	err := Delete(ctx, e)
+	_, err := Delete(ctx, e)
 	suite.Equal(expectedErr, err)
 }
 
-func (suite *MethodWrappersTestSuite) TestDelete_DeletesEntry() {
-	ctx := context.Background()
-	e := newMethodWrappersTestsMockEntry("foo")
-	e.On("Delete", ctx).Return(nil)
+func (suite *MethodWrappersTestSuite) TestDelete_EntryDeletionInProgress_UpdatesCache() {
+	e := newMethodWrappersTestsMockEntry("bar")
+	e.SetTestID("/foo/bar")
+	e.On("Delete", mock.Anything).Return(false, nil)
 
-	suite.cache.On("Get", mock.Anything, mock.Anything).Return(nil, nil)
-	suite.cache.On("Delete", mock.Anything).Return([]string{})
+	suite.cache.On("Delete", allOpKeysIncludingChildrenRegex(e.id())).Return([]string{})
+	suite.cache.On("Get", "List", "/foo").Return(newEntryMap(), nil)
+	suite.cache.On("Delete", opKeyRegex("List", "/foo")).Return([]string{})
 
-	err := Delete(ctx, e)
+	deleted, err := Delete(context.Background(), e)
 	if suite.NoError(err) {
+		suite.False(deleted)
 		e.AssertExpectations(suite.T())
+		suite.cache.AssertExpectations(suite.T())
 	}
 }
 
-func (suite *MethodWrappersTestSuite) TestDelete_ClearsEntryCache() {
-	e := newMethodWrappersTestsMockEntry("foo")
-	e.On("Delete", mock.Anything).Return(nil)
+func (suite *MethodWrappersTestSuite) TestDelete_EntryDeletionInProgress_NoCachedListResult_IgnoresParentCache() {
+	e := newMethodWrappersTestsMockEntry("bar")
+	e.SetTestID("/foo/bar")
+	e.On("Delete", mock.Anything).Return(false, nil)
 
-	suite.cache.On("Get", mock.Anything, mock.Anything).Return(nil, nil)
-	suite.cache.On("Delete", opKeysRegex(e.id())).Return([]string{})
+	suite.cache.On("Delete", mock.Anything).Return([]string{})
+	suite.cache.On("Get", "List", "/foo").Return(nil, nil)
 
-	err := Delete(context.Background(), e)
+	deleted, err := Delete(context.Background(), e)
 	if suite.NoError(err) {
-		suite.cache.AssertExpectations(suite.T())
+		suite.False(deleted)
+		suite.cache.AssertNotCalled(suite.T(), "Delete", opKeyRegex("List", "/foo"))
 	}
 }
 
-func (suite *MethodWrappersTestSuite) TestDelete_DeletesEntryFromParentsCachedEntryMap() {
+func (suite *MethodWrappersTestSuite) TestDelete_DeletedEntry_UpdatesCache() {
 	e := newMethodWrappersTestsMockEntry("bar")
 	e.SetTestID("/foo/bar")
-	e.On("Delete", mock.Anything).Return(nil)
+	e.On("Delete", mock.Anything).Return(true, nil)
 
 	entryMap := newEntryMap()
 	entryMap.mp["bar"] = e
+	suite.cache.On("Delete", allOpKeysIncludingChildrenRegex(e.id())).Return([]string{})
 	suite.cache.On("Get", "List", "/foo").Return(entryMap, nil)
-	suite.cache.On("Delete", mock.Anything).Return([]string{})
 
-	err := Delete(context.Background(), e)
+	deleted, err := Delete(context.Background(), e)
 	if suite.NoError(err) {
-		suite.cache.AssertCalled(suite.T(), "Get", "List", "/foo")
+		suite.True(deleted)
+		e.AssertExpectations(suite.T())
+		suite.cache.AssertExpectations(suite.T())
 		suite.NotContains(entryMap.mp, "bar")
 	}
 }
 
+func (suite *MethodWrappersTestSuite) TestDelete_DeletedEntry_NoCachedListResult_IgnoresParentCache() {
+	e := newMethodWrappersTestsMockEntry("bar")
+	e.SetTestID("/foo/bar")
+	e.On("Delete", mock.Anything).Return(true, nil)
+
+	suite.cache.On("Delete", mock.Anything).Return([]string{})
+	suite.cache.On("Get", "List", "/foo").Return(nil, nil)
+
+	deleted, err := Delete(context.Background(), e)
+	if suite.NoError(err) {
+		suite.True(deleted)
+		// If Delete did not ignore the parent cache, then there'd be a nil pointer panic
+		// because Delete calls EntryMap#Delete. Thus if we get to this point, that means
+		// a panic did not occur so the test passed.
+	}
+}
+
 func TestMethodWrappers(t *testing.T) {
 	suite.Run(t, new(MethodWrappersTestSuite))
 }