Instant Invalidate for ORT and Traffic Ops #299
Conversation
traffic_ops/app/lib/UI/Server.pm
Outdated
| my $host_name = $self->param("host_name"); | ||
| print STDERR Dumper($updated); |
There was a problem hiding this comment.
Should take these out, yea?
traffic_ops/app/lib/UI/Server.pm
Outdated
| my $use_reval_pending = $self->db->resultset('Parameter')->search( { -and => [ 'name' => 'use_reval_pending', 'config_file' => 'global' ] } )->get_column('value')->single; | ||
|
|
||
| if ( defined($use_reval_pending) && $use_reval_pending == 1 ) { | ||
| print STDERR "updating upd_pending and reval_pending\n"; |
There was a problem hiding this comment.
Definitely. Must have missed those.
dg4prez
force-pushed
the
regex_reval
branch
3 times, most recently
from
46a00c5
to
0518b91
Compare
dg4prez
changed the title
dg4prez
changed the title
dg4prez
force-pushed
the
regex_reval
branch
2 times, most recently
from
4a35771
to
a683daf
Compare
dg4prez
changed the title
dg4prez
force-pushed
the
regex_reval
branch
3 times, most recently
from
0332016
to
57fb7a8
Compare
| @@ -0,0 +1,26 @@ | |||
| /* | |||
|
|
|||
There was a problem hiding this comment.
this migration file needs a later timestamp on the name. see existing migrations - https://github.com/apache/incubator-trafficcontrol/tree/master/traffic_ops/app/db/migrations
it needs to be timestamped later than the add_client_steering migration...
| @@ -76,7 +76,9 @@ sub get_config_metadata { | |||
| $data_obj->{'info'}->{'cdn_name'} = $cdn_name; | |||
| $data_obj->{'info'}->{'cdn_id'} = $server->cdn->id; | |||
| $data_obj->{'info'}->{'tm_url'} = $tm_url; | |||
| $data_obj->{'info'}->{'tm_cache_url'} = $tm_cache_url; | |||
| if ( $tm_cache_url ) { | |||
There was a problem hiding this comment.
should this be called to_cache_url instead? it would be nice to get rid of all references to 'tm' at some point if possible...
| &log( $self, "Set upd_pending = 1 for all applicable caches", "OPER" ); | ||
| my $use_reval_pending = $self->db->resultset('Parameter')->search( { -and => [ 'name' => 'use_reval_pending', 'config_file' => 'global' ] } )->get_column('value')->single; | ||
|
|
||
| if ( defined($use_reval_pending) && $use_reval_pending == 1 ) { |
There was a problem hiding this comment.
isn't it
&& $use_reval_pending == '1'
but i dunno. maybe perl is smart enough to figure it out...
traffic_ops/app/lib/UI/Server.pm
Outdated
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| while ( my $row = $rs_servers->next ) { | ||
| if ( $parent_pending{ $row->host_name } ) { | ||
| if ( $parent_pending{ $row->host_name } && $parent_reval_pending{ $row->host_name } ) { |
There was a problem hiding this comment.
can't we collapse this if/elseif...seems like the only thing that is different is the
parent_pending => 0|1,
parent_reval_pending => 0|1
maybe a ternary operator?
parent_pending => (condition) ? \1 : \0,
parent_reval_pending => (condition) ? \1 : \0,
traffic_ops/app/lib/UI/Server.pm
Outdated
|
|
||
| my $use_reval_pending = $self->db->resultset('Parameter')->search( { -and => [ 'name' => 'use_reval_pending', 'config_file' => 'global' ] } )->get_column('value')->single; | ||
|
|
||
| if ( defined($use_reval_pending) && $use_reval_pending == 1 ) { |
There was a problem hiding this comment.
you can save a few lines of code here by getting rid of the else and moving
$update_server->update( { upd_pending => $updated } );
below the remaining if...and also leave it out of the if...
If that didn't make sense. basically i'm saying no need to call $update_server->update( { upd_pending => $updated } ); twice
There was a problem hiding this comment.
Adjusted per discussion.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
|
|
||
| #### If this is a syncds run, check to see if we can bail. | ||
| my $syncds_update = 0; | ||
| if ( $script_mode == $REVALIDATE ) { |
There was a problem hiding this comment.
This seems like it can use some cleanup. Can you do the check to see if $traffic_ops_host is defined first and then check the script mode?
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| @@ -213,7 +250,7 @@ | |||
| &touch_file('remap.config'); | |||
| last; | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I think we should use wood for the bike shed.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| @@ -226,6 +263,7 @@ | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
is this new line necessary?
There was a problem hiding this comment.
Maybe we could use metal for the roof of the bikeshed.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| @@ -262,6 +316,7 @@ sub usage { | |||
| print "\t<Mode> = report - prints config differences and exits.\n"; | |||
| print "\t<Mode> = badass - attempts to fix all config differences that it can.\n"; | |||
| print "\t<Mode> = syncds - syncs delivery services with what is configured in Traffic Ops.\n"; | |||
| print "\t<Mode> = revalidate - checks for updated revalidations in Traffic Ops and applies them. Requires Traffic Ops 1.2.\n"; | |||
There was a problem hiding this comment.
requires traffic ops 2.1 not 1.2
| ( $log_level >> $DEBUG ) && print "DEBUG Setting update flag in Traffic Ops to $status.\n"; | ||
|
|
||
| my %headers = ( 'Cookie' => $cookie ); | ||
| my $response = $lwp_conn->post( $url, [ 'updated' => $status ], %headers ); | ||
| my $url = $traffic_ops_host . $uri; | ||
| my $response = $lwp_conn->post( $url, [ 'updated' => $status, 'reval_updated' => $reval_status ], %headers ); |
There was a problem hiding this comment.
is it ok if reval_status is null here? Didnt trace the code, I just assume that can happen.
There was a problem hiding this comment.
Yes. Traffic ops only looks for it if use_reval_pending is enabled, and reval_status won't be used if that's the case.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| ( $dispersion > 0 ) && &sleep_timer($dispersion); | ||
| } | ||
|
|
||
| ( $log_level >> $WARN ) && print "\n"; |
There was a problem hiding this comment.
Is this meant to just print a carriage return?
There was a problem hiding this comment.
I'm a really big fan of carriage returns for some reason.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| @@ -759,9 +967,9 @@ sub check_syncds_state { | |||
| ( $log_level >> $ERROR ) && print "ERROR Traffic Ops is signaling that no update is waiting to be applied.\n"; | |||
| } | |||
|
|
|||
| my $stj = &lwp_get("$traffic_ops_host\/datastatus"); | |||
| my $stj = &lwp_get("/datastatus"); | |||
There was a problem hiding this comment.
again, we should use the API endpoint here
| for ( my $i = $duration; $i > 0; $i-- ) { | ||
| ( $log_level >> $WARN ) && print "."; | ||
| sleep 1; | ||
| $reval_clock--; | ||
| if ($reval_clock < 1 && $script_mode != $BADASS && $reval_in_use == 1 ) { | ||
| ( $log_level >> $WARN ) && print "\n"; |
There was a problem hiding this comment.
again, do we mean to just print a new line?
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| @@ -1481,8 +1783,8 @@ sub get_header_comment { | |||
| my $to_host = shift; | |||
| my $toolname; | |||
|
|
|||
| my $url = "$to_host/api/1.1/system/info.json"; | |||
| my $result = &lwp_get($url); | |||
| my $uri = "/api/1.1/system/info.json"; | |||
There was a problem hiding this comment.
can we use the 1.2 version if the API instead? Not sure if it changed at all.
traffic_ops/bin/traffic_ops_ort.pl
Outdated
|
|
||
| return if (!defined($cfg_file_tracker->{$filename}->{'fname-in-TO'})); | ||
|
|
||
| return "$traffic_ops_host\/genfiles\/view\/$hostname_short\/" . $cfg_file_tracker->{$filename}->{'fname-in-TO'}; | ||
| #return "$traffic_ops_host\/genfiles\/view\/$hostname_short\/" . $cfg_file_tracker->{$filename}->{'fname-in-TO'}; |
There was a problem hiding this comment.
do we need this commented out line or can it be removed?
There was a problem hiding this comment.
Let's get rid of it!
traffic_ops/bin/traffic_ops_ort.pl
Outdated
| } | ||
| else { | ||
| ( $syncds_update ) = &check_syncds_state(); | ||
| if ( $syncds_update < 1 ) { |
There was a problem hiding this comment.
We aren't doing anything in this if{}
|
👍 I checked ORT, looks good to me now. |
| -- SQL in section 'Up' is executed when this migration is applied | ||
| ALTER TABLE server ADD COLUMN reval_pending boolean NOT NULL DEFAULT false; | ||
| INSERT INTO parameter (name, config_file, value, secure) VALUES ('use_reval_pending', 'global', '0', False); | ||
| insert into profile_parameter (profile, parameter) values ((select id from profile where name = 'GLOBAL'), (select id from parameter where name = 'use_reval_pending')); |
There was a problem hiding this comment.
@dg4prez - so this isn't going to work. when you run db/admin.pl --env=test updgrade to migrate the to_test database, this fails because there is no GLOBAL profile in the to_test db. I'm not exactly sure what the right answer is off the top of my head to fix this but this needs to be fixed because you can't run tests in the master branch currently. Maybe @dewrich or @dangogh have some ideas...
|
Brought this up with @dg4prez the other day. IMO we should add a GLOBAL profile to the test DB via fixtures. I added it manually and it worked. |
This code change adds a revalidate command line option to ort which will only check the regex_revalidate file. This option requires the use of the reval_pending value, which is enabled by setting the new parameter "use_reval_pending" in global parameters to "1" (the default is zero.) If this value is not found, this option will gracefully quit and syncds should be used. If reval_pending is in use then revalidate will verify that no parents are pending and run. Should syncds be run, it will also verify that no parents have reval_pending flagged in addition to the normal "upd_pending" flag, preventing revalidation in incorrect order.
Because this only checks and modifies the regex_revalidate.config file, this can be run via ort more frequently than normal syncds flags.
Edit: Added a feature by which ORT will check for revalidation files prior to starting a dispersion wait period, and then every sixty seconds (if the dispersion is long enough) thereafter. In this way we can maintain full coverage of invalidation at a rate of 1 pass per minute for the entire hour.
ORT has also been updated to support the new config files API.