initial commit for CSI adapter #6
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @krishchow! |
k8s-ci-robot
added
the
size/XXL
|
Your branch contains |
| } | ||
|
|
||
| func (i *IdentityServer) GetPluginInfo(ctx context.Context, req *csi.GetPluginInfoRequest) (*csi.GetPluginInfoResponse, error) { | ||
| return &csi.GetPluginInfoResponse{}, nil |
There was a problem hiding this comment.
Instead of stub results, consider IdentityServer to 'inherit' from csi.UnimplementedIdentityServer.
There was a problem hiding this comment.
For the IdentityServer, I would actually prefer to do it this way. Most of the logic is already outlined in another commit, and when I open the second PR to add that here, we might as well try to keep the diff as small as possible.
| } | ||
|
|
||
| func (n NodeServer) NodeStageVolume(ctx context.Context, request *csi.NodeStageVolumeRequest) (*csi.NodeStageVolumeResponse, error) { | ||
| panic("implement me") |
There was a problem hiding this comment.
Same, let NodeServer 'inherit' csi.UnimplementedNodeServer.
There was a problem hiding this comment.
Same as the IdentityServer, the actual implementation for the NodeServer is mostly already finished in an external PR, and I think it would be more valuable to keep future diffs smaller
Rebased and removed, I thought I added that to the .gitignore 😅 |
main.go
Outdated
|
|
||
| func main() { | ||
| sigs := make(chan os.Signal, 1) | ||
| signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM, syscall.SIGSEGV) |
There was a problem hiding this comment.
Not sure we want to 'properly handle' SIGSEGV. IMO a segmentation fault should crash an application unless the app can really do something 'intelligent' about it.
There was a problem hiding this comment.
SEGFault should not lead to forced termination of any I/O in progress. The whole point of gracefully handling this signal to complete I/O in progress before exiting. This should still be here and added to the other repos as well if it’s not already there
There was a problem hiding this comment.
That makes sense to me, I have removed that signal for the time being
krishchow
force-pushed
the
initial
branch
2 times, most recently
from
c57ffcc to
6b93c4d
Compare
|
All the outstanding comments have been addressed. LGTM |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: krishchow, wlan0 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This PR aims to add the basic boilerplate for the COSI CSI adapter. Future PRs will add implementations of the Node controller and Identity controller for the CSI adapter.