internal/coordinator/pool: check if EC2 instance is a buildlet

Before destroying an untracked EC2 instance, verify that it is a
buildlet. Non buildlets will be destroyed manually as they are most
likely instances created while an operator is debugging a problem.

For golang/go#36841

Change-Id: Id0f65192ce4e72f9ecc495dd3e94750724091b51
Reviewed-on: https://go-review.googlesource.com/c/build/+/249121
Run-TryBot: Carlos Amedee <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: cagedmantis

internal/coordinator/pool/ec2.go

@@ -341,6 +341,12 @@ func (eb *EC2Buildlet) destroyUntrackedInstances() {
 	}
 	deleteInsts := make([]string, 0, len(insts))
 	for _, inst := range insts {
+		if !isBuildlet(inst.Name) {
+			// Non-buildlets have not been created by the EC2 buildlet pool. Their lifecycle
+			// should not be managed by the pool.
+			log.Printf("destroyUntrackedInstances: skipping non-buildlet %q", inst.Name)
+			continue
+		}
 		if eb.isRemoteBuildlet(inst.Name) {
 			// Remote buildlets have their own expiration mechanism that respects active SSH sessions.
 			log.Printf("destroyUntrackedInstances: skipping remote buildlet %q", inst.Name)

internal/coordinator/pool/ec2_test.go

@@ -512,11 +512,11 @@ func TestEC2BuildletRetrieveAndSetInstanceTypes(t *testing.T) {
 
 func TestEC2BuildeletDestroyUntrackedInstances(t *testing.T) {
 	awsC := cloud.NewFakeAWSClient()
-	create := func() *cloud.Instance {
+	create := func(name string) *cloud.Instance {
 		inst, err := awsC.CreateInstance(context.Background(), &cloud.EC2VMConfiguration{
 			Description: "test instance",
 			ImageID:     "image-x",
-			Name:        instanceName("host-test-type", 10),
+			Name:        name,
 			SSHKeyID:    "key-14",
 			Tags:        map[string]string{},
 			Type:        "type-x",
@@ -529,10 +529,11 @@ func TestEC2BuildeletDestroyUntrackedInstances(t *testing.T) {
 	}
 	// create untracked instances
 	for it := 0; it < 10; it++ {
-		_ = create()
+		_ = create(instanceName("host-test-type", 10))
 	}
-	wantTrackedInst := create()
-	wantRemoteInst := create()
+	wantTrackedInst := create(instanceName("host-test-type", 10))
+	wantRemoteInst := create(instanceName("host-test-type", 10))
+	_ = create("debug-tiger-host-14") // non buildlet instance
 
 	pool := &EC2Buildlet{
 		awsClient: awsC,
@@ -556,9 +557,10 @@ func TestEC2BuildeletDestroyUntrackedInstances(t *testing.T) {
 		},
 	}
 	pool.destroyUntrackedInstances()
+	wantInstCount := 3
 	gotInsts, err := awsC.RunningInstances(context.Background())
-	if err != nil || len(gotInsts) != 2 {
-		t.Errorf("awsClient.RunningInstances(ctx) = %+v, %s; want single inst and no error", gotInsts, err)
+	if err != nil || len(gotInsts) != wantInstCount {
+		t.Errorf("awsClient.RunningInstances(ctx) = %+v, %s; want %d instances and no error", gotInsts, err, wantInstCount)
 	}
 }
 

internal/coordinator/pool/gce.go

@@ -683,7 +683,7 @@ func (p *GCEBuildlet) cleanZoneVMs(zone string) error {
 				}
 			}
 		}
-		isBuildlet := strings.HasPrefix(inst.Name, "buildlet-")
+		isBuildlet := isBuildlet(inst.Name)
 
 		if isBuildlet && !sawDeleteAt && !p.instanceUsed(inst.Name) {
 			createdAt, _ := time.Parse(time.RFC3339Nano, inst.CreationTimestamp)