Fix/return multi errors on create failed

[xujihui1985]

this PR fix the problem describe in this PR #2997.
When container creation fails and triggers a cleanup process that also fails, the original creation error is overwritten by the cleanup error.This change ensures that both the primary creation error and the subsequent cleanup error are captured and reported, providing better visibility into the failure sequence.

[xujihui1985]

@YJDoc2 Hi， would you like to have a look at this PR?

[YJDoc2]

Hey, this is technically fine, nothing wrong per say ; but I don't like this implementation 😅
The particular issue I have with this is that this introduces a specific error type which is general ("multi" error) but actually used only for one specific case. The ideal way would be to have some kind of error chaining like what anyhow provides, so we know that these two individual errors occurred, in a stack-trace like fashion, but I have no idea how we can do this with thiserror.

I'll try to think on this, and see if I can think of some other way, otherwise approve and merge this in a couple of days. (Bit busy now, probably go over on the weekend)

[xujihui1985]

Hey, this is technically fine, nothing wrong per say ; but I don't like this implementation 😅 The particular issue I have with this is that this introduces a specific error type which is general ("multi" error) but actually used only for one specific case. The ideal way would be to have some kind of error chaining like what anyhow provides, so we know that these two individual errors occurred, in a stack-trace like fashion, but I have no idea how we can do this with thiserror.

I'll try to think on this, and see if I can think of some other way, otherwise approve and merge this in a couple of days. (Bit busy now, probably go over on the weekend)

I agree with you. The current implementation is too specific to this case, but I haven't found a better way to address it.

What I tried was adding a new extension trait for Result<T, E> with an implementation like this:

self.cleanup_container().join(outer)?;

However, outer is moved in this context unless I clone it, which isn't feasible since outer isn't clonable.
thanks for the reply. I'm glad to discuss with you about the design.

[YJDoc2]

Hey, so I thought about this -

1. This case is special because this is probably the only place where we do something after we encounter an error instead of immediately returning, and that might also create an error. And we want to pass both errors to the caller
2. Because this is a library, we do not want something like anyhow, because it will coerce all errors into a common string type, and caller won't be able to discern between them.

After thinking, I think we can modify your approach slightly to solve the issues I have with it -

1. Instead of calling it MultiError, lets call it ContainerCreateError . This makes it very specific, and indicates the cause/origin of error clearly (imo)
2. Instead of using vec for it, let's make the ContainerCreateError struct (i.e. currently MultiError struc) to have two fields - run_error : Box<LibcontainerError> and cleanup_error : Option<Box<LibcontainerError>> (probably with better names) . This way the caller will be able to specify which error is which. This is definitely open for discussion, as even currently caller can extract errors, they just don't have specific names/position in the vector. Alternatively we can also use a 2-tuple for this.

I still don't think this is the most ergonomic way for this, but maybe a step in right direction? As I said this is a very special and specific case where we need to return two errors instead of directly returning after the first.

wdyt? Comments and suggestions very welcome :)

[xujihui1985]

Hi @YJDoc2 , I have updated the code according to your advice.

[YJDoc2]

LGTM! Thanks @xujihui1985 !
Given that you have implemented this change, I think you also agreed with my deign suggestions, but I will wait in case you want to comment anything or suggest any alternatives. Otherwise merge in some time.

[YJDoc2]

Ok, I'm going ahead and merging this. Also in my last comment, typo : deign -> design 😅