Addressing TODOs for NodeGetVolumeMetrics and NodeUnstageVolume #3985
Conversation
mye956
force-pushed
the
ebsTodos
branch
2 times, most recently
from
c6db056 to
f1bea74
Compare
mye956
changed the title
agent/engine/task_manager_test.go
Outdated
| @@ -2227,3 +2231,157 @@ func TestTaskWaitForHostResources(t *testing.T) { | |||
| topTask, err = taskEngine.topTask() | |||
| assert.Error(t, err) | |||
| } | |||
|
|
|||
| func TestUnstageVolumesHappy(t *testing.T) { | |||
There was a problem hiding this comment.
in future we can reference the "happy" case as just Test and "sad" cases can be named Test + some designation specific to their failure mode. (I see that you do this below: TestUnstageVolumesNoTimeoutError)
There was a problem hiding this comment.
sure, it might be more helpful using a table as well for testing as @amogh09 suggested!
mye956
force-pushed
the
ebsTodos
branch
2 times, most recently
from
f25ee53 to
b0f2a3d
Compare
| } | ||
|
|
||
| func (cfg *EFSVolumeConfig) GetVolumeId() string { | ||
| return "" |
There was a problem hiding this comment.
I worry about adding something like this when there's an existing ID for EFS which we might want to return
"fileSystemId": "fs-1234"
There was a problem hiding this comment.
I agree. I recall this was sort of the reason why I originally opted to type-cast since not all volumes types will have fields like volume ID, device name, etc.
| } | ||
|
|
||
| func (cfg *DockerVolumeConfig) GetVolumeId() string { | ||
| return "" |
There was a problem hiding this comment.
ditto here -- is there a unique identifier we should return for a docker volume besides its name (which I believe is unique per host)?
There was a problem hiding this comment.
^ same response as above
There was a problem hiding this comment.
I assume the docker volume name?
mye956
force-pushed
the
ebsTodos
branch
2 times, most recently
from
030ec45 to
c15a7a8
Compare
Summary
This PR will address all of the leftover TODOs for
NodeGetVolumeMetricsandNodeUnstageVolumefunctionality for EBS-backed tasks.Implementation details
agent/engine/task_manager.go: ChangedUnstageVolumesto return a list of any errors that were encountered when trying to unstage a volume via the CSI driver. We'll then log all encountered errors. This will be helpful for having unit tests since otherwise we wouldn't know whether or not there were any issues.agent/taskresource: Added new getters within theVolumeinterface. This is to avoid typecasting the volume within theNodeGetVolumeMetricsas mentioned in #3929.agent/stats/engine_unix.go: No longer type casting the task volume withinfetchEBSVolumeMetricsin order to obtain the EBS volume and will instead use the getters implemented inagent/taskresourceRelated PR: #3929
Testing
New unit tests:
New tests cover the changes: Yes
Description for the changelog
Added more unit testing for NodeUnstageVolume and NodeGetVolumeMetrics as well as address other leftover TODOs for EBS-backed task
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.