internal/gomote, internal/gomote/protos: add create instance implemen…

…tation This change implements the endpoint to create gomote instances for the gomote GRPC service. In the process of implementing creates, various other changes were needed: - Refactoring the remote session pool. - Extending the fake schedule used for testing. Updates golang/go#48742 Change-Id: I0c74e38539428d028917200ccd6bd0c58fa14801 Reviewed-on: https://go-review.googlesource.com/c/build/+/370662 Trust: Carlos Amedee <[email protected]> Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
golang · Dec 22, 2021 · 12521bb · 12521bb
1 parent b2d4c05
commit 12521bb
Show file tree

Hide file tree

Showing 9 changed files with 954 additions and 349 deletions.
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
@@ -355,7 +355,7 @@ func main() {
  dashV1 := legacydash.Handler(gce.GoDSClient(), maintnerClient, string(masterKey()), grpcServer)
  dashV2 := &builddash.Handler{Datastore: gce.GoDSClient(), Maintner: maintnerClient}
  gs := &gRPCServer{dashboardURL: "https://build.golang.org"}
- gomoteServer := gomote.New(remote.NewSessionPool(context.Background()))
+ gomoteServer := gomote.New(remote.NewSessionPool(context.Background()), sched)
  protos.RegisterCoordinatorServer(grpcServer, gs)
  gomoteprotos.RegisterGomoteServiceServer(grpcServer, gomoteServer)
  http.HandleFunc("/", grpcHandlerFunc(grpcServer, handleStatus)) // Serve a status page at farmer.golang.org.

diff --git a/internal/coordinator/remote/remote.go b/internal/coordinator/remote/remote.go
@@ -9,7 +9,6 @@ import (
  "fmt"
  "log"
  "sort"
- "strings"
  "sync"
  "time"
 
@@ -22,58 +21,27 @@ const (
  remoteBuildletCleanInterval = time.Minute
 )
 
-// Session stores the metadata for a remote buildlet session.
+// Session stores the metadata for a remote buildlet Session.
 type Session struct {
- mu sync.Mutex
-
- builderType string // default builder config to use if not overwritten
+ BuilderType string // default builder config to use if not overwritten
+ Expires time.Time
+ HostType string
+ ID string // unique identifier for instance "user-bradfitz-linux-amd64-0"
+ OwnerID string // identity aware proxy user id: "accounts.google.com:userIDvalue"
  buildlet buildlet.Client
  created time.Time
- expires time.Time
- hostType string
- name string // dup of key
- user string // "user-foo" build key
-}
-
-// KeepAlive will renew the remote buildlet session by extending the expiration value. It will
-// periodically extend the value until the provided context has been cancelled.
-func (s *Session) KeepAlive(ctx context.Context) {
- go internal.PeriodicallyDo(ctx, time.Minute, func(ctx context.Context, _ time.Time) {
- s.renew(ctx)
- })
 }
 
 // renew extends the expiration timestamp for a session.
-func (s *Session) renew(ctx context.Context) {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- s.expires = time.Now().Add(remoteBuildletIdleTimeout)
+// The SessionPool lock should be held before calling.
+func (s *Session) renew() {
+ s.Expires = time.Now().Add(remoteBuildletIdleTimeout)
 }
 
 // isExpired determines if the remote buildlet session has expired.
+// The SessionPool lock should be held before calling.
 func (s *Session) isExpired() bool {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- // check that the expire timestamp has been set and that it has expired.
- return !s.expires.IsZero() && s.expires.Before(time.Now())
-}
-
-// Buildlet returns the buildlet client associated with the Session.
-func (s *Session) Buildlet() buildlet.Client {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- return s.buildlet
-}
-
-// Name returns the buildlet's name.
-func (s *Session) Name() string {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- return s.name
+ return !s.Expires.IsZero() && s.Expires.Before(time.Now())
 }
 
 // SessionPool contains active remote buildlet sessions.
@@ -106,22 +74,22 @@ func NewSessionPool(ctx context.Context) *SessionPool {
 }
 
 // AddSession adds the provided session to the session pool.
-func (sp *SessionPool) AddSession(user, builderType, hostType string, bc buildlet.Client) (name string) {
+func (sp *SessionPool) AddSession(ownerID, username, builderType, hostType string, bc buildlet.Client) (name string) {
  sp.mu.Lock()
  defer sp.mu.Unlock()
 
  for n := 0; ; n++ {
- name = fmt.Sprintf("%s-%s-%d", user, builderType, n)
+ name = fmt.Sprintf("%s-%s-%d", username, builderType, n)
  if _, ok := sp.m[name]; !ok {
  now := time.Now()
  sp.m[name] = &Session{
- builderType: builderType,
+ BuilderType: builderType,
  buildlet: bc,
  created: now,
- expires: now.Add(remoteBuildletIdleTimeout),
- hostType: hostType,
- name: name,
- user:  user,
+ Expires: now.Add(remoteBuildletIdleTimeout),
+ HostType: hostType,
+ ID:  name,
+ OwnerID: ownerID,
  }
  return name
  }
@@ -186,16 +154,22 @@ func (sp *SessionPool) Close() {
  })
 }
 
-// List returns a list of all active sessions sorted by session name.
+// List returns a list of all active sessions sorted by session ID.
 func (sp *SessionPool) List() []*Session {
  sp.mu.RLock()
  defer sp.mu.RUnlock()
 
  var ss []*Session
  for _, s := range sp.m {
- ss = append(ss, s)
+ ss = append(ss, &Session{
+ BuilderType: s.BuilderType,
+ Expires: s.Expires,
+ HostType: s.HostType,
+ ID: s.ID,
+ OwnerID: s.OwnerID,
+ })
  }
- sort.Slice(ss, func(i, j int) bool { return ss[i].name < ss[j].name })
+ sort.Slice(ss, func(i, j int) bool { return ss[i].ID < ss[j].ID })
  return ss
 }
 
@@ -207,41 +181,50 @@ func (sp *SessionPool) Len() int {
  return len(sp.m)
 }
 
-// Session retrieves a session from the pool.
+// Session retrieves information about the instance associated with a session from the pool.
 func (sp *SessionPool) Session(buildletName string) (*Session, error) {
  sp.mu.Lock()
  defer sp.mu.Unlock()
 
- if rb, ok := sp.m[buildletName]; ok {
- rb.expires = time.Now().Add(remoteBuildletIdleTimeout)
- return rb, nil
+ if s, ok := sp.m[buildletName]; ok {
+ s.renew()
+ return &Session{
+ BuilderType: s.BuilderType,
+ Expires: s.Expires,
+ HostType: s.HostType,
+ ID: s.ID,
+ OwnerID: s.OwnerID,
+ }, nil
  }
  return nil, fmt.Errorf("remote buildlet does not exist=%s", buildletName)
 }
 
-// userFromGomoteInstanceName returns the username part of a gomote
-// remote instance name.
-//
-// The instance name is of two forms. The normal form is:
-//
-// user-bradfitz-linux-amd64-0
-//
-// The overloaded form to convey that the user accepts responsibility
-// for changes to the underlying host is to prefix the same instance
-// name with the string "mutable-", such as:
-//
-// mutable-user-bradfitz-darwin-amd64-10_8-0
-//
-// The mutable part is ignored by this function.
-func userFromGomoteInstanceName(name string) string {
- name = strings.TrimPrefix(name, "mutable-")
- if !strings.HasPrefix(name, "user-") {
- return ""
+// Buildlet returns the buildlet client associated with the Session.
+func (sp *SessionPool) BuildletClient(buildletName string) (buildlet.Client, error) {
+ sp.mu.RLock()
+ defer sp.mu.RUnlock()
+
+ s, ok := sp.m[buildletName]
+ if !ok {
+ return nil, fmt.Errorf("remote buildlet does not exist=%s", buildletName)
  }
- user := name[len("user-"):]
- hyphen := strings.IndexByte(user, '-')
- if hyphen == -1 {
- return ""
+ return s.buildlet, nil
+}
+
+// KeepAlive will renew the remote buildlet session by extending the expiration value. It will
+// periodically extend the value until the provided context has been cancelled.
+func (sp *SessionPool) KeepAlive(ctx context.Context, buildletName string) error {
+ sp.mu.Lock()
+ defer sp.mu.Unlock()
+
+ s, ok := sp.m[buildletName]
+ if !ok {
+ return fmt.Errorf("remote buildlet does not exist=%s", buildletName)
  }
- return user[:hyphen]
+ go internal.PeriodicallyDo(ctx, time.Minute, func(ctx context.Context, _ time.Time) {
+ sp.mu.Lock()
+ s.renew()
+ sp.mu.Unlock()
+ })
+ return nil
 }
diff --git a/internal/coordinator/remote/remote_test.go b/internal/coordinator/remote/remote_test.go
@@ -16,11 +16,11 @@ import (
 func TestSessionRenew(t *testing.T) {
  start := time.Now()
  s := Session{
- expires: start,
+ Expires: start,
  }
- s.renew(context.Background())
- if !s.expires.After(start) {
- t.Errorf("Session.expires = %s; want a time > %s", s.expires, start)
+ s.renew()
+ if !s.Expires.After(start) {
+ t.Errorf("Session.expires = %s; want a time > %s", s.Expires, start)
  }
 }
 
@@ -37,7 +37,7 @@ func TestSessionIsExpired(t *testing.T) {
  for _, tc := range testCases {
  t.Run(tc.desc, func(t *testing.T) {
  s := &Session{
- expires: tc.expires,
+ Expires: tc.expires,
  }
  if got := s.isExpired(); got != tc.want {
  t.Errorf("Session.isExpired() = %t; want %t", got, tc.want)
@@ -52,7 +52,7 @@ func TestSessionPool(t *testing.T) {
 
  wantInstances := 4
  for i := 0; i < wantInstances; i++ {
- sp.AddSession("test-user", "builder-type-x", "host-type-x", &buildlet.FakeClient{})
+ sp.AddSession("accounts.google.com:user-xyz-124", "test-user", "builder-type-x", "host-type-x", &buildlet.FakeClient{})
  }
  sp.destroyExpiredSessions(context.Background())
  if sp.Len() != wantInstances {
@@ -66,16 +66,16 @@ func TestSessionPoolList(t *testing.T) {
 
  wantCount := 4
  for i := 0; i < wantCount; i++ {
- sp.AddSession(fmt.Sprintf("user-%d", i), "builder", "host", &buildlet.FakeClient{})
+ sp.AddSession("accounts.google.com:user-xyz-124", fmt.Sprintf("user-%d", i), "builder", "host", &buildlet.FakeClient{})
  }
  got := sp.List()
  if len(got) != wantCount {
  t.Errorf("SessionPool.List() = %v; want %d sessions", got, wantCount)
  }
  for it, s := range got[:len(got)-1] {
- if s.name > got[it+1].name {
- t.Fatalf("SessionPool.List(): Session[%d].name=%s > Session[%d].name=%s; want sorted by name",
- it, s.name, it+1, got[it+1].name)
+ if s.ID > got[it+1].ID {
+ t.Fatalf("SessionPool.List(): SessionInstance[%d].ID=%s > SessionInstance[%d].ID=%s; want sorted by name",
+ it, s.ID, it+1, got[it+1].ID)
  }
  }
 }
@@ -86,7 +86,7 @@ func TestSessionPoolDestroySession(t *testing.T) {
 
  var sn []string
  for i := 0; i < 4; i++ {
- name := sp.AddSession(fmt.Sprintf("user-%d", i), "builder", "host", &buildlet.FakeClient{})
+ name := sp.AddSession("accounts.google.com:user-xyz-124", fmt.Sprintf("user-%d", i), "builder", "host", &buildlet.FakeClient{})
  sn = append(sn, name)
  }
  for _, name := range sn {
@@ -95,22 +95,3 @@ func TestSessionPoolDestroySession(t *testing.T) {
  }
  }
 }
-
-func TestSessionPoolUserFromGomoteInstanceName(t *testing.T) {
- testCases := []struct {
- desc string
- buildletName string
- user string
- }{
- {"mutable", "user-bradfitz-linux-amd64-0", "bradfitz"},
- {"non-mutable", "mutable-user-bradfitz-darwin-amd64-10_8-0", "bradfitz"},
- {"invalid", "yipeee", ""},
- }
- for _, tc := range testCases {
- t.Run(tc.desc, func(t *testing.T) {
- if got := userFromGomoteInstanceName(tc.buildletName); got != tc.user {
- t.Errorf("userFromGomoteInstanceName(tc.buildletName) = %q; want %q", got, tc.user)
- }
- })
- }
-}
diff --git a/internal/coordinator/schedule/fake_schedule.go b/internal/coordinator/schedule/fake_schedule.go
@@ -9,23 +9,36 @@ package schedule
 
 import (
  "context"
+ "sync"
 
  "golang.org/x/build/buildlet"
  "golang.org/x/build/types"
 )
 
 // Fake is a fake scheduler.
-type Fake struct{}
+type Fake struct {
+ mu sync.Mutex
+ state SchedulerState
+}
 
 // NewFake returns a fake scheduler.
-func NewFake() *Fake { return &Fake{} }
+func NewFake() *Fake {
+ return &Fake{
+ state: SchedulerState{
+ HostTypes: []SchedulerHostState{},
+ },
+ }
+}
 
 // State returns the state of the fake scheduler.
-func (f *Fake) State() (st SchedulerState) { return SchedulerState{} }
+func (f *Fake) State() (st SchedulerState) { return f.state }
 
 // WaiterState is the waiter state of the fake scheduler.
 func (f *Fake) WaiterState(waiter *SchedItem) (ws types.BuildletWaitStatus) {
- return types.BuildletWaitStatus{}
+ return types.BuildletWaitStatus{
+ Message: "buildlet created",
+ Ahead: 0,
+ }
 }
 
 // GetBuildlet returns a fake buildlet client for the requested buildlet.