-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: remove engine binary path check #435
refactor: remove engine binary path check #435
Conversation
f0d829b
to
9e9db8a
Compare
ref: longhorn/longhorn 8094 Signed-off-by: Jack Lin <jack.lin@suse.com>
9e9db8a
to
1722335
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Out of PR, I noticed that when a process fails to create, it is not unregistered. Could you please create a ticket to address this issue, @ChanYiLin? Thanks. /pkg/process/process_manager.go#L254-L268 if err := pm.registerProcess(p); err != nil {
return nil, err
}
p.UpdateCh <- p
if err := p.Start(); err != nil {
// initializing failed so we sent event about the failed state, but still return the process rpc below
// this is to be consistent with the prior implementation
logrus.WithError(err).Errorf("Process Manager: failed to init new process %v", req.Spec.Name)
p.UpdateCh <- p
} else {
logrus.Infof("Process Manager: created process %v", req.Spec.Name)
}
return p.RPCResponse(), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mergify backport v1.6.x |
✅ Backports have been created
|
ref: longhorn/longhorn#8094
The original check limit the pattern of the engine binary path.
But there is no need to check the engine binary path.
Even the path is wrong, we catch the error and show the error message.