Skip to content

[Important Fix] Add release name (NS) to the Statsd address in mesh config#6349

Merged
istio-testing merged 1 commit intoistio:masterfrom
ymesika:configmapFix
Jun 18, 2018
Merged

[Important Fix] Add release name (NS) to the Statsd address in mesh config#6349
istio-testing merged 1 commit intoistio:masterfrom
ymesika:configmapFix

Conversation

@ymesika
Copy link
Copy Markdown
Member

@ymesika ymesika commented Jun 16, 2018

In recent PR of mine (#6133) I moved the statsd address to the global values.
The value in the configmap should be with the NS because otherwise pods with injected istio-proxy outside of the istio-system namespace won't be able to connect to the statsd destination causing the istio-proxy container to crash.

@ymesika
Copy link
Copy Markdown
Member Author

ymesika commented Jun 16, 2018

/assign @gyliu513

@ymesika
Copy link
Copy Markdown
Member Author

ymesika commented Jun 18, 2018

/assign @rshriram

Copy link
Copy Markdown
Member

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

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

/lgtm

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyliu513, ymesika

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@istio-testing istio-testing merged commit 3960690 into istio:master Jun 18, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jun 18, 2018

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

Test name Commit Details Rerun command
prow/istio-pilot-e2e.sh e965e44 link /test istio-pilot-e2e
prow/e2e-bookInfoTests.sh e965e44 link /test e2e-bookInfo
Details

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.

@hklai
Copy link
Copy Markdown
Contributor

hklai commented Jun 18, 2018

Any idea why this was not caught in the tests?

Also this is the only fix that is needed on top of the RC so far. We probably will just create a release branch, and cherrypick this to the RC.

@ymesika
Copy link
Copy Markdown
Member Author

ymesika commented Jun 19, 2018

@hklai It's just a fix for a PR (#6133) that was merged a day earlier than this PR.

@hklai
Copy link
Copy Markdown
Contributor

hklai commented Jun 19, 2018

@ymesika #6133 was merged 5 days ago right? And hence it was picked up in the monthly RC.

@ymesika
Copy link
Copy Markdown
Member Author

ymesika commented Jun 19, 2018

@hklai Thanks for clarifying

@hklai
Copy link
Copy Markdown
Contributor

hklai commented Jun 19, 2018

@ymesika Well my question remains. Why was the problem not caught by any automatic tests? If not, can we add some?

@ymesika
Copy link
Copy Markdown
Member Author

ymesika commented Jun 19, 2018

My guess is that we run all test related stuff in the same ns: Istio pods + test services pods.
It's in my queue to further investigate and add a case when the test services aren't in Istio's ns.

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.

7 participants