Skip to content

Change coredump path and add integration test #6205

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
Closed
wants to merge 4 commits into from
Closed

Change coredump path and add integration test #6205

wants to merge 4 commits into from

Conversation

ostromart
Copy link
Contributor

  1. Change coredump dir from /etc/istio/proxy/ to /var/istio/proxy/
  2. Add e2e integration test to verify core is generated.

@ldemailly
Copy link
Member

thanks a lot for doing this !
do you have a link for a build (test build) where envoy crashed and the core is available to download as artifact ?

@ostromart
Copy link
Contributor Author

I haven't added that yet, will take a look.

@ostromart ostromart mentioned this pull request Jun 13, 2018
Merged
@ldemailly ldemailly mentioned this pull request Jun 13, 2018
Closed
@andraxylia
Copy link
Contributor

Is there an issue with having the core in /etc that is solved by having the core in /var?

@codecov
Copy link

codecov bot commented Jun 14, 2018 •
edited
Loading

Codecov Report

Merging #6205 into master will decrease coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #6205    +/-   ##
=======================================
- Coverage      68%     67%   -<1%     
=======================================
  Files         351     345     -6     
  Lines       30671   30806   +135     
=======================================
- Hits        20718   20504   -214     
- Misses       9109    9465   +356     
+ Partials      844     837     -7
Impacted Files Coverage Δ
istioctl/cmd/istioctl/authn.go 50% <0%> (-23%) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 59% <0%> (-21%) ⬇️
mixer/adapter/stackdriver/helper/common.go 74% <0%> (-9%) ⬇️
mixer/adapter/rbac/rbacStore.go 76% <0%> (-6%) ⬇️
mixer/adapter/solarwinds/log_handler.go 54% <0%> (-4%) ⬇️
mixer/pkg/runtime/handler/env.go 81% <0%> (-4%) ⬇️
mixer/adapter/rbac/controller.go 50% <0%> (-3%) ⬇️
pilot/pkg/networking/plugin/authz/rbac.go 79% <0%> (-1%) ⬇️
istioctl/cmd/istioctl/config.go 2% <0%> (-1%) ⬇️
pilot/pkg/proxy/envoy/v2/debug.go 25% <0%> (-1%) ⬇️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0f1ea7...dc1d557. Read the comment docs.

@ostromart ostromart force-pushed the core_pattern branch 2 times, most recently from 64e84d4 to 23188a6 Compare June 14, 2018 17:47
@ostromart
Copy link
Contributor Author

@andraxylia this is mostly about #5847 but piggybacking the dir change since we (me, @costinm and @mandarjog) agreed that /var was the right place for cores rather than /etc.

@ldemailly
Copy link
Member

/etc is for (typically readonly/not modified configs)
/var is for "variable" (writeable) thing that don't necessarily need super durability, typically log files, cores, etc...

@ostromart ostromart force-pushed the core_pattern branch 4 times, most recently from 6b34da1 to 3084cb2 Compare June 18, 2018 20:49
@ostromart ostromart force-pushed the core_pattern branch 4 times, most recently from 8202b09 to f0e6cb4 Compare June 18, 2018 23:23
@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ostromart
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mandarjog

Assign the PR to them by writing /assign @mandarjog in a comment when ready.

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

@ostromart ostromart force-pushed the core_pattern branch 3 times, most recently from 054adda to 1f43e85 Compare June 19, 2018 00:36
@istio-testing
Copy link
Collaborator

istio-testing commented Jun 19, 2018 •
edited
Loading

@ostromart: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-unit-tests.sh dc1d557 link /test istio-unit-tests
prow/e2e-bookInfoTests.sh dc1d557 link /test e2e-bookInfo
prow/istio-pilot-e2e.sh dc1d557 link /test istio-pilot-e2e
prow/e2e-simpleTests.sh dc1d557 link /test e2e-simple

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.

@ldemailly
Copy link
Member

poke

@ostromart
Copy link
Contributor Author

Would love to get this in but it's stuck on some strange behavior in circle minikube ignoring all uname settings. One other thing I want to try after vacation is to make the change in the circle environment itself rather than docker container.

@ldemailly
Copy link
Member

I didn’t know you were in vacation, enjoy it, it’ll wait :)

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 6, 2018
@istio-testing
Copy link
Collaborator

@ostromart: PR needs rebase.

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.

@stale
Copy link

stale bot commented Jul 20, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Jul 20, 2018
@andraxylia
Copy link
Contributor

@ostromart can you update this PR? Should't it be in 1.0?

@stale stale bot removed the stale label Jul 20, 2018
@rshriram rshriram added the stale label Jul 20, 2018
@rshriram rshriram closed this Jul 20, 2018
@andraxylia
Copy link
Contributor

@rshriram @ostromart this is useful, why did you close it?

@ostromart
Copy link
Contributor Author

I can reopen it against 1.0 when I get a bit of time to look at it again. It's long been stuck on CircleCI annoyingly resetting uname -c to 0 no matter what I do. Last try is going to be to set it to unlimited in .circleci to see if it's something to do with the platform behavior.

@ostromart ostromart mentioned this pull request Jul 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldemailly ldemailly Awaiting requested review from ldemailly

2 more reviewers

@rshriram rshriram rshriram approved these changes

@andraxylia andraxylia andraxylia approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
needs-rebase Indicates a PR needs to be rebased before being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ostromart @ldemailly @andraxylia @istio-testing @rshriram @googlebot