-
Notifications
You must be signed in to change notification settings - Fork 280
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.
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
Fluentd Logging System Part 1 #652
Conversation
Can one of the admins verify this patch? |
jenkins ok to test |
jenkins add to whitelist |
Test FAILed. |
Okay. There are 2 issues in both fluentd integration test. Please let me know if you can think of any reason why it occurs.
|
…Used requests' ConnectionError class instead of ConnectionRefusedError which is not supported in python2. Added user='root' inside fluentd container run function so that it can access conf file existing in a root folder within a container.
I fixed errors. Please test it again! |
Jenkins test this please |
Jenkins ok to test |
Test FAILed. |
Looks like there is an issue with Pytorch container?
Do you think it is due to the change? Let me test |
Test FAILed. |
@rkooo567 it seems there are two issues
|
@@ -459,6 +489,26 @@ def stop_all(self, graceful=True): | |||
else: | |||
c.kill() | |||
|
|||
def _is_valid_logging_state_to_connect(self, all_labels): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change the logic of this part. I will make new clipper connection to turn on use_log_centralization
flag if there is a fluentd instance running in a cluster regardless of use_log_centralization
flag of the current DockerContainerManager
instance. As you can see the current logic is that if the flag is different from the cluster context (meaning if use_centralization
is on but there's no fluentd instance), it will cause an error. I will change this to
- If Fluentd instance is within a cluster and the current flag is off -> current flag is on.
- If current flag is on but there's no fluentd instance running -> ClipperException
- Otherwise same
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Yes! Finally passed tests. @simon-mo. Please review the PR and leave me some comments. Also, are there some other people who will be involved in code review? |
@withsmilo If you have time, can you also review the PR? I will appreciate it! |
@rkooo567 Sure, I will review this PR tonight. |
Sorry about the delay. I'll review this over the weekend. |
or not os.path.isfile(self._file_path): | ||
self._file_path = self.build_temp_file() | ||
|
||
# Logging-TODO: Currently, it copies the default conf from clipper_fluentd.conf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this as an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment to #625 instead (because it will be anyway handled at PR2, and it is not merged yet). If you still want me to add this to a new issue, I will do it after this pr is merged! Please let me know
@@ -40,7 +40,7 @@ def signal_handler(signal, frame): | |||
|
|||
if __name__ == '__main__': | |||
signal.signal(signal.SIGINT, signal_handler) | |||
clipper_conn = ClipperConnection(DockerContainerManager()) | |||
clipper_conn = ClipperConnection(DockerContainerManager(use_centralized_log=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it turned off by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to expose this option to users by adding it to the example code. If you think it is better removing it, I will do that! Let me know
Test FAILed. |
@simon-mo Can you run Jenkins again? It failed at [integration_py3_docker_metric] 19-04-27:19:31:52 ERROR [clipper_metric_docker.py:126] Failed to parse: http://localhost:31119/api/v1/series?match[]=clipper_mc_pred_total |
Jenkins test this please |
Test FAILed. |
Hmm.. I got the same error. idk how we fail to parse url. When I urlparse in the interactive shell, it looks fine.. I will try to figure out soon |
|
Test PASSed. |
This is the part of Logging project. #625.
Summary of PR.
use_centralized_log=True
forDockerContainerManager
.clipper_admin/docker/logging/fluentd/clipper_fluentd.conf
. It will be done withinFluentdConfig
class. It will also write the correct port number.Note
Since @simon-mo mentions that Grafana or other logging tools can be potentially used, I tried to make this part as pluggable as possible. If we want to use a different logging system, we can just change
logging_system
to a different class and create a new class that has same public functions asFluentd
class. I didn't define Interface because I thought it was too much at this point. I will write about it in README.md later.Testing
use_centralized_log
. You can check this from_is_valid_logging_state_to_connect
withinDockerContainerManager