BaseOperation.deleteList swallows return value making the BaseOperation.delete not behave as expected

[XComp]

Describe the bug

I wanted to test code where client.configMaps().withLabels(labels).delete() is called but an 404 HTTP error is returned. The expected behavior would be that the delete() call returns false. But it doesn't because of BaseOperation.deleteList() swallowing the return value of the internally used delete method. This results in not returning the right return value in BaseOperation.delete:456.

Fabric8 Kubernetes Client version

next (development version)

Steps to reproduce

1. Create ConfigMap with labels
2. Mock the server response to return a 404:

final String path = String.format("/api/v1/namespaces/%s/configmaps/%s", NAMESPACE, configMapName);
server.expect().delete().withPath(path).andReturn(404, "").once();

3. Call client.configMaps().withLabels(labels).delete() on that label and evaluate the response.

Expected behavior

false should be returned, but true was observed.

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

next (development version)

Environment

other (please specify in additional context)

Fabric8 Kubernetes Client Logs

No response

Additional context

This is an internal bug independent of any environment as far as I can see. That's why I labeled the Environment as other

[shawkins]

I'm letting this issue take over for #2931 as well. I know it's not strictly the same thing, but it should get addressed / resolved along with this.

Essentially the resolution here is to have deleteList return a boolean and for that to be returned by delete as well.

The next issue is what should be the return value for delete(empty list) - currently it's true. There is no documentation on MultiDeleteable as to what the behavior should be.

The last is that the delete attempt will short circuit at the first failure, including a 404. You could also make the case for continuing the delete attempts against the other items.

Possible alternatives:

List<T> deleteList() - return the list of items that were successfully deleted. 404 of an individual item would effectively be ignored (it's just a concurrency issue between when you obtained the list, and when you process the deletes). For any other exception the list of items already deleted should be attached.

List<T> deleteList(List<T>) - could behave exactly the same as above. If you pass in an empty list, it should return an empty list.

[XComp]

We should also update the JavaDoc of Deletable. returning null seems to be wrong/out-dated (?). And add proper explanation about the contract of delete to the MultiDeletable interface

[shawkins]

We should also update the JavaDoc of Deletable.

Yes that was missed with the change to remove returning inappropriate Booleans - #3853

[shawkins]

@XComp please see if the #3983 pr makes sense for addressing the set of issues mentioned here.

[manusa]

List deleteList() - return the list of items that were successfully deleted. 404 of an individual item would effectively be ignored (it's just a concurrency issue between when you obtained the list, and when you process the deletes). For any other exception the list of items already deleted should be attached.

List deleteList(List) - could behave exactly the same as above. If you pass in an empty list, it should return an empty list.

What's the behavior for kubectl: does it short-circuit? does it return exit != 0 in case deletion is not successful?

[shawkins]

https://github.com/kubernetes/kubectl/blob/fdbcc2710435e59cee996dccd6d219b4ed98fff3/pkg/cmd/delete/delete.go#L289

For the regular delete I don't believe so. It uses a visitor against the response to determine what, if anything was deleted, and any additional messages to show the user.

So there are three styles of delete:

• single item - configMap.withName("name").delete
• collection (or bulk) of the same item type configMaps.delete - requires processing the response to tell exactly what was deleted
• list client.resourceList().delete, which is like kubectl delete -f

[shawkins]

kubectl delete -f

Will attempt to delete each item, and show what was deleted or not found.

Options instead of introducing deleteList are:

• a breaking change to just return void from delete and have the collection context to attempt each delete.
• leave delete a returning a boolean, but document for a collection the return value will always be true and change the implementation to attempt each delete.

Additionally:

• consider deprecating / removing the additional delete signatures. delete(item...) and delete(list) based upon one of the above changes, these are just shortcuts for client.resourceList(items).delete(). We have not added something like client.configMaps().resourceList(list), but that could be considered with Further consolidate and expand resource methods #3973

@andreaTP @manusa what are your preferences?

[andreaTP]

It was my understanding that a possible option is to do a breaking change and always return List from delete.

[manusa]

consider deprecating / removing the additional delete signatures. delete(item...) and delete(list) based upon one of the above changes

+1 I think we should strive for consistency in the code-base and provide single ways (+implementations) to achieve a given purpose. IMHO Answers to questions such as "How should I delete a Pod? How should I delete a list of Pods? How should I delete a collection of resources?" should have a unique and consistent answer.

@andreaTP @manusa what are your preferences?

Based on what we discussed yesterday, I'd opt for completely refactoring and changing the behavior of the delete operation.

Scenarios

• A: Single named item: client.xxx().withName("the-name").delete();
• B: Single loaded item: client.xxx().withItem(xxx).delete()
• C: Single/Multiple loaded items: client.load($yaml).delete()
• D: Bulk operation: client.xxx().withField($key, $value).withLabel($label)/.../.delete();

Behaviors

For scenarios A-C I think we should replace the current signature (if possible) and return the List/Collection (from server) of effectively deleted resources (since we know which they are beforehand). Then we might discuss about short-circuiting or not + exception handling.

For scenario D, the operation should not return anything (void). In case of failure sending the request, then an Exception is thrown.

Motivation

So far we are covering A-C and provide multiple implementations + method signatures. As I mentioned before, I think that we should make things consistent and simplify both the API and the implementation.

We are not yet covering D, which I think we should, since for big clusters would provide significant performance improvements.

[shawkins]

A does not naturally obtain a result.

B returning a list may seem odd to users, but is consistent with C.

There will be a signature conflict between A-B and D if they return something different.

In looking at the kubectl it just reports the IDs in all these cases. So a list of strings, rather than the full objects, would be better.

However as you are saying unless you we put in a lot more effort, we are not going to extract the id list from D. Based upon these observations I'm advocating that either we just keep returning boolean and updated the javadocs for the multi-deletes that it's not a meaningful value, or just return void for all of them.

[shawkins]

On the other delete signatures - delete(item...) and delete(list). We have as well create(item...) and createOrUpdate(item...) where

configMap.create(item) is the same as configMap.resource(item).create()