Hkatz/dashboard and summary overhaul #180

hjkatz · 2020-06-28T17:46:06Z

Why?

This is one of the largest PRs I'll be submitting. It's big because it does a complete overhaul of the dashboard (now "web") package and summary packages to support the changes.

Goals:

Support Namespace list view
Support individual Namespace view
Support a more logical structure for a Summary
Add testing to Summary

What?

Major Changes:

pkg/dashboard -> pkg/web to support more than just "the dashboard" view as there is also a list view too
"dashboard" -> "web" in general
Split templates by kind
Templates and Handlers are more general purpose and can support future additions easily
Adjust Summary structure to be more logically consistent with what it's summarizing (Summary -> Namespaces -> Deployments -> Containers)

Minor Changes:

Allow web ClusterRole to list Namespaces
Typo fixes, Squarespace fixes (stuff pulled in by accident)
/namespaces is the new default route
Test fixes, Typo Fixes, etc...
Add outer structure to Summary
Move commonly used labels to pkg/utils

P.S. After this there are a few more PRs but they're all 1-2 commits in size and should be quick. They include:

Batch Summary Deployments list into a single API call to improve dashboard speed significantly
Add Namespace list search bar/filtering and other friendly js improvements
Add Namespace to blank Summary to support specific Namespaces in a more consistent structure
Some final VPA touchups to make the business logic of updating VPAs clearer to modify and test
Minor typos and QoL changes to catch some bugs (these will be batched into 1 PR)

This PR was already getting enormous so I'm trying to slice it here. I'm comfortable modifying just about anything in this PR now since the hard part was really getting past this point :)

…airwindsOps#103)" This reverts commit e55026d.

