601783 - Deactivate screensaver on Rev3 Fedora when running talos

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #585286 +++

Bug 585286 seems to have successfully deactivated the screensaver on mochitest and reftest machines, thanks.

AFAICS the extra commands added there are not being run on talos machines.

The existence of bug 585286 suggests that the screensaver is also sometimes running on talos machines, which would be expected to affect results.

Comment 1

[[e-mail] (:bhearsum)]

Off to John for prioritization

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Strictly, bug 585286 added code to factory for unittests to run 'xset s reset', see attachment 472788 [details] [diff] [review]. See attachment 466896 [details] [diff] [review] for karl's take on doing the same for talos.

Comment 3

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #1)
> Off to John for prioritization

If the screen saver is not disabled, and this impacts production talos numbers, this should be fixed quickly.


:karlt: can we close this as a DUP of (already fixed) bug 585286 - or is there something else we still need to do ?

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #3)
> (In reply to comment #1)
> > Off to John for prioritization
> 
> If the screen saver is not disabled, and this impacts production talos numbers,
> this should be fixed quickly.
> 
> 
> :karlt: can we close this as a DUP of (already fixed) bug 585286 - or is there
> something else we still need to do ?

That bug was for unit tests, this bug is for talos.  This should be very simple to fix and should be able to use almost the same code.

If nobody has fixed this by the next time that I test a mozilla-tests patch, I will take it.  The code that needs to be added would be

+        if 'fed' in self.OS:
+            self.addStep(ShellCommand(
+                name='disable_screensaver',
+                command=['xset', 's', 'reset'],
+            ))

Not sure exactly where to put this in TalosFactory.

Comment 5

[Mike Taylor [:bear]]

in TalosFactory.addCleanupSteps() would be my suggestion - this step is run before anything is downloaded or processed.

Comment 6

[Mike Taylor [:bear]]

dustin, assigning this to you - ping me when you take a look at it so I can walk you thru it

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: 601783.patch

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Verified on mozilla-tests2.  Before I land this, jhford: why use 'xset s reset' instead of 'xset s off'?

Comment 9

[Karl Tomlinson (:karlt)]

'xset s off' only changes the screen saver timeout.

man XSetScreenSaver:
"A timeout of 0 disables
 the screen saver (but an activated screen saver is not deactivated)"

The screen saver timeout already seems to be 0.
See bug 585286 comment 1 and 11.

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 490940 [details] [diff] [review]
601783.patch

Landed in changeset:   1248:8e866243561c, merged to the production-0.8 branch in 1250:30537b778f04

Comment 11

[[e-mail] (:bhearsum)]

Is this done now?

Comment 12

[Karl Tomlinson (:karlt)]

"argv: ['xset', '-s', 'reset']"

"xset:  unknown option -s"

That should be ['xset', 's', 'reset'].

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: m570843-puppet-manifests-r1.patch

Wow, how did I manage to screw that up?

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: m601783-buildbotcustom-r2.patch

Correct patch (sorry)

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 495612 [details] [diff] [review]
m601783-buildbotcustom-r2.patch

Thanks :)

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 495612 [details] [diff] [review]
m601783-buildbotcustom-r2.patch

Landed on default and production-0.8.

Comment 17

[[e-mail] (:bhearsum)]

Clearing reconfig flag.

Comment 18

[Karl Tomlinson (:karlt)]

That's running successfully now, thanks.