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

Fix runner pod dnsConfig #1227

Merged
merged 7 commits into from
Apr 20, 2022
Merged

Fix runner pod dnsConfig #1227

merged 7 commits into from
Apr 20, 2022

Conversation

billimek
Copy link
Contributor

@billimek billimek commented Mar 15, 2022

... this may be missing based on details from this issue

Fixes #1226
Fixes #1224

... this may be missing based on details from [this issue](actions#1226)

Signed-off-by: Jeff Billimek <jeff@billimek.com>
Signed-off-by: GitHub <noreply@github.com>
@billimek billimek marked this pull request as ready for review March 15, 2022 20:08
Fixes actions#1224

Signed-off-by: GitHub <noreply@github.com>
@billimek
Copy link
Contributor Author

I've been unable to build this as a docker container against the Makefile/Dockerfile in the master branch due to strange errors that occur on MacOS, a linux VM, and even github codespaces all with the same error about a read-only filesystem:

RUN --mount=target=.   --mount=type=cache,mode=0777,target=${GOCACHE}   export GOOS=${TARGETOS} GOARCH=${TARGETARCH} GOARM=${TARGETVARIANT#v} &&   go build -o /out/manager main.go &&   go build -o /out/github-webhook-server ./cmd/githubwebhookserver:                                           
#13 0.428 container_linux.go:380: starting container process caused: process_linux.go:545: container init caused: rootfs_linux.go:75: mounting "/var/lib/docker/overlay2/ygto3kzjlp7rm7fq3guf88gdb/merged/cache" to rootfs at "/workspace/${GOCACHE}" caused: mkdir /var/lib/docker/buildkit/executor/cncbc4ch5a1h2ysgmn7zb8i3v/rootfs/workspace/${GOCACHE}: read-only file system

image

However, I was able to apply the changes and build against the Makefile/Dockerfile based off of the code in the v0.21.1 tag. Using this, and the associated built and pushed actions-runner-controller docker image (billimek/actions-runner-controller:dnsConfig-b25a0fd6) & changed CRDs, I was able to successfully run and apply the dnsConfig in my actions-runner-controller environment

@splitice
Copy link

splitice commented Mar 24, 2022

I've also got a similar issue trying to build (linux docker)

Signed-off-by: GitHub <noreply@github.com>
* master: (30 commits)
  chore(deps): update azure/setup-helm action to v2.1 (actions#1328) [skip ci]
  Add more usages of RUNNER_VERSION to Renovate config. (actions#1313)
  chore: fix typo (actions#1316) [skip ci]
  chore: bump chart to latest (actions#1319)
  Fix release workflow
  Prevent runners from stuck in Terminating when pod disappeared without standard termination process (actions#1318)
  ci: pin go version to the known working version (actions#1303)
  chore: bump chart to latest (actions#1300)
  Fix runner pod to be cleaned up earlier regardless of the sync period (actions#1299)
  Make the hard-coded runner startup timeout to avoid race on token expiration longer (actions#1296)
  docs: highlight why persistent are not ideal
  fix(deps): update module sigs.k8s.io/controller-runtime to v0.11.2
  chore(deps): update dependency actions/runner to v2.289.2
  chore(deps): update helm/chart-releaser-action action to v1.4.0 (actions#1287)
  refactor(runner/entrypoint): check for externalstmp (actions#1277)
  docs: redundant words
  docs: wording
  docs: add autoscaling also causes problems
  chore: new line for consistency
  docs: use the right font
  ...
@billimek
Copy link
Contributor Author

I think I sorted the local build issues by using the make docker-buildx (vs make docker-build).

We've been running the backported fix now in our org-wide runners without issue and would love to see what next steps are necessary to progress this PR?

@toast-gear
Copy link
Collaborator

toast-gear commented Apr 13, 2022

We need to do some testing and can merged. v0.23.x was on hold until we resolved any regressions and bugs born from the v0.22.x codebase. v0.22.0 was a major architectural change so we froze non-essential merges until the codebase was solid. We're going to start working towards getting everything merged for v0.23.0.

@billimek can you raise an issue for the controller build on a Mac including details and when it broke, perhaps go back through the history of the controller Dockerfile and see when it stopped building?

@toast-gear
Copy link
Collaborator

toast-gear commented Apr 19, 2022

Have you actually tried installing this? Haven't digged into it yet but I get the below error when trying to install your branch's chart:

Error: failed to install CRD crds/actions.summerwind.dev_runnerreplicasets.yaml: CustomResourceDefinition in 
version "v1" cannot be handled as a CustomResourceDefinition: v1.CustomResourceDefinition.Spec: v1.CustomResourceDefinitionSpec.Versions: []v1.CustomResourceDefinitionVersion: 
v1.CustomResourceDefinitionVersion.Schema: v1.CustomResourceValidation.OpenAPIV3Schema: v1.JSONSchemaProps.Properties: v1.JSONSchemaProps.Properties: v1.JSONSchemaProps.Properties: 
v1.JSONSchemaProps.Properties: v1.JSONSchemaProps.Properties: readObjectStart: expect { or n, but 
found ", error found in #10 byte of ...|},"type":"object"},"|..., bigger context ...|items":
{"type":"string"},"type":"array"}},"type":"object"},"type":"array"},"dockerEnabled":{"type":"|...

EDIT regenerating the manifests fixes it, @billimek can you regenerate them make manifest, commit them, test it works and comment back when done?

@billimek
Copy link
Contributor Author

I think the latest merge from master requires an additional generation of the replica set CRD. I know what to do. Will push a working update today.

FWIW we are running these changes "in prod" everywhere now without issue. Can't wait to eventually go back to the official image & code!

* master:
  chore(deps): update dependency actions/runner to v2.290.1
  chore(deps): update actions/stale action to v5 (actions#1338)
  refactor: use apt-get instead of apt (actions#1342)
  Removed `modprobe` Script (actions#1247) [skip ci]
  Fix scale down condition to exclude skipped (actions#1330)
  chore: migrate to actions stale bot (actions#1334)
  refactor: location of some runner cmds (actions#1337)
  Improved Bash Logger (actions#1246)
  chore(deps): update dependency actions/runner to v2.290.0
Signed-off-by: Jeff Billimek <jeff@billimek.com>
@billimek
Copy link
Contributor Author

@toast-gear this is done - FYI the make manifest operation did create some changes in the config/crd/bases which I assume is also expected.

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.

LGTM. Thanks a lot for your contribution @billimek ☺️

@mumoshu mumoshu changed the title add podSpec for DnsConfig Fix type for Pod DNSConfig Apr 20, 2022
@mumoshu mumoshu changed the title Fix type for Pod DNSConfig Fix type for runner pod dnsConfig Apr 20, 2022
@mumoshu mumoshu changed the title Fix type for runner pod dnsConfig Fix runner pod dnsConfig Apr 20, 2022
@mumoshu mumoshu merged commit 13bfa2d into actions:master Apr 20, 2022
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.

dnsConfig doesn't seem to translate to created pods for the Runners Type of dnsConfig added in #764 wrong
4 participants