Revamp e2e Tests (FairwindsOps#118)

Merge Master

The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode.

This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode.

Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode.

Hkatz/add vpa update mode

Merge masters

Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests

Also: - Update vpa tests - Fix vpa-opt-out logic

Co-Authored-By: Andrew Suderman <[email protected]>

codecov · 2020-06-28T18:27:47Z

Codecov Report

Merging #180 into master will increase coverage by 7.50%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   44.84%   52.35%   +7.50%     
==========================================
  Files           9        8       -1     
  Lines         660      573      -87     
==========================================
+ Hits          296      300       +4     
+ Misses        340      247      -93     
- Partials       24       26       +2

Impacted Files	Coverage Δ
pkg/kube/client.go	`17.64% <ø> (ø)`
pkg/utils/utils.go	`100.00% <ø> (ø)`
pkg/summary/summary.go	`49.42% <57.14%> (+0.05%)`	⬆️
pkg/vpa/vpa.go	`71.79% <66.66%> (ø)`
pkg/dashboard/helpers/helpers.go	`78.66% <80.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a4f125...5fa8536. Read the comment docs.

hjkatz · 2020-06-28T18:28:52Z

I literally cannot believe all the e2e tests passed. That's incredible! :D

hjkatz · 2020-07-20T14:07:32Z

@lucasreed/@sudermanjr Any chance y'all could lend some 👀 to this PR? 😃

sudermanjr · 2020-07-20T15:15:34Z

@lucasreed/@sudermanjr Any chance y'all could lend some 👀 to this PR? 😃

I will try to get a chance to look at this this week.

rbren

Haven't tested locally but the code looks great!

cmd/web.go

pkg/kube/client.go

pkg/web/health.go

cmd/web.go

CONTRIBUTING.md

Co-authored-by: Harrison Katz <[email protected]>

sudermanjr

Overall, functionality seems great and I love the namespaced views on the dashboard.

I'm really not a fan of the dashboard -> web rename. It seems to suggest that it does more than display information, which it does not do.

The fact that it also changes the name of all of the kubernetes objects means we will have to completely change the chart, as well as the users experiencing a very heavy shift in what they see in their clusters when deploying.

TL;DR, it seems like a lot of code churn with zero functional purpose.

lucasreed · 2020-07-21T21:56:10Z

I'm really not a fan of the dashboard -> web rename. It seems to suggest that it does more than display information, which it does not do.

The fact that it also changes the name of all of the kubernetes objects means we will have to completely change the chart, as well as the users experiencing a very heavy shift in what they see in their clusters when deploying.

TL;DR, it seems like a lot of code churn with zero functional purpose.

Agreed, simply changing the package name back to dashboard would make the PR much smaller which would be nice. A lot of secondary changes are only because of the name change (i.e. CONTRIBUTING + README)

lucasreed

Adding official review based on comments above: unclear what the purpose is of renaming the dashboard package. Lets change that back and all the documentation changes around it to have a smaller, more manageable PR.

hjkatz · 2020-07-22T12:22:15Z

I'm really not a fan of the dashboard -> web rename. It seems to suggest that it does more than display information, which it does not do.

The fact that it also changes the name of all of the kubernetes objects means we will have to completely change the chart, as well as the users experiencing a very heavy shift in what they see in their clusters when deploying.

TL;DR, it seems like a lot of code churn with zero functional purpose.

Agreed, simply changing the package name back to dashboard would make the PR much smaller which would be nice. A lot of secondary changes are only because of the name change (i.e. CONTRIBUTING + README)

Will do y'all! I originally wanted to rename pkg/dashboard when I found out that it was going to hold more than just a single dashboard page and it felt weird to have dashboard.Dashboard() as well as dashboard.NamespaceList(). I'll revert back to pkg/dashboard and we can move forward from there.

(Edit: It also looks a bit strange to see dashboard.GetRouter(dashboard.OnPort(8000), ...) sort of calls. But it's just an aesthetic thing and supporting existing workflows/Helm charts is much more important imo.)

hjkatz · 2020-07-22T12:38:23Z

Ok, pkg/web has been un-renamed back to pkg/dashboard. The PR diff looks a lot smaller now. The file pkg/dashboard/dashboard.go diff is a bit hard to read because git is trying to be smart but it's not pattern matching well on the diff chunks, so I recommend viewing that file alone or in the side-by-side view. @sudermanjr @lucasreed

lucasreed

LGTM, sorry again for the delay!

Harrison Katz and others added 30 commits February 28, 2020 15:17

Revert "Make units consistent for memory fixes issue FairwindsOps#78 (F…

73f3246

…airwindsOps#103)" This reverts commit e55026d.

Add FormatResourceList and test cases

6ad3a58

Use v1 resource memory constants instead of hardcoded strings

091491f

Merge pull request #1 from FairwindsOps/master

14442ea

Revamp e2e Tests (FairwindsOps#118)

Merge pull request #2 from FairwindsOps/master

5e72ef0

Merge Master

Add a VPAUpdateMode to the vpa.Reconciler

85dae07

The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode.

Add ability to specify vpa update mode

ab9148e

This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode.

Extract variables instead of hardcoded values

69f688e

Summary fix for multiple containers in a pod (FairwindsOps#123)

5b16cbd

go.mod updated for some reason

67e89ee

Add support for all VPA UpdateModes

5ebd109

Use ParseBool for deployment enabled label value

3d59677

Use ParseBool for namespace enabled label value

bc50617

Merge branch 'master' into hkatz/add-vpa-update-mode

701411a

Use lowercase for switch statement

ac94d48

Merge pull request #3 from hjkatz/hkatz/add-vpa-update-mode

eb83cb6

Hkatz/add vpa update mode

Merge pull request #4 from FairwindsOps/master

1e14372

Merge masters

Add Deployment Exclusion Annotation

65d1340

Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests

Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode)

662477b

Also: - Update vpa tests - Fix vpa-opt-out logic

Add more logging to VPA reconciler

a7e18b2

Fix namespace logs to use Name instead of the full Namespace

c57d004

Update README

563d78d

Fix Typo aut -> auto

8035039

Co-Authored-By: Andrew Suderman <[email protected]>

Fix grammar matched -> match

01f6610

Co-Authored-By: Andrew Suderman <[email protected]>

Use vpa-update-mode as an annotation for Deployment specific VPA control

ef29113

Fix ineffectual assignment

114565d

Merge branch 'master' into hkatz/vpa-deployment-exclusion-and-updates

828c09d

Fix test label

a68fe5f

Use index to get correct vpa

8576ca6

hjkatz force-pushed the hkatz/dashboard-and-summary-overhaul branch from 5cdf1c0 to 38889be Compare June 28, 2020 17:50

Harrison Katz added 4 commits June 28, 2020 14:12

Attempt to fix e2e

216806a

Fix incorrect args usage for namespace

b885817

Add oidc auth support

33297c9

Remove redundant return statement from health.go

5db0769

hjkatz force-pushed the hkatz/dashboard-and-summary-overhaul branch from fca34b3 to 5db0769 Compare June 28, 2020 18:12

sudermanjr self-assigned this Jul 20, 2020

sudermanjr removed the request for review from mgoodhart5 July 20, 2020 15:15

Merge branch 'master' into hkatz/dashboard-and-summary-overhaul

7b2f783

lucasreed requested a review from rbren July 20, 2020 18:11

rbren approved these changes Jul 20, 2020

View reviewed changes

cmd/web.go Outdated Show resolved Hide resolved

pkg/kube/client.go Show resolved Hide resolved

pkg/web/health.go Outdated Show resolved Hide resolved

hjkatz commented Jul 21, 2020

View reviewed changes

cmd/web.go Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

Update cmd/web.go

cb0bb73

Co-authored-by: Harrison Katz <[email protected]>

sudermanjr reviewed Jul 21, 2020

View reviewed changes

lucasreed suggested changes Jul 21, 2020

View reviewed changes

hjkatz force-pushed the hkatz/dashboard-and-summary-overhaul branch from 6fd77b9 to 84ba32b Compare July 22, 2020 12:36

Rename web back to -> dashboard

eae73cd

hjkatz force-pushed the hkatz/dashboard-and-summary-overhaul branch from 84ba32b to eae73cd Compare July 22, 2020 13:15

hjkatz requested a review from lucasreed July 22, 2020 21:01

Merge branch 'master' into hkatz/dashboard-and-summary-overhaul

5fa8536

sudermanjr self-requested a review August 5, 2020 17:01

lucasreed approved these changes Aug 7, 2020

View reviewed changes

lucasreed merged commit 746e30a into FairwindsOps:master Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hkatz/dashboard and summary overhaul #180

Hkatz/dashboard and summary overhaul #180

hjkatz commented Jun 28, 2020 •

edited

Loading

codecov bot commented Jun 28, 2020 •

edited

Loading

hjkatz commented Jun 28, 2020

hjkatz commented Jul 20, 2020

sudermanjr commented Jul 20, 2020

rbren left a comment

sudermanjr left a comment •

edited

Loading

lucasreed commented Jul 21, 2020

lucasreed left a comment

hjkatz commented Jul 22, 2020 •

edited

Loading

hjkatz commented Jul 22, 2020

lucasreed left a comment

Hkatz/dashboard and summary overhaul #180

Hkatz/dashboard and summary overhaul #180

Conversation

hjkatz commented Jun 28, 2020 • edited Loading

Why?

What?

codecov bot commented Jun 28, 2020 • edited Loading

Codecov Report

hjkatz commented Jun 28, 2020

hjkatz commented Jul 20, 2020

sudermanjr commented Jul 20, 2020

rbren left a comment

Choose a reason for hiding this comment

sudermanjr left a comment • edited Loading

Choose a reason for hiding this comment

lucasreed commented Jul 21, 2020

lucasreed left a comment

Choose a reason for hiding this comment

hjkatz commented Jul 22, 2020 • edited Loading

hjkatz commented Jul 22, 2020

lucasreed left a comment

Choose a reason for hiding this comment

hjkatz commented Jun 28, 2020 •

edited

Loading

codecov bot commented Jun 28, 2020 •

edited

Loading

sudermanjr left a comment •

edited

Loading

hjkatz commented Jul 22, 2020 •

edited

Loading