[Issue 1907] TO API for backup edge cachegroup #2029

Vijay-1 · 2018-03-21T20:29:42Z

[Issue 1907] Backup edge cache group TO API / TR cr-config changes

#1907

Contains the following changes:
Traffic Router - Backup edge configuration parsing and decision making code changes
Traffic Ops - Mojolicious Perl CRUD APIs

This is the initial version of code. Only minimal testing has been done. Will run more cases and update. Hosting it in advance to get review comments :-)

asfgit · 2018-03-21T20:31:53Z

Can one of the admins verify this patch?

asfgit · 2018-03-21T20:34:43Z

Can one of the admins verify this patch?

dangogh · 2018-03-21T21:31:05Z

@Vijay-1 can you change the title of this PR so it's more descriptive of the actual change? A link in the title doesn't work well.. Something like this is good:
[Issue 1907] TO API for backup edge cachegroup

limited · 2018-03-22T15:42:08Z

CHANGELOG.md

@@ -34,6 +34,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
  - /api/1.3/system/info
  - /api/1.3/types (R)
 - Fair Queuing Pacing: Using the FQ Pacing Rate parameter in Delivery Services allows operators to limit the rate of individual sessions to the edge cache. This feature requires a Trafficserver RPM containing the fq_pacing experimental plugin AND setting 'fq' as the default Linux qdisc in sysctl. 
+- Backup Edge Cache group: Backup Edge group for a particular cache group can be configured using Traffic Router's cr-config.json configuration file. With this, users will be able control traffic within portions of their network, thereby avoiding choosing fallback cache groups using geo coordinates(getClosestAvailableCachegroup). This can be controlled using "backupLocations" which contains configuration for backup cache groups parsed from the cr-config.json which gets populated using the APIs detailed [here](http://traffic-control-cdn.readthedocs.io/en/latest/development/traffic_ops_api/v12/cachegroup_fallbacks.html)


Since the changelog is user visible, its better to describe the changes as configured using Traffic Ops API. You could also make the description more concise as you have more detailed documentation elsewhere.

Consider something along the lines of:

Configure a backup list of edge cache groups through the Traffic Ops API. A Backup edge cache group is used if the matched group in the CZF is not available. If the backup edge cache groups are not available, geo location can optionally be used as a further backup. See APIs here...

mitchell852

Seems to be some copy/paste errors and I think the API response structure needs to be rethought. Also, make sure you read our API guidelines for some ideas on the response structure.

