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

Improve performance of retrieving windows desktops #52062

Open
rosstimothy opened this issue Feb 12, 2025 · 0 comments
Open

Improve performance of retrieving windows desktops #52062

rosstimothy opened this issue Feb 12, 2025 · 0 comments

Comments

@rosstimothy
Copy link
Contributor

Problem

To retrieve a single windows desktop one must use the GetWindowsDesktops RPC with a DesktopFilter. The backend implementation does a range read and applies the filter on each desktop even though when the filter is given with a hostID and a name it should only match a single desktop.

func (s *WindowsDesktopService) GetWindowsDesktops(ctx context.Context, filter types.WindowsDesktopFilter) ([]types.WindowsDesktop, error) {
startKey := backend.ExactKey(windowsDesktopsPrefix)
result, err := s.GetRange(ctx, startKey, backend.RangeEnd(startKey), backend.NoLimit)
if err != nil {
return nil, trace.Wrap(err)
}
var desktops []types.WindowsDesktop
for _, item := range result.Items {
desktop, err := services.UnmarshalWindowsDesktop(item.Value,
services.WithExpires(item.Expires), services.WithRevision(item.Revision))
if err != nil {
return nil, trace.Wrap(err)
}
if !filter.Match(desktop) {
continue
}
desktops = append(desktops, desktop)
}
// If both HostID and Name are set in the filter only one desktop should be expected
if filter.HostID != "" && filter.Name != "" && len(desktops) == 0 {
return nil, trace.NotFound("windows desktop \"%s/%s\" doesn't exist", filter.HostID, filter.Name)
}
return desktops, nil
}

Each time a desktop is heartbeated via UpsertWindowsDesktop the entire desktop range is read to find the matching host.

// If the desktop exists, check access,
// if it doesn't, continue.
existing, err := a.authServer.GetWindowsDesktops(ctx,
types.WindowsDesktopFilter{HostID: s.GetHostID(), Name: s.GetName()})
if err == nil && len(existing) != 0 {
if err := a.checkAccessToWindowsDesktop(existing[0]); err != nil {
return trace.Wrap(err)
}
} else if err != nil && !trace.IsNotFound(err) {
return trace.Wrap(err)
}

In clusters with a large number of desktops this results in excessive CPU consumption to find a single desktop.

Image

Additionally, the GetWindowsDesktops RPC is not paginated, and may result in OOM or exceeding the gRPC message sizes if called in a cluster with a large number of desktops.

Proposal

  1. Modify the GetWindowsDesktops backend method to do a point read if the filter contains a hostID and a name. While this doesn't address the higher level concerns the RPC presents, it will eliminate the excessive CPU consumption during desktop heartbeats and can safely be backported.

  2. Deprecate the GetWindowsDesktops RPC and add a GetWindowsDesktop and possibly a ListWindowsDesktop RPC. The latter may not be necessary since the same can be achieved with the ListResources or ListUnifiedResources RPCs.

zmb3 added a commit that referenced this issue Feb 12, 2025
zmb3 added a commit that referenced this issue Feb 12, 2025
zmb3 added a commit that referenced this issue Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants