Cachegroup ui update with fallbacks by mattjackson220 · Pull Request #3203 · apache/trafficcontrol

mattjackson220 · 2019-01-10T21:17:52Z

What does this PR do?

Fixes #1907

Updates the Cache Group page in Traffic Portal to include Cache Group Failovers and Fallback to Geo Failover checkbox, updates Cache Group API to include fallbackToClosest field, updates Traffic Portal documentation for changes

Which TC components are affected by this PR?

What is the best way to verify this PR?

Go to Topology -> Cache Groups.
Create a new Cache Group and select EDGE_LOC for the type.
Verify that the Failover Locations section appears at the bottom.
Add Cache Groups from the drop down menu.
Click Create and ensure the cachegroup_fallbacks table was updated.
Click Topology -> Cache Groups again. Find the group that was created.
Ensure the Failover Locations section is there and accurate with the groups you added.
Drag and Drop to reorder.
Click trash can on the right to delete a failover group.
Click Update and verify that the cachegroup_fallbacks table was updated.

Check all that apply

This PR includes tests
This PR includes documentation updates
This PR includes an update to CHANGELOG.md
This PR includes all required license headers
This PR includes a database migration (ensure that migration sequence is correct)
This PR fixes a serious security flaw. Read more: www.apache.org/security

… updated cache group API to allow control of cachegroup.fallbackToClosest through POST,PUT, and GET, updated documentation.

…com/mattjackson220/trafficcontrol into cachegroup_ui_update_with_fallbacks

asfgit · 2019-01-10T21:21:05Z

Can one of the admins verify this patch?

chadgilloth

I checked out this branch and verified the UI changes by adding/removing/ordering fallbacks for new and edited Cache Groups.

mitchell852

a few suggestions

mitchell852 · 2019-01-16T15:24:07Z

  font-size: medium;
