[sheepdog] [PATCH v5 2/2] collie: add a new option --progress to "node recovery" for showing recovery progress

[Liu Yuan]

On Mon, Aug 05, 2013 at 10:30:24AM +0900, Hitoshi Mitake wrote:
> At Sun, 4 Aug 2013 22:51:43 +0800,
> Liu Yuan wrote:
> > 
> > On Sun, Aug 04, 2013 at 05:30:22PM +0900, Hitoshi Mitake wrote:
> > > From: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > 
> > > This patch adds a new option --progress (or -P) to the node recovery
> > > subcommand. With this subcommand, users can show a progress of
> > > recovery process.
> > > 
> > > Example:
> > > $ sudo collie node recovery --progress
> > >  99.7 % [==============================================>] 7047 / 7068
> > > 
> > > The denominator (7068 in the above case) indicates a number of entire
> > > object which should be checked. The numerator (7047 in the above case)
> > > indicates a number of objects which is already checked or copied.
> > > 
> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > ---
> > > v5:
> > >  - remove an unnecesary comment
> > >  - fix an invalid usage of recovery_state in showing 100% progress
> > > 
> > > v4:
> > >  - refactor the loop of get_recovery_state()
> > > 
> > > v3:
> > >  - make struct recovery_state a general mechanism for getting recovery
> > >    status. Ordinal "collie node recovery" uses struct recovery_state
> > >    for detecting recovery state instead of a result of request.
> > > 
> > > v2:
> > >  - don't use new variables for indicating the progress
> > >  - clean coding style
> > >  -- change names of struct recovery_info's members
> > >  -- fill_recovery_progress() -> get_recovery_progress()
> > > 
> > >  collie/node.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 86 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/collie/node.c b/collie/node.c
> > > index 4230af5..0ba14b4 100644
> > > --- a/collie/node.c
> > > +++ b/collie/node.c
> > > @@ -13,6 +13,7 @@
> > >  
> > >  static struct node_cmd_data {
> > >  	bool all_nodes;
> > > +	bool recovery_progress;
> > >  } node_cmd_data;
> > >  
> > >  static void cal_total_vdi_size(uint32_t vid, const char *name, const char *tag,
> > > @@ -120,10 +121,89 @@ static int node_info(int argc, char **argv)
> > >  	return EXIT_SUCCESS;
> > >  }
> > >  
> > > +static int get_recovery_state(struct recovery_state *state)
> > > +{
> > > +	int ret;
> > > +	struct sd_req req;
> > > +
> > > +	sd_init_req(&req, SD_OP_STAT_RECOVERY);
> > > +	req.data_length = sizeof(*state);
> > > +
> > > +	ret = collie_exec_req(sdhost, sdport, &req, state);
> > > +	if (ret < 0) {
> > > +		fprintf(stderr, "Failed to execute request\n");
> > > +		return -1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int node_recovery_progress(void)
> > > +{
> > > +	int result;
> > > +	unsigned int prev_nr_total;
> > 
> > nr_total never changed, simply name it as nr_total and we should use uint64_t
> > for it
> 
> If node leaving happens during recovery process, nr_total would be
> changed. And I think we use the last value.
> 
> > 
> > > +	struct recovery_state rstate;
> > > +
> > > +	/*
> > > +	 * ToDos
> > > +	 *
> > > +	 * 1. Calculate size of actually copied objects.
> > > +	 *    For doing this, not so trivial changes for recovery process are
> > > +	 *    required.
> > > +	 *
> > > +	 * 2. Print remaining physical time.
> > > +	 *    Even if it is not so acculate, the information is helpful for
> > > +	 *    administrators.
> > > +	 */
> > > +
> > > +	result = get_recovery_state(&rstate);
> > > +	if (result < 0)
> > > +		return EXIT_SYSFAIL;
> > > +
> > > +	if (!rstate.in_recovery)
> > > +		return EXIT_SUCCESS;
> > > +
> > > +	do {
> > > +		prev_nr_total = rstate.nr_total;
> > > +
> > > +		result = get_recovery_state(&rstate);
> > > +		if (result < 0)
> > > +			break;
> > > +
> > > +		if (!rstate.in_recovery) {
> > > +			show_progress(prev_nr_total, prev_nr_total, true);
> > > +			break;
> > > +		}
> > > +
> > > +		switch (rstate.state) {
> > > +		case RW_PREPARE_LIST:
> > > +			printf("\rpreparing a checked object list...");
> > > +			break;
> > > +		case RW_NOTIFY_COMPLETION:
> > > +			printf("\rnotifying a completion of recovery...");
> > > +			break;
> > > +		case RW_RECOVER_OBJ:
> > > +			show_progress(rstate.nr_finished, rstate.nr_total,
> > > +				true);
> > 
> > Could you fix your editor so that next line can align to left parenthesis?
> 
> If I remember correctly, this alignment based on a parenthesis is not
> in our coding style (we discussed about it before).

>From time to time, I apply patches that fixes unalignment of parameters. And
since all the code in the source files do the paramenter alignment, why not this
patch follow the convention?

Thanks
Yuan