Add NodeStage to internal CSI implementation #3900
Conversation
fierlion
force-pushed
the
fierlion/nodeStageDaemon
branch
5 times, most recently
from
06652c6
to
6142610
Compare
| func isValidVolumeContext(volContext map[string]string) bool { | ||
| //There could be multiple volume attributes in the volumeContext map | ||
| //Validate here case by case | ||
| if partition, ok := volContext[VolumeAttributePartition]; ok { |
There was a problem hiding this comment.
Do we need extra handlings if volContext[VolumeAttributePartition] returns a not OK? It returns a true if it happens.
| } | ||
|
|
||
| func isValidVolumeContext(volContext map[string]string) bool { | ||
| //There could be multiple volume attributes in the volumeContext map |
There was a problem hiding this comment.
Not sure if the comment here is accurate. Aren't we only validating partition attribute here?
| ) | ||
|
|
||
| // InFlight is a struct used to manage in flight requests per volumeId. | ||
| type InFlight struct { |
There was a problem hiding this comment.
Do we persist this or plan to persist this somehow?
| import ( | ||
| "sync" | ||
|
|
||
| "k8s.io/klog/v2" |
There was a problem hiding this comment.
What's the difference betwen klog vs. our existing logging?
| func (m *NodeMounter) PathExists(path string) (bool, error) { | ||
| return mountutils.PathExists(path) | ||
| } | ||
|
|
||
| func (m *NodeMounter) NeedResize(devicePath string, deviceMountPath string) (bool, error) { | ||
| return mountutils.NewResizeFs(m.Exec).NeedResize(devicePath, deviceMountPath) |
There was a problem hiding this comment.
Would this mean everytime we call this function we need to initialize a new ResizeFs? Is it necessary?
| // This check is already performed on the controller side | ||
| // However, because it is potentially security-sensitive, we redo it here to be safe |
There was a problem hiding this comment.
If they are executing the same code and nothing changed, how recheck makes it more secure?
|
|
||
| partition := "" | ||
| if part, ok := volumeContext[VolumeAttributePartition]; ok { | ||
| if part != "0" { |
There was a problem hiding this comment.
Since it is a string, is it possible to have something like "00" here which actually means the same thing?
There was a problem hiding this comment.
nit(non-blocking): perhaps we can add a bit more context of what we're checking for here with some comments
| } | ||
| } | ||
|
|
||
| source, err := d.findDevicePath(devicePath, volumeID, partition) |
There was a problem hiding this comment.
Can you add a comment here about if the partition is the "", what does that mean?
| msg := fmt.Sprintf("failed to check if target %q exists: %v", target, err) | ||
| return nil, status.Error(codes.Internal, msg) | ||
| } | ||
| // When exists is true it means target path was created but device isn't mounted. |
There was a problem hiding this comment.
When will this happen? Another volume had been mounted to this path and was unmounted?
fierlion
force-pushed
the
fierlion/nodeStageDaemon
branch
13 times, most recently
from
bec3465
to
76c47eb
Compare
There was a problem hiding this comment.
Btw do we need to make updates in Makefile - https://github.com/aws/amazon-ecs-agent/blob/master/ecs-agent/daemonimages/csidriver/Makefile to somehow use the docker file? or that will be used already?
We depend on the https://github.com/aws/amazon-ecs-agent/blob/master/ecs-agent/daemonimages/csidriver/Makefile to build the tar file.
fierlion
force-pushed
the
fierlion/nodeStageDaemon
branch
4 times, most recently
from
27dc11a
to
9fcbe0e
Compare
fierlion
force-pushed
the
fierlion/nodeStageDaemon
branch
from
9fcbe0e
to
27d6eb2
Compare
fierlion
force-pushed
the
fierlion/nodeStageDaemon
branch
from
4f41ef2
to
5bce96b
Compare
Summary
This adds NodeStage to the internal/minimal EBS CSI Driver. This code comes directly from https://github.com/kubernetes-sigs/aws-ebs-csi-driver and is used to create a minimal Node-specific driver.
Implementation details
The driver is updated to use the
public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-csi-ebsbase image which contains all requisite mount utilities in the path while still keeping the overall driver to ~50mb in size. The NodeStage implementation adds only the files and dependencies which are directly tied to the EBS volume Mount/format/fs resize.Testing
Manual test process:
NodeStagefrom ecs-agent-based csiclient with hard coded device name and volume IDNew tests cover the changes: no
Description for the changelog
Update Minimal CSI Driver with NodeStage method.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.