+}
+
+.allFallbacks{


this file (_form.scss) is really intended for generic form styling. i would leave the drag and drop stuff here because that could potentially be used for any form. however, the stuff that is specific to the cache group form should go in it's own file. so you should create this:

traffic_portal/app/src/common/modules/form/cacheGroup/_form.cacheGroup.scss

then you can move all the cachegroup specific styles to that file.

and then you'll have to add that file to main.scss

mitchell852 · 2019-01-16T15:29:16Z

+    }
+
+    $scope.getFallbackOptions = function() {
+        for (var i = 0; i < $scope.cacheGroups.length; i++) {


can you put some comments in this function? hard to figure out what it's doing exactly.

mitchell852 · 2019-01-16T15:29:51Z

+    $scope.moveAbove = true;
+
+    $scope.updateFallbacks = function(cacheGroup) {
+        if (cacheGroup.fallbacks == null) {


can you put some comments in this function? hard to figure out what it's doing exactly.

mitchell852 · 2019-01-16T17:03:05Z

+                    <div class="allFallbacks" id="allFallbacks">
+                        <div class="fallback" ng-repeat="fb in cacheGroup.fallbacks" dnd-enable="true" pageid="cacheGroupFallback">
+                            <i class="fa fa-ellipsis-v fallback-grip drop-child" aria-hidden="true"></i>
+                            <div class="fallback-list drop-child">{{ $index + 1 }}.     {{fb}}</div>


this should probalby be like:

{{ $index + 1 }}    {{fb}}

ocket8888

You have two spaces after periods in a lot of places - I see this in the docs a lot for some reason. What's up with that? Do people think it looks better? The compiler will produce differing output for LaTeX (which ignores the extra space) and HTML (which I believe includes and renders it) so I'd prefer to avoid it, even if it is minor.

ocket8888 · 2019-01-16T17:26:18Z

+
+.. seealso:: :ref:`tp-configure-cache-groups`
+
+#. Go to 'Topology', click on Cache Groups, and click on your desired Cache Group or click the + button to create a new Cache Group.


Can you make all instances of 'Cache Group' in the text into terms, so that they link to the glossary? You don't need to worry about it in section titles, and I'm not gonna make you do it in the API docs - there are a lot of instances in the API that need to be updated and they aren't the responsibility of this PR. They'll be done in a more general-purpose PR.

Example:

This links to the glossary: :term:`Cache Group` Terms need to be followed by a space or punctuation, so to make it plural you need to do: :term:`Cache Group`\ s

ocket8888 · 2019-01-16T17:26:59Z

+.. _cachegroup-fallback-qht:
+
+******************************
+Configure CacheGroup Fallbacks


For consistency please use "Cache Group" rather than "CacheGroup"

ocket8888 · 2019-01-16T17:36:12Z

+
+#. Go to 'Topology', click on Cache Groups, and click on your desired Cache Group or click the + button to create a new Cache Group.
+
+	.. figure:: cachegroup_fallback_qht/00.png


The instructions make reference to a '+' button, but this screenshot cuts off said button.

I'm also not sold on showing production Cache Groups, imo it'd be better to use mock data (preferably with CDN-in-a-Box) to avoid any possible legal problems. Of course, I'm not a lawyer so maybe this is totally fine.

done. and you are right, i removed the production Cache Groups

ocket8888 · 2019-01-16T17:38:34Z

+
+		Failover Locations Delete
+
+#. Click the Update button (if editing existing Cache Group) or the Create button (if creating new Cache Group) in order to save the Failovers to the Cache Group.


This one's just a suggestion: you can use the guilabel text role to make things render like buttons

Example:

... or the :guilabel:`Create` button...

ocket8888 · 2019-01-16T17:40:40Z

 	ds_requests
 	steering
 	ciab
+	cachegroup_fallback


Since the name of the file is cachegroup_fallback_qht.rst this won't include it. You could either change the filename or this line. It's up to you, but in my opinion it'd be best to change the filename; we already know it's a Quick How-to because it's in that section so I don't think the _qht adds any useful information.

ocket8888 · 2019-01-16T17:43:00Z

+  width:5%;
+}
+
+.hovering {


Why couldn't you just use :hover?

ocket8888 · 2019-01-16T17:44:37Z

+            el.addEventListener(
+                'mouseover',
+                function(e) {
+                    this.classList.add('hovering');


Do you need this event listener? Is there a reason you can't write your CSS rule with e.g. .grabable:hover instead of .hovering?

done. i moved it into fallback:hover instead

ocket8888 · 2019-01-16T17:48:20Z

                </div>
            </div>
+            <div class="form-group" ng-class="{'has-error': hasError(cacheGroupForm.fallbacks), 'has-feedback': hasError(cacheGroupForm.fallbacks)}" ng-hide="!isEdgeLoc(cacheGroup.typeId)">
+                <label class="control-label col-md-2 col-sm-2 col-xs-12">Failover Locations</label>


You seem to be mixing the terms "Failover Location" and "Failover Cache Group". This should really just use "Cache Group" everywhere (including in the documentation) because the user is selecting a Cache Group and Physical Locations are another object entirely which could cause confusion.

ocket8888

So "Cache Group" should link to the glossary everywhere it appears in the documentation, even inside tables - the only exceptions are section titles and code-blocks. You could even do it in a figure title, though I won't make you.

ocket8888 · 2019-01-16T22:41:14Z

+
+******************************
+Configure Cache Group Fallbacks
+******************************


You need to add an asterisk to the underline/overline to match the title length

ocket8888 · 2019-01-16T22:42:32Z

 | not_a_parent             | parent.config     | This is a boolean flag and is considered 'true' if it exists and has any value except 'false'.                          |
 |                          |                   | This prevents servers with this parameter in their profile from being inserted into the ``parent.config`` generated for |
-|                          |                   | servers with this server's Cache Group as a parent of their CacheGroup. This is primarily useful for when edge caches   |
+|                          |                   | servers with this server's Cache Group as a parent of their Cache Group. This is primarily useful for when edge caches   |


This table is malformed. It should also link "Cache Group" to the glossary. (It should also use the .. table directive but that's not your problem)

ocket8888 · 2019-01-16T22:43:03Z

-:Type:       The Type of this Cache Group (see :ref:`tp-configure-types`)
-:Latitude:   A geographic latitude assigned to this Cache Group
-:Longitude:  A geographic longitude assigned to this Cache Group
+:Name:                  The full name of this Cache Group


"Cache Group" should link to the glossary

ocket8888 · 2019-01-16T22:48:42Z

+  width:5%;
+}
+
+.fallback:hover {


You know, since this is a SCSS file you could use SCSS to put this inside of the .fallback rule. It'd look something like:

.fallback { background-color: #fff; &:hover { background: rgba(38,185,154,0.07); } }

Just a suggestion. I think it's a real shame that we spent all that time and effort fixing the sass compiler when 90% of our styling doesn't even make use of it.

…s of "Cache Group" to glossary

ocket8888

Wow, man. You didn't need to link to the glossary everywhere - though I'm grateful you did because otherwise I would've - just the stuff you made/edited.

mitchell852

The code looks good from a TP perspective

mitchell852 · 2019-01-23T17:29:15Z

@mattjackson220 - If i edit an 'EDGE_LOC' cache group, i can add fallbacks just fine. So imagine that I add 2 fallbacks all is good. but if i change the CG type to 'MID_LOC' i still see the fallbacks in the database...my guess is this is probably fine and won't hurt anything. my guess is that fallbacks are simply ignored if type != EDGE_LOC but i just wanted to point it out in case there are any negative effects of this.

If you wanted to be really safe, I suppose on CG update if type != EDGE_LOC, you could set fallbacks = []. either in the UI or the API...or both. otherwise, this seems to work great in TP. nice work.

rob05c

Go code looks good, tested traffic_ops_golang, everything behaves as expected, GET, GET /id PUT POST DELETE work as expected, passing fallbackToClosest sets and updates as expected, omitting it works as expected and sets the default in POST, null in PUT.

For anyone else reading this: this "new" field is in 1.1 not 1.5 because it's not actually new, the struct field and docs were added a long time ago, it's a bug that the app wasn't updating the field correctly.

This includes setting null in a PUT. That's unfortunately how PUT works. It unfortunately may cause issues for people with old files. But the alternative, behaving unexpectedly and in violation of the TC docs and RFC method, is even more dangerous.

Actually, @mattjackson220 now that I think of it: would you mind putting something in the CHANGELOG.md warning people about it, specifically that a PUT of an old cachegroup JSON without it (likely created from a GET) will result in a null field?

Other than that, everything in traffic_ops_golang looks good.

Note I only reviewed and tested traffic_ops_golang, not anything else in this PR.

… errors if fallbacks present on non-EDGE_LOC cache group. updated CHANGELOG

ocket8888 · 2019-01-24T22:38:49Z

+        // removes Cache Group fallbacks if type has changed and is no longer EDGE_LOC
+        if (!$scope.isEdgeLoc(cacheGroup.typeId)) {
+            let currentFallbacksCount = cacheGroup.fallbacks.length;
+            for (var i = 0; i < currentFallbacksCount; i++) {


Is it necessary to do this iteration? Could cacheGroup.fallbacks be set to an empty array, (or null/undefined, not sure which is appropriate tbh).

I did it this way so that it adds the previously selected fallbacks back into the list of available options in the drop down (and in the correct order where they started). Ultimately cacheGroup.fallbacks ends up as an empty array, but this also does the clean up to get them back as options

ocket8888 · 2019-01-25T17:30:58Z

I don't think this actually fixes #3003 . That issue is related to the /api/1.1/cachegroup_fallbacks endpoint which is still in Perl, and from what I can tell this didn't touch the Perl code, nor rewrite that (method of that) endpoint.

mattjackson220 added 3 commits January 10, 2019 13:56

updated Cache Group page in Traffic Portal for Cache Group Failovers,…

a68c4ee

… updated cache group API to allow control of cachegroup.fallbackToClosest through POST,PUT, and GET, updated documentation.

updated Cache Group page in Traffic Portal for Cache Group Failovers,…

5d3a06d

… updated cache group API to allow control of cachegroup.fallbackToClosest through POST,PUT, and GET, updated documentation.

Merge branch 'cachegroup_ui_update_with_fallbacks' of https://github.…

9277fe8

…com/mattjackson220/trafficcontrol into cachegroup_ui_update_with_fallbacks

mitchell852 added Traffic Portal v1 related to Traffic Portal version 1 new feature A new feature, capability or behavior labels Jan 10, 2019

mattjackson220 added 4 commits January 11, 2019 09:10

fix needed for new cachegroup page

dce7840

updated to use EDGE_LOC instead of id

7c91014

updated more to use EDGE_LOC instead of id

fb373b6

updated so types and cachegroups pull before page load

6c1a1de

chadgilloth reviewed Jan 11, 2019

View reviewed changes

mitchell852 reviewed Jan 16, 2019

View reviewed changes

mitchell852 added Traffic Ops related to Traffic Ops documentation related to documentation labels Jan 16, 2019

ocket8888 suggested changes Jan 16, 2019

View reviewed changes

updated to incorporate review comments

3ef1ae4

ocket8888 suggested changes Jan 16, 2019

View reviewed changes

mattjackson220 added 2 commits January 17, 2019 10:42

updated after review comments and linked all documentation occurrence…

4d68e45

…s of "Cache Group" to glossary

updated after review comments

ae50278

ocket8888 approved these changes Jan 17, 2019

View reviewed changes

removed markups from figure alt sections

5320dce

mitchell852 approved these changes Jan 23, 2019

View reviewed changes

rob05c approved these changes Jan 24, 2019

View reviewed changes

updated so fallbacks are removed if not EDGE_LOC in TP and API throws…

1162756

… errors if fallbacks present on non-EDGE_LOC cache group. updated CHANGELOG

ocket8888 reviewed Jan 24, 2019

View reviewed changes

mitchell852 merged commit f29e557 into apache:master Jan 25, 2019


		.. seealso:: :ref:`tp-configure-cache-groups`

		#. Go to 'Topology', click on Cache Groups, and click on your desired Cache Group or click the + button to create a new Cache Group.


		#. Go to 'Topology', click on Cache Groups, and click on your desired Cache Group or click the + button to create a new Cache Group.

		.. figure:: cachegroup_fallback_qht/00.png


		Failover Locations Delete

		#. Click the Update button (if editing existing Cache Group) or the Create button (if creating new Cache Group) in order to save the Failovers to the Cache Group. No newline at end of file

Conversation

mattjackson220 commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Which TC components are affected by this PR?

What is the best way to verify this PR?

Check all that apply

Uh oh!

asfgit commented Jan 10, 2019

Uh oh!

chadgilloth left a comment

Choose a reason for hiding this comment

Uh oh!

mitchell852 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocket8888 Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattjackson220 commented Jan 10, 2019 •

edited

Loading

ocket8888 Jan 16, 2019 •

edited

Loading

mitchell852 commented Jan 23, 2019 •

edited

Loading