Test-infra: Podutils: $GOPATH/bin is not part of $PATH

[krzyzacy]

I can always set an env in my prowjob definition, not sure if this should be handled by podutils, since in a few jobs people build some go program and runs it.

cc @cjwagner @BenTheElder @stevekuznetsov

[cjwagner]

We set GOPATH from the pod utilities so it might make sense to add the bin to PATH as well?
That being said @sebastienvas noted that maybe the pod utilities shouldn't be setting GOPATH.

I think it is important to provide an easy way to configure a job for use with golang because of how common that case is, but we should also make sure that the pod utilities remain generic and don't become golang specific. Not sure where the line should be drawn.

[sebastienvas]

My points was that the container itself or the job should be setting env. The use case would be to use golang:1.10 out of the box which does not work today.

[BenTheElder]

the pod-uitilities also do not document that GOPATH is set afaict, you have to re-read the implementation to glean that detail..

[cjwagner]

There are a lot of pod utility configuration options and features that are still undocumented. I added https://github.com/kubernetes/test-infra/blob/master/prow/pod-utilities.md a while back after trying to use the pod utilities myself, but it isn't a complete.
It should document what environment variable are set and more of the configuration options like the gcs upload configuration and the various path strategies that are available.

[fejta]

I think the job author should configure the environment this way if this behavior is desired. It should not happen automatically as this is business logic for specific use cases, like jobs that use go rather than python, bash, etc

[krzyzacy]

@fejta currently it configures $GOPATH, but not $GOPATH/bin. I think ideally we want to have something like --mode = {go, python, bash, ...}, and if --mode=go, it can configure all go stuff for entrypoint.

[fejta]

Prow should not try and provide a runtime environment.

The runtime environment is the image you hand your container. This simplicity and focus is part of prows power.

It is questionable whether it should set gopath (and long term we need to remove entrypoint as this corrupts the user provided runtime environment). Instead it should not in a known location on a well known volume what it checked out and where.

[fejta]

*note in a known location

[krzyzacy]

so user should supply $GOPATH on prowjob env, and alias cloneref to $GOPATH?

[fejta]

Cloneref is an init container, it's environment is hidden from and irrelevant to the user provided test container.

If the user wants a gopath, yes they should configure one. Given that nearly 100% of our jobs use the same image, it shouldn't be honorous.

We could also work on providing images for various runtimes which do useful things, similar to https://github.com/GoogleCloudPlatform/cloud-builders

[cjwagner]

If the user wants a gopath, yes they should configure one. Given that nearly 100% of our jobs use the same image, it shouldn't be honorous.

If jobs using the pod utilities need to use a specific image doesn't that kind of defeat the point of the pod utilities (we could have just rolled everything into the image like bootstrap)?
While nearly all of our jobs use the same image, other users of Prow frequently need to supply their own image. Will those job authors be able to use our image as a base image even if they require other base images or are docker containers limited to a single base image?

[stevekuznetsov]

What's the use-case for supporting a specific user-specified $GOPATH? @sebastienvas is not correct, you _can_ use the default Go images for running whatever you want. We set up our own $GOPATH and the go binary is present, so everything works.

[stevekuznetsov]

When someone is providing a container image for use as the test environment, they should be providing a set of compile-time/run-time dependencies for their tests. As they should be expecting pod-utils to provide _all_ of the source code into the container, it does not make sense for them to also want to provide a path for it. They are given the code, somewhere, and the working directory is appropriate. Not sure why the absolute path should matter.

[sebastienvas]

@stevekuznetsov I tried using a golang image and it does not work, and the reason is that the golang image does not have an entrypoint. Also the golang images sets GOPATH, which is overwritten by podutils. DO you have an example of it working instead of stating that I am wrong ?

[BenTheElder]

is that the golang image does not have an entrypoint.

You should set command when using the podutils, podutils hijacks the entrypoint anyhow...?

Also the golang images sets GOPATH, which is overwritten by podutils

the golang image shouldn't be shipping with anything in $GOPATH anyhow

Having $GOPATH/bin in the path is handy for actually using GOPATH, but it can be set by the job script.

Some of our tooling leverages tempdirs or a local bin dir for this which is probably better for test scripts than installing to the host $PATH anyhow. Happy to discuss if you need a hand. (note: not actually using the golang image currently for other reasons)

[sebastienvas]

