platform/aws: add Name tag to created images

[jlebon]

When creating AMIs, in addition to setting an AMI name, let's also add
an explicit Name tag with the same value. This can make it more
convenient to search for AMIs when filtering by multiple tags.

[jlebon]

/cc @ashcrow

[ashcrow]

This looks like it will do for our needs. In the future we may want to add general tag support, but this works for now!

[ashcrow]

/cc @bgilbert

[arithx]

Is there any chance that a SIGTERM could leave the image existing but not having the new Name tag?

Would it potentially be worthwhile to validate the tag exists on the found image in the next block and add them if they don't exist?

cc @bgilbert

[jlebon]

I'm open to that, though note we don't do this currently for pre-existing snapshots either.

[euank]

I think it does for snapshots. The code that tags a snapshot runs unconditionally, even if the snapshot is "done" / "complete"

mantle/platform/api/aws/images.go

Lines 258 to 264 in d49d04a

	// post-process
	err := a.CreateTags([]string{snapshotID}, map[string]string{
	"Name": imageName,
	})
	if err != nil {
	return nil, fmt.Errorf("couldn't create tags: %v", err)
	}

Can you reference where it doesn't pick up tagging given a poorly timed SIGTERM?

[jlebon]

Right, I was initially looking at how FindSnapshot returns early (and thus skips the post-processing) if we don't fall back to searching for pre-existing import tasks:

mantle/platform/api/aws/images.go

Lines 126 to 132 in d49d04a

	if len(snapshotRes.Snapshots) == 1 {
	snapshotID := *snapshotRes.Snapshots[0].SnapshotId
	plog.Infof("found existing snapshot %v", snapshotID)
	return &Snapshot{
	SnapshotID: snapshotID,
	}, nil
	}

But actually, we'd only get there if the Name tag was already correct judging by the filters. So I think you're right, we do implicitly do this for snapshots just from how we search for them in the first place. If it wasn't tagged, then the import task fallback will pick it up and complete it (assuming the task wasn't GC'ed already).

[euank]

Yup, reading it again it's certainly non-obvious that FindSnapshot works that way.

I think it's up to you whether we make this idempotent/resumable or not.
As long as there's only async cleanup tasks that rely on the name tag, not plume itself, I don't think it really matters that much.

[bgilbert]

This code runs in the plume prerelease (i.e., CL release) path, even though CL releases don't actually need the tag. Since the rest of the code in that path tries pretty hard to be idempotent, I'd be in favor of also doing so here if possible.

[jlebon]

I updated this to always tag pre-existing images! ⬇️

[euank]

We've gone down the path of tagging AMIs before. Note that these tags are not visible across accounts, which makes them have limited utility.
xref coreos/bugs#111 (comment)

Assuming you still want this with the above caveat, this LGTM

[ashcrow]

@euank thanks for that ... good to know. The context around using tags is to allow our image pruning jobs to find the AMIs.

Reference: openshift/os#44

[jlebon]

Note that unlike the previous version, I haven't actually tested this. It seems mostly straightforward, though I can test it in a while.

[bgilbert]

LGTM. Please test and then I'll merge.

[jlebon]

Tested working!