HDDS-10742. Add option to close all pipelines

[Montura]

What changes were proposed in this pull request?

For now ozone CLI allows to get pipelines list or close pipeline by PipelineId.

Suggested a new option --all for ClosePipelineSubcommand to close all pipelines (or some filtered pipeline subset):

1. get pipeline list from ScmClient
2. filter CLOSED pipelines (to close only OPEN and ALLOCATED)
3. filter pipelines by REPLICATION or REPLICATION_TYPE or REPLICATION_FACTOR (as in ListPipelinesSubcommand done)
4. close the remain pipelines set

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10742

How was this patch tested?

This subcommand triggers ClosePipelineSubcommand that is already tested.

[adoroszlai]

Thanks @Montura for the patch.

[adoroszlai]

Thanks @Montura for updating the patch.

[adoroszlai]

Thanks @Montura for continuing work on this. Seems to work well. I'll wait for @errose28 to take another look, too.

[errose28]

Overall LGTM, just a few minor things:

• A command like ozone admin pipeline close --replication=EC 1 will close pipeline 1 even if it is a Ratis pipeline.
  • It would be good to disallow the pipeline filter options when ID is specified, but I'm not sure how easy that is to do with pico CLI.
  • This is a minor details so if it's not easy to do it is ok as is.
• It would be nice to print out how many pipeline close commands were sent when the command finishes. Maybe a message like Sending close command for 5 pipelines or something like that.

[Montura]

• A command like ozone admin pipeline close --replication=EC 1 will close pipeline 1 even if it is a Ratis pipeline.

  • It would be good to disallow the pipeline filter options when ID is specified, but I'm not sure how easy that is to do with pico CLI.

• Before my changes ozone admin pipeline close PIPELINE_ID was working only for specified PIPELINE_ID, so when user provides PIPELINE_ID he knows it's replicataion factor and replication type. There were no filters for this command.
• Filters for replication type and factor appiled only for ozone admin pipeline list --filters command.
• I thought that it would be nice to add the same filter options when run ozone admin pipeline close --all to be able to close only EC or only RATIS pipelines for example.

[Montura]

• Before my changes ozone admin pipeline close PIPELINE_ID was working only for specified PIPELINE_ID, so when user provides PIPELINE_ID he knows it's replicataion factor and replication type. There were no filters for this command.

I realized, @errose28 means that you call ozone admin close pipeline 1 --replication=EC and filter is ignored...

Maybe a message like When close pipeline by ID filters are ignored. will solve this issue for a while?

• CommandLine.ArgGroup doesn't support CommandLine.Mixin for know. I tried to add filters only for a new option --all. I'l look into int deeper

[errose28]

Thanks for the continued updates @Montura.

[errose28]

Sorry if the last comment was not clear. We should also fail the command here instead of closing the pipeline anyways since the user did something invalid. We can probably throw IllegalArgumentException like FilterPipelineOptions#getReplicationFilter does.

[Montura]

@errose28, let's sync :)

1. We can log an error and don't close pipeline (if/else block): I have to fix current behaviour with else block.

if (filterOptions.getReplicationFilter().isPresent()) {
  System.err.println("Replication filters can only be used with --all");
} else {
  scmClient.closePipeline(...);
}

2. We can log an error and throw and exception (if block), like org.apache.hadoop.hdds.scm.cli.cert.InfoSubcommand does: I have to fix current behaviour with throw an exception.

if (filterOptions.getReplicationFilter().isPresent()) {
  System.err.println("Replication filters can only be used with --all");
  throw new IllegalArgumentException("Replication filters can only be used with --all");
}
scmClient.closePipeline(...);

3. We can just throw and exception (if block), like org.apache.hadoop.hdds.scm.cli.pipeline.ListPipelineSubcommand does : I have to fix current behaviour with throw an execption instead of log to System.err.

if (filterOptions.getReplicationFilter().isPresent()) {
  throw new IllegalArgumentException("Replication filters can only be used with --all");
}
scmClient.closePipeline(...);

What is the best one (or what do you expect) ?

[Montura]

@errose28, could you comment please the previous message

[errose28]

I just tried all three of these to double check and option 3 is what we want. There is logic farther up to print just the message to stderr and not the stack trace. Option 2 will print the message twice. Option 1 will exit 0 when it should exit nonzero.

Just looking at the example you provided I think cert.InfoSubcommand might print the message twice as well, but I did not try it out.

[errose28]

Hi @Montura apologies for the delay. It's late night here currently but I will check this out tomorrow. Thanks for outlining the options.

[errose28]

Just tried out the suggestions. Once this is done I think we are good to merge 👍

[Montura]

Done!

[errose28]

Thanks for the updates @Montura. I just retriggered the unrelated recon test failure and once its green we can merge.

[adoroszlai]

Thanks @Montura for the patch, @errose28 for the review.