I did set a command; I used a script from the checkout and it failed (I don't really remember the reason, maybe I should have inlined exec in the command). Please try it and make it work and show me a working example. I actually spent 2 days trying to get a container work with podutils (my container run as a user), basically creating a new image, so I think the goal of being able to re-use existing images is just not there.

[stevekuznetsov]

Here's a job we have:

  - agent: kubernetes
    always_run: true
    context: ci/prow/unit
    decorate: true
    name: pull-ci-ns-ttl-controller-unit
    rerun_command: /test unit
    spec:
      containers:
      - command:
        - go
        args:
        - test
        - ./...
        image: golang:1.10
        name: test

[krzyzacy]

@sebastienvas https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/test-infra/janitors.yaml#L33-L58 is one of our working job, can you link to the job you have issues for?

side question, shouldn't the job above be

      containers:
      - command:
        - go
        args:
        - test
        - ./...

[stevekuznetsov]

Yeah, oops -- we didn't have a PR for that repo so I created a postsubmit ProwJob, but the result is identical:

apiVersion: prow.k8s.io/v1
kind: ProwJob
metadata:
  annotations:
    prow.k8s.io/job: pull-ci-ns-ttl-controller-unit
  creationTimestamp: null
  labels:
    prow.k8s.io/job: pull-ci-ns-ttl-controller-unit
    prow.k8s.io/refs.org: openshift
    prow.k8s.io/refs.pull: "0"
    prow.k8s.io/refs.repo: ci-ns-ttl-controller
    prow.k8s.io/type: presubmit
  name: 3f5949ce-bcf9-11e8-81a4-54ee753e2644
spec:
  agent: kubernetes
  cluster: default
  context: ci/prow/unit
  decoration_config:
    gcs_configuration:
      bucket: origin-ci-test
      default_org: openshift
      default_repo: origin
      path_strategy: single
    gcs_credentials_secret: gcs-publisher-credentials
    grace_period: 15000000000
    timeout: 14400000000000
    utility_images:
      clonerefs: clonerefs:latest
      entrypoint: entrypoint:latest
      initupload: initupload:latest
      sidecar: sidecar:latest
  job: pull-ci-ns-ttl-controller-unit
  pod_spec:
    containers:
    - command:
      - go
      args:
      - test
      - ./...
      image: golang:1.10
      name: test
      resources: {}
  refs:
    base_ref: master
    base_sha: f4373aa13466c8d3750e4c166861fbce92b93eae
    org: openshift
    repo: ci-ns-ttl-controller
  report: true
  rerun_command: /test unit
  type: postsubmit
status:
  prev_report_state: ""
  startTime: 2018-09-20T17:18:53Z
  state: triggered

Output:

$ oc logs -f -c test 3f5949ce-bcf9-11e8-81a4-54ee753e2644
go: disabling cache (/.cache/go-build) due to initialization failure: mkdir /.cache: permission denied
?       github.com/openshift/ci-ns-ttl-controller/cmd/ci-ns-ttl-controller  [no test files]
ok      github.com/openshift/ci-ns-ttl-controller/pkg/controller    0.018s

[stevekuznetsov]

@sebastienvas you should feel empowered to reach out to me when you have issues like this -- I will admit 1000% we need better doc. Until then, just ask! If I'm not around, Cole should be able to help. There's no need for you to waste two days!

[stevekuznetsov]

Regarding the original report, I agree @BenTheElder @krzyzacy we should add $GOPATH/bin to $PATH

[sebastienvas]

Thanks. I ll try to reproduce it. In my case I was not calling go directly but instead calling a scripts that calls go from the checkout path. (ie installing gometalinters and run them)

[stevekuznetsov]

Sure -- share your config, output and how you're iterating on a Slack message or issue on this repo and I'll try to help

[BenTheElder]

@sebastienvas this is probablydue to running as a different user id which the golang image is not exactly supporting in this case, adding $GOPATH/bin to path would likely fix this.

[BenTheElder]

This is a common problem unrelated to the podutils, if you change your uid then lots of things are suddenly not writeable. I'm not sure which route around this we want to take, are you free to discuss at some point? (send me a note, we can drop details on the solution here after) We're probably going to have this problem ourselves more in the future.

[stevekuznetsov]

OpenShift, for security, randomizes the UID that any container runs with but ensures that GID=0, so if UNIX permissions need to be set correctly, we can work around it by setting group ownership. Since the areas of the filesystem that are being touched by the tests in pod-utils are mounted in, it should be fine, unless you're trying to do system-level things in an unprivileged container without access to root user?

[BenTheElder]

it sounds like this is touching things in the container filesystem, not the pod-utils mounts.

[BenTheElder]

This will happen for eg the go stdlib. It's easy to wind up touching the container FS, though it _is_ avoidable and tests should avoid it.

[stevekuznetsov]

Hmm yeah, touching things in the container FS should not be happening outside of container image builds -- no DEB or RPM installs, no system configuration, etc. How does the Go stdlib touch the container FS?

[BenTheElder]

How does the Go stdlib touch the container FS?

If you try to build in a mode the stdlib wasn't compiled for, go will try to rebuild it somewhere in GOROOT IIRC which is likely in the image/container FS. That sort of thing is somewhat annoying to avoid.

[fejta]

Long term (medium term) we need to move entrypoint out of the user container and into sidecar

[stevekuznetsov]

Wow, that's pretty irritating @BenTheElder

@fejta agreed, we should hammer out a design doc for it

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale

[krzyzacy]

^^ any long term plans? Currently we worked around by using our runner.sh entrypoint which is
/pony this is fine

[k8s-ci-robot]

@krzyzacy:

In response to this:

^^ any long term plans? Currently we worked around by using our runner.sh entrypoint which is
/pony this is fine

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[stevekuznetsov]

@krzyzacy https://github.com/kubernetes/test-infra/issues/9469#issuecomment-423267409

Letting people change things in the container FS randomly is not going to happen ... no way to make that possible.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[krzyzacy]

/this-is-fine
/close

[k8s-ci-robot]

@krzyzacy:

In response to this:

/this-is-fine
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@krzyzacy: Closing this issue.

In response to this:

/this-is-fine
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[spiffxp]

/reopen
Bumped into this again. This isn't really intuitive or documented behavior. The comment adjacent to the GOPATH setting mentions maybe not doing this when we can assume go modules.

[k8s-ci-robot]

@spiffxp: Reopened this issue.

In response to this:

/reopen
Bumped into this again. This isn't really intuitive or documented behavior. The comment adjacent to the GOPATH setting mentions maybe not doing this when we can assume go modules.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[spiffxp]

I'm not super clear why it's necessary for decoration to set GOPATH to /home/prow/go in the first place. I notice there a lot of jobs that set it back to /go via:

env:
- name: GOPATH
  value: /go

[krzyzacy]

@spiffxp fwiw most jobs here use kubetest, which has https://github.com/kubernetes/test-infra/blob/master/images/kubekins-e2e/Dockerfile#L23-L28 already

[BenTheElder]

/home/prow/go makes sense as a default $GOPATH if we're going to set one because go env GOPATH without one set is going to be $HOME/go and our $USER is usually prow.

It plays well with various tooling that expects a $HOME if nothing else. Otherwise $HOME would have to be / which is _fun_.

GOPATH is mostly going away with go modules, but go geting a binary outside of a module still puts binaries in $(go env GOPATH)/bin/.

setting it to /go seems to be a legacy thing in some SIG-Node jobs, probably to match old bootstrap configs that set /go/src as the root of cloning.

[BenTheElder]

It is not obvious that we should manipulate PATH in the pod-utilities though, since we'd need to append it to whatever PATH the container image already has configured this would probably have to be done in the entrypoint-hijack binary, and that is supposed to be invisible...

[spiffxp]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[CarlJi]

Prow is a great CI system and now we have used it for some private repos. to match the real business scenario, we want to set the specific GOPATH environment variable in the presubmit job like this:

  - agent: kubernetes
    always_run: true
    context: pull-unit-test
    decorate: true
    name: pull-unit-test
    rerun_command: /test pull-unit-test
    spec:
      containers:
      - command:
        - go
        args:
        - test
        - ./...
        env:
        - name: GOPATH
          value: "/home/prow/go/xxx" # any user-defined value
        image: xxxx
        name: test

but currently plank will always overwrite it(https://github.com/kubernetes/test-infra/blob/master/prow/pod-utils/decorate/podspec.go#L552), which seems not be elegant since the original Kubernetes POD template do support such usage(https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#define-an-environment-variable-for-a-container). So we suggest to change the logic here(https://github.com/kubernetes/test-infra/blob/master/prow/pod-utils/decorate/podspec.go#L628) as to prefer user-defined environment variable than the default one of podutils if user already defined it in the job config.

does it sound good? if yes, i can submit a Pull Request to adjust it.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.