Missing error handling in batch APIs

[embano1]

Currently only one VAPI batch operation is supported, i.e. attach-tag-to-multiple-objects. This VAPI operation returns a 200 even in case of partial tagging operation failures. This can lead to inconsistencies when assigning a tag to multiple objects, e.g. partial results. The caller is actually not aware of such an result because we don't inspect the response body (nil) from VAPI before returning:

return c.Do(ctx, url.Request(http.MethodPost, spec), nil)

This can (has?) lead to hidden bugs in production where individual failures go unnoticed. #2208 will fix this for the attach-multiple-tags-to-object action, but we should discuss first whether we break existing users of the call discussed in this issue when fixing the bug.

Related to this issue is that the VAPI simulator does not fully implement the vCenter VAPI behavior, e.g. missing validation for cardinality and missing object references, create tag by ID (URN), etc.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[embano1]

@dougm this might be a breaking change, not sure what you think about fixing this behavior?

[dougm]

@embano1 yes, I agree we should return the batch error from that method. Existing code should already check err != nil at least and the change won't break the method signature, so I think we're ok.

Improving the vcsim behavior would be great too.

[embano1]

Existing code should already check err != nil at least

I don't think semantic (in response body) errors are surfaced by these methods currently:

		switch b := resBody.(type) {
		case io.Writer:
			_, err := io.Copy(b, res.Body)
			return err
		default:
			d := json.NewDecoder(res.Body)
			if isAPI(req.URL.Path) {
				// Responses from the /api endpoint are not wrapped
				return d.Decode(resBody)
			}
			// Responses from the /rest endpoint are wrapped in this structure
			val := struct {
				Value interface{} `json:"value,omitempty"`
			}{
				resBody,
			}
			return d.Decode(&val)
		}

govmomi/vapi/rest/client.go

Line 147 in f49dcf5

func (c *Client) Do(ctx context.Context, req *http.Request, resBody interface{}) error {

That's why I think it would actually be a breaking change when we surface tagging specific errors into error.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Mark as fresh by adding the comment /remove-lifecycle stale.