longhorn: [BUG] Can not delete type=`bi` VolumeSnapshot if related backing image not exist

@innobead https://github.com/longhorn/longhorn/issues/6291

Oh I got it Yeah

@innobead return 404 not found is api server implementation from client it is quite hard to analyze the error, unless we check the error message, but then we need to unify the error message from server Currently I don’t have any idea for this middle layer cc @shuo-wu

@ChanYiLin What I mean is not client side but server side, like add your change in the below code in general to handle for all endpoint functions. WDYT?

/api/router.go#L16-L24

func HandleError(s *client.Schemas, t HandleFuncWithError) http.Handler {
	return api.ApiHandler(s, http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		if err := t(rw, req); err != nil {
                    logrus.WithError(err).Warnf("HTTP handling error")  
                    
                    if datastore.ErrorIsNotFound(err) {
		        rw.WriteHeader(http.StatusNotFound)
                        return nil
		    }

		    apiContext := api.GetApiContext(req)
		    apiContext.WriteErr(err)
		}
	}))
}

I think this solution is good enough for current issue and give the right status code (404) for all not found error For other errors, as you said, we are currently restricted by the lib.

I think we can create another issue for handling other errors

@ChanYiLin It seems PRs merged. Please remember to move it to ready for testing if all done.

@ChanYiLin Sounds good, please help create the issue to track it later by self-assigning, also I am not sure if the go-rancher has any new version w/ a related improvement. or not or even we can contribute back to it.

@innobead return 404 not found is api server implementation from client it is quite hard to analyze the error, unless we check the error message, but then we need to unify the error message from server

Currently I don’t have any idea for this middle layer

cc @shuo-wu

@ChanYiLin What I mean is not client side but server side, like add your change in the below code in general to handle for all endpoint functions. WDYT?

/api/router.go#L16-L24

func HandleError(s *client.Schemas, t HandleFuncWithError) http.Handler {
	return api.ApiHandler(s, http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		if err := t(rw, req); err != nil {
                    logrus.WithError(err).Warnf("HTTP handling error")  
                    
                    if datastore.ErrorIsNotFound(err) {
		        rw.WriteHeader(http.StatusNotFound)
                        return nil
		    }

		    apiContext := api.GetApiContext(req)
		    apiContext.WriteErr(err)
		}
	}))
}

Hi @innobead there is no need to backport to 1.3.X and 1.4.X since backing image with csi feature is only introduced in v1.5.x thanks!

Hi @innobead ,

Sorry I tested on v1.4.3-rc1 this morning, VolumeSnpshotClass, VolumeSnapshotContent, VolumeSnapshot, can create without problem, but I just notice that create PVC and backing image from VolumeSnapshot not support, I am sorry for the misleading information.

Hi @innobead there is no need to backport to 1.3.X and 1.4.X since backing image with csi feature is only introduced in v1.5.x thanks!

It seems backup type snapshot will have the same issue If someone delete the backupVolume which is created by the volumesnapshot through UI Then one can’t delete the volumesnapshot after as well

Oh I found the reason why it failed

• In csi, before deleting the backing image, it will get the backing image through api (not datastore) in generated_client

func (c *BackingImageClient) ById(id string) (*BackingImage, error) {
	resp := &BackingImage{}
	err := c.rancherClient.doById(BACKING_IMAGE_TYPE, id, resp)
	if apiError, ok := err.(*ApiError); ok {
		if apiError.StatusCode == 404 {
			return nil, nil
		}
	}
	return resp, err
}

we do check if it is not found (404) and return nil, and should bypass the deletion process

• However, in server part, we raise 500 error with backing image not found

func (s *Server) BackingImageGet(rw http.ResponseWriter, req *http.Request) error {
	apiContext := api.GetApiContext(req)

	id := mux.Vars(req)["name"]

	bi, err := s.m.GetBackingImage(id)
	if err != nil {
		return errors.Wrapf(err, "failed to get backing image '%s'", id)
	}

	apiContext.Write(toBackingImageResource(bi, apiContext))
	return nil
}

So the csi failed to process because of the error.

Removed from 1.5.1 and 1.4.3 which will be released in the near future, so this can be tackled in the following release instead.