Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable GOPROXY in default preset #13648

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@cblecker
Copy link
Member

commented Jul 28, 2019

fixes #13645.

Create a preset that adds the GOPROXY=https://proxy.golang.org env var to all jobs on prow.k8s.io.

Summary of benefits:

  • proxy.golang.org is a Google-run service on Borg. Should be highly reliable.
  • As both prow and proxy.golang.org are on the Google network, it should be faster to download.
  • As the cached modules don't require a full version control checkout, the download size should be smaller (read: faster)
  • For any jobs that don't use go modules, this is a noop.

@k8s-ci-robot k8s-ci-robot requested review from alvaroaleman and matthyx Jul 28, 2019

@cblecker cblecker removed request for alvaroaleman and matthyx Jul 28, 2019

@cblecker cblecker changed the title [WIP] Enabled GOPROXY preset by default Enable GOPROXY preset by default Jul 29, 2019

@cblecker cblecker force-pushed the cblecker:default-goproxy branch from fa19394 to e65a949 Jul 29, 2019

@cblecker cblecker changed the title Enable GOPROXY preset by default Enable GOPROXY in default preset Jul 29, 2019

@cblecker

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

/hold
for discussion

/cc @BenTheElder @cjwagner @fejta @stevekuznetsov

@@ -635,3 +635,7 @@ presets:
hostPath:
path: /sys/fs/cgroup
type: Directory
# enable GOPROXY by default

This comment has been minimized.

Copy link
@cjwagner

cjwagner Jul 29, 2019

Member

I don't think we support a 'default' preset that applies to all jobs implicitly and I'm not sure that we'd want to since that would be pretty opaque to job authors.

func mergePreset(preset Preset, labels map[string]string, containers []v1.Container, volumes *[]v1.Volume) error {
for l, v := range preset.Labels {
if v2, ok := labels[l]; !ok || v2 != v {
return nil

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 29, 2019

Member

FWIW:

  • presets upstream are meant to be opaque for injecting environment (primarily credential) details
  • this setting only affects jobs that use go modules directly from within the container (not bazel or docker in docker)
  • in many settings this would be configured with a default on the machine (EG a corporate GOPROXY)

Setting this by default across the board makes sense, I did mention in previous discussion that the prow owners may not be OK with that though 🤷‍♂

This comment has been minimized.

Copy link
@cblecker

cblecker Jul 29, 2019

Author Member

I can confirm that the code as it is today will apply this "default" preset to every job (CI actually errors if this is set in the job spec because it merges and detects the duplicate).

+1 to everything Ben said. I can't think of any job on prow.k8s.io that wouldn't either benefit from this being set, or disregard it as a noop.

This comment has been minimized.

Copy link
@spiffxp

spiffxp Jul 29, 2019

Member

I am fine with this, but let's make sure it's documented:

This comment has been minimized.

Copy link
@cblecker

cblecker Jul 29, 2019

Author Member

Created #13665 for follow up

cblecker added some commits Jul 29, 2019

@cblecker cblecker force-pushed the cblecker:default-goproxy branch from e65a949 to 246b49c Jul 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@spiffxp
Copy link
Member

left a comment

this was new behavior to me, I like its use for good, but can we doc it?

@@ -635,3 +635,7 @@ presets:
hostPath:
path: /sys/fs/cgroup
type: Directory
# enable GOPROXY by default

This comment has been minimized.

Copy link
@spiffxp

spiffxp Jul 29, 2019

Member

I am fine with this, but let's make sure it's documented:

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

LGTM label has been added.

Git tree hash: 35eabee58731807806ba8fb89b2bcec46df40c71

subsequent lgtm

@cblecker

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit b84ad08 into kubernetes:master Jul 29, 2019

6 checks passed

cla/linuxfoundation cblecker authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Job succeeded.
Details
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-yamllint Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@cblecker: Updated the following 2 configmaps:

  • job-config configmap in namespace default using the following files:
    • key generated-security-jobs.yaml using file config/jobs/kubernetes-security/generated-security-jobs.yaml
    • key dependencies.yaml using file config/jobs/kubernetes/sig-testing/dependencies.yaml
  • config configmap in namespace default using the following files:
    • key config.yaml using file prow/config.yaml

In response to this:

fixes #13645.

Create a preset that adds the GOPROXY=https://proxy.golang.org env var to all jobs on prow.k8s.io.

Summary of benefits:

  • proxy.golang.org is a Google-run service on Borg. Should be highly reliable.
  • As both prow and proxy.golang.org are on the Google network, it should be faster to download.
  • As the cached modules don't require a full version control checkout, the download size should be smaller (read: faster)
  • For any jobs that don't use go modules, this is a noop.

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 k8s-ci-robot added this to the v1.16 milestone Jul 29, 2019

@cblecker cblecker deleted the cblecker:default-goproxy branch Jul 29, 2019

cblecker added a commit to cblecker/test-infra that referenced this pull request Jul 30, 2019

Revert "Merge pull request kubernetes#13648 from cblecker/default-gop…
…roxy"

This reverts commit b84ad08, reversing
changes made to 57a4dc6.

k8s-ci-robot added a commit that referenced this pull request Jul 30, 2019

Merge pull request #13674 from cblecker/revert-13648
Revert "Merge pull request #13648 from cblecker/default-goproxy"
@spiffxp

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

This broke pull-kubernetes-dependencies and was reverted via #13674

We can probably try this again once kubernetes/kubernetes#80732 merges

@cblecker

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

PR to re-enable: #13686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.