Skip to content

Commit

Permalink
Fix shared EBS discovery implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
mye956 committed Sep 6, 2023
1 parent c099550 commit 653fc5d
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 34 deletions.
54 changes: 54 additions & 0 deletions agent/ebs/watcher_test.go
Expand Up @@ -18,6 +18,7 @@ package ebs

import (
"context"
"fmt"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -313,3 +314,56 @@ func TestHandleEBSAckTimeout(t *testing.T) {
ebsAttachment, ok := taskEngineState.(*dockerstate.DockerTaskEngineState).GetEBSByVolumeId(volumeID)
assert.False(t, ok)
}

// TestHandleMismatchEBSAttachment tests handling an EBS attachment but found a different volume attached
// onto the host during the scanning process.
func TestHandleMismatchEBSAttachment(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

ctx := context.Background()
taskEngineState := dockerstate.NewTaskEngineState()
eventChannel := make(chan statechange.Event)
mockDiscoveryClient := mock_ebs_discovery.NewMockEBSDiscovery(mockCtrl)

watcher := newTestEBSWatcher(ctx, taskEngineState, eventChannel, mockDiscoveryClient)

testAttachmentProperties := map[string]string{
apiebs.ResourceTypeName: apiebs.ElasticBlockStorage,
apiebs.DeviceName: deviceName,
apiebs.VolumeIdName: volumeID,
}

expiresAt := time.Now().Add(time.Millisecond * testconst.WaitTimeoutMillis)
ebsAttachment := &apiebs.ResourceAttachment{
AttachmentInfo: attachmentinfo.AttachmentInfo{
TaskARN: taskARN,
TaskClusterARN: taskClusterARN,
ContainerInstanceARN: containerInstanceARN,
ExpiresAt: expiresAt,
Status: status.AttachmentNone,
AttachmentARN: resourceAttachmentARN,
},
AttachmentProperties: testAttachmentProperties,
}

var wg sync.WaitGroup
wg.Add(1)
mockDiscoveryClient.EXPECT().ConfirmEBSVolumeIsAttached(deviceName, volumeID).
Do(func(deviceName, volumeID string) {
wg.Done()
}).
Return(fmt.Errorf("%w; expected EBS volume %s but found %s", apiebs.ErrInvalidVolumeID, volumeID, "vol-321")).
MinTimes(1)

err := watcher.HandleResourceAttachment(ebsAttachment)
assert.NoError(t, err)

pendingEBS := watcher.agentState.GetAllPendingEBSAttachmentsWithKey()
foundVolumes := apiebs.ScanEBSVolumes(pendingEBS, watcher.discoveryClient)

assert.Empty(t, foundVolumes)
ebsAttachment, ok := taskEngineState.(*dockerstate.DockerTaskEngineState).GetEBSByVolumeId(volumeID)
assert.True(t, ok)
assert.ErrorIs(t, ebsAttachment.GetError(), apiebs.ErrInvalidVolumeID)
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 7 additions & 17 deletions ecs-agent/api/resource/ebs_discovery.go
Expand Up @@ -15,12 +15,10 @@ package resource

import (
"context"
"errors"
"fmt"
"strings"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger"

"github.com/pkg/errors"
)

const (
Expand All @@ -44,26 +42,18 @@ func NewDiscoveryClient(ctx context.Context) *EBSDiscoveryClient {
}

// ScanEBSVolumes will iterate through the entire list of pending EBS volume attachments within the agent state and checks if it's attached on the host.
func ScanEBSVolumes[T GenericEBSAttachmentObject](t map[string]T, dc EBSDiscovery) []string {
func ScanEBSVolumes[T GenericEBSAttachmentObject](pendingAttachments map[string]T, dc EBSDiscovery) []string {
var err error
var foundVolumes []string
for key, ebs := range t {
for key, ebs := range pendingAttachments {
volumeId := strings.TrimPrefix(key, ebsResourceKeyPrefix)
deviceName := ebs.GetAttachmentProperties(DeviceName)
err = dc.ConfirmEBSVolumeIsAttached(deviceName, volumeId)
if err != nil {
if err == ErrInvalidVolumeID || errors.Cause(err) == ErrInvalidVolumeID {
logger.Warn("Found a different EBS volume attached to the host. Expected EBS volume:", logger.Fields{
"volumeId": volumeId,
"deviceName": deviceName,
})
} else {
logger.Warn("Failed to confirm if EBS volume is attached to the host. ", logger.Fields{
"volumeId": volumeId,
"deviceName": deviceName,
"error": err,
})
if !errors.Is(err, ErrInvalidVolumeID) {
err = fmt.Errorf("%w; failed to confirm if EBS volume is attached to the host", err)
}
ebs.SetError(err)
continue
}
foundVolumes = append(foundVolumes, key)
Expand Down
8 changes: 8 additions & 0 deletions ecs-agent/api/resource/resource_attachment.go
Expand Up @@ -216,6 +216,14 @@ func (ra *ResourceAttachment) SetError(err error) {
ra.err = err
}

// GetError returns the error field for a resource attachment.
func (ra *ResourceAttachment) GetError() error {
ra.guard.RLock()
defer ra.guard.RUnlock()

return ra.err
}

// EBSToString returns a string representation of an EBS volume resource attachment.
func (ra *ResourceAttachment) EBSToString() string {
ra.guard.RLock()
Expand Down

0 comments on commit 653fc5d

Please sign in to comment.