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.

Sign up for GitHub

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

Jump to bottom

Fix overlong junit filename prefixes #30894

Merged
merged 1 commit into from
Aug 18, 2016
Merged

Fix overlong junit filename prefixes #30894

merged 1 commit into from
Aug 18, 2016

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Aug 18, 2016 •
edited by k8s-oncall
Loading

Build scripts construct filenames that are longer than 255 chars when producing JUnit output for tests and KUBE_COVER=y:

vagrant@devbox:~/work/kubernetes/src/k8s.io/kubernetes (test %) $ KUBE_JUNIT_REPORT_DIR=/tmp/art KUBE_COVER=y make test
E0818 15:33:29.312549    9426 conversion.go:589] Warning: could not generate autoConvert functions for k8s.io/kubernetes/pkg/apis/extensions/v1beta1.HorizontalPodAutoscalerSpec <-> k8s.io/kubernetes/pkg/apis/autoscaling.HorizontalPodAutoscalerSpec
E0818 15:33:29.323685    9426 conversion.go:589] Warning: could not generate autoConvert functions for k8s.io/kubernetes/pkg/apis/extensions/v1beta1.JobSpec <-> k8s.io/kubernetes/pkg/apis/batch.JobSpec
E0818 15:33:29.344775    9426 conversion.go:589] Warning: could not generate autoConvert functions for k8s.io/kubernetes/pkg/apis/extensions/v1beta1.RollingUpdateDeployment <-> k8s.io/kubernetes/pkg/apis/extensions.RollingUpdateDeployment
E0818 15:33:29.349260    9426 conversion.go:589] Warning: could not generate autoConvert functions for k8s.io/kubernetes/pkg/apis/extensions/v1beta1.ScaleStatus <-> k8s.io/kubernetes/pkg/apis/extensions.ScaleStatus
Running tests for APIVersion: v1,apps/v1alpha1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1beta1,autoscaling/v1,batch/v1,batch/v2alpha1,certificates/v1alpha1,extensions/v1beta1,federation/v1beta1,policy/v1alpha1,rbac.authorization.k8s.io/v1alpha1,imagepolicy.k8s.io/v1alpha1
+++ [0818 15:33:30] Saving coverage output in '/tmp/k8s_coverage/v1,apps/v1alpha1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1beta1,autoscaling/v1,batch/v1,batch/v2alpha1,certificates/v1alpha1,extensions/v1beta1,federation/v1beta1,policy/v1alpha1,rbac.authorization.k8s.io/v1alpha1,imagepolicy.k8s.io/v1alpha1/20160818-153330'
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_genutils.stdout: File name too long
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_kube-apiserver_app.stdout: File name too long
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_hyperkube.stdout: File name too long
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_kube-apiserver_app_options.stdout: File name too long
ok      k8s.io/kubernetes/cmd/genutils  0.002s
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_kubelet_app.stdout: File name too long
qok     k8s.io/kubernetes/cmd/kube-apiserver/app/options    0.159s
tee: /tmp/art/junit_v1,apps-v1alpha1,authentication.v1beta1,authorization.v1beta1,autoscaling-v1,batch-v1,batch-v2alpha1,certificates-v1alpha1,extensions-v1beta1,federation-v1beta1,policy-v1alpha1,rbac.authorization.v1alpha1,imagepolicy.v1alpha1_20160818-153330-cmd_kube-proxy_app.stdout: File name too long

Let's shorten filenames a bit more to make them fit in 255 chars. This is probably temporary solution too, the bulletproof one being (e.g.) using sha1sum of KUBE_TEST_API but that would produce files with meaningless filenames so for now maybe it may wait.

