Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Feature/update concourse #1489

Closed
wants to merge 7 commits into from
Closed

Conversation

edude03
Copy link

@edude03 edude03 commented Jul 14, 2017

This is the configuration I'm using in production at the moment. This PR should supersede PR #1365 as it updates to a newer version and adds a few fixes.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 14, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @edude03. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 14, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 14, 2017
@@ -49,6 +49,8 @@ spec:
- "land-worker"
- "--name=${HOSTNAME}"
env:
- name: CONCOURSE_BAGGAGECLAIM_DRIVER
value: "btrfs"
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should be configurable.

@unguiculus
Copy link
Member

/cc @frodenas

@k8s-ci-robot
Copy link
Contributor

@unguiculus: GitHub didn't allow me to request PR reviews from the following users: frodenas.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @frodenas

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.

@unguiculus unguiculus self-assigned this Jul 17, 2017
@unguiculus
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 17, 2017
spec:
type: {{ .Values.web.service.type }}
ports:
- name: atc
port: {{ .Values.concourse.atcPort }}
targetPort: atc
- name: atc-https
port: 443
targetPort: atc
- name: tsa
port: {{ .Values.concourse.tsaPort }}
targetPort: tsa

Choose a reason for hiding this comment

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

Can you also make nodePort configurable (if service type is LoadBalancer or NodePort)?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not 100% sure how to do that. If the service type is LoadBalancer then port 443, else what would be the appropriate port?

@viglesiasce
Copy link
Contributor

As mentioned in #1365 the workers fail to start with this configuration. Issue open at concourse/concourse#1230

@edude03
Copy link
Author

edude03 commented Jul 18, 2017

@viglesiasce - that issue is fixed in my chart, it's because your worker volume has less than 10GB free causing an integer underflow. In my chart, I set the worker volume size default to 20GB.

@viglesiasce
Copy link
Contributor

@edude03 I still have it crashing albeit with a different error now (file: command not found). One weird thing is that its still referencing a 10G fs even though the PVCs are 20G:

