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

feat: Docker registry mirror #478

Merged
merged 2 commits into from
Apr 25, 2021
Merged

Conversation

rofafor
Copy link
Contributor

@rofafor rofafor commented Apr 23, 2021

  • Switched to use jq in startup.sh
  • Should enable docker registry mirror configuration if dockerdWithinRunnerContainer has been set, that's needed due to the current Docker Hub rate-limiting

DinD: Mirror only

apiVersion: actions.summerwind.dev/v1alpha1
kind: RunnerDeployment
spec:
  template:
    spec:
      dockerdWithinRunnerContainer: true
      dockerMTU: 1500
      dockerRegistryMirror: https://mirror.xxx.xxx
[Sat Apr 24 12:40:03 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Using /etc/docker/daemon.json with the following content
{
  "registry-mirrors": [
    "https://mirror.xxx.xxx"
  ]
}
[Sat Apr 24 12:40:03 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Using /etc/supervisor/conf.d/dockerd.conf with the following content
[program:dockerd]
command=/usr/local/bin/dockerd
autostart=true
autorestart=true
stderr_logfile=/var/log/dockerd.err.log
stdout_logfile=/var/log/dockerd.out.log
[Sat Apr 24 12:40:03 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Starting supervisor

DinD: Mirror + MTU

apiVersion: actions.summerwind.dev/v1alpha1
kind: RunnerDeployment
spec:
  template:
    spec:
      dockerdWithinRunnerContainer: true
      dockerMTU: 1500
      dockerRegistryMirror: https://mirror.xxx.xxx
[Sat Apr 24 12:46:40 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Using /etc/docker/daemon.json with the following content
{
  "mtu": 1500,
  "registry-mirrors": [
    "https://mirror.xxx.xxx"
  ]
}
[Sat Apr 24 12:46:40 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Using /etc/supervisor/conf.d/dockerd.conf with the following content
[program:dockerd]
command=/usr/local/bin/dockerd
autostart=true
autorestart=true
stderr_logfile=/var/log/dockerd.err.log
stdout_logfile=/var/log/dockerd.out.log
environment=DOCKERD_ROOTLESS_ROOTLESSKIT_MTU=1500
[Sat Apr 24 12:46:40 UTC 2021] [INFO] [/usr/local/bin/startup.sh] Starting supervisor

Mirror + MTU

      dockerdWithinRunnerContainer: false
      dockerMTU: 1500
      dockerRegistryMirror: https://mirror.xxx.xxx
$ kubectl exec -it ghe-runner-deployment-r8hhs-lx2ck -c docker -- cat /proc/1/cmdline
dockerd--host=unix:///var/run/docker.sock--host=tcp://0.0.0.0:2376--tlsverify--tlscacert/certs/server/ca.pem--tlscert/certs/server/cert.pem--tlskey/certs/server/key.pem--mtu1500--registry-mirror=https://mirror.xxx.xxx%

@rofafor
Copy link
Contributor Author

rofafor commented Apr 24, 2021

Seems to be working for me. I spotted the Docker pulls on the mirror's log as well.

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

@rofafor Well done! Thank you so much for the enhancement, and sharing the detailed test report. LGTM 👍

Note:

I was in an impression that making our entrypoint.sh dependent on the existence of jq should be done carefully even though our runner image has it preinstalled. That might break things for users who uses their own runner image that omit jq. But as it isn't very hard to remove jq dependency from entrypoint.sh even after this change, I think it's good to go for now.

@mumoshu mumoshu merged commit 6b77a2a into actions:master Apr 25, 2021
@@ -587,6 +587,8 @@ spec:
# false (default) = Docker support is provided by a sidecar container deployed in the runner pod.
# true = No docker sidecar container is deployed in the runner pod but docker can be used within teh runner container instead. The image summerwind/actions-runner-dind is used by default.
dockerdWithinRunnerContainer: true
# Optional Docker registry mirror, only applicable if dockerdWithinRunnerContainer = true
dockerRegistryMirror: https://mirror.gcr.io/
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rofafor Worth adding mention to https://cloud.google.com/container-registry/docs/pulling-cached-images for more information? I'd appreciate it if you could add it here or to our docs in another PR

@liamgib
Copy link
Contributor

liamgib commented Apr 27, 2021

Thanks for implementing this.

I believe this requires a helm chart bump though?
Looks like the chart hasn't published since the appVersion is the same.

@rofafor

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 28, 2021

@liamgib How are you willing to use the chart, btw? There're changes only in CRDs. If you've already installed the chart before, the only way to upgrade CRDs is to use kubectl, as helm upgrade is unable to upgrade existing CRDs at all.

@liamgib
Copy link
Contributor

liamgib commented Apr 28, 2021

@mumoshu

Using helm chart would be good, but I tried using kubectl and still not working.

Cleanup

helm uninstall actions-runner-controller
kubectl delete crd horizontalrunnerautoscalers.actions.summerwind.dev
kubectl delete crd runnerdeployments.actions.summerwind.dev
kubectl delete crd runnerreplicasets.actions.summerwind.dev
kubectl delete crd runners.actions.summerwind.dev

Reinstall

kubectl apply -f https://github.com/summerwind/actions-runner-controller/releases/download/v0.18.2/actions-runner-controller.yaml

I also note that there is noReference to dockerRegistryMirror in the v.0.18.2 controller.
https://github.com/summerwind/actions-runner-controller/releases/download/v0.18.2/actions-runner-controller.yaml

Was this released yet?

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 28, 2021

@liamgib Nope. You need to build your own actions-runner-controller image! This might help.

@liamgib
Copy link
Contributor

liamgib commented Apr 28, 2021

No worries, thank you @mumoshu

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.

None yet

3 participants