1319487 - Don't set APPDATA, LOCALAPPDATA, TEMP, TMP in gecko windows (generic worker) task definitions

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Pete Moore [:pmoore][:pete]]

When bug 1306989 was raised, these environment variables were referencing a temporary profile on the C: drive, which was breaking some tasks.

As a workaround, these environment variables were set explicitly to an alternative location underneath the task directory.

Since this workaround was applied, the root source of the problem was discovered and resolved, such that task users no longer use temporary profiles, but instead have profiles located in the normal/default locations.

I think therefore it makes sense to now remove the workaround, which I believe is no longer needed.

Initial try push here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b46922cf82c3413581a6dc8cdbe687078ccdb7f4

Comment 1

[Rob Thijssen [:grenade (EET/UTC+0300)]]

if those variables are not set, the task writes temp data to the c: drive. we don't want that because c: is slow and it's size is limited. i believe these variables still need to be set to directories under the task directory.

Comment 2

[Pete Moore [:pmoore][:pete]]

OK, that is a fair point. If it is working, maybe we can leave it as it is. Note, we could get in trouble if any processes use other means to decide where these folder locations are (e.g. with syscalls / querying registry etc). However, maybe all processes refer to the environment variables, and we're ok.

By the way, I noticed on the latest mozilla-inbound push, these env vars aren't getting set, e.g. see: https://tools.taskcluster.net/task-inspector/#aSgB9iBzTH6VLkXjVLZj8A/

In the env: { .... } section of the task payload, APPDATA, LOCALAPPDATA, TEMP, TMP are not listed. :/

Comment 3

[Rob Thijssen [:grenade (EET/UTC+0300)]]

yes, its only test jobs that set these vars (not build) currently. i had intended to add the setting of these vars to build jobs as well, but in light of the current confusion around access rights for SHGetSpecialFolderLocation in bug 1319453, i'm waiting till i understand the problem better.

Comment 4

[Pete Moore [:pmoore][:pete]]

(In reply to Pete Moore [:pmoore][:pete] from comment #2)
> Note, we could get in trouble if any processes use other means to decide
> where these folder locations are (e.g. with syscalls / querying registry
> etc).

In bug 1319453 we can see an example of exactly this. For example, see https://public-artifacts.taskcluster.net/S_Ld9Uv-RfOFL8OK0kfc5g/0/public/logs/live_backing.log

In here, we see:


Z:\task_1479996319>test-getspecialfolderlocation.exe
Got 'C:\Users\task_1479996319\AppData\Roaming'

yet, the env var shows:

Z:\task_1479996319>set
APPDATA=Z:\task_1479996319\AppData\Roaming
....

So it looks like there are at least two methods to retrieve APPDATA location - via syscalls, and via the environment. And we know from bug 1319453 that we do indeed use the syscall in places. Therefore, it really looks like we shouldn't edit the env var to be something different.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1319453#c14 there appears to be a formal way to modify the underlying values, so I think this might be a better approach.

The other alternative is that we modify the setting when creating the OS (according to https://support.microsoft.com/en-us/kb/973289 this is the only supported way to change this setting).

The advantage of this, is that we only have one setting to change, and it can be done independently of the worker, as an environment setting. It is also supported, and there is only one setting to change. Also since we build our own images for Windows in AWS, I believe we should be in a position to modify this setting (since we do not use pre-built images).

So I'd still propose for now, that we remove the env vars, to avoid conflicts with the syscall values, and if we agree it is best to update the OS images to change default location of user profiles, do that in a separate bug.

Comment 5

[Pete Moore [:pmoore][:pete]]

Attachment: bug1319487_mozilla-central_v1.patch

Hey Rob,

In light of the discovery that we're using syscalls in tests that will return the underlying value on C: rather than the version we specify in the env var, this patch ensures that at least we are consistent. And hopefully we can get them moved over to Z: drive some other way (via OS installation configuration or other syscalls).

If you disagree with this approach, let's discuss.

Thanks,
Pete

Comment 6

[Rob Thijssen [:grenade (EET/UTC+0300)]]

there's nothing wrong with the patch but it's premature. the reason those env vars are being set is because not having them set means temp data get's written to the c: drive which slows everything down. the setting of those vars is a workaround until g-w is able to correctly set them to paths under the tasks dir on the z: drive. by all means, remove the env var setting in tree, but please do it after the underlying problem, that they were created as a workaround for, is fixed.

Comment 7

[Pete Moore [:pmoore][:pete]]

(In reply to Rob Thijssen (:grenade - GMT) from comment #6)
> there's nothing wrong with the patch but it's premature. the reason those
> env vars are being set is because not having them set means temp data get's
> written to the c: drive which slows everything down. the setting of those
> vars is a workaround until g-w is able to correctly set them to paths under
> the tasks dir on the z: drive. by all means, remove the env var setting in
> tree, but please do it after the underlying problem, that they were created
> as a workaround for, is fixed.

The problem with setting the environment variables, is that their values conflict with the values returned by e.g. the syscall that is made in firefox in bug 1319453. This means the system has conflicting information on what the values should be, and different processes will (and do) infer different values for them, leaving the system in a inconsistent state. If the env var was the only source of truth on what the value is, it would be correct to set it to an alternative location, but setting it to an alternative value when the internal state (which we know is referenced in our tests) is something else, could cause obscure bugs, or worse still disguise real problems.

Therefore I think we have to set them to the less-performant values, in order to retain integrity.

If there are concerns of a serious performance penalty, we should capture numbers. The NSS builds and tests all run on the C: drive, and run pretty quickly.

Ted, independent third opinion on the matter?

Comment 8

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

Comment #4 had me thinking that using SHSetKnownFolderPath *and* setting the env vars was the right approach -- no?

Comment 9

[Pete Moore [:pmoore][:pete]]

(In reply to Dustin J. Mitchell [:dustin] from comment #8)
> Comment #4 had me thinking that using SHSetKnownFolderPath *and* setting the
> env vars was the right approach -- no?

The two options (I'm aware of) that we have for setting the *underlying* values are:

1) using SHSetKnownFolderPath for all affected folders (see how many there potentially are here: https://msdn.microsoft.com/en-us/library/windows/desktop/dd378457(v=vs.85).aspx), or

2) setting them all in one fell swoop at OS-installation time with one setting here: https://support.microsoft.com/en-us/kb/973289 

Both of these would affect the _underlying_ values as interpreted by the kernel, which would be reflected in the environment variable settings automatically, by virtue of us calling https://msdn.microsoft.com/en-us/library/windows/desktop/bb762270(v=vs.85).aspx (which we do when we prepare a process). Therefore we shouldn't need to explicitly set the environment variables at all, but allow the kernel to provide them to us, and only modify the underlying internal values that they are derived from.

Comment 10

[Rob Thijssen [:grenade (EET/UTC+0300)]]

can you explain what is meant by conflict? i was under the impression that setting the environment variables makes everything that uses the environment variables to determine paths, work correctly (as in store temp data under the task dir on the fast z: drive). as far as i've seen in builds and tests, only the marionette server uses the system call to determine the currently wrong location on the c: drive. all other tests currently running in production are correctly writing temp data to the z: drive because of the workaround/hack which sets the environment variables.

i really think the solution is to first fix the user profile creation so that it sets the known folder paths to directories under the task directory and only then remove the workaround which is currently working perfectly by keeping temp data from production tests on the z: drive.

Comment 11

[(not currently active) Ted Mielczarek]

The system settings (as returned by the syscalls) are intended to be the ground source of truth, and they're Microsoft's recommended way of getting these paths. If there's mismatch between those and the environment variables things are likely to break in hard-to-diagnose ways, and we're really not setting the environment up properly.

If we can set the paths properly at OS install time or user creation time that seems like it would be the solution least prone to subtle breakage. If that's not feasible then calling `SHSetKnownFolderPath` for everything sounds like it should work, but it does leave us open to breaking if Microsoft adds a new known folder type that we're not handling.

Comment 12

[Rob Thijssen [:grenade (EET/UTC+0300)]]

it may not be necessary to change default folder paths for everything. for example, i've not seen builds that write to c:\users\taskxxx\documents, etc... not to say it doesn't happen, just that i've not seen it, and it's not created speed issues for builds.

the folders that get hit heavily during build/test task runs are
- temp
- appdata\local
- appdata\roaming

if we could call SHSetKnownFolderPath against those 3 to start with, it would keep the vast majority (if not all) of task data on the fast drives.

Comment 13

[Pete Moore [:pmoore][:pete]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> The system settings (as returned by the syscalls) are intended to be the
> ground source of truth, and they're Microsoft's recommended way of getting
> these paths. If there's mismatch between those and the environment variables
> things are likely to break in hard-to-diagnose ways, and we're really not
> setting the environment up properly.

This was the cause of new bug 1323061.

> If we can set the paths properly at OS install time or user creation time
> that seems like it would be the solution least prone to subtle breakage. If
> that's not feasible then calling `SHSetKnownFolderPath` for everything
> sounds like it should work, but it does leave us open to breaking if
> Microsoft adds a new known folder type that we're not handling.

I also prefer to set at AMI creation time, for the same reasons. Is this feasible Rob?

In the meantime, I'd propose we land this change to unblock bug 1323061.

Before taking further action to change the default location, it might be worth collecting stats of build/test times under both scenarios, to see how much it affects performance.

Comment 14

[Rob Thijssen [:grenade (EET/UTC+0300)]]

(In reply to Pete Moore [:pmoore][:pete] from comment #13)

> I also prefer to set at AMI creation time, for the same reasons. Is this
> feasible Rob?

no, of course not. the task users are not known at ami creation time. they are only known by the process that creates them. the user known folders have to be set by the same process that creates the task user, eg: generic-worker.

> In the meantime, I'd propose we land this change to unblock bug 1323061.

only fixing the user creation process can unblock that.

Comment 15

[Pete Moore [:pmoore][:pete]]

(In reply to Rob Thijssen (:grenade - GMT) from comment #14)
> (In reply to Pete Moore [:pmoore][:pete] from comment #13)
> 
> > I also prefer to set at AMI creation time, for the same reasons. Is this
> > feasible Rob?
> 
> no, of course not. the task users are not known at ami creation time. they
> are only known by the process that creates them. the user known folders have
> to be set by the same process that creates the task user, eg: generic-worker.

The instructions for customising the default user profile location are here:
https://support.microsoft.com/en-us/kb/973289

Now that we build our own OS images, are we able to do this at image creation time?

Comment 16

[Pete Moore [:pmoore][:pete]]

Also looks like there might be some helpful information here: https://support.microsoft.com/en-us/kb/949977

Thanks Rob.

Comment 17

[Pete Moore [:pmoore][:pete]]

And lastly: https://support.microsoft.com/en-us/kb/929831

Comment 18

[Rob Thijssen [:grenade (EET/UTC+0300)]]

it's disingenuous to suggest that a change of this nature (rebuilding the windows amis from pre-sysprep state) is going to be straightforward or could be approached with this level of nonchalance. it's not trivial to implement in the manner suggested and i fully expect it would be accompanied by an entire herd of yaks to be shaved.

Comment 19

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

I'm hearing a lot of "that won't work because.."

Is there a positive solution?

Comment 20

[Pete Moore [:pmoore][:pete]]

(In reply to Dustin J. Mitchell [:dustin] from comment #19)
> I'm hearing a lot of "that won't work because.."
> 
> Is there a positive solution?

I'm happy to experiment with changing the FolderLocation settings in the Unattend.xml file.

Q: Please can you provide full details about the Windows AMI creation process, in order that I can perform the steps myself? Many thanks.

Comment 21

[Pete Moore [:pmoore][:pete]]

In the meantime, I'd propose we land the patch in this bug, which has been shown to solve the issue (see https://bugzilla.mozilla.org/show_bug.cgi?id=1323061#c4).

We can also experiment with SHSetKnownFolderPath - but I don't think this is the correct solution, for the reason Ted also cited in comment 11.

If we've reached stalemate here, I'd propose we organise a meeting with involved parties, to work out a resolution, rather than discuss too much in this bug.

Comment 22

[Pete Moore [:pmoore][:pete]]

I've added a utility method for SHSetKnownFolderPath here:
https://github.com/taskcluster/runlib/blob/85938334b16f58e0fe0f9bb2699a317a08c1a84f/win32/extra_windows.go#L110-L127

Still need to add some code to call it from generic worker.

Comment 23

[Pete Moore [:pmoore][:pete]]

Added known folder GUIDs here:
https://github.com/taskcluster/runlib/commit/a3f808177c1e7e7c12748ae6a74b03f68ad255b2

... still need to make the calls to set a subset of them.

Comment 24

[Pete Moore [:pmoore][:pete]]

(In reply to Rob Thijssen (:grenade - GMT) from comment #12)

> the folders that get hit heavily during build/test task runs are
> - temp
> - appdata\local
> - appdata\roaming

It looks like there isn't a KNOWNFOLDERID for the user temp directory.

Kind of interestingly, this article has an explanation of why: https://blogs.msdn.microsoft.com/oldnewthing/20150417-00/?p=44213

So there is a syscall which uses (just) env vars to determine temp directory: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364992(v=vs.85).aspx

Note, the system TEMP and TMP env vars are set to %USERPROFILE%\AppData\Local\Temp and therefore it may just be sufficient to change this to something else that uses e.g. %USERNAME% in the path, so that it gets resolved differently for each user. However, I've spotted that there is a KNOWNFOLDERID called FOLDERID_Profile which hopefully will affect %USERPROFILE%, and thus indirectly affect TEMP and TMP.

I will experiment.

FOLDERID_Profile

For the user temp directory, we might be ok to create the directory manually and then set TEMP/TMP accordingly. It could be though, that there is some syscall out there  

The user appdata folders appear to be:
  * FOLDERID_RoamingAppData
  * FOLDERID_LocalAppData

I'm still not feeling too confident about changing these at runtime, but I

Comment 25

[Pete Moore [:pmoore][:pete]]

Whoops, bit of a botched edit there ^^^^ :-)

Please ignore everything after "I will experiment."

Comment 26

[Pete Moore [:pmoore][:pete]]

Per https://msdn.microsoft.com/en-us/library/windows/desktop/bb762512(v=vs.85).aspx it is not possible to redirect (point to a different location) folders of category KF_CATEGORY_FIXED - hence, we cannot update FOLDERID_Profile - this can *only* be updated at OS creation time.

However, we can redirect FOLDERID_RoamingAppData and FOLDERID_LocalAppData and we can update per user TEMP/TMP env vars by setting global TEMP/TMP env vars on the environment to a suitable location, including %USERNAME% in the env var (such as Z:\%USERNAME\TEMP).

Comment 27

[Pete Moore [:pmoore][:pete]]

typo: Z:\%USERNAME%\TEMP

Comment 28

[Pete Moore [:pmoore][:pete]]

I'm testing changes now...

Comment 29

[Pete Moore [:pmoore][:pete]]

So some tweaks are still needed.

When _running as current user_, these calls need to be made outside of the generic worker, as CreateProcess will use its own env vars as the basis for the subprocess it creates, so in order that these are set correctly, SHSetKnownFolderPath needs to be called for FOLDERID_RoamingAppData and FOLDERID_LocalAppData before the generic worker is even started, otherwise it will not provide any effect.

To summarize then, in order for the Temp, AppData\Roaming and AppData\Local to be set correctly *when running as current user* all the steps should happen before the worker starts up. The steps are

1) set TMP env var to something like Z:\%USERNAME%\Temp
2) set TEMP env var to something like Z:\%USERNAME%\Temp
3) call SHSetKnownFolderPath with FOLDERID_LocalAppData and something like Z:\%USERNAME%\AppData\Local
4) call SHSetKnownFolderPath with FOLDERID_RoamingAppData and something like Z:\%USERNAME%\AppData\Roaming

Note [1] might help with 3 and 4 if you want to do this in powershell. Otherwise I can provide a command line utility that will also do it.

[1] http://stackoverflow.com/questions/25709398/set-location-of-special-folders-with-powershell

======================

Note, when we run as a task user, rather than as the current user, these calls need to be made from inside generic worker. I've committed the code for this today, but we can't roll out due to the ongoing win 7 performance problems that caused us to switch to running as current user in the first place.

Comment 30

[Pete Moore [:pmoore][:pete]]

Also note, the referenced folders need to be created too - probably best to do that first, before the above 4 steps.

Comment 31

[Rob Thijssen [:grenade (EET/UTC+0300)]]

it's not possible to create the referenced folders in processes that don't have a valid value for %USERNAME%. the process that creates the user, must therefore also create the necessary folders.

Comment 32

[Amy Rich [:arr] [:arich]]

(In reply to Pete Moore [:pmoore][:pete] from comment #29)

Per the decision on Wednesday to use the generic worker user, is this work necessary? It sounds like this is support for the task user?

Comment 33

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Comment on attachment 8814089 [details] [diff] [review]
bug1319487_mozilla-central_v1.patch

minor nit: the whole line could be just removed.

Comment 34

[Pete Moore [:pmoore][:pete]]

(In reply to Amy Rich [:arr] [:arich] from comment #32)
> (In reply to Pete Moore [:pmoore][:pete] from comment #29)
> 
> Per the decision on Wednesday to use the generic worker user, is this work
> necessary? It sounds like this is support for the task user?

Hey Amy,

It is a good question, as this is a bit confusing. There are two different (but related) issues at play here:

1) Whether to run tasks as GenericWorker user, or create separate users per task
2) Whether user profile folder locations should be on default C:\ locations or moved to Z:\, regardless of whether this is for the one GW user, or for multiple task users

The SHSetKnownFolderPath stuff is to address the desire to get everything on Z: (point 2) and is needed regardless of the choice for point 1.

Note, this isn't needed if we decide:

a) Having on C: does not impact performance (jmaher has been capturing data on this, and can chime in)
b) We are happy to just set env vars, and accept risks of not setting underlying values (note, >1 bugs have been raised because of limitations of this approach - so I prefer to attack this problem)

Note, even when we agree that b) should be done, there are again two choices:

I) at OS creation time
II) via multiple SHSetKnownFolderPath calls and tweaking of TEMP/TMP setting in default user profile

I) is preferable/safer/more thorough than II). However I) is not universally supported, so the SHSetKnownFolderPath work I've been doing is a compromise to support option II).

Comment 35

[Pete Moore [:pmoore][:pete]]

For II), I've created https://github.com/taskcluster/knownfolder/ which can be used in OpenCloudConfig if desired, when running as GenericWorker user.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce53ace37c22
don't explicitly set env vars APPDATA, LOCALAPPDATA, TEMP, TMP in generic-worker tasks,r=grenade

Comment 37

[Pete Moore [:pmoore][:pete]]

(In reply to Rob Thijssen (:grenade - GMT) from comment #31)
> it's not possible to create the referenced folders in processes that don't
> have a valid value for %USERNAME%. the process that creates the user, must
> therefore also create the necessary folders.

I believe if we update the default user profile, we are ok. See

https://github.com/mozilla-releng/OpenCloudConfig/pull/29/files

I'm not sure how to test this change locally though.

I believe if this registry setting is applied before a user profile is created, the user profile creation will result in TMP/TEMP being set correctly, which should be the case as this DSC runs before the GenericWorker user profile is created on first login of the GenericWorker user.

Let me know if you'd prefer me to apply this change to just one alpha/beta worker type, before landing everywhere. Thanks!

Comment 38

[Pete Moore [:pmoore][:pete]]

Attachment: Github Pull Request for OpenCloudConfig

This would apply the change universally - let me know if you prefer me to land on just one alpha worker type first, and I'll create a separate PR.

Comment 39

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8819337 [details] [review]
Github Pull Request for OpenCloudConfig

Removing review, as I've discovered that the .DEFAULT profile is not the same as the default user profile.

So I'm working on a different solution, that mounts the registry hive for the default user profile, modifies it, and unmounts...

More to follow!

Comment 40

[Pete Moore [:pmoore][:pete]]

(In reply to Pete Moore [:pmoore][:pete] from comment #39)

> More to follow!

The powershell snippet looks like this, and works locally:

=================================

# utility function to replace/create a registry key, depending on whether it exists already
function SetKey($registryPath, $name, $value)
{
    if (!(Test-Path $registryPath)) {
        New-Item -Path $registryPath -Force | Out-Null
    }
    New-ItemProperty -Path $registryPath -Name $name -Value $value `
    -PropertyType ExpandString -Force | Out-Null
}

# mount default user profile registry hive
reg load "HKLM\DefaultUser" "C:\Documents and Settings\Default User\ntuser.dat"

# set TMP env var in profile
SetKey -registryPath "HKLM:\DefaultUser\Environment" -name "TMP" -value "Z:\Users\%USERNAME%\AppData\Roaming\Temp"

# set TEMP env var in profile
SetKey -registryPath "HKLM:\DefaultUser\Environment" -name "TEMP" -value "Z:\Users\%USERNAME%\AppData\Roaming\Temp"

# clean up handles so we can unmount registry hive
[gc]::collect()

# unmount registry hive
reg unload HKLM\DefaultUser

==============================

I'll integrate this with OpenCloudConfig on Monday, need to head off now! I'll also need to check it works with Windows 10 / Windows 2012 R2.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce53ace37c22

Comment 42

[Pete Moore [:pmoore][:pete]]

These env vars are no longer included in the task definitions.

Comment 43

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.