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

Component: support redirection #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
142 changes: 138 additions & 4 deletions src/main/perl/Component.pm
Expand Up @@ -5,6 +5,7 @@ use LC::Sysinfo;
use CAF::History qw($IDX);
use CAF::Reporter qw($HISTORY);
use EDG::WP4::CCM::Path 16.8.0;
use Module::Load;
use parent qw(Exporter CAF::Object);

our ($this_app, @EXPORT, $NoAction, $SYSNAME, $SYSVERS);
Expand Down Expand Up @@ -311,8 +312,7 @@ sub Configure
{
my ($self, $config) = @_;

$self->error('Configure() method not implemented by component');
return;
return $self->_redirect("Configure", $config);
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? Previously the code was only called if the subclass didn't override the Configure() method. So how does a module author use this new facility?

Copy link
Member Author

Choose a reason for hiding this comment

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

if no redirect is defined, it still reports an error as before. that behaviour has not changed.
i'm sure i fully understand what you want to know. you can use the new facility by setting the REDIRECT variable. i can clarify the pod below if needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ned21 ned21 Jan 22, 2018

Choose a reason for hiding this comment

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

Thanks for the example, but that doesn't really help! (Or rather it helps a lot in understanding the intent of this code.) It seems very unintuitive when reading the code that the parameters in a Readonly hash relate to a profile attribute that then select a file from a subdirectory which is not even named the same as the file. (Ceph/ v ceph). It's much less code but a significantly higher barrier to anyone approaching the code to figure out what's happening.

In particular using a hash to ultimately invoke a method is unintuitive and breaks the OO concept of inheritance. I think we should keep to using inheritance or other well-understood OO concepts. e.g.

# This component usesa sub-module to select the right code to run
sub Configure {
  my ($self, $config) = @_;
  my $submod = $config->getTree("/path/to/module");
  $submod = "Luminous" unless defined $submod;
  return $self->redirect($submod);  # redirect is a method in the super class.

feels more naturally OO for not significantly more code.

Copy link
Member Author

Choose a reason for hiding this comment

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

this has been a while, but i don't agree with your view

in your example, the Luminous and /path/to/module are proper constants and as such should be Readonly anyway; so you end up with code like

Readonly $PATH => '/path/to/module';
Readonly $DEFAULT => 'Luminous';

sub Configure {
my ($self, $config) = @_;
  my $submod = $config->getTree($PATH);
  $submod = $DEFAULT unless defined $submod;
  return $self->redirect($submod);  # redirect is a method in the super class.
}

now, what is so OO about having to copy the Configure method each time? (and for completeness, the example is missing a bunch of error handling and with fixed $PATH doesn't allow aliasing, so the full code block to copy/paste will be even bigger)
your other remarks wrt "profile attribute" and naming, that is legacy quattor code. there are no standards, i would definitley prefer not haing to do this and just use the same for all, but for some reaon our base component classes have lower case names, and i can hardly use packager as an attribute name in ceph to point out a certain version/flavour of the software.

maybe if we can settle on a proper attribute name and it's ok to have components start with uppercase, i can make the code do its magic based on single $REDIRECT_DEFAULT reaonly instead of a hashref?

Copy link
Contributor

@ned21 ned21 Oct 29, 2018

Choose a reason for hiding this comment

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

My issue is with this being completely a data definition:

Readonly our $REDIRECT => {
    name => 'release',
    default => 'Luminous',
};

If I am trying to follow this code I am at a dead end. It's necessary to grep the code base for where $REDIRECT is used and then understand the abstraction. Compare this to my example, I can see instantly (because I am familiar with the standard OO abstraction) that I need to check the definition of the redirect function in the super-class.

}

=item Unconfigure($config): boolean
Expand All @@ -326,8 +326,7 @@ sub Unconfigure
{
my ($self, $config) = @_;

$self->error('Unconfigure() method not implemented by component');
return;
return $self->_redirect("Unconfigure", $config);
}


Expand Down Expand Up @@ -391,6 +390,141 @@ sub _initialize
return SUCCESS;
}

=item _redirect

Support component redirection: allow a component to switch
to a child module based on configuration information.

The component needs a C<REDIRECT> readonly hashref with keys

=over

=item name (mandatory): element name that will be used to lookup
the child module name in the component configuration. If no value is found,
and no default is defined, an error is reported and C<_redirect> returns.

=item default (optional): default child module value

=item lower (optional): when true, no capitalisation of the
component namespace when generating the child module package name.
(This does not affect the child module name.)

=back

Caveats

=over

=item NoActionSupported: this is declared by the (parent) component
and applies for all redirections.

=item C<LC::Exception> context (C<EC>): this is declared
by the (parent) component, be careful when defining another one in the
redirections.

=back

Example spma component has 'packager' configuration to switch between
C<yum>, C<yumng>, C<ips> and C<apt>.

The simplified implementation is as follows:

The C<spma> component package contains
package NCM::Component::spma;
...
use parent qw(NCM::Component);
use Readonly;
Readonly our $REDIRECT => {
name => 'packager',
lower => 1,
default => 'yum',
}

This means that the module to use is based on the value
of the C<packager> element in the component tree (based on
C<prefix()>; in this case C</software/components/spma/packager>).
In none is defined, the default C<yum> value is used.

The actual module that is loaded from namespace based on the package name,
with the packagename capitalised.
In this example, by default this would be in C<NCM::Component::Spma>.
However, with the C<lower> REDIRECT value true, the capitalisation is
not performed and the resulting package is C<NCM::Component::spma::yum>.

=cut

sub _redirect
{
my ($self, $method, $config) = @_;

my $whoami = ref($self);

my $redirect;

# TODO: prevent loop?
local $@;
eval {
my $varname = $whoami."::REDIRECT";
no strict 'refs';
$redirect = ${$varname};
use strict 'refs';
};
if ($@) {
$self->verbose("No REDIRECT defined for $whoami: $@");
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this get printed? It's only verbose not warn or error so can't be too bad but what are the consequences?

Copy link
Member Author

Choose a reason for hiding this comment

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

it get printed if you do not override Configure (so you forgot or you need REDIRECT, in which case you don't need to override Configure). sometimes you think you did set REDIRECT, but it fails, and then it is useful to know what went wrong (well, imho).

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like an artefact of using an unnatural indirection via a hash, which I think would be avoided by the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is some limitation of perl OO. in python this would be

class Magic(object):
    REDIRECT = None
    def moremagic(self):
        is self.REDIRECT is not None:
            return do_something(self.REDIRECT)

class Magic2(Magic):
    REDIRECT = {something else}

but you can't do this easily in perl

}

if ($redirect) {
my $elname = $redirect->{name};
if (!defined($elname)) {
$self->error("Invalid REDIRECT for $whoami: missing name");
return;
}

# redirect current component
# based on spma.pm code call_entry_point
my $tree = $config->getTree($self->prefix());
my $childname = $tree->{$elname} || $redirect->{default};
if (!$childname) {
$self->error("No REDIRECT childname for $whoami found: ",
"no name config (element $elname) and no REDIRECT default set");
return;
}

my @packagename = split(/::/, $whoami);
$packagename[-1] = ucfirst($packagename[-1]) if ! $redirect->{lower};
push(@packagename, $childname);

my $childpackagename = join('::', @packagename);

local $@;
my @warns;
eval {
local $SIG{__WARN__} = sub { push(@warns, $_[0]); };
load $childpackagename;
};
if ($@) {
$self->error("REDIRECT bad Perl code in $childpackagename: $@");
return;
}

# no real point in reporting the warnings on error
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. You say there's no real point doing this but do it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll check again. i copied this code from somewhere else in ncm-ncd

Copy link
Member Author

Choose a reason for hiding this comment

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

the remark is wrt reporting the warnings also in the previous error block. it won't add much and i can't just put this block before the error block, as it might mess up $@. so you only see the warnings when there's no error.

# and we must be sure that $@ does not get redefined during $self->warn
foreach my $warn (@warns) {
$self->warn("REDIRECT warning during loading of package $childpackagename: $warn");
}

$self->verbose("Redirecting to $childname (package $childpackagename)");
bless($self, $childpackagename);

return $self->$method($config);
} else {
# Default: nothing works
$self->error("$method() method not implemented by component");
return;
}
}


=back

=head1 Legacy methods
Expand Down
12 changes: 12 additions & 0 deletions src/test/perl/NCM/Component/Component1/Regular.pm
@@ -0,0 +1,12 @@
package NCM::Component::Component1::Regular;

use strict;
use warnings;
use parent qw(NCM::Component::component1);

sub Configure
{
return __PACKAGE__." Configure";
}

1;
13 changes: 13 additions & 0 deletions src/test/perl/NCM/Component/Component1/Subby.pm
@@ -0,0 +1,13 @@
package NCM::Component::Component1::Subby;

use strict;
use warnings;
use parent qw(NCM::Component::component1);


sub Unconfigure
{
return __PACKAGE__." Unconfigure";
}

1;
13 changes: 13 additions & 0 deletions src/test/perl/NCM/Component/component1.pm
@@ -0,0 +1,13 @@
package NCM::Component::component1;

use strict;
use warnings;

use parent qw(NCM::Component);
use Readonly;
Readonly our $REDIRECT => {
name => 'otherone',
default => 'Regular',
};

1;
42 changes: 39 additions & 3 deletions src/test/perl/component.t
Expand Up @@ -8,7 +8,7 @@ BEGIN {
}

use Test::More;
use Test::Quattor qw(component1 component-fqdn);
use Test::Quattor qw(component1 component-fqdn component1_redirect component1_redirect_none);
use Test::Quattor::Object;

# insert the this_app before load but after Test::Quattor
Expand All @@ -20,7 +20,7 @@ BEGIN {
}

use NCM::Component;

use NCM::Component::component1;

=head1 NoAction is set on load via this_app

Expand All @@ -33,7 +33,7 @@ my $obj = Test::Quattor::Object->new();
=head1 test NCM::Component init

=cut

my $cfg = get_config_for_profile('component1');
my $cmp1 = NCM::Component->new('component1', $obj);
isa_ok($cmp1, 'NCM::Component', 'NCM::Component instance 1 created');
is($cmp1->prefix(), "/software/components/component1", "prefix for component1");
Expand Down Expand Up @@ -101,5 +101,41 @@ my $realhostname = "something.else.example.org";
is($cmp2->get_tree("/system/network/realhostname"), $realhostname, "realhostname set");
is($cmp2->get_fqdn(), $realhostname, "fqdn from realhostname");

=head1 Configure / Unconfigure

=cut

ok(!defined($cmp1->Configure($cfg)), "NCM::Component returns undef (not implemented)");
is($obj->{LOGLATEST}->{ERROR}, 'Configure() method not implemented by component', 'Configure not implemented error');

ok(!defined($cmp1->Unconfigure($cfg)), "NCM::Component returns undef (not implemented)");
is($obj->{LOGLATEST}->{ERROR}, 'Unconfigure() method not implemented by component', 'Unconfigure not implemented error');

=head1 redirect

=cut

my $cfgr = get_config_for_profile('component1_redirect');

$cmp1 = NCM::Component::component1->new('component1', $obj);
isa_ok($cmp1, 'NCM::Component::component1', 'is a NCM::Component::component1');
is($cmp1->Configure($cfg), 'NCM::Component::Component1::Regular Configure', 'Redirect to default Regular');
isa_ok($cmp1, 'NCM::Component::Component1::Regular', 'is now a NCM::Component::Component1::Regular');
ok(!defined($cmp1->Unconfigure($cfg)), 'Redirect to default Regular has no Unconfigure');


$cmp1 = NCM::Component::component1->new('component1', $obj);
isa_ok($cmp1, 'NCM::Component::component1', 'is a NCM::Component::component1');
ok(!defined($cmp1->Configure($cfgr)), 'Redirect to name Subby has no Configure');
isa_ok($cmp1, 'NCM::Component::Component1::Subby', 'is now a NCM::Component::Component1::Subby');
is($cmp1->Unconfigure($cfgr), 'NCM::Component::Component1::Subby Unconfigure', 'Redirect to name Subby');

my $cfgn = get_config_for_profile('component1_redirect_none');
$cmp1 = NCM::Component::component1->new('component1', $obj);
ok(!defined($cmp1->Configure($cfgn)), "NCM::Component returns undef (redirect does not exist)");
like($obj->{LOGLATEST}->{ERROR},
qr{REDIRECT bad Perl code in NCM::Component::Component1::DoesNotExist: Can't locate NCM/Component/Component1/DoesNotExist.pm in \@INC},
'redirect does not exist error');


done_testing;
6 changes: 6 additions & 0 deletions src/test/resources/component1_redirect.pan
@@ -0,0 +1,6 @@
object template component1_redirect;


prefix "/software/components/component1";
"active" = true;
"otherone" = "Subby";
6 changes: 6 additions & 0 deletions src/test/resources/component1_redirect_none.pan
@@ -0,0 +1,6 @@
object template component1_redirect_none;


prefix "/software/components/component1";
"active" = true;
"otherone" = "DoesNotExist";