Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2137 closed enhancement (fixed)

update the boss module according to the new statistics model

Reported by: naokikambe Owned by: naokikambe
Priority: medium Milestone: Sprint-20120904
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 3 Internal?: no

Description (last modified by naokikambe)

update the boss module according to the new model of #2135

[Additional Description]

  • Change the format of statistics data

Change the boss module so that it returns the format of statistics data which the stats module revised in #2136 can receive.

  • Remove the sendstats command

Remove sendstats command because the boss module cannot actively send statistics data to the stats module after #2136.

  • Update the manpages

Update the mappages of the boss module according to the above changes.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by naokikambe

  • Owner set to naokikambe
  • Status changed from new to accepted

comment:2 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

The branch is ready for reviewing. The changes are small. This ticket depends on #2135 and #2136. A changelog entry will be included in #2136.

comment:3 Changed 7 years ago by shane

  • Milestone changed from New Tasks to StatsRedesign

comment:4 Changed 7 years ago by naokikambe

  • Description modified (diff)

comment:5 Changed 7 years ago by jelte

  • Milestone changed from StatsRedesign to Sprint-20120821

comment:6 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:7 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to naokikambe

The changes in get_stats_data() in boss make the stats daemon quit during startup. That obviously needs to be fixed :)

But also, it is probably a good idea not catch formatting errors in the stats daemon, and log an error if that happens (but to keep running)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Hello, thank you for reviewing.

Replying to jelte:

The changes in get_stats_data() in boss make the stats daemon quit during startup. That obviously needs to be fixed :)

Sorry, I couldn't understand that. Do you mean the new _get_stats_data() method can stop the stats daemon starting? Could you explain how should I fix? In the former code, boss might have accidentally stopped because of the wrong format.

But also, it is probably a good idea not catch formatting errors in the stats daemon, and log an error if that happens (but to keep running)

Do you mean boss should check the format of statistics data being sent out? I don't think the validation is always needed because the format is constant in boss. And should it log if it is invalid? The stats module logs an error when it receives the wrong format from boss.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to naokikambe

Replying to jelte:

The changes in get_stats_data() in boss make the stats daemon quit during startup. That obviously needs to be fixed :)

Sorry, I couldn't understand that. Do you mean the new _get_stats_data() method can stop the stats daemon starting? Could you explain how should I fix? In the former code, boss might have accidentally stopped because of the wrong format.

The problem is that the new _get_stats_data() makes a response message of the form
{ "boot_time": <some time> }

And the stats daemon expects the format
{ "owner": <owner>,

"data": <data>

}

So currently, upon startup of bind10, the stats daemon immediately crashes:

2012-08-13 15:37:04.438 INFO  [b10-stats.stats] STATS_STARTING starting
Traceback (most recent call last):
  File "/home/jelte/repos/bind10/src/bin/stats/b10-stats", line 503, in <module>
    stats.start()
  File "/home/jelte/repos/bind10/src/bin/stats/b10-stats", line 166, in start
    args["owner"], **args["data"])
KeyError: 'owner'
2012-08-13 15:37:04.446 INFO  [b10-boss.boss] BIND10_PROCESS_ENDED process 18047 of b10-stats ended with status 256

But also, it is probably a good idea not catch formatting errors in the stats daemon, and log an error if that happens (but to keep running)

Do you mean boss should check the format of statistics data being sent out? I don't think the validation is always needed because the format is constant in boss. And should it log if it is invalid? The stats module logs an error when it receives the wrong format from boss.

Oh, I don't think the boss should necessarily check its output, but I'd check in stats.py.in whether the 'args' value does indeed contain "owner" and "data" before continuing (and log an error otherwise);
e.g. something like either

    if "owner" in args and "data" in args:
        <process>
    else:
        logger.error(blabla)

or

    if "owner" not in args or "data" not in args:
        raise StatsError("Badly formatted response to get_stats, owner or data missing")

but in the latter case, I'd make a raised statserror not fatal (but catch it, log it as an error, and continue to run)


comment:10 in reply to: ↑ 9 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to jelte

Hello, thank you for the good explanation.

Since #2136, #2137, and #2138, the protocol between the stats module and other target module is widely changed. Could you refer to #2135 and the wikipage StatsModule for details of this wide change?

In the new codes, the stats module asks each module to send back statistics data. The target module can return only statistics data which were contents of the data argument before.

Since the stats module knows the name of the target module, the target module doesn't need to inform its name to the stats module, that is, the owner argument wasn't needed. But only in the boss case, this owner actually made no sense before, and the stats module required it.

In #2136, the stats module checks the format of statistics data returned from the target module by using the method isc.conf.module_spec.validate_statistics(). If it fails, the stats module logs an error and continues to run.

All fixes to the stats module for this change will be done on #2136. For looking the both behaviors of the new stats module and the new boss, merging trac2136 into trac2137 is needed. #2136 is going on reviewing, but I've now merged trac2136 into trac2137 and pushed into the remote branch. Could you review that codes again?:-)

comment:11 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to naokikambe

In the future, if a ticket uses changes from a branch that is not done yet, it is probably easier to branch the new ticket from the branch it depends on (i.e. in this case, create the branch trac2137 from the branch for trac2136, instead of from master); so that the branch itself works, even if the original branch is changed later, git should be able to handle it (as long as you don't rebase pushed branches, or force updates to origin). Just be sure not to merge it to master before the original branch gets merged (since that would merge both at once).

Anyway, I now only see 1 commit related to 2137, which removes the sendstats command and revives the getstats command, is that correct? That change looks fine, and I am assuming the other changes are addressed in the review of 2136.

So this branch looks ok, and can be merged once 2136 is :)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by naokikambe

Replying to jelte:

Anyway, I now only see 1 commit related to 2137, which removes the sendstats command and revives the getstats command, is that correct?

Yes, that's correct.

So this branch looks ok, and can be merged once 2136 is :)

Thank you! If reviewing #2136 is done, I will merge trac2136 into the master and then merge this branch(trac2137). I will close this ticket after that.

comment:13 in reply to: ↑ 12 Changed 7 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed

I've merged it after merging trac2136, so I'm closing the ticket.
Thanks,

comment:14 Changed 7 years ago by naokikambe

  • Total Hours changed from 0 to 3
Note: See TracTickets for help on using tickets.