Skip to content

upstream: trigger original dst clean only when hosts are there#6152

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/original_dst_log
Mar 5, 2019
Merged

upstream: trigger original dst clean only when hosts are there#6152
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/original_dst_log

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

@ramaraochavali ramaraochavali commented Mar 2, 2019

Description: The log line Cleaning up stale original dst hosts is triggering quite often when upstream log is enabled even there are no hosts. My intention was to control this log line, but I think it does not harm to guard whole cleanup logic with this condition.
Risk Level: Low
Testing: Low
Docs Changes: N/A
Release Notes: N/A

@ramaraochavali
original dst log line
cfee835 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
venilnoronha
venilnoronha reviewed Mar 2, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@venilnoronha venilnoronha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd refrain from this change because this log, although frequent, signals that the cleanup operation had started. Would it make sense to rather drop the log level to trace and update the message?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

ramaraochavali commented Mar 3, 2019

@venilnoronha That was my original thought - but then looking at the log lines in the loop, if we drop this to trace and leave other two log lines in debug they seem to come out of context. But I agree we will loose the information that it is running - Are you OK, making all three log lines to trace? I am OK changing it like that.

@ramaraochavali
change to trace
58625f3 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali changed the title upstream: trigger original dst clean only when hosts are there upstream: change original dst cleanup log lines to trace Mar 3, 2019
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

ramaraochavali commented Mar 3, 2019

@venilnoronha I went ahead and changed all three log lines to trace. LMK, if you think there is a better alternative.

@venilnoronha
Copy link
Copy Markdown
Member

venilnoronha commented Mar 3, 2019

Can we keep your previous logic (and the 3 debug logs) and add an additional trace log outside the if condition?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

ramaraochavali commented Mar 3, 2019 •
edited
Loading

I see. I can do that.
@venilnoronha done. PTAL

@ramaraochavali
add additional trace
b42c079 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali changed the title upstream: change original dst cleanup log lines to trace upstream: trigger original dst clean only when hosts are there Mar 3, 2019
venilnoronha
venilnoronha reviewed Mar 3, 2019
View reviewed changes
source/common/upstream/original_dst_cluster.cc Outdated
} else {
ENVOY_LOG(debug, "Removing stale host {}.", host->address()->asString());
to_be_removed.emplace_back(host);
ENVOY_LOG(trace, "stale original dst hosts cleanup triggered");
Copy link
Copy Markdown
Member

@venilnoronha venilnoronha Mar 3, 2019

Choose a reason for hiding this comment

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

nit: s/stale/Stale
nit: s/triggered/triggered.

@ramaraochavali
comments
bce1701 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

ramaraochavali commented Mar 3, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Mar 3, 2019

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #6152 (comment) was created by @ramaraochavali.

see: more, trace.

venilnoronha
venilnoronha approved these changes Mar 3, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@venilnoronha venilnoronha 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!

@dio
Copy link
Copy Markdown
Member

dio commented Mar 3, 2019

@snowp could you help on reviewing this?

@dio dio assigned snowp Mar 3, 2019
snowp
snowp approved these changes Mar 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks

@mattklein123 mattklein123 merged commit 0b0c57b into envoyproxy:master Mar 5, 2019
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
@ramaraochavali @fredlas
upstream: trigger original dst clean only when hosts are there (envoy…
797b9a5 
…proxy#6152)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@ramaraochavali ramaraochavali deleted the fix/original_dst_log branch March 6, 2019 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+2 more reviewers

@venilnoronha venilnoronha venilnoronha approved these changes

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ramaraochavali @venilnoronha @dio @snowp @mattklein123