pvc/concourse-work-dir-cb-worker-0   Bound     pvc-eee13f69-6b6a-11e7-8d3c-42010a8001ca   20Gi       RWO           standard       47s
pvc/concourse-work-dir-cb-worker-1   Bound     pvc-f98712a0-6b6a-11e7-8d3c-42010a8001ca   20Gi       RWO           standard       29s
{"timestamp":"1500349341.113278866","source":"baggageclaim","message":"baggageclaim.fs.run-command.failed","log_level":2,"data":{"args":["bash","-e","-x","-c","\n\t\ti
f [ ! -e $IMAGE_PATH ] || [ \"$(stat --printf=\"%s\" $IMAGE_PATH)\" != \"$SIZE_IN_BYTES\" ]; then\n\t\t\ttouch $IMAGE_PATH\n\t\t\ttruncate -s ${SIZE_IN_BYTES} $IMAGE_P
ATH\n\t\tfi\n\n\t\tlo=\"$(losetup -j $IMAGE_PATH | cut -d':' -f1)\"\n\t\tif [ -z \"$lo\" ]; then\n\t\t\tlo=\"$(losetup -f --show $IMAGE_PATH)\"\n\t\tfi\n\n\t\tif ! fil
e $IMAGE_PATH | grep BTRFS; then\n\t\t\t/concourse-work-dir/3.3.2/assets/btrfs/mkfs.btrfs --nodiscard $IMAGE_PATH\n\t\tfi\n\n\t\tmkdir -p $MOUNT_PATH\n\n\t\tif ! mount
point -q $MOUNT_PATH; then\n\t\t\tmount -t btrfs $lo $MOUNT_PATH\n\t\tfi\n\t"],"command":"/bin/bash","env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sb
in:/bin","MOUNT_PATH=/concourse-work-dir/volumes","IMAGE_PATH=/concourse-work-dir/volumes.img","SIZE_IN_BYTES=10266165248"],"error":"exit status 32","session":"3.1","s
tderr":"+ '[' '!' -e /concourse-work-dir/volumes.img ']'\n++ stat --printf=%s /concourse-work-dir/volumes.img\n+ '[' 10266165248 '!=' 10266165248 ']'\n++ cut -d: -f1\n
++ losetup -j /concourse-work-dir/volumes.img\n+ lo=/dev/loop1\n+ '[' -z /dev/loop1 ']'\n+ file /concourse-work-dir/volumes.img\n+ grep BTRFS\nbash: line 11: file: com
mand not found\n+ /concourse-work-dir/3.3.2/assets/btrfs/mkfs.btrfs --nodiscard /concourse-work-dir/volumes.img\n+ mkdir -p /concourse-work-dir/volumes\n+ mountpoint -
q /concourse-work-dir/volumes\n+ mount -t btrfs /dev/loop1 /concourse-work-dir/volumes\nmount: unknown filesystem type 'btrfs'\n","stdout":"btrfs-progs v4.4\nSee http:
//btrfs.wiki.kernel.org for more information.\n\nLabel:              (null)\nUUID:               ef46655e-598c-48b7-ba0a-663834486417\nNode size:          16384\nSecto
r size:        4096\nFilesystem size:    9.56GiB\nBlock group profiles:\n  Data:             single            8.00MiB\n  Metadata:         DUP             497.50MiB\n
  System:           DUP              12.00MiB\nSSD detected:       no\nIncompat features:  extref, skinny-metadata\nNumber of devices:  1\nDevices:\n   ID        SIZE
 PATH\n    1     9.56GiB  /concourse-work-dir/volumes.img\n\n"}}
{"timestamp":"1500349341.113433123","source":"baggageclaim","message":"baggageclaim.failed-to-set-up-driver","log_level":2,"data":{"error":"failed to create btrfs file
system: exit status 32"}}

@viglesiasce
Copy link
Contributor

Indeed file is not available on this Container Optimized OS image:

# file
bash: file: command not found
# cat /etc/lsb-release
CHROMEOS_AUSERVER=https://tools.google.com/service/update2
CHROMEOS_BOARD_APPID={76E245CF-C0D0-444D-BA50-36739C18EB00}
CHROMEOS_CANARY_APPID={90F229CE-83E2-4FAF-8479-E368A34938B1}
CHROMEOS_DEVSERVER=
CHROMEOS_RELEASE_APPID={76E245CF-C0D0-444D-BA50-36739C18EB00}
CHROMEOS_RELEASE_BOARD=lakitu-signed-mpkeys
CHROMEOS_RELEASE_BRANCH_NUMBER=60
CHROMEOS_RELEASE_BUILDER_PATH=lakitu-release/R59-9460.60.0
CHROMEOS_RELEASE_BUILD_NUMBER=9460
CHROMEOS_RELEASE_BUILD_TYPE=Official Build
CHROMEOS_RELEASE_CHROME_MILESTONE=59
CHROMEOS_RELEASE_DESCRIPTION=9460.60.0 (Official Build) stable-channel lakitu
CHROMEOS_RELEASE_NAME=Chrome OS
CHROMEOS_RELEASE_PATCH_NUMBER=0
CHROMEOS_RELEASE_TRACK=stable-channel
CHROMEOS_RELEASE_VERSION=9460.60.0
DEVICETYPE=OTHER
GOOGLE_RELEASE=9460.60.0
HWID_OVERRIDE=LAKITU DEFAULT

repository: https://kubernetes-charts.storage.googleapis.com/
condition: postgresql.enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing condition: postgresql.enabled will break #1056

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I didn't mean to remove that

@k8s-ci-robot
Copy link
Contributor

@edude03: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e 4657a9b link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@viglesiasce
Copy link
Contributor

Hey @edude03, we merged #1528 with Naive set by default so that all platforms work out of the gate. Can you revert the two commits related to the baggage claim driver and then rebase?

Then we will get the rest of this PR in. Sorry for the delay.

@unguiculus
Copy link
Member

Marking this as stale. Please update within the next week.

@unguiculus
Copy link
Member

Closing as stale.

@unguiculus unguiculus closed this Sep 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants