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.

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

Specification file for logging implementation #94

Merged
merged 2 commits into from
Apr 28, 2017

Conversation

GowthamShanmugam
Copy link
Contributor

@GowthamShanmugam GowthamShanmugam commented Dec 19, 2016

Signed-off-by: GowthamShanmugam gshanmug@redhat.com

tendrl-bug-id: #28

@GowthamShanmugam
Copy link
Contributor Author

@brainfunked @shtripat please review

@GowthamShanmugam
Copy link
Contributor Author

@shtripat please review this

@@ -0,0 +1,174 @@
= Specification for logging implementation

Logs and alerts need to be treated as messages emitted by a component.
Copy link
Member

Choose a reason for hiding this comment

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

"...emitted by Tedrl modules" may be I would say

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will change it

Copy link
Contributor

Choose a reason for hiding this comment

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

'..by Tendrl'?

@@ -0,0 +1,174 @@
= Specification for logging implementation
Copy link
Member

Choose a reason for hiding this comment

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

"Generic message mechanism for logs and alerts in Tendrl"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will change it

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


Logs and alerts need to be treated as messages emitted by a component.

== Problem description
Copy link
Member

Choose a reason for hiding this comment

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

The problem statement for this case is something like

The logs and alerts in Tendrl are handled differently and there is no standard mechanism which makes sure no internal details are exposed as part of alerts/notifications sent to end user.

Ideally logs messages and alerts are both messages which are emitted from an application. Tendrl needs to treat them as one entity and a common handler should decide what to be done with the message which is emitted and received at handler."

The below text talks more of implementation how and what you want to do rather :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok :)

* Notifications and alerts need to be delivered to the node agent alert sockets.
* Commons needs to implement a logging library that hides all the implementation
details and configuration options from any of the components using the library.
Components should be able to blindly emit messages to the common logging module
Copy link
Member

Choose a reason for hiding this comment

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

We should rather call it Message Handler module in Tendrl I feel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Logs and alerts need to be treated as messages.

== Proposed change
* Create a new class called "Message" in common with attributes name, type,
Copy link
Member

Choose a reason for hiding this comment

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

I feel we would need info about the module which is raising this message so that module specific logging configuration can be used for logging the messages. I am coming from a point where each module like node_agent, ceph_integration, gluster_integartion etc can have their own log-levels and loggers defined.
Does not look very clear how this scenario will be taken care with this model..

```
* Expected output format for the alert in node_agent socket:
```
{
Copy link
Member

Choose a reason for hiding this comment

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

This looks very minimal and alerts might need additional fields related to acknowledgment etc. You may refer skyring for detailed object structure may be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack required for events right?

message, priority(empty for log), alert_id ,log_level(empty for alert),
function_name, line_number.
* Using name retrieve the particular logging handler.
* Create a new function for Message class and process the object values.
Copy link
Member

Choose a reason for hiding this comment

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

I feel there should be a generic class defined as MessageHandler and extend this into two classes namely LogMessageHandler and AlertHandler. This class would have a handle() function which is specific in nature and takes care of the actual logic.

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 21, 2016

Choose a reason for hiding this comment

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

@brainfunked told me we have to log alert also in log file. so spiting like LogMessageHandler and AlertHandler is good?

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 22, 2016

Choose a reason for hiding this comment

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

yes i feel same .. we cant split


=== Tendrl/node_agent impact

* Import the Message class in all the modules and Change log statements to
Copy link
Member

Choose a reason for hiding this comment

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

I feel we should add the framework in one PR and do the mass change in different PR. Makes life easier and doesnt halt everybody else :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok .. i will change as per your suggestion

@@ -0,0 +1,174 @@
= Specification for logging implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,174 @@
= Specification for logging implementation

Logs and alerts need to be treated as messages emitted by a component.
Copy link
Contributor

Choose a reason for hiding this comment

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

'..by Tendrl'?

* A schema needs to be defined for emitted messages. This could be a json schema.
The schema needs to include metadata that allows the exact point of origin of
the message to be known.
* Message priorities, types etc. need to be defined at the time of emission.
Copy link
Contributor

Choose a reason for hiding this comment

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

'..at the point of origin'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

and it should be able to deliver them to either syslog, log files, alert
sockets etc. depending upon the metadata and configuration.
* Syslog must be the default logging configuration. Logging to individual
configuration files should be thoroughly discouraged.
Copy link
Contributor

Choose a reason for hiding this comment

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

'..discouraged by default.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


Logs and alerts need to be treated as messages emitted by a component.

== Problem description
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, the updates to in-flight operations, #55, need to be logged under INFO. The updates themselves should just be messages emitted that have the same structure and appropriate metadata. This metadata includes operation uuids. This implies that the details of the jobs being created also need to be logged. This is where tracing and debugging is addressed. The updates being logged provide an id. This id pertains to a specific job. This job being logged enables end-to-end tracing of the job and it's updates across the deployment. This is very important, since it allows an administrator to filter the logs by id and get all the required details.

Copy link

@t0ffel t0ffel left a comment

Choose a reason for hiding this comment

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

Hi, I'm with Common Logging. I was asked to take a look at this PR.
I'm not very familiar with tendrl, so please forgive me if my questions occasionally sound stupid.


== Problem description

* Logs and alerts need to be treated as messages emitted by a component.
Copy link

Choose a reason for hiding this comment

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

Apologies in advance as I am a bit out of context on Tendrl, but what is the difference between logs and alerts within the scope of this specification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Log nothing but a information, erros. etc. It helps an end_user for the debugging the problem. Alert also a message but it is immediately forward to end_user. Alerts can be debug with the help of log message.


== Proposed change
* Create a new class called "Message" in common with attributes name, type,
message, priority(empty for log), alert_id ,log_level(empty for alert),
Copy link

Choose a reason for hiding this comment

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

Several questions:

  1. What about timestamp?
  2. Is there a place for extra metadata that might accompany the message?
  3. Do you plan to have possible values/expected format for these fields? i.e.
    • does the log_level have 1-1 mapping to syslog's severity?
    • What is the format of the message? Can it be a multiline backtrace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.ya i have toadd timestamp also,
2.I have declared class called Message for holding the message and other details. And alert and log can be handled from that object.
3.
* yes log_level have 1-1 mapping to syslog's : log level also came from object, with the help of log level i am calling syslog function.
@brainfunked your comments pleases

```
* Import the Message class in all the modules and Change log statements to
populate the instance of message class and pass the object to function.
* Expected output format for log and alert in log file:
Copy link

Choose a reason for hiding this comment

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

what about sending logs to syslog? What will the logs/alerts look like in syslog/journal? Will you be using any special systemd journal fields?

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 23, 2016

Choose a reason for hiding this comment

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

no we are not using any special journal,
using loglevel we are calling exact syslog function to log the message

alert is always warning. For log Based n loglevel it is decided (info, error....)

* Expected output format for log and alert in log file:
```
Format: "%(asctime)s - %(levelname)s - %(message)s"
Message: "%(filename)s - %(function)s - %(lineno)s - %(message)s"
Copy link

Choose a reason for hiding this comment

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

I'm a bit confused. In https://github.com/Tendrl/specifications/pull/94/files#diff-5a251eddb4570c9ab1b30f91c2d17180R54 it says that json will be stored.
Also what exactly do you mean by separate lines for Format and Message?

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 23, 2016

Choose a reason for hiding this comment

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

What is my idea is Create class called Message, This message class have all the attributes like
"name" (log_handler_name) (each module have different logging conf, so we are going to log message in common place, so we need log_handler for that corresponding module).
"type" (alert or log) (we log alert also but alert have one more step, alert is passed to enduser immediately ).
"log level" - (it tells what is the log_level like info, error.... for log the message in syslog, alert is default warning).
"timestamp"
"severity" (for log it is empty)
"aler_id"
"tag" (for alert)
"message" - message is nothing but text what they expected to log or alert. Developer will hard code the message. Why i gave message format means
if you give log format Format: "%(asctime)s - % (filename) - %(function) - %(linenumber) %(levelname)s - %(message)s" then log message logged with filename, functionname, loneno where the loglevel function called.

Here we are collecting all object in handler function (in message class) and logging from that function itself. So log message will take handler function's name, filename, lineno in log file. To avoid this the handler will collect the filename, function name, lineno automatically where the object is came from and it will append these details with message and then log that message in log file. So that appending format is Message: "%(filename)s - %(function)s - %(lineno)s - %(message)s".

we are using message object to collect all info, no need any json, i have to modify that line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

format is nothing but we give logging format when we create log handler,
message format nothing but appending extra info with message automatically

@GowthamShanmugam
Copy link
Contributor Author

@shtripat @brainfunked @nnDarshan @anmolbabu please review this

@GowthamShanmugam
Copy link
Contributor Author

@GowthamShanmugam
Copy link
Contributor Author

@brainfunked @shtripat @nnDarshan please review this

Logs and alerts need to be treated as messages emitted by Tendrl.

== Problem description
* The logs, alerts and flight_operations_updates in Tendrl are handled differently
Copy link
Member

Choose a reason for hiding this comment

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

in flight operations updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

log_level(info/error..)
message
message_trace_back_flag(True/False)
message_uuid (optional for log)
Copy link
Member

Choose a reason for hiding this comment

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

It should not optional for logs may be we should use current job id as message id so that tracing of the error becomes easier while debugging.

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 29, 2016

Choose a reason for hiding this comment

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

@shtripat yes actually my logic also same why i mentioned here as optional means, for flight operation i am using job id as message id, for alerting i am using alert id as message id but what about normal exception logs. It does not required any id. What i try to mentioned here is for normal log messages it is optional

Copy link
Member

Choose a reason for hiding this comment

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

May be make the sentences more clearer in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

object values.
* If the message type is Alert then store the message in log file with uuid (log level
for alert is always warning). and put the object in node_agent socket.
* If the message type is flight_operation then log the message in file with message
Copy link
Member

Choose a reason for hiding this comment

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

as INFO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat ok i will change it

=== Tendrl/node_agent impact

* Create socket to receive alert.
* Import message class from common and change all log statement (it is handled in another PR).
Copy link
Member

Choose a reason for hiding this comment

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

Put the link to other PR here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat For this specification is any required?

Copy link
Member

Choose a reason for hiding this comment

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

All I meant is instead if saying handled in another PR put the reference of that PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat ok i will do it

Message: ""%(asctime)s - %(filename)s - %(function)s - %(lineno)s - %(levelname)s
- %(message)s"

(No need to specify any log format in configuration file. We can remove the format
Copy link
Member

Choose a reason for hiding this comment

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

Need to POC this I feel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat done

are emitted from an application. Tendrl needs to treat them as one entity and
a common handler should decide what to be done with the message which is emitted
and received at handler.
== Use Cases
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need to leave a line above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anmolbabu done

we have to find the function name, line number, file name and append this data
with message manually).
```
from inspect import getframeinfo, stack
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not looking properly in html generated from adoc use asciidoctor <file_name>.adoc after installing asciidoctor. The command generates a html file in current directory and that you can open in any browser and check it. You probably need to leave a line before or not sure if is a valid symbol in adoc. Please check it.

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Dec 29, 2016

Choose a reason for hiding this comment

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

@anmolbabu this symbol should start from first column without any space, then it will work

Copy link
Member

Choose a reason for hiding this comment

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

@GowthamShanmugam better follow a line gap and start from first column of line always.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GowthamShanmugam please check https://github.com/GowthamShanmugam/specifications/blob/4bdc50f125b05162fcf36117266b2fcba21ae745/specs/logging_implementation.adoc
for seeing how the code part looks currently. It looks mis-aligned and not as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will correct it


== Notifications/Monitoring impact

None
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably say:

The collectd plugin to handle collectd detected threshold breaches should be customized to call the utility function described in this adoc instead of .directly posting the alert to socket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


=== Tendrl/gluster_integration impact

* Import message class from common and change all log statement (it will be in another PR).
Copy link
Contributor

@anmolbabu anmolbabu Dec 29, 2016

Choose a reason for hiding this comment

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

We should also be adding this:

gluster-integration needs to invoke the utility described in this PR for every alert detected by it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly for ceph-integration also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will add this

@GowthamShanmugam
Copy link
Contributor Author

@brainfunked @shtripat @nthomas-redhat @nnDarshan @anmolbabu please review this

@mkudlej
Copy link

mkudlej commented Jan 2, 2017

Please add usmqe/usmqe-tests#14 as reference for testing alerts.

@GowthamShanmugam
Copy link
Contributor Author

@mkudlej done

@GowthamShanmugam
Copy link
Contributor Author

@anmolbabu @shtripat @brainfunked @nthomas-redhat please review

* Notifications sent to the administrator should provide sufficient context to
enable debugging of the problem via investigation into the log files.
* Notifications and alerts need to be delivered to the node agent alert sockets.
* Commons needs to implement a logging library that hides all the implementation
Copy link
Member

Choose a reason for hiding this comment

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

commons module??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat *common

Copy link
Member

Choose a reason for hiding this comment

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

I mean you should say "common module"

for alert is always warning). and put the object in node_agent socket.
2. If the message type is flight operation then log the message in file with message
uuid (uuid is used to trace the filght operation).
3. If the type is Log then process then store message in log file..
Copy link
Member

Choose a reason for hiding this comment

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

grammar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat i will change it

"type" : "string",
"tags" : "string",
"ackedby" : "string",
"acked" : "boolean"
Copy link
Member

Choose a reason for hiding this comment

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

Should we have ack_comment as well here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat ack_comment? why this is required , @anmolbabu your suggestion please

Copy link
Member

@shtripat shtripat Jan 3, 2017

Choose a reason for hiding this comment

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

While acking a notification user might provide some comment and that we can store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anmolbabu your suggestion please

Copy link
Contributor

Choose a reason for hiding this comment

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

@GowthamShanmugam @shtripat comment looks like is required but I missed it somehow in my spec that actually talks about alert structure. But its my opinion that, instead of having changes to the alert structure and details around it be effected in 2 places this spec and also https://github.com/Tendrl/specifications/blob/master/specs/pluggable_alert_delivery.adoc. So I feel we can open a bug to capture this point in pluggable_alert_delivery.adoc and just refer that spec in this spec for details about alerts. @shtripat what do you suggest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM


import uuid

class alert(Message):
Copy link
Member

Choose a reason for hiding this comment

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

Class name starts with caps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shtripat i will change it

Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer my comment @ line 208

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GowthamShanmugam
Copy link
Contributor Author

@TimothyAsir @brainfunked @anmolbabu @nthomas-redhat @shtripat At the time of logging a message, if the socket fails then log message never reach to logging framework. In this case we have to intimate the end user like socket is down and log messages are not logged successfully. Other wise the log message will lost. The problem here is without socket we can log the message anywhere, we need some mechanism to handle this case.

@shtripat
Copy link
Member

@GowthamShanmugam I am not sure how to tackle, but as a preferable option I would like to log these kind of failures in some common tendrl files say /var/log/tendrl/tendrl.log. Just a wierd thought...

@nthomas-redhat
Copy link
Contributor

nthomas-redhat commented Feb 15, 2017

@TimothyAsir @brainfunked @anmolbabu @nthomas-redhat @shtripat At the time of logging a message, if the socket fails then log message never reach to logging framework. In this case we have to intimate the end user like socket is down and log messages are not logged successfully. Other wise the log message will lost. The problem here is without socket we can log the message anywhere, we need some mechanism to handle this case.

@GowthamShanmugam
In case of logger failure :
log errors to stderr and messages to stdout , this should ultimately go into systemd journals.
we should be able to alert to etcd directly via the node agent in case there's a problem with the socket .
Errors should be logged to stderr always regardless of logger failed or not

@GowthamShanmugam
Copy link
Contributor Author

@nthomas-redhat i will change as per your suggestion

@@ -0,0 +1,308 @@
= Generic message mechanism for logs and alerts in Tendrl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the " at the end.


* Store the operation details in etcd:
```
1. Operation messages with inorder_id
Copy link
Contributor

Choose a reason for hiding this comment

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

The etcd message directory locations are missing. Add them.

For job specific updates, the following structure is expected:

  • /queue/<job_id> where job_id is the UUID of the job with no prefixes or suffixes.
  • /queue/<job_id>/parameters containing all the parameters for the job.
  • /queue/<job_id>/messages containing the sequentially generated keys, each containing a single message pertaining to the job.

In addition to these, there needs to be a specific file that updates with the job's overall status and marks the job as finished, either successfully or with errors, with the error details etc. @anivargi your inputs are needed here. The concern is to avoid multiple queries per poll for a job, if possible.

Copy link
Contributor Author

@GowthamShanmugam GowthamShanmugam Feb 22, 2017

Choose a reason for hiding this comment

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

@brainfunked job_key is not available for flows, i had discuss with @shtripat and @nnDarshan they said when the job is received by component update parameter with job_key.
Because no need to change signature of any function for call job_key separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brainfunked
My understanding is all the parameter of jobs are stored under a non-sequential key i.e
/queue/<job_id>/parameters, this is done by api during job submission. i have to this with @anivargi help.

Message handler module should place the job updates under /queue/<job_id>/messages/ for particular job.

And /queue/<job_id>/finished contains job status like job finished with error, errors, etc...

is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

@GowthamShanmugam
Here is how it needs to be
/queue/<job_id> will be a directory
/queue/<job_id>/status is the status of the current job i.e new, processing, finished, error
/queue/<job_id>/errors is the errors caused in the job if the status is error.
/queue/<job_id>/parameters is the parameters of the job which is queued.
/queue/<job_id>/messages will be a directory containing all the messages.
/queue/<job_id>/messages/sequence will be the sequence number and value will be the message string

Other possible attributes under /queue/<job_id>/.. could be integration_id, run, flow, type, node_ids.

Hope its clear now.
//@brainfunked

Copy link
Contributor

Choose a reason for hiding this comment

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

@anivargi where would the "run" , "node_ids" etc items go as per this new structure?

Copy link
Contributor

@anivargi anivargi Feb 28, 2017

Choose a reason for hiding this comment

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

@r0h4n the run would come in as /queue/<job_id>/run and node_ids will come in as /queue/<job_id>/node_ids. Note that node_ids would be a comma separated value since we cannot represent an array in etcd value like we do in json today.

tendrl-bug-id: Tendrl#28

Signed-off-by: GowthamShanmugam <gshanmug@redhat.com>
@GowthamShanmugam
Copy link
Contributor Author

@brainfunked @r0h4n @shtripat Spec is updated

@anivargi
Copy link
Contributor

anivargi commented Mar 2, 2017

@GowthamShanmugam slight changes are going to be have to made as follows:

/queue/:job_id will be a directory
/queue/:job_id/payload will be the json dump of the job with the same structure we have today
/queue/:job_id/status will be the global status which needs to be updated as per the currentstatus of the job, default will be 'new'
/queue/:job_id/errors will be a message if the status is error
/queue/:job_id/messages will be the log messages related to the job.

The request_id is not need anymore for the API to know about the job messages.
CC: @r0h4n @brainfunked

@anivargi
Copy link
Contributor

anivargi commented Mar 2, 2017

@brainfunked @GowthamShanmugam I would also like to recommend if we could put the messages under something like /messages/jobs/:job_id, would that be a option?

@anivargi
Copy link
Contributor

anivargi commented Mar 2, 2017

@GowthamShanmugam We need to move the messages per job under /messages/jobs

/messages/jobs/:job_id will be a directory
/messages/jobs/:job_id/:sequence will have value of the message

Other non job related messages will reside under
/messages/events

CC: @brainfunked

@GowthamShanmugam
Copy link
Contributor Author

@brainfunked @anivargi @r0h4n as per above discussion i have modified job structure https://github.com/Tendrl/commons/pull/175/files

@r0h4n
Copy link
Contributor

r0h4n commented Mar 7, 2017

@anivargi I think we have missed the "parent_id" field in this job structure

@GowthamShanmugam
Copy link
Contributor Author

@r0h4n @anivargi as per current job structure parent id also comes under payload

@GowthamShanmugam
Copy link
Contributor Author

GowthamShanmugam commented Mar 16, 2017

@anivargi @brainfunked
Messages are stored in different places

  1. If cluster_id present
    /clusters/cluster_id/Messages
    else
    /nodes/node_id/Messages
  2. If job_id present
    stored in /Messages/jobs
    also /Messages/events
    else:
    only /Messages/events
    is this ok (or) job_updates should be stored in /Messages/jobs only not in /Messages/events, /clusters/cluster_id/Messages or /nodes/node_id/Messages.

which is correct?

@r0h4n r0h4n merged commit ca1bb5c into Tendrl:master Apr 28, 2017
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.

None yet