mitchell852 · 2018-03-28T16:50:35Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+              "text": "Backup configuration for 51 successful."
+            }
+         ],
+       "response": {


this data structure doesn't follow the pattern currently represented in the rest of the API. Instead it should look more like this:

response: [ { "order": -1, "name" :"GROUP2" }, { "order": 1, "name" :"GROUP3" } ]

there is no need for GROUP1->fallbacks->list

it is implied from the structure of the route (GET /api/1.2/cachegroups/:id/fallbacks) that a list (array) is being returned.

+1, the extra keys will make client code unnecessarily complex.

mitchell852 · 2018-03-28T16:51:01Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+**POST /api/1.2/cachegroups/:id/fallbacks**
+
+  Create cache group.


you're not creating a cache group. can you add a description of what this route is used for?

mitchell852 · 2018-03-28T16:52:33Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+  **Request Example** ::
+
+    {
+        "fallbacks": 


I don't think there is any need for the fallbacks key.

mitchell852 · 2018-03-28T16:53:01Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+                          "text":"Backup configuration for 51 successful."
+                  }
+                  ],
+        "response":{


again, this should just be an array of "fallbacks"

mitchell852 · 2018-03-28T16:54:46Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+|
+
+**PUT /api/1.2/cachegroups/:id/fallbacks**


How is PUT different than POST?

POST should be for creating fallbacks for a cachegroup

PUT should be for updating a specific fallback so the route should be PUT /api/1.2/cachegroups/:id/fallbacks/:fb_id

mitchell852 · 2018-03-28T16:58:40Z

traffic_ops/app/lib/TrafficOpsRoutes.pm

+
+	# -- CACHEGROUP-Fallbacks: CRUD
+	$r->get("/api/$version/cachegroups/:id/fallbacks" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'CachegroupFallback#show', namespace => $namespace );
+	$r->post("/api/$version/cachegroups/:id/fallbacks" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'CachegroupFallback#create', namespace => $namespace );


what happens if i do 2 POSTs in a row. does the 2nd POST wipe out the fallbacks of the first or does it add to it?

what is the intent of PUT? does a PUT wipe out all the fallbacks and replace them?

mitchell852 · 2018-03-28T16:59:19Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+/api/1.2/cachegroups/fallbacks
++++++++++++++++++++++++++++++
+
+**GET /api/1.2/cachegroups/:id/fallbacks**


can you add a description of what this route is for?

mitchell852 · 2018-03-28T17:00:31Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+**PUT /api/1.2/cachegroups/:id/fallbacks**
+
+  Create cache group.


can you add a description of what this route is used for. It is not clear to me what a PUT does. Does it replace all existing fallbacks?

mitchell852 · 2018-03-28T17:00:55Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+**DELETE /api/1.2/cachegroups/{:id}/fallbacks**
+
+  Delete cache group. The request to delete a cache group, which has servers or child cache group, will be rejected.


Need a real description here.

mitchell852 · 2018-03-28T17:05:08Z

traffic_ops/app/lib/API/Cachegroup.pm

@@ -57,6 +57,7 @@ sub index {
 				"lastUpdated"                   => $row->last_updated,
 				"parentCachegroupId"            => $row->parent_cachegroup_id,
 				"parentCachegroupName"          => ( defined $row->parent_cachegroup_id ) ? $idnames{ $row->parent_cachegroup_id } : undef,
+				"fallback_to_closest"           => \$row->fallback_to_closest,


API responses need to be camelcase

Vijay-1 · 2018-03-29T14:25:00Z

@mitchell852
I have addressed your comments on the API Response structure.

I have a comment on one of your comments:

PUT should be for updating a specific fallback so the route should be PUT >>/api/1.2/cachegroups/:id/fallbacks/:fb_id

In this we dont have a specific fallback (i.e) no unique id associated with fallback. This configuration is always tied with a cache group id and hence cache group id alone is enough.

mitchell852 · 2018-03-29T15:59:23Z

@Vijay-1 - can you just do 3 endpoints? I still dont' understand the need for the PUT.

GET /api/cachegroups/:id/fallbacks <-- return an array of fallbacks for the cachegroup
POST /api/cachegroups/:id/fallbacks <-- create fallbacks for a cachegroup. so the json request is an array of fallbacks
DELETE /api/cg/:id/fallbacks <-- delete all fallbacks for the cachegroup

mitchell852 · 2018-03-29T16:02:14Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+**PUT /api/1.2/cachegroups/:id/fallbacks**
+
+  Updates an already existing fallback list for the cache group (Existing fallback list will be deleted). 


why don't you delete this route and just use POST instead where POST always creates fallbacks and removes the current ones if there are any. PUTs are used for "updates" and you are not updating a resource here. Instead, you are deleting and creating so the POST makes more sense.

@mitchell852

This has be discussed earlier and here is the summary of how it was thought about to start :
1. POST: receive a full list of ordered backups and create the fallback list. If the fallback list already exists, render a failure.

PUT: receive a full list of ordered backups, delete the existing
fallback list, and create a new fallback list. If the fallback list
does not exist yet, render a failure.

DELETE: delete the existing fallback list for a cachegroup. If the
fallback list does not exist, render a failure.

GET: return the existing fallback list for a cachegroup. If the
fallback list does not exist, render a failure.

So essentially POST creates a resource., PUT can update (replace / update) a resource which is already POST-ed. PUT can only operate on an already POST-ed resource. Does that make sense.

May be i should reword that sentence like the following:

Updates / replaces an already existing fallback list for the cache group.

yes, it makes sense but i don't really understand why you'd want 4 endpoints with extra logic in the POST and PUT endpoints (if exists, fail or if not exists, fail).

Instead if you just make the POST do a delete all/create (which is really a replace), then it can be used in both scenarios. then there becomes no confusion about the difference between POST and PUT

Ok. Makes sense. I agree that we can do away with PUT. Hope every one is fine with this.

DylanVolz · 2018-03-30T17:31:11Z

After looking over how this endpoint can be written and go, and our plan for api design moving forward (https://cwiki.apache.org/confluence/display/TC/API+Guidelines) I would suggest a few changes:

Change the route to not be nested. We are moving away from nested routes to remove ambiguity related to keys and simplify routing concerns.
A good example is @mitchell852 's recent PR for comments on Delivery Service Requests. https://github.com/apache/incubator-trafficcontrol/pull/2027/files#diff-e1d71774f8ad89e82bd76378c68555e3R117
So instead of "/api/$version/cachegroups/:id/fallbacks" the route could be formed: "/api/$version/cachegroup_fallbacks" and the cachegroup id would be provided as a query parameter.

Change the api response to include the cachegroup id in each fallback object in the array. This allows for more options for filtering and more accurately represents the data in the database.

Change the name key to something more descriptive / tied to the fallback cachegroup to make it clear.

With these changes the GET request would change from:

GET /api/1.3/cachegroups/1/fallbacks

	{
       "response": [
          {
                "order":10,
                "name":"GROUP2"
          },
          {
          	"order":11,
          	"name":"GROUP3"
          }
       ]
    }

to:

  GET /api/1.3/cachegroup_fallbacks?cachegroupID=1

  	{
       "response": [
          {
               "cachegroupID":1
               "order":10,
               "fallbackName":"GROUP2"
          },
          {
                "cachegroupID":1
          	"order":11,
          	"fallbackName":"GROUP3"
          }
       ]
    }

These changes will allow the api to easily use some prebuilt components and will give us some additional functionality for free in go, such as being able to filter by the fallback group's name and get all the cachegroups that fall back to it for example.

Vijay-1 · 2018-04-03T14:59:08Z

In perl/ Mojo, i can see this query params only for indexes, where we are fetching an entire set and ordering , say for example all available cache groups/ delivery services etc and ordering them by id / name etc.
But in this case we always go by a specific cache id. When i fetch data for /api/cachegroup_fallbacks?cgid=1, i have to return results in a particular format and for ?fbid, i have to pack results in particular format.

Say for example:
for /cachegroup_fallbacks?fallbackId=5

GET response may be a list of cache groups which has server 5 as backup
{"response":{"fallbackID":"5","list":["GROUP1","GROUP2"]}}

whereas /cachegroup_fallbacks?cacheGroupId=5
Backup configuration for server 5 along with its order
{"response":[{"cacheGroupId":"1","order":9,"name":"GROUP6"},{"cacheGroupId":"1","order":10,"name":"GROUP2"}]}

is that what you meant?

I dont think current Perl/ Mojo implementation has any thing of this sort.

AFIK, the only point i see in supporting these APIs is the similarity it may share with GO APIs. Is this alone is enough to change nested routes to the ones which supports query params? Why cant we leave this as such for Perl / Mojo?

hongfeiweb · 2018-04-03T16:18:25Z

@DylanVolz - i am not convinced that moving the URL to top level (as cachegroup_fallbacks) for this functionality is the right direction. I think the guideline wiki cited a good use of toplevel for profile & parameters. In such case, the parameters and profiles are loosely associated where params don't necessarily get associated with any profile. The fallbacks however are different in that it's just some additional, optional attributes to an otherwise regular cachgroup therefore should be accessible only through ../cachgroup/... hierarchy ihmo. I don't think the database storage normalization has to necessarily influence the API representation.

mitchell852 · 2018-04-03T21:16:47Z

@hongfeiweb - we're trying to move away from "route" or "path" parameters to remove some of the ambiguity that they have caused. Also, to condense our number of routes.

For example, we have routes like this:

GET /api/servers/{id}
GET /api/servers/{hostname}

We'd rather just have one route that supports multiple query params.

GET /api/servers?id=4 or GET /api/servers?hostname=foo-bar

It's not that I'm opposed to nested routes per se, but it's hard to have nested routes if you eliminate route/path parameters.

I've updated the TO API guidelines to reflect our new approach to APIs as we move forward with Golang:

https://cwiki.apache.org/confluence/display/TC/API+Guidelines

Vijay-1 · 2018-04-04T11:53:23Z

@DylanVolz
I have tried to address your comments. Request your review on the same.

rawlinp · 2018-04-11T16:24:25Z

@Vijay-1 - I just wanted to make sure you were aware that the cachegroup API was recently rewritten in Go if you wanted to make your changes there rather than in Perl. As of #2080 merging, the Perl cachegroup routes are no longer used.

DylanVolz · 2018-04-13T14:54:03Z

@mitchell852 I think this looks good now, I verified it works as expected. The only outstanding issue is the merging of #2080 that @rawlinp pointed out. Until that is updated this feature won't be able to be used, but I am unsure that blocks this PR necessarily.

mitchell852 · 2018-04-16T16:32:42Z

@rawlinp - can you look at the changes made to TR?

mitchell852 · 2018-04-16T16:55:45Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+  **Request Example** ::
+
+    [


To be consistent with the other routes, I would have expected the request to look like:

[ { "cacheGroupId": 1, "fallbackId": 27, "fallbackOrder": 10 }] instead of using fallbackName

and what does this do exactly? Create the row in the cachegroup_fallbacks table if it doesn't exist? and update the row if it does exist?

mitchell852 · 2018-04-16T16:59:21Z

docs/source/development/traffic_ops_api/v12/cachegroup_fallbacks.rst

+
+|
+
+**DELETE /api/1.2/cachegroup_fallbacks**


you can't delete just one fallback? you can only delete all of them for a cachegroup?

seems like you should be able to do both (delete one fallback or ALL fallbacks for a cg)

DELETE /api/1.2/cachegroup_fallbacks?id=45
DELETE /api/1.2/cachegroup_fallbacks?cacheGroupId=3

mitchell852 · 2018-04-16T17:02:59Z

traffic_ops/app/lib/API/Cachegroup.pm

@@ -164,6 +166,7 @@ sub update {
 		latitude                       => $params->{latitude},
 		longitude                      => $params->{longitude},
 		parent_cachegroup_id           => $params->{parentCachegroupId},
+		fallback_to_closest            => $params->{fallbackToClosest},


Because this field was not added to Traffic Portal, users of the TP will always be setting this to null when they update a cache group. Is that OK?

I think you need to add this field to the Traffic Portal - form.cacheGroup.tpl.html

mitchell852 · 2018-04-16T17:39:52Z

@Vijay-1 - I think you need to rethink your design of this feature. The way you have it built will be very difficult to incorporate into the Traffic Portal which uses POSTs for create only and PUTs for update. Here are my suggestions:

GET /cachegroup_fallbacks[?id=|?cachegroupId=|?fallbackId=]

Get all cachegroup fallbacks
Get a specific cachegroup fallback
Get all cachegroup fallbacks for a cg
Get all cgs for a fallback

POST /cachegroup_fallbacks [ { "cacheGroupId": 1, "fallbackId": 27, "fallbackOrder": 10 }, {...} ]

create one or more fallbacks (no deleting existing fallbacks with this route)
no query param on this route. use the cacheGroupId param to determine which cachegroup to create fallback for.

PUT /cachegroup_fallbacks[?id=] { "cacheGroupId": 1, "fallbackId": 27, "fallbackOrder": 42 }

update a cachegroup fallback by id

DELETE /cachegroup_fallbacks[?id=|?cachegroupId=]

delete a specific fallback
delete all fallbacks for a cachegroup

Vijay-1 · 2018-04-16T18:24:23Z

@mitchell852
PUT was there in the original proposal but it was removed based on one of your comments. I think, you want PUT to update which means alter an existing list instead of deleting an entire list and inserting which was PUTs original behaviour we had earlier. Am i right? If yes, i believe the original proposal should work fine instead of altering the list.

Given below is the original proposal:

POST: receive a full list of ordered backups and create the
fallback list. If the fallback list already exists, render a failure.
PUT: receive a full list of ordered backups, delete the existing
fallback list, and create a new fallback list. If the fallback list
does not exist yet, render a failure.
DELETE: delete the existing fallback list for a cachegroup. If the
fallback list does not exist, render a failure.
GET: return the existing fallback list for a cachegroup. If the
fallback list does not exist, render a failure.

Also i am not sure why we need Get all cachegroup fallbacks. I dont see any point in pulling all the cachegroup fallbacks instead it makes sense that we always retrieve fallbacks with respect to a cache group.

Also I am not sure on why we need to delete a specific fallback if there is an update (PUT) available

Vijay-1 · 2018-04-17T02:39:23Z

@mitchell852
Also i think the way cachegroup_fallbacks is designed , this table has no id. So I am not sure on the query parameter id on GET.

It should be either cacheGroupId or fallbackId. Both Ids are actually cache group Ids(cachegroup Table). Since cachegroup_fallbacks is always dependent on cachegroup only we can have the following GET API endpoints.
Get a specific cachegroup's fallback (?cacheGroupId)
Get all cachegroups which has a specific fallback (?fallbackId)

mitchell852 · 2018-04-17T14:21:34Z

@Vijay-1

The Traffic Portal typically works like this. Here are a few scenarios:

you retrieve a list of something. in this case, probably a list of fallbacks for a cachegroup.
you can click on a fallback in the list to edit it

or

you retrieve a list of something
you can click on something in the list to delete it

or

you retrieve a list of something
you can add a new thing to the list

so basically if you want to integrate your apis into the TP, you need endpoints to do the following:

fetch a list of fallbacks for a cachegroup or cachegroups for a fallback (GET /cachegroup_fallbacks[?cachegroupId=|?fallbackId=])
fetch one cg fallback (GET /cachegroup_fallbacks[?cachegroupId=4&fallbackId=6]) <-- since you have no ID field, maybe you use both to identify a row to fetch
update one cg fallback (PUT /cachegroup_fallbacks[?cachegroupId=4&fallbackId=6]) <-- since you have no ID field, maybe you use both to identify a row to update
delete one fallback (DELETE /cachegroup_fallbacks[?cachegroupId=4&fallbackId=6]) <-- since you have no ID field, maybe you use both to identify a row to delete
create one or more fallbacks (POST /cachegroup_fallbacks [ { "cacheGroupId": 1, "fallbackId": 27, "fallbackOrder": 10 }, {...} ]) <-- this just adds to the list. it doesn't wipe it out first.

without these routes, you are going to have a hard time integrating these apis into the traffic portal. actually, you could probably do without #2 and #3 and just go with:

fetch list
delete one
create one (or more) <-- no deleting thought. just add to the existing list

Vijay-1 · 2018-04-19T12:46:54Z

@mitchell852

Some minot modification from your suggestions. Request your views before actually modifying those.
fetch list
Cache group for a fallbackId
Example: GET api/{version}/cachegroup_fallbacks?fallbackId=6
Fallbacks for a cacheGroupId
Example: GET /api/{version}/cachegroup_fallbacks?cacheGroupId=1

delete one
DELETE all the fallbacks for a cache
Example: DELETE api/{Version}/cachegroup_fallbacks?cacheGroupId=1
DELETE a specific fallback for a cache
Example: DELETE api/{Version}/cachegroup_fallbacks?cacheGroupId=1&fallbackId=5"

create one
POST /api/{version}/cachegroup_fallbacks?cacheGroupId=1";
POST /api/{version}/cachegroup_fallbacks";

   POST will update if an already existing cache:fallback combination is sent in . if no such combination exists it will create one

Most of this functionalities is already there except for POST's deletion and Delete's specific row requirement.

mitchell852 · 2018-04-19T21:50:57Z

@Vijay-1 - i think that makes sense

fetch list
Cache group for a fallbackId
Example: GET api/{version}/cachegroup_fallbacks?fallbackId=6 returns
[ { cachegroupId: 3, cachegroupName: 'cg-3', fallbackId: 6, fallbackName: 'fb-6' }, { cachegroupId: 4, cachegroupName: 'cg-4', fallbackId: 6, fallbackName: 'fb-6' } ]

Fallbacks for a cacheGroupId
Example: GET /api/{version}/cachegroup_fallbacks?cacheGroupId=3 returns
[ { cachegroupId: 3, cachegroupName: 'cg-3', fallbackId: 6, fallbackName: 'fb-6' }, { cachegroupId: 3, cachegroupName: 'cg-3', fallbackId: 7, fallbackName: 'fb-7' } ]

^^ although the cg is not needed in the response, it keeps the structure the same regardless of which query param you send in (fallbackId or cacheGroupId).

delete one
DELETE all the fallbacks for a cache
Example: DELETE api/{Version}/cachegroup_fallbacks?cacheGroupId=1
DELETE a specific fallback for a cache
Example: DELETE api/{Version}/cachegroup_fallbacks?cacheGroupId=1&fallbackId=5"

but for POST (create), why not just one route like this:

POST /api/{version}/cachegroup_fallbacks where the json request is an array of one or more that looks like this:

[ { cachegroupId: 3, fallbackId: 6 }, { cachegroupId: 3, fallbackId: 7 }, { cachegroupId: 3, fallbackId: 8 } ]

^^ insert a row for each item in the array. if it is already in the database table, skip it.

Vijay-1 · 2018-04-20T14:56:02Z

@mitchell852
Looks like you missed the Order parameter:
PUT Request
[ { "cacheGroupId": 1, "fallbackName": "GROUP2", "fallbackOrder": 10 } ]

fallbackName instead of fallback Id is just a convenience, so that it will be use full while debugging 😀

GET Response
{ "response": [ { "cacheGroupId":1, "cacheGroupName":"GROUP1", "fallbackId":2, "fallbackOrder":10, "fallbackName":"GROUP2" } ] }

Also POST , updates rather than skipping , because there may be a change in Order

mitchell852 · 2018-05-15T14:43:29Z

this seems ok from an api perspective. need someone to review/test the TR part.

@rawlinp @rivasj @elsloo @ajschmidt any takers?

mitchell852 · 2018-05-15T14:44:55Z

traffic_ops/app/lib/UI/Topology.pm

@@ -250,6 +250,15 @@ sub gen_crconfig_json {
            if ( $row->type->name =~ m/^EDGE/ ) {
                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'latitude'}  = $row->cachegroup->latitude + 0;
                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'longitude'} = $row->cachegroup->longitude + 0;
+                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'backupLocations'}->{'fallbackToClosest'} = $row->cachegroup->fallback_to_closest ? "true" : "false";
+
+                my $rs_backups = $self->db->resultset('CachegroupFallback')->search({ primary_cg => $row->cachegroup->id}, {order_by => 'set_order'});


gen_crconfig_json was rewritten in Golang so this logic will need to be added there as well otherwise it will be lost. You might want to reach out to @rob05c

@Vijay-1 - this can't be merged until this is addressed.

mitchell852 · 2018-05-15T14:50:26Z

traffic_ops/app/db/migrations/20180321000000_cache_group_fallback.sql

@@ -0,0 +1,38 @@
+/*
+


this migration won't work. the date is too old.

rawlinp

TR changes still seem good, but I have some suggestions on the TO side

rawlinp · 2018-05-22T15:00:45Z

docs/source/development/traffic_ops.rst

@@ -367,264 +367,3 @@ Traffic Ops accesses each extension through the addition of a URL route as a cus
 Development Configuration
 --------------------------
 To incorporate any custom Extensions during development set your PERL5LIB with any number of directories with the understanding that the PERL5LIB search order will come into play, so keep in mind that top-down is how your code will be located.  Once Perl locates your custom route or Perl package/class it 'pins' on that class or Mojo Route and doesn't look any further, which allows for the developer to *override* Traffic Ops functionality.
-
-API


Did you mean to remove this section? Seems totally unrelated to this change and maybe pulled in accidentally when rebasing

rawlinp · 2018-05-22T15:46:39Z

traffic_ops/app/lib/API/Cachegroup.pm

@@ -57,6 +57,7 @@ sub index {
 				"lastUpdated"                   => $row->last_updated,
 				"parentCachegroupId"            => $row->parent_cachegroup_id,
 				"parentCachegroupName"          => ( defined $row->parent_cachegroup_id ) ? $idnames{ $row->parent_cachegroup_id } : undef,
+				"fallbackToClosest"           => \$row->fallback_to_closest,


I saw API reference documentation for the new cachegroup_fallbacks endpoint, but the existing cachegroup API reference documentation should also be updated to show this new fallbackToClosest field.

rawlinp · 2018-05-22T15:50:20Z

traffic_ops/app/db/migrations/20180521000000_cache_group_fallback.sql

+
+-- cachegroup_fallbacks
+CREATE TABLE cachegroup_fallbacks (
+    primary_cg bigint,


Should primary_cg and backup_cg both be NOT NULL as well? I'm not sure null values would make sense there.

There wont be any Database OP with these values as Null. Do we really need to do this?

If there's no DB op with these values as null, then that's all the more reason to do it. Generally we should always be using NOT NULL wherever it makes sense. That way if somebody changes the API implementation, they can't accidentally make it so that it accepts null values.

rawlinp · 2018-05-22T15:58:04Z

traffic_ops/app/db/migrations/20180521000000_cache_group_fallback.sql

+    UNIQUE (primary_cg, set_order)
+); 
+
+ALTER TABLE cachegroup ADD COLUMN fallback_to_closest BOOLEAN DEFAULT TRUE;


This should also probably be NOT NULL? Might save us from a bunch of unnecessary null checks when reading from the DB.

NOT NULL may fail any DB Ops if the User doesnt supply fallback_to_closest. Also, this is not a mandatory field by design and hence i think we dont have to change this.

Currently it looks like your perl API implementation protects against clients not supplying fallbackToClosest by just inserting the value true in that case. However, since the cachegroup API is now written in and handled by the golang API, it will never insert a value there and will always default to true. Eventually we'll need to implement it there too so that we can actually set it to false when needed. I think columns that have a default could always be made to be NOT NULL by having the API insert the default when null, but generally I don't feel too strongly about it.

rawlinp · 2018-05-22T16:13:34Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+		$cache_id = $param_array[0]{cacheGroupId};
+	}
+
+	if ( !defined($params) ) {


seems like this check should be above the previous one

Yes. Will address.

rawlinp · 2018-05-22T16:38:50Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+	}
+
+	#only integers
+	if ( $cache_id !~ /^\d+?$/ ) {


All the validation code should probably reside in its own function like is_valid_cachegroup_fallback that checks the request format, required fields, types, etc. and returns helpful error messages for invalid requests. It should be used here and reused in update. Right now $config->{something} could be undefined and would probably give the message "No such Cachegroup id undefined" which isn't very clear. Plus it'll make it easier to write the validation code when porting it to Go.

mitchell852

you have to make sure the logic that you added to topology.pm is added to golang

mitchell852 · 2018-05-23T01:35:00Z

traffic_ops/app/lib/UI/Topology.pm

@@ -250,6 +250,15 @@ sub gen_crconfig_json {
            if ( $row->type->name =~ m/^EDGE/ ) {
                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'latitude'}  = $row->cachegroup->latitude + 0;
                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'longitude'} = $row->cachegroup->longitude + 0;
+                $data_obj->{'edgeLocations'}->{ $row->cachegroup->name }->{'backupLocations'}->{'fallbackToClosest'} = $row->cachegroup->fallback_to_closest ? "true" : "false";
+
+                my $rs_backups = $self->db->resultset('CachegroupFallback')->search({ primary_cg => $row->cachegroup->id}, {order_by => 'set_order'});


@Vijay-1 - this can't be merged until this is addressed.

Vijay-1 · 2018-05-23T10:57:15Z

@mitchell852
Ok. Have to get the required GoLang code inside leaving the API (GoLang) part for now. Will try to get this done.

@rawlinp
I will address this and let you know.

rawlinp · 2018-05-25T16:37:15Z

lib/go-tc/cachegroupfallback.go

+// A List of CACHEGROUPFALLBACKs Response
+// swagger:response CACHEGROUPFALLBACKsResponse
+// in: body
+type CACHEGROUPFALLBACKsResponse struct {


camelcase should be used to keep in style with the rest of the go code: CachegroupFallbacksResponse

Ok. Will change this.

rawlinp · 2018-05-25T16:44:56Z

traffic_ops/app/db/migrations/20180521000000_cache_group_fallback.sql

+
+-- cachegroup_fallbacks
+CREATE TABLE cachegroup_fallbacks (
+    primary_cg bigint,


If there's no DB op with these values as null, then that's all the more reason to do it. Generally we should always be using NOT NULL wherever it makes sense. That way if somebody changes the API implementation, they can't accidentally make it so that it accepts null values.

rawlinp · 2018-05-25T16:55:33Z

traffic_ops/app/db/migrations/20180521000000_cache_group_fallback.sql

+    UNIQUE (primary_cg, set_order)
+); 
+
+ALTER TABLE cachegroup ADD COLUMN fallback_to_closest BOOLEAN DEFAULT TRUE;


Currently it looks like your perl API implementation protects against clients not supplying fallbackToClosest by just inserting the value true in that case. However, since the cachegroup API is now written in and handled by the golang API, it will never insert a value there and will always default to true. Eventually we'll need to implement it there too so that we can actually set it to false when needed. I think columns that have a default could always be made to be NOT NULL by having the API insert the default when null, but generally I don't feel too strongly about it.

rawlinp · 2018-05-25T17:04:28Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+	#only integers
+	if ( $id !~ /^\d+?$/ ) {
+		&log( $self, "No such Cachegroup id $id");
+		return $self->success([]);


shouldn't this return a 500 alert not a success?

rawlinp · 2018-05-25T17:04:48Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+	my $cachegroup = $self->db->resultset('Cachegroup')->search( { id => $id } )->single();
+	if ( !defined($cachegroup) ) {
+		&log( $self, "No such Cachegroup $id");
+		return $self->success([]);


404 not found?

rawlinp · 2018-05-25T17:38:06Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+
+		my $rs_data = $self->db->resultset('CachegroupFallback')->create($values)->insert();
+		if ( !defined($rs_data)) {
+			&log( $self, "Database operation for backup configuration for cache group $cache_id failed.");


see comment on L58

rawlinp · 2018-05-25T17:40:50Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+	foreach my $config (@{ $params }) {
+		my $rs_backup = $self->db->resultset('Cachegroup')->search( { id => $config->{fallbackId} } )->single();
+		if ( !defined($rs_backup) ) {
+			&log( $self, "ERROR Backup config: No such Cache Group $config->{fallbackId}");


see comment on L58

rawlinp · 2018-05-25T17:41:06Z

traffic_ops/app/lib/API/CachegroupFallback.pm

+		} 
+
+		if ( ($rs_backup->type->name ne "EDGE_LOC") ) {
+			&log( $self, "ERROR Backup config: $config->{name} is not EDGE_LOC");


see comment on L58

rawlinp · 2018-05-25T18:13:53Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

@@ -57,6 +57,36 @@ and (st.name = 'REPORTED' or st.name = 'ONLINE' or st.name = 'ADMIN_DOWN')
 		if ttype == RouterTypeName {
 			routerLocs[cachegroup] = latlon
 		} else {
+			primaryCacheId := ""


rather than re-querying the cachegroup's ID here, you should just update the query on L35 to also select cg.id and scan that into a variable.

In fact, you might even be able to eliminate the query on L65 altogether by modifying the query on L35 to include a subquery. Something like:

select cg.name, ..., ARRAY(select c.name from cachegroup_fallbacks cgf join cachegroup c on c.id = cgf.backup_cg where cgf.primary_cg = cg.id) ... from ...

I haven't tried that query out, but I think it's worth a shot rather than running an extra query per cachegroup. If it's possible, you should be able to scan that result into a slice of strings and just iterate over it.

rawlinp · 2018-05-25T18:18:02Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

+				return nil, nil, errors.New("Failed while retrieving from cachegroup: " + err.Error())
+			}
+
+			dbRows, err := db.Query(`select backup_cg from cachegroup_fallbacks where primary_cg = $1 order by set_order`, primaryCacheId)


you can use a join here and remove the extra query on L79:

select cgf.backup_cg, c.name from cachegroup_fallbacks cgf join cachegroup c on c.id = cgf.backup_cg

rawlinp · 2018-05-25T18:49:54Z

traffic_ops/app/db/migrations/20180521000000_cache_group_fallback.sql

@@ -0,0 +1,38 @@
+/*


this file will need renamed again because another migration was recently merged

rawlinp · 2018-05-25T20:28:55Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

+
+			index := 0
+			for dbRows.Next() {
+				backup_id := ""


in golang the typical naming style is to use camelcase, i.e. backupID

rawlinp · 2018-05-29T13:49:54Z

lib/go-tc/cachegroupfallback.go

@@ -20,33 +20,33 @@ package tc
 * under the License.
 */

-// A List of CACHEGROUPFALLBACKs Response
-// swagger:response CACHEGROUPFALLBACKsResponse
+// A List of cachegroupFallbacks Response


The c and g should be capitalized to match the rest of the CacheGroup structs (capitalizing the first letter is most important in Go because it makes the struct exportable).

rawlinp · 2018-05-29T14:11:01Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

@@ -49,20 +49,20 @@ and (st.name = 'REPORTED' or st.name = 'ONLINE' or st.name = 'ADMIN_DOWN')

 	for rows.Next() {
 		cachegroup := ""
+		primaryCacheID := ""


kind of a nitpick, but I think this should be primaryCacheID := 0 to match the int DB type

rawlinp

alright I think everything I've suggested has been addressed, but you'll need to git rebase master this branch before it can be merged.

Tomorrow I'll try to find the time to pull this down and give it a whirl.

Contains the following changes: Traffic Router - Backup edge configuration parsing and decision making Traffic Ops - Mojolicious Perl CRUD APIs Address comments from Eric and Jeremy Addressing Dylan's comment on avoiding nested routes Addessing Dylan's comment on POSt Request / Response . API endpoints changes based on Jeremey's suggestion Resolved conflict in traffic_ops.rst Addressing Jeremy's comment on GET endpoint's return statements. Changes: 1. POST - Add new fallbacks. 2. PUT - Update existing fallbacks. Renamed 20180321000000_cache_group_fallback.sql -> 20180521000000_cache_group_fallback.sql

…achegroup_fallbacks.rst to docs/source/api/v12

…up APIs 2. Validating fallback inputs

1. Avoiding multiple DB querries using Join 2. &log to error logs

… 2. GoLang naming conventions

Vijay-1 · 2018-06-01T15:54:20Z

@rawlinp rebased

rawlinp

Tested out the API changes, found only some nits that can easily be remedied in the golang version when written.

However, the golang CRConfig generation needs fixed before this can be merged. I still need to test the TR changes and will update when done.

rawlinp · 2018-06-06T17:40:33Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

@@ -32,7 +32,7 @@ func makeLocations(cdn string, db *sql.DB) (map[string]tc.CRConfigLatitudeLongit

 	// TODO test whether it's faster to do a single query, joining lat/lon into servers
 	q := `
-select cg.name, t.name as type, cg.latitude, cg.longitude from cachegroup as cg
+select cg.name, cg.id, t.name as type, cg.latitude, cg.longitude from cachegroup as cg


You need to also select cg.fallback_to_closest here and scan the value into a var fallbackToClosest *bool. If it's null, set BackupLocations.FallbackToClosest below to true. Otherwise, set it to the dereferenced value.

Also, I noticed the CRConfig generated sets the value of fallbackToClosest to a string rather than a boolean. Since booleans are an actual type in Go, I'm thinking we should set the values to booleans rather than strings and make sure Traffic Router handles that properly.

I took a look and found a bunch of bools which gets written as string in this particular part. For example=> sslEnabled. Do we need to modify it for FallbackToClosest?

I will take a look at parsing FallbackToClosest and make changes.

I think the string boolean thing might have stemmed from Perl not having real booleans. Now that it's been migrated to Go, we could use real booleans in the JSON. But since there are a bunch of string bools in the CRConfig already, I guess it's better to be consistent and keep it as a string bool.

rawlinp · 2018-06-06T20:30:31Z

.../src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java

+		if (closestCacheLocation != null) {
+			LOGGER.debug("Got closest CZ cache group " + closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+			if (track != null) {
+				track.setFromBackupCzGroup(true);


Why do we set this to true here? If we reach this point, there were no available backup cachegroups (or none configured), so we shouldn't track it as from a backup cachegroup, right?

The cache which gets selected in this cache is not a primary one. This is again a backup cache configured via Coordinates and hence it gets tracked as backup.

Ok, I can agree with that. It would be useful to track those occurrences as "backup hits".

rawlinp · 2018-06-06T21:20:47Z

Finished testing the TR changes; they still look good except for my question about the tracking.

…andled

rawlinp · 2018-06-12T21:21:55Z

traffic_ops/traffic_ops_golang/crconfig/edgelocations.go

 			return nil, nil, errors.New("Error scanning cachegroup: " + err.Error())
 		}
 		if ttype == RouterTypeName {
 			routerLocs[cachegroup] = latlon
 		} else {
+			q := `select cachegroup.name from cachegroup_fallbacks
+join cachegroup on cachegroup_fallbacks.backup_cg = cachegroup.id
+and cachegroup_fallbacks.primary_cg = $1


I missed this in my previous review, but this query is missing order by cachegroup_fallbacks.set_order. Without this the set_order ordering is not enforced.

Right. I missed that. Addressed.

…ed properly in GOLang implementation

i "think" the requested changes were made but not really sure

Vijay-1 · 2018-06-13T16:39:39Z

Yes. The changes requested for edgeLocations.go for CRConfig were addressed.

Vijay-1 mentioned this pull request Mar 21, 2018

Changes for Backup Edge Cache Group #1908

Closed

limited reviewed Mar 22, 2018

View reviewed changes

mitchell852 requested changes Mar 28, 2018

View reviewed changes

mitchell852 added Traffic Ops API WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) Traffic Router related to Traffic Router labels Mar 28, 2018

mitchell852 reviewed Mar 29, 2018

View reviewed changes

dangogh changed the title ~~Implements https://github.com/apache/incubator-trafficcontrol/issues/…~~ [Issue 1907] TO API for backup edge cachegroup Apr 3, 2018

mitchell852 removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Apr 16, 2018

mitchell852 requested changes Apr 16, 2018

View reviewed changes

mitchell852 reviewed May 15, 2018

View reviewed changes

mitchell852 requested changes May 15, 2018

View reviewed changes

rawlinp requested changes May 22, 2018

View reviewed changes

mitchell852 previously requested changes May 23, 2018

View reviewed changes

rawlinp requested changes May 25, 2018

View reviewed changes

rawlinp reviewed May 25, 2018

View reviewed changes

rawlinp requested changes May 29, 2018

View reviewed changes

rawlinp requested changes May 30, 2018

View reviewed changes

Vijay-1 added 6 commits June 1, 2018 11:45

Moved cachegroup_fallbacks.rst from development/traffic_ops_api/v12/c…

14aa10b

…achegroup_fallbacks.rst to docs/source/api/v12

CrConfig's backupLocation Json node generation logic in GoLang

efed1cf

Addressing Rawlin's comment on; 1. Updating the document for cachegro…

7a902f0

…up APIs 2. Validating fallback inputs

Addressing Rawlin's comments:

85a1067

1. Avoiding multiple DB querries using Join 2. &log to error logs

Rawlin's comments 1. Adding cache group id for audit logs for DELETE.…

5f65244

… 2. GoLang naming conventions

rawlinp requested changes Jun 6, 2018

View reviewed changes

rawlinp reviewed Jun 6, 2018

View reviewed changes

Rawlins comment - parsing fallback to closest and make sure null is h…

1458808

…andled

rawlinp requested changes Jun 12, 2018

View reviewed changes

Rawlins comment on using group by to enforce order which is not handl…

4174ac1

…ed properly in GOLang implementation

rawlinp approved these changes Jun 13, 2018

View reviewed changes

elsloo self-requested a review June 14, 2018 17:09

elsloo added the new feature A new feature, capability or behavior label Jun 14, 2018

elsloo merged commit 5be4909 into apache:master Jun 14, 2018


		POST /api/1.2/cachegroups/:id/fallbacks

		Create cache group.


		PUT /api/1.2/cachegroups/:id/fallbacks

		Create cache group.


		DELETE /api/1.2/cachegroups/{:id}/fallbacks

		Delete cache group. The request to delete a cache group, which has servers or child cache group, will be rejected.


		PUT /api/1.2/cachegroups/:id/fallbacks

		Updates an already existing fallback list for the cache group (Existing fallback list will be deleted).

[Issue 1907] TO API for backup edge cachegroup #2029

[Issue 1907] TO API for backup edge cachegroup #2029

Conversation

Vijay-1 commented Mar 21, 2018 • edited Loading

asfgit commented Mar 21, 2018

asfgit commented Mar 21, 2018

dangogh commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-1 commented Mar 29, 2018

mitchell852 commented Mar 29, 2018

mitchell852 Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DylanVolz commented Mar 30, 2018

Vijay-1 commented Apr 3, 2018

hongfeiweb commented Apr 3, 2018

mitchell852 commented Apr 3, 2018

Vijay-1 commented Apr 4, 2018

rawlinp commented Apr 11, 2018

DylanVolz commented Apr 13, 2018

mitchell852 commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 commented Apr 16, 2018 • edited Loading

Vijay-1 commented Apr 16, 2018

Vijay-1 commented Apr 17, 2018

mitchell852 commented Apr 17, 2018

Vijay-1 commented Apr 19, 2018

mitchell852 commented Apr 19, 2018 • edited Loading

Vijay-1 commented Apr 20, 2018

mitchell852 commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawlinp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-1 commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vijay-1 commented Mar 21, 2018 •

edited

Loading

dangogh commented Mar 21, 2018 •

edited

Loading

mitchell852 Mar 29, 2018 •

edited

Loading

mitchell852 commented Apr 16, 2018 •

edited

Loading

mitchell852 commented Apr 16, 2018 •

edited

Loading

mitchell852 commented Apr 19, 2018 •

edited

Loading