ncm-ceph: fix incorrect paths for 'ls' and 'cat' commands on el6 #324
Conversation
|
Test FAILed. |
kwaegema
changed the title
| @@ -34,6 +34,8 @@ use Sys::Hostname; | |||
| our $EC=LC::Exception::Context->new->will_store_all; | |||
| Readonly my $OSDBASE => qw(/var/lib/ceph/osd/); | |||
| Readonly my $JOURNALBASE => qw(/var/lib/ceph/log/); | |||
| Readonly::Array our @LS_COMMAND => ('/bin/ls'); | |||
| Readonly::Array our @CAT_COMMAND => ('/bin/cat'); | |||
|
Test FAILed. |
| @@ -44,7 +44,7 @@ Readonly my $CRUSH_TT_FILE => 'ceph/crush.tt'; | |||
| # Get the osd name from the host and path | |||
| sub get_osd_name { | |||
| my ($self, $host, $location) = @_; | |||
| my $id = $self->run_command_as_ceph_with_ssh(['/usr/bin/cat', "$location/whoami"], $host) or return 0; | |||
| my $id = $self->run_cat_command_as_ceph_with_ssh(["$location/whoami"], $host) or return 0; | |||
There was a problem hiding this comment.
Actually, I prefer the previous version.
Just
my $id = $self->run_command_as_ceph_with_ssh([@CAT_COMMAND, "$location/whoami"], $host) or return 0;The other method doesn't give any benefit.
There was a problem hiding this comment.
the issue is that we have 2 "modules" that then define the same readonly.
There was a problem hiding this comment.
Why?
I see command management is done in NCM::Component::Ceph::commands. It should export whatever constants it needs, maybe with a tag, just as it does for @SSH_COMMAND.
Say:
package NCM::Component::Ceph::commands;
use base qw(Exporter);
use Readonly;
Readonly::Array our @CAT_COMMAND => ...;
our %EXPORT_TAGS = (commands => [qw(@CAT_COMMAND @SSH_COMMAND...)];And in the consumers:
use NCM::Component::Ceph::Commands qw(:commands);
# Use @SSH_COMMAND and @CAT_COMMAND at will.There was a problem hiding this comment.
yes, we were considering this before for the SSH_COMMAND some time ago. But the issue still stays, that , in our main ceph.pm, we do:
use base qw(NCM::Component NCM::Component::Ceph::commands NCM::Component::Ceph::crushmap NCM::Component::Ceph::daemon NCM::Component::Ceph::config NCM::Component::Ceph::compare );
This way, we duplicate the includes 😄
|
retest this please |
|
Test PASSed. |
|
Could you improve the title of this PR please, it will end up in the release notes and this one isn't clear and looks a bit silly. |
kwaegema
changed the title
| '/usr/bin/ssh', '-o', 'ControlMaster=auto', | ||
| '-o', 'ControlPersist=600', '-o', 'ControlPath=/tmp/ssh_mux_%h_%p_%r' | ||
| ); | ||
|
|
||
| Readonly::Array my @CAT_COMMAND => ('/bin/cat'); |
There was a problem hiding this comment.
I'll merge as is. Address the qw requirement in the other pull request.
|
Test PASSed. |
|
@kwaegema after merging other PRs this one won't merge cleanly anymore. Please, rebase or resolve the conflicts and it's good to merge. |
|
Test PASSed. |
|
LGTM |
|
@Piojo rebase done |
ncm-ceph: fix incorrect paths for 'ls' and 'cat' commands on el6
No description provided.