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.

Sign up for GitHub

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

Jump to bottom

dont interact with desktop client while in standby #1700

Merged
merged 3 commits into from
Apr 30, 2024
Merged

dont interact with desktop client while in standby #1700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42f20a7
skip desktop health checks and spawning in standby
James-Pickett Apr 30, 2024
159f07b
dont interact with desktop client in standby
James-Pickett Apr 30, 2024
8578fab
update test to include mock InModernStandby call
James-Pickett Apr 30, 2024
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
21 changes: 21 additions & 0 deletions ee/desktop/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,13 @@ func (r *DesktopUsersProcessesRunner) killDesktopProcesses(ctx context.Context)
}

func (r *DesktopUsersProcessesRunner) SendNotification(n notify.Notification) error {
if r.knapsack.InModernStandby() {
r.slogger.Log(context.TODO(), slog.LevelDebug,
"modern standby detected, skipping notification send",
)
return errors.New("modern standby detected, skipping notification send")
}

if len(r.uidProcs) == 0 {
return errors.New("cannot send notification, no child desktop processes")
}
Expand Down Expand Up @@ -436,6 +443,13 @@ func (r *DesktopUsersProcessesRunner) refreshMenu() {
}
}

if r.knapsack.InModernStandby() {
r.slogger.Log(context.TODO(), slog.LevelDebug,
"modern standby detected, skipping menu refresh",
)
return
}

// Tell any running desktop user processes that they should refresh the latest menu data
for uid, proc := range r.uidProcs {
client := client.New(r.userServerAuthToken, proc.socketPath)
Expand Down Expand Up @@ -531,6 +545,13 @@ func (r *DesktopUsersProcessesRunner) writeDefaultMenuTemplateFile() {
}

func (r *DesktopUsersProcessesRunner) runConsoleUserDesktop() error {
if r.knapsack.InModernStandby() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we skip here or in the execute loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here makes sense! Looking here versus execute, I think when we're in modern standby we would still want to refresh menu data (

launcher/ee/desktop/runner/runner.go

Line 235 in 42f20a7

case <-menuRefreshTicker.C:
) but maybe not tell clients to pick up the new data (

launcher/ee/desktop/runner/runner.go

Line 439 in 42f20a7

// Tell any running desktop user processes that they should refresh the latest menu data
) -- so putting the InModernStandby check into these individual functions makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not tell the clients in standby.

I forget how menu refresh works, if we skip it, will we miss things, or will we pick things up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will still pick up the menu changes and can still have the most updated menu available when we come out of standby

the data to update the menu comes in via the control server update

r.slogger.Log(context.TODO(), slog.LevelDebug,
"modern standby detected, skipping desktop process spawning and health checks",
)
return nil
}

if !r.processSpawningEnabled {
// Desktop is disabled, kill any existing desktop user processes
r.killDesktopProcesses(context.Background())
Expand Down
3 changes: 3 additions & 0 deletions ee/desktop/runner/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ func TestDesktopUserProcessRunner_Execute(t *testing.T) {
mockKnapsack.On("KolideServerURL").Return("somewhere-over-the-rainbow.example.com")
mockKnapsack.On("DesktopEnabled").Return(true)
mockKnapsack.On("Slogger").Return(slogger)
mockKnapsack.On("InModernStandby").Return(false)

if os.Getenv("CI") != "true" || runtime.GOOS != "linux" {
// Only expect that we call Debug (to set the DEBUG flag on the process) if we actually expect
Expand Down Expand Up @@ -298,6 +299,7 @@ func TestUpdate(t *testing.T) {
mockKnapsack.On("KolideServerURL").Return("somewhere-over-the-rainbow.example.com")
mockKnapsack.On("DesktopEnabled").Return(true)
mockKnapsack.On("Slogger").Return(multislogger.NewNopLogger())
mockKnapsack.On("InModernStandby").Return(false)

dir := t.TempDir()
r, err := New(mockKnapsack, nil, WithUsersFilesRoot(dir))
Expand Down Expand Up @@ -332,6 +334,7 @@ func TestSendNotification_NoProcessesYet(t *testing.T) {
mockKnapsack.On("KolideServerURL").Return("somewhere-over-the-rainbow.example.com")
mockKnapsack.On("DesktopEnabled").Return(true)
mockKnapsack.On("Slogger").Return(multislogger.NewNopLogger())
mockKnapsack.On("InModernStandby").Return(false)

dir := t.TempDir()
r, err := New(mockKnapsack, nil, WithUsersFilesRoot(dir))
Expand Down
Loading