Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix quadratic complexity in Reconciler.Reconcile(). #10989

Merged
merged 5 commits into from
Mar 10, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions api/types/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,16 +563,6 @@ func DeduplicateDatabases(databases []Database) (result []Database) {
// Databases is a list of database resources.
type Databases []Database

// Find returns database with the specified name or nil.
func (d Databases) Find(name string) Database {
for _, database := range d {
if database.GetName() == name {
return database
}
}
return nil
}

// ToMap returns these databases as a map keyed by database name.
func (d Databases) ToMap() map[string]Database {
m := make(map[string]Database)
Expand Down
21 changes: 14 additions & 7 deletions api/types/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,21 @@ type ResourceWithLabels interface {
// ResourcesWithLabels is a list of labeled resources.
type ResourcesWithLabels []ResourceWithLabels
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do: it is rather popular in the codebase. I could work to replace it, but not everywhere ResourcesWithLabelsMap makes sense.


// Find returns resource with the specified name or nil.
func (r ResourcesWithLabels) Find(name string) ResourceWithLabels {
for _, resource := range r {
if resource.GetName() == name {
return resource
}
// ResourcesWithLabelsMap is like ResourcesWithLabels, but a map from resource name to its value.
type ResourcesWithLabelsMap map[string]ResourceWithLabels

// ToMap returns these databases as a map keyed by database name.
func (r ResourcesWithLabels) ToMap() ResourcesWithLabelsMap {
rm := make(ResourcesWithLabelsMap, len(r))

// there may be duplicate resources in the input list.
// by iterating from end to start, the first resource of given name wins.
for i := len(r) - 1; i >= 0; i-- {
resource := r[i]
rm[resource.GetName()] = resource
}
return nil

return rm
}

// Len returns the slice length.
Expand Down
50 changes: 50 additions & 0 deletions api/types/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,53 @@ func TestMatchSearch_ResourceSpecific(t *testing.T) {
})
}
}

func TestResourcesWithLabels_ToMap(t *testing.T) {
mkServerHost := func(name string, hostname string) ResourceWithLabels {
server, err := NewServerWithLabels(name, KindNode, ServerSpecV2{
Hostname: hostname + ".example.com",
Addr: name + ".example.com",
}, nil)
require.NoError(t, err)

return server
}

mkServer := func(name string) ResourceWithLabels {
return mkServerHost(name, name)
}

tests := []struct {
name string
r ResourcesWithLabels
want ResourcesWithLabelsMap
}{
{
name: "empty",
r: nil,
want: map[string]ResourceWithLabels{},
},
{
name: "simple list",
r: []ResourceWithLabels{mkServer("a"), mkServer("b"), mkServer("c")},
want: map[string]ResourceWithLabels{
"a": mkServer("a"),
"b": mkServer("b"),
"c": mkServer("c"),
},
},
{
name: "first duplicate wins",
r: []ResourceWithLabels{mkServerHost("a", "a1"), mkServerHost("a", "a2"), mkServerHost("a", "a3")},
want: map[string]ResourceWithLabels{
"a": mkServerHost("a", "a1"),
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require.Equal(t, tt.r.ToMap(), tt.want)
})
}
}
14 changes: 7 additions & 7 deletions lib/services/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ type ReconcilerConfig struct {
// Matcher is used to match resources.
Matcher Matcher
// GetCurrentResources returns currently registered resources.
GetCurrentResources func() types.ResourcesWithLabels
GetCurrentResources func() types.ResourcesWithLabelsMap
// GetNewResources returns resources to compare current resources against.
GetNewResources func() types.ResourcesWithLabels
GetNewResources func() types.ResourcesWithLabelsMap
// OnCreate is called when a new resource is detected.
OnCreate func(context.Context, types.ResourceWithLabels) error
// OnUpdate is called when an existing resource is updated.
Expand Down Expand Up @@ -83,7 +83,7 @@ func NewReconciler(cfg ReconcilerConfig) (*Reconciler, error) {
}, nil
}

// Reconcile reconciles currently registered resources with new resources and
// Reconciler reconciles currently registered resources with new resources and
// creates/updates/deletes them appropriately.
//
// It's used in combination with watchers by agents (app, database, desktop)
Expand Down Expand Up @@ -123,9 +123,9 @@ func (r *Reconciler) Reconcile(ctx context.Context) error {

// processRegisteredResource checks the specified registered resource against the
// new list of resources.
func (r *Reconciler) processRegisteredResource(ctx context.Context, newResources types.ResourcesWithLabels, registered types.ResourceWithLabels) error {
func (r *Reconciler) processRegisteredResource(ctx context.Context, newResources types.ResourcesWithLabelsMap, registered types.ResourceWithLabels) error {
// See if this registered resource is still present among "new" resources.
if new := newResources.Find(registered.GetName()); new != nil {
if new := newResources[registered.GetName()]; new != nil {
return nil
}

Expand All @@ -139,10 +139,10 @@ func (r *Reconciler) processRegisteredResource(ctx context.Context, newResources

// processNewResource checks the provided new resource agsinst currently
// registered resources.
func (r *Reconciler) processNewResource(ctx context.Context, currentResources types.ResourcesWithLabels, new types.ResourceWithLabels) error {
func (r *Reconciler) processNewResource(ctx context.Context, currentResources types.ResourcesWithLabelsMap, new types.ResourceWithLabels) error {
// First see if the resource is already registered and if not, whether it
// matches the selector labels and should be registered.
registered := currentResources.Find(new.GetName())
registered := currentResources[new.GetName()]
if registered == nil {
if r.cfg.Matcher(new) {
r.log.Infof("%v %v matches, creating.", new.GetKind(), new.GetName())
Expand Down
8 changes: 4 additions & 4 deletions lib/services/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ func TestReconciler(t *testing.T) {
Matcher: func(rwl types.ResourceWithLabels) bool {
return MatchResourceLabels(test.selectors, rwl)
},
GetCurrentResources: func() types.ResourcesWithLabels {
return test.registeredResources
GetCurrentResources: func() types.ResourcesWithLabelsMap {
return test.registeredResources.ToMap()
},
GetNewResources: func() types.ResourcesWithLabels {
return test.newResources
GetNewResources: func() types.ResourcesWithLabelsMap {
return test.newResources.ToMap()
},
OnCreate: func(ctx context.Context, r types.ResourceWithLabels) error {
onCreateCalls = append(onCreateCalls, r)
Expand Down
4 changes: 2 additions & 2 deletions lib/srv/app/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,10 @@ func (m *monitoredApps) setResources(apps types.Apps) {
m.resources = apps
}

func (m *monitoredApps) get() types.ResourcesWithLabels {
func (m *monitoredApps) get() types.ResourcesWithLabelsMap {
m.mu.Lock()
defer m.mu.Unlock()
return append(m.static, m.resources...).AsResources()
return append(m.static, m.resources...).AsResources().ToMap()
}

// New returns a new application server.
Expand Down
4 changes: 2 additions & 2 deletions lib/srv/app/watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ func (s *Server) startResourceWatcher(ctx context.Context) (*services.AppWatcher
return watcher, nil
}

func (s *Server) getResources() (resources types.ResourcesWithLabels) {
return s.getApps().AsResources()
func (s *Server) getResources() (resources types.ResourcesWithLabelsMap) {
return s.getApps().AsResources().ToMap()
}

func (s *Server) onCreate(ctx context.Context, resource types.ResourceWithLabels) error {
Expand Down
4 changes: 2 additions & 2 deletions lib/srv/db/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,10 @@ func (m *monitoredDatabases) setCloud(databases types.Databases) {
m.cloud = databases
}

func (m *monitoredDatabases) get() types.ResourcesWithLabels {
func (m *monitoredDatabases) get() types.ResourcesWithLabelsMap {
m.mu.Lock()
defer m.mu.Unlock()
return append(append(m.static, m.resources...), m.cloud...).AsResources()
return append(append(m.static, m.resources...), m.cloud...).AsResources().ToMap()
}

// New returns a new database server.
Expand Down
7 changes: 2 additions & 5 deletions lib/srv/db/watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,8 @@ func (s *Server) startCloudWatcher(ctx context.Context) error {
}

// getResources returns proxied databases as resources.
func (s *Server) getResources() (resources types.ResourcesWithLabels) {
for _, database := range s.getProxiedDatabases() {
resources = append(resources, database)
}
return resources
func (s *Server) getResources() (resources types.ResourcesWithLabelsMap) {
Tener marked this conversation as resolved.
Show resolved Hide resolved
return s.getProxiedDatabases().AsResources().ToMap()
}

// onCreate is called by reconciler when a new database is created.
Expand Down
8 changes: 4 additions & 4 deletions lib/srv/desktop/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (s *WindowsService) startDesktopDiscovery() error {
// pre-filtered by nature of using an LDAP search with filters.
Matcher: func(r types.ResourceWithLabels) bool { return true },

GetCurrentResources: func() types.ResourcesWithLabels { return s.lastDiscoveryResults },
GetCurrentResources: func() types.ResourcesWithLabelsMap { return s.lastDiscoveryResults },
GetNewResources: s.getDesktopsFromLDAP,
OnCreate: s.upsertDesktop,
OnUpdate: s.upsertDesktop,
Expand Down Expand Up @@ -117,7 +117,7 @@ func (s *WindowsService) ldapSearchFilter() string {
}

// getDesktopsFromLDAP discovers Windows hosts via LDAP
func (s *WindowsService) getDesktopsFromLDAP() types.ResourcesWithLabels {
func (s *WindowsService) getDesktopsFromLDAP() types.ResourcesWithLabelsMap {
if !s.ldapReady() {
s.cfg.Log.Warn("skipping desktop discovery: LDAP not yet initialized")
return nil
Expand All @@ -144,14 +144,14 @@ func (s *WindowsService) getDesktopsFromLDAP() types.ResourcesWithLabels {

s.cfg.Log.Debugf("discovered %d Windows Desktops", len(entries))

var result types.ResourcesWithLabels
result := make(types.ResourcesWithLabelsMap)
for _, entry := range entries {
desktop, err := s.ldapEntryToWindowsDesktop(s.closeCtx, entry, s.cfg.HostLabelsFn)
if err != nil {
s.cfg.Log.Warnf("could not create Windows Desktop from LDAP entry: %v", err)
continue
}
result = append(result, desktop)
result[desktop.GetName()] = desktop
}

// capture the result, which will be used on the next reconcile loop
Expand Down
2 changes: 1 addition & 1 deletion lib/srv/desktop/windows_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ type WindowsService struct {
// when desktop discovery is enabled.
// no synchronization is necessary because this is only read/written from
// the reconciler goroutine.
lastDiscoveryResults types.ResourcesWithLabels
lastDiscoveryResults types.ResourcesWithLabelsMap

// Windows hosts discovered via LDAP likely won't resolve with the
// default DNS resolver, so we need a custom resolver that will
Expand Down