(there was recent attempt to shorten the filenames by removing k8s.ios, but that was not enough for KUBE_COVER=y)

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Aug 18, 2016
@lavalamp lavalamp added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 18, 2016
lavalamp
lavalamp reviewed Aug 18, 2016
View reviewed changes
hack/make-rules/test.sh
@@ -157,7 +157,10 @@ junitFilenamePrefix() {
mkdir -p "${KUBE_JUNIT_REPORT_DIR}"
local KUBE_TEST_API_NO_SLASH="${KUBE_TEST_API//\//-}"
# This file name isn't parsed by anything, and tee needs a shorter file name.
# Convert strings like junit_v1,apps-v1alpha1,authentication.k8s.io-v1beta1
# to junit_v1,apps.k-v1a1,authentication.k-v1b1
Copy link
Member

Choose a reason for hiding this comment

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

The most important part of a comment is the why, not the what! Please tell future us why you're making this change. :)

@lavalamp
Copy link
Member

Please fix comment and we'll get this in asap.

@ivan4th ivan4th force-pushed the fix-overlong-junit-prefixes branch from 79058a1 to 070aba6 Compare August 18, 2016 19:54
@ivan4th
Copy link
Contributor Author

ivan4th commented Aug 18, 2016

hack/make-rules/test.sh, line 161 [r1] (raw file):

Previously, lavalamp (Daniel Smith) wrote…

The most important part of a comment is the why, not the what! Please tell future us why you're making this change. :)

There was earlier comment above this explanation concerning the overlong filenames (someone was trying to make filenames shorter but not hard enough :), but it was somewhat wrong because 255 char limit isn't imposed by tee, so I fixed it. As of what the sed command does, I thought that it wasn't very readable (like many sed commands), so I added the description, but ok, perhaps it's indeed a little redundant.

Comments from Reviewable

@ivan4th
Copy link
Contributor Author

ivan4th commented Aug 18, 2016

Fixed.

Comments from Reviewable

@ivan4th ivan4th force-pushed the fix-overlong-junit-prefixes branch from 070aba6 to 98ea7b9 Compare August 18, 2016 20:25
@k8s-bot
Copy link

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 98ea7b9.

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@lavalamp
Copy link
Member

LGTM, thanks!!

@lavalamp
Copy link
Member

In fact, the queue is currently blocked presumably because it can't parse junit from a flaky test-go run.

@krousey (build cop) how do you feel about manually merging this?

@krousey
Copy link
Contributor

krousey commented Aug 18, 2016

@lavalamp I'm find with a manual merge, but I don't see how this is blocking the build. There's a data race detection, but I don't see the junit parsing error.

@krousey krousey merged commit 0e5e6f4 into kubernetes:master Aug 18, 2016
@krousey
Copy link
Contributor

krousey commented Aug 18, 2016

Manual merge on faith that @lavalamp saw something that I couldn't find.

@lavalamp
Copy link
Member

I saw a failed individual test, but queue made it red instead of orange,
which means it failed to find a relevant junit file to parse, presumably
because the upload failed due to the name being huge.

On Thu, Aug 18, 2016 at 3:04 PM, krousey notifications@github.com wrote:

Manual merge on faith that @lavalamp https://github.com/lavalamp saw
something that I couldn't find.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30894 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglgrr7LtAQZuJGzIWQ0dVi4tFEaj4ks5qhNbrgaJpZM4JnqJw
.

@ivan4th ivan4th mentioned this pull request Aug 22, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Aug 23, 2016
Merge pull request #31117 from ivan4th/fix-overlong-junit-prefixes-2n…
87d89a9 
…d-attempt

Automatic merge from submit-queue

Fix overlong junit filename prefixes (2nd attempt)

This is followup for #30894.
Turned out that filename shortening I used isn't enough in some cases (scroll to the right):
```
vagrant@devbox:~/work/kubernetes/src/k8s.io/kubernetes (test %) $ mkdir -p /tmp/art && time KUBE_JUNIT_REPORT_DIR=/tmp/art KUBE_COVER=y make test WHAT=cmd/libs/go2idl/client-gen/testoutput/clientset_generated/test_internalclientset/typed/testgroup.k8s.io/unversioned
Running tests for APIVersion: v1,apps/v1alpha1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1beta1,autoscaling/v1,batch/v1,batch/v2alpha1,certificates/v1alpha1,extensions/v1beta1,federation/v1beta1,policy/v1alpha1,rbac.authorization.k8s.io/v1alpha1,imagepolicy.k8s.io/v1alpha1
+++ [0822 13:57:46] Saving coverage output in '/tmp/k8s_coverage/v1,apps/v1alpha1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1beta1,autoscaling/v1,batch/v1,batch/v2alpha1,certificates/v1alpha1,extensions/v1beta1,federation/v1beta1,policy/v1alpha1,rbac.authorization.k8s.io/v1alpha1,imagepolicy.k8s.io/v1alpha1/20160822-135746'
tee: /tmp/art/junit_v1,apps-v1a1,authentication.v1b1,authorization.v1b1,autoscaling-v1,batch-v1,batch-v2a1,certificates-v1a1,extensions-v1b1,federation-v1b1,policy-v1a1,rbac.authorization.v1a1,imagepolicy.v1a1_20160822-135746-cmd_libs_go2idl_client-gen_testoutput_clientset_generated_test_internalclientset_typed_testgroup.k8s.io_unversioned.stdout: File name too long
ok      k8s.io/kubernetes/cmd/libs/go2idl/client-gen/testoutput/clientset_generated/test_internalclientset/typed/testgroup.k8s.io/unversioned   0.038s
ls: cannot access '/tmp/art/junit_v1,apps-v1a1,authentication.v1b1,authorization.v1b1,autoscaling-v1,batch-v1,batch-v2a1,certificates-v1a1,extensions-v1b1,federation-v1b1,policy-v1a1,rbac.authorization.v1a1,imagepolicy.v1a1_20160822-135746*.stdout': No such file or directory
Makefile:118: recipe for target 'test' failed
make: *** [test] Error 1

real    0m49.623s
user    2m35.224s
sys     0m9.200s
```

Looks like we're have no choice here besides just using a hash as filename prefix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ivan4th @lavalamp @k8s-bot @krousey @googlebot @k8s-github-robot