Skip to content

Conversation

@petemoore
Copy link
Member

Bugzilla Bug: 1608828

This initial PR integrates the generic-worker repository wholesale into the monorepo under /workers/generic-worker. The build scripts, CI tasks, package imports are updated as necessary, but the project is not restructured in any way. Future PRs can restructure or reorganise code as appropriate.

This is initially a draft pull request until I have everything working.

Git history from the generic-worker repository has been preserved.

Miles Crabill and others added 30 commits July 17, 2019 17:28
* replace docker library code with shells out to docker cli using exec.Command

* commit dep changes

* dereference exitError

* disable checking ExitCode() which requires go1.12

* use ExitStatus and syscall to get exit code pre ExitError() which was added in go1.12

* Ran dep ensure

* remove docker constraint from Gopkg.toml

* add command logging to docker Execute()

* pass GOPATH and current go binary when calling sudo in build.sh

* lookup docker in path
Bug 1567073 - log all commands that are executed on the host
Bug 1559777 - support deploying generic-worker linux workers to EC2
Bug 1568782 - error artifacts do not have a content type
Bug 1568471 - close HTTP response bodies when finished with them
Bug 1567632 - internally cache interactive username (lazily) on macOS
win2016 worker_type: s/aamazon/amazon
@petemoore
Copy link
Member Author

petemoore commented Feb 6, 2020

Hi @djmitche, I've reduced this to 16 commits on top of the generic worker repository. I also rewrote the generic-worker history to remove the /vendor subfolder, which was very large and produced a lot of noise. With go modules, we don't have the /vendor folder, so I preferred to remove it rather than import it all, only for it to get deleted later (and it led to quite spurious contribution statistics).

It is probably easier to browse through the commits in sequence than view the complete diff, since that includes the entire generic-worker codebase.

Note, I haven't tackled updating the monorepo release process to publish generic-worker binaries, or integrated the build with yarn yet, but I figured these things can be done incrementally once this PR lands. This gets generic-worker building in the CI, although due to bug 1613498 we should keep a close eye on CI results before merging PRs, since the PR interface may make it appear that all CI tasks have passed when there are worker failures.

So despite not having all the i's dotted and t's crossed yet, I think this should be in good enough shape for a first pass.

Note, in the end I closed taskcluster/community-tc-config#195 in favour of an alternative approach.

@petemoore petemoore marked this pull request as ready for review February 6, 2020 13:51
@petemoore petemoore requested a review from a team as a code owner February 6, 2020 13:51
@petemoore petemoore requested a review from djmitche February 6, 2020 13:51
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

This looks good, and the per-line comments below are minor and/or easy to handle in a followup.

Another good followup will be to get version = ".." in main.go substituted appropriately in yarn release. That should be a pretty easy modification to infrastructure/tooling/src/release/tasks.js.

I did get some issues running go test locally.

^C(go1.13.7) (v12.14.1) dustin@lamport ~/p/taskcluster2 [add-generic-worker] $ go test -v ./clients/client-go/integrationtest/                                                                                                                                                             
=== RUN   TestGarbageNamespaces                                                                                                                                                                                                                                                            
2020/02/07 15:19:09 Error: Get /api/index/v1/namespaces/garbage: unsupported protocol scheme ""

(and in general this seems to hang)

# github.com/taskcluster/taskcluster/v24/workers/generic-worker/gwconfig   
workers/generic-worker/gwconfig/config.go:30:3: undefined: PublicEngineConfig
=== RUN   TestLiveLog
--- FAIL: TestLiveLog (0.00s)
    livelog_test.go:35: Could not initiate livelog process:
        WARNING: Livelog terminated early with error 'exec: "livelog": executable file not found in $PATH' and output:
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x69a261]
=== RUN   TestTcProxy  
--- FAIL: TestTcProxy (0.00s)  
    testutil.go:17: TASKCLUSTER_{CLIENT_ID,ACCESS_TOKEN,ROOT_URL env vars not set, but GW_SKIP_INTEGRATION_TESTS not set
FAIL                                                                                  

It'd be nice to remove the following, too (it didn't run because of the build failure above)

workers/generic-worker/main_test.go:            t.Fatalf("Git revision could not be determined - got '%v' but expected to match regular expression '^[0-9a-f](40)$'\n"+

as that parameter should be enforced by the build/release mechanics and isn't required for testing.

The approach we've taken to ease the process for contributors is to make tests (yarn test or go test ./...) "just work" out of the box, even if that means skipping tests that need credentials. Then we set $NO_TEST_SKIP in CI to turn those skips into failures. That way a contributor can make changes and run the tests locally pretty easily, but we still get to see test failures in the PR if they have broken some test that was otherwise skipped -- and since most tests are unit tests with suitable fakes, that situation is fairly unusual. For example, the websocktunnel tests "just work" out of the box and would catch bugs in modifications to the expose package.

So I think the major thing I'd like to see before landing here is to make the above skip when necessary bits (credentials, livelog) aren't available, and to update the build config so that a "straight" go test ./... succeeds -- either with a null engine, or defaulting to the multiuser engine?

.taskcluster.yml Outdated
taskclusterProxy: true
mounts:
- content:
url: https://storage.googleapis.com/golang/go1.13.7.linux-amd64.tar.gz
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be good if this substituted ${go}, perhaps listing the go sha256 as a variable in the same $let at the top of the file. Then there's just one place to change the go version.

README.md Outdated
* [ui/src/components/Snackbar](ui/src/components/Snackbar#readme)
* [ui/src/components/SpeedDial](ui/src/components/SpeedDial#readme)
* [ui/src/components/StatusLabel](ui/src/components/StatusLabel#readme)
* [generic-worker](workers/generic-worker#readme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name here comes from the #-level heading at the top of the file, if you'd like to change it (e.g., to capitalize it)


// imports inscrutably uses the old tc-client path, so fix that up. Note
// that this must be the full path to allow `yarn release` to update it
// correctly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a bug in goimports, but does indeed seem to be fixed now. Maybe it just found the other library in some GOPATH somewhere and that GOPATH has since been deleted? 🤷‍♀️

PublicConfig struct {
PublicEngineConfig
AuthBaseURL string `json:"authBaseURL"`
AuthRootURL string `json:"authRootURL"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will require a README update as well, and it would be good to mention that the *RootURL configs are only for use in testing and development.

…cf427c431/src/cmd/compile/internal/gc/lex.go#L80-L90 - fixes intermittent DLL call wrapper failures due to allocated objects being moved by garbage collector before call completes
@petemoore petemoore merged commit 5e81653 into master Feb 11, 2020
@imbstack imbstack deleted the add-generic-worker branch April 8, 2020 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants