Bug 1516458 - take rootURL as config and set TASKCLUSTER_ROOT_URL and TASKCLUSTER_PROXY_URL by djmitche · Pull Request #135 · taskcluster/generic-worker

djmitche · 2018-12-28T22:01:47Z

For tests, we just run `go get github.com/taskcluster/taskcluster-proxy, so this uses the newest version. I'm not sure how to accomplish that in the prod deployment.

coveralls · 2018-12-28T22:10:26Z

Coverage decreased (-52.9%) to 17.7% when pulling b8cc67a on djmitche:bug1516458 into 7859cdf on taskcluster:master.

djmitche · 2018-12-29T04:20:59Z

hm, well, this isn't working yet..

djmitche · 2018-12-29T04:21:17Z

(somehow the tests passed for me locally once, but not anymore)

taskcluster_proxy.go

djmitche · 2018-12-31T14:38:40Z

OK, I think I figured it out -- generateCommand is called before the features are initialized, so the Env property of the task payload has already been read at feature initialization time. I switched to modifying each exec.Cmd's .Env property directly, as suggested by

    // generate commands, in case features want to modify them
    for i := range task.Payload.Command {
        err := task.generateCommand(i) // platform specific                                                                                                                                                                                                                                 
        if err != nil {
            panic(err)
        }    
    }

This fixes the tests for me locally. The error in CI on Windows is

C:\Users\task_1546265954\gopath1.10.3\src\github.com\taskcluster\generic-worker\testdata\TestTaskclusterProxy\task_1546265954>go run "curlget.go" $TASKCLUSTER_PROXY_URL/queue/v1/task/KTBKfEgxR5GdfIIREQIvFQ/runs/0/artifacts/SampleArtifacts/_/X.txt 
2018/12/31 14:28:57 Get http://localhost:34569/queue/v1/task/KTBKfEgxR5GdfIIREQIvFQ/runs/0/artifacts/SampleArtifacts/_/X.txt: dial tcp: lookup localhost: getaddrinfow: A non-recoverable error occurred during a database lookup.
exit status 1

which confuses me -- the PR makes the following change to the URL:

-                               fmt.Sprintf("http://localhost:%v/queue/v1/task/KTBKfEgxR5GdfIIREQIvFQ/runs/0/artifacts/SampleArtifacts/_/X.txt", config.TaskclusterProxyPort),
+                               fmt.Sprintf("$TASKCLUSTER_PROXY_URL/queue/v1/task/KTBKfEgxR5GdfIIREQIvFQ/runs/0/artifacts/SampleArtifacts/_/X.txt"),

so both before and (from the logging) after, it's http://localhost:<port>/.... Why would it suddenly have trouble looking up localhost?

djmitche · 2018-12-31T15:20:41Z

The Travis build is failing because env vars aren't available to PRs

djmitche · 2018-12-31T17:37:19Z

The same TestTaskclusterProxy test that's failing here seems to be failing reliably on master, just differently:

https://taskcluster-artifacts.net/flKO8uO0T3yA5rfyfQe2fg/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/FbZXyJP7TDSirPQLhqdaIQ/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/EGzzMzPNR6-gF9QiW5p9Lg/0/public/logs/live_backing.log

--- FAIL: TestTaskclusterProxy (67.03s)
	helper_test.go:211: Scheduled task b9dUhBvITfWfhHwCVfMPCQ
	helper_test.go:224: Something went wrong executing worker - got exit code 69 but was expecting exit code 0

(note when looking at those logs, there are two different test cases named TestTaskclusterProxy)

..so perhaps this has never worked, and my patch is unrelated?

petemoore · 2019-01-02T13:31:37Z

The same TestTaskclusterProxy test that's failing here seems to be failing reliably on master, just differently:

https://taskcluster-artifacts.net/flKO8uO0T3yA5rfyfQe2fg/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/FbZXyJP7TDSirPQLhqdaIQ/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/EGzzMzPNR6-gF9QiW5p9Lg/0/public/logs/live_backing.log
--- FAIL: TestTaskclusterProxy (67.03s)
	helper_test.go:211: Scheduled task b9dUhBvITfWfhHwCVfMPCQ
	helper_test.go:224: Something went wrong executing worker - got exit code 69 but was expecting exit code 0
(note when looking at those logs, there are two different test cases named TestTaskclusterProxy)

..so perhaps this has never worked, and my patch is unrelated?

It looks like it may be related to your patch. See

https://tools.taskcluster.net/groups/WtVE-HaNSjW2zOvFQ2veGQ/tasks/b9dUhBvITfWfhHwCVfMPCQ/runs/0/logs/public%2Flogs%2Flive.log#L27-59

petemoore · 2019-01-02T13:34:46Z

I suspect changes to the taskcluster proxy test(s) will be needed in light of taskcluster/taskcluster-proxy#38

petemoore · 2019-01-02T13:35:52Z

Although the comments in that PR say "This should be 100% backward compatible" so I'm not sure. Looking deeper now...

djmitche · 2019-01-02T14:31:37Z

This PR contains the fixes to make generic-worker work with the latest tc-proxy (after
taskcluster/taskcluster-proxy#38). But since generic-worker does not use a pinned version of the proxy, it makes sense that landing the tc-proxy PR broke CI for generic-worker until this is landed.

So we're back to localhost not resolving on windows. Googling suggests that's because %SYSROOT% isn't set, and from the look of the logs, it is not set:

https://taskcluster-artifacts.net/NIlDOfEvR5msuvQlCi03gw/0/public/logs/live_backing.log

Z:\task_1546277086\gopath1.10.3\src\github.com\taskcluster\generic-worker\testdata\TestTaskclusterProxy\task_1546277086>set
COMSPEC=C:\windows\system32\cmd.exe
GOPATH=Z:\task_1546277086\gopath1.10.3
GOROOT=Z:\task_1546277086\go1.10.3\go
PATH=Z:\task_1546277086\git\bin;Z:\task_1546277086\gopath1.10.3\bin;Z:\task_1546277086\go1.10.3\go\bin;C:\Program Files\Mercurial;C:\mozilla-build\7zip;C:\mozilla-build\info-zip;C:\mozilla-build\kdiff3;C:\mozilla-build\moztools-x64\bin;C:\mozilla-build\mozmake;C:\mozilla-build\msys\bin;C:\mozilla-build\msys\local\bin;C:\mozilla-build\nsis-3.0b3;C:\mozilla-build\nsis-2.46u;C:\mozilla-build\python;C:\mozilla-build\python\Scripts;C:\mozilla-build\python3;C:\mozilla-build\upx391w;C:\mozilla-build\wget;C:\mozilla-build\yasm;C:\windows\system32;C:\windows;C:\windows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0\;C:\mozilla-build\python27;C:\mozilla-build\python27\Scripts;C:\mozilla-build\vim\vim72;C:\CoreUtils\bin;C:\mozilla-build\buildbotve\scripts;c:\Program Files\Microsoft Windows Performance Toolkit\;C:\Users\cltbld\AppData\Local\Programs\Common\Microsoft\Visual C++ for Python\9.0;C:\ProgramData\chocolatey\bin;C:\Program Files\Puppet Labs\Puppet\bin;C:\mozilla-build\hg;C:\Program Files\GNU\GnuPG\pub;C:\Program Files\Mercurial\;
PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC
PROMPT=$P$G
TASKCLUSTER_PROXY_URL=http://localhost:34569
TASKCLUSTER_ROOT_URL=https://taskcluster.net
TASK_ID=NIlDOfEvR5msuvQlCi03gw
tcexitcode=0

taskcluster_proxy.go

build.sh

aws.go

main.go

gwconfig/config.go

docs/features.md

taskcluster_proxy.go

taskcluster_proxy_test.go

tcproxy/tcproxy_test.go

testdata/curlget.go

This has a few advantages: * permits adding the queue:create-artifact:.. scope * avoides an unnecessary additional call to queue.task(..) by the proxy * duplicates docker-worker's approach to minimize chance of behavior change

djmitche · 2019-01-02T20:29:27Z

So, I think the options are:

Always have an accessToken (this is beyond my capabilities)
Change the order of feature initialization and put the value in Payload.Env
Special-case this feature and add a set command to the batch script

@petemoore which do you prefer?

djmitche · 2019-01-02T21:39:56Z

However, after this PR, env is []string{"TASKCLUSTER_PROXY_URL=.."}, and that is put into cmd.Env unchanged, so all of the other environment variables disappear.

This was slightly off-base: env is not []string{"TASKCLUSTER_PROXY_URL=.."}, it is still nil. But the taskcluster_proxy feature later appends "TASKCLUSTER_PROXY_URL=.." to cmd.Env, changing that field from nil to []string{"TASKCLUSTER_PROXY_URL=.."}.

I added some debugging to catch the case where the process environment is required and accessToken is nil, but that doesn't catch this particular error.

So, I think editing cmd.Env in a feature's setup function is not a good idea. The options, then, are

Change the order of feature initialization and put the value in Payload.Env
Special-case this feature and add a set command to the batch script

I'm leaning toward the second one.

djmitche · 2019-01-02T23:56:08Z

It's green!

process/process_windows.go

plat_all-unix-style.go

djmitche · 2019-01-03T18:57:12Z

OK, that's all green except for coveralls. I don't know why it thinks coveraged dropped 52.9% - that's obviously wrong!

petemoore

Last change request, and then we're done!

You can ignore the nit - the other change request is some straggling code that I think you accidentally left in when you added and then removed some debugging statements...

win32/win32_windows.go

petemoore · 2019-01-03T19:05:42Z

plat_windows.go


+// Set an environment variable in each command.  This can be called from a feature's
+// NewTaskFeature method to set variables for the task.
+func (task *TaskRun) setVariable(variable string, value string) error {


This is just a nit, not a required change.

Nit: I would have probably called this setEnvironmentVariable for clarity, and made it a function of the environment rather than the task, but it doesn't really matter. This avoids the duplication of the loop over the task commands in both platform implementations. If you provide a method that just updates an environment block in a platform-independent way, you can have a single loop over the task commands, that is platform independent. It doesn't really matter though.

Note, originally the windows and all-unix-style process implementations were quite different, but with go 1.10 it became possible to use the standard library for running processes as a different user under windows, at which point it became possible to have very similar implementations for both. I intend to clean up some of the duplication in a future PR, and at that point will probably introduce a separate go type for an environment block (something like type Environment []string) that will have its own func (env *Environment) Set(envVar string, value string), func (env *Environment) Clear(envVar string) functions for modifying it. So it makes sense to rework this at the same time, so we can leave it as is for now.

I'll leave that to you :)

* factor out and test non-platform-specific MergeEnvLists * handle the task-user and current-user modes differently

With this change, the proxy knows the rootUrl, and the task has a reliable indication of the proxy URL There is no provision for a feature to set a task environment variable, and in fact features are not even initialized yet when the task commands are created, so this special-cases the TaskclusterProxyFeature for inclusion in the task environment.

petemoore

Many thanks Dustin!

Dustin determined that the code coverage drop is simply due to the PR coming from a forked repo, and that we disabled access to production taskcluster for integration tests from forked repos. When this lands on master, we can check the code coverage report there.

…rootURL

…erty names in bulk property activities

djmitche · 2019-01-04T14:25:52Z

Coverage looks good on master. Note that Travis doesn't support using env vars on PRs!

djmitche self-assigned this Dec 28, 2018

djmitche requested review from petemoore and walac December 28, 2018 22:01

djmitche force-pushed the bug1516458 branch 2 times, most recently from 557de62 to 851aded Compare December 28, 2018 22:38

djmitche removed request for petemoore and walac December 29, 2018 04:21

djmitche force-pushed the bug1516458 branch from 851aded to fadc75e Compare December 29, 2018 04:31

djmitche commented Dec 31, 2018

View reviewed changes

taskcluster_proxy.go Outdated Show resolved Hide resolved

djmitche force-pushed the bug1516458 branch from fadc75e to 36bbbbf Compare December 31, 2018 14:16

djmitche force-pushed the bug1516458 branch from 36bbbbf to adba931 Compare December 31, 2018 14:41

djmitche force-pushed the bug1516458 branch 2 times, most recently from 0e61869 to edf05e1 Compare December 31, 2018 16:34

djmitche force-pushed the bug1516458 branch from edf05e1 to 34df186 Compare December 31, 2018 17:37

djmitche requested review from petemoore and walac December 31, 2018 17:37

petemoore suggested changes Jan 2, 2019

View reviewed changes

taskcluster_proxy.go Outdated Show resolved Hide resolved

build.sh Outdated Show resolved Hide resolved

aws.go Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

gwconfig/config.go Outdated Show resolved Hide resolved

docs/features.md Show resolved Hide resolved

petemoore suggested changes Jan 2, 2019

View reviewed changes

taskcluster_proxy.go Outdated Show resolved Hide resolved

taskcluster_proxy_test.go Outdated Show resolved Hide resolved

tcproxy/tcproxy_test.go Show resolved Hide resolved

testdata/curlget.go Outdated Show resolved Hide resolved

djmitche force-pushed the bug1516458 branch from 34df186 to 739c63f Compare January 2, 2019 16:31

Bug 1516458 - pass an explicit list of scopes to the proxy

0ff6a4c

This has a few advantages: * permits adding the queue:create-artifact:.. scope * avoides an unnecessary additional call to queue.task(..) by the proxy * duplicates docker-worker's approach to minimize chance of behavior change

djmitche force-pushed the bug1516458 branch from 141f88e to a22567c Compare January 2, 2019 20:27

djmitche force-pushed the bug1516458 branch from a22567c to cec6697 Compare January 2, 2019 20:59

djmitche force-pushed the bug1516458 branch 3 times, most recently from fbf6cb7 to 30ab2b1 Compare January 2, 2019 23:19

djmitche requested a review from petemoore January 2, 2019 23:56

petemoore suggested changes Jan 3, 2019

View reviewed changes

process/process_windows.go Outdated Show resolved Hide resolved

petemoore reviewed Jan 3, 2019

View reviewed changes

plat_all-unix-style.go Outdated Show resolved Hide resolved

Bug 1516458 - set TASKCLUSTER_ROOT_URL in tasks

8a28fab

djmitche force-pushed the bug1516458 branch 3 times, most recently from d07d20b to 07c912c Compare January 3, 2019 17:50

djmitche requested review from petemoore and removed request for walac January 3, 2019 18:56

petemoore suggested changes Jan 3, 2019

View reviewed changes

djmitche added 4 commits January 3, 2019 19:16

Bug 1516458 - improve handling of env vars

4b043b1

* factor out and test non-platform-specific MergeEnvLists * handle the task-user and current-user modes differently

Bug 1516458 - document new rootUrl and proxy behavior

c9b74f5

Bug 1516458 - supply TASKCLUSTER_ROOT_URL in CI

7f4f1b1

djmitche force-pushed the bug1516458 branch from 07c912c to 7f4f1b1 Compare January 3, 2019 19:16

djmitche requested a review from petemoore January 3, 2019 19:17

petemoore approved these changes Jan 4, 2019

View reviewed changes

petemoore added 2 commits January 4, 2019 08:15

Bug 1516458 - swap tabs for spaces in usage text and provide example …

38297d1

…rootURL

Bug 1516458 - code beautification - fix alphabetical ordering of prop…

b8cc67a

…erty names in bulk property activities

petemoore merged commit 01bde78 into taskcluster:master Jan 4, 2019

Conversation

djmitche commented Dec 28, 2018

Uh oh!

coveralls commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djmitche commented Dec 29, 2018

Uh oh!

djmitche commented Dec 29, 2018

Uh oh!

Uh oh!

djmitche commented Dec 31, 2018

Uh oh!

djmitche commented Dec 31, 2018

Uh oh!

djmitche commented Dec 31, 2018

Uh oh!

petemoore commented Jan 2, 2019

Uh oh!

petemoore commented Jan 2, 2019

Uh oh!

petemoore commented Jan 2, 2019

Uh oh!

djmitche commented Jan 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djmitche commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djmitche commented Jan 2, 2019

Uh oh!

djmitche commented Jan 2, 2019

Uh oh!

Uh oh!

Uh oh!

djmitche commented Jan 3, 2019

Uh oh!

petemoore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petemoore Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

djmitche Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

petemoore left a comment

Choose a reason for hiding this comment

Uh oh!

djmitche commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Dec 28, 2018 •

edited

Loading

djmitche commented Jan 2, 2019 •

edited

Loading