Prettify error messages for exec.Commands in Fluid

[TrafalgarZZZ]

Ⅰ. Describe what this PR does

Fluid relies on exec.Command to execute some operations and interacts with K8s API-Server(e.g. The helm for install/check/delete engine-related resources). exec.Command will always returns a meaningless err that would confuses users about the cause of their problems.

For example, when Fluid CSI Plugin fails to bind mount a FUSE mount point onto a pod, an event is issued on the pod showing messages like:

MountVolume.Setup failed for volume "default-jfsdemo": rpc error: code = InvalidArgument desc = exit status 1

This PR prettifies such error messages with a clearer explanation about the cause. For example, the event message mentioned above would be changed to:

MountVolume.Setup failed for volume "default-jfsdemo": rpc error: code = Internal desc = timeout waiting for FUSE mount point to be ready

which indicates that it's highly possible the FUSE pod is still not ready on that node

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

[cheyang]

How about change it into

if err != nil {
    errMsg := fmt.Sprintf("failed to execute InstallRelease() command: %s", err)
    log.Error(err, errMsg, "command", cmd.String())
    err = errors.Wrap(err, "failed to create engine-related kubernetes resources")
}



return err

[TrafalgarZZZ]

With such implementation, log.Error may print double err info in the log.

And, errors.Wrap returns something like failed to create engine-related kubernetes resources: exit status 1 which I think is more confusing.

[cheyang]

ok

[codecov]

Codecov Report

Merging #2718 (6682531) into master (05589e9) will decrease coverage by 0.01%.
The diff coverage is 72.72%.

❗ Current head 6682531 differs from pull request most recent head 0993071. Consider uploading reports for the commit 0993071 to get more accurate results

@@            Coverage Diff             @@
##           master    #2718      +/-   ##
==========================================
- Coverage   67.55%   67.54%   -0.01%     
==========================================
  Files         381      381              
  Lines       21968    21982      +14     
==========================================
+ Hits        14840    14848       +8     
- Misses       5381     5386       +5     
- Partials     1747     1748       +1     

Impacted Files	Coverage Δ
pkg/utils/mount.go	84.84% <40.00%> (-8.14%)	⬇️
pkg/utils/helm/utils.go	82.88% <100.00%> (+0.80%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[cheyang]

How about using errors.As instead of strings.HasPrefix?

if errors.Is(err, exec.ErrNotFound) {
    return errors.New("fusemount command not found")
}
var fuseMountErr *exec.ExitError
if errors.As(err, &fuseMountErr) {
    switch fuseMountErr.ExitCode() {
    case 1:
        return errors.New("timeout waiting for FUSE mount point to be ready")
    case 2:
        return fmt.Errorf("subPath \"%s\" not exists under FUSE mount", subPath)
    }
}
return err

[TrafalgarZZZ]

Thx. Changed to errors.As

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[TrafalgarZZZ]

/test fluid-e2e

[fluid-e2e-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheyang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cheyang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cheyang]

/lgtm