Hkatz/refactor reconciler #128
Conversation
…airwindsOps#103)" This reverts commit e55026d.
Revamp e2e Tests (FairwindsOps#118)
The vpa.Reconciler function read a bit funny to me: a bunch of private methods were listed before the exported methods. When someone is going to read that file, they most likely want to see the public API of the struct before they see the private functions that implement it. This commit reorders things so that the public API is listed first in the file.
The vpa.Reconciler currently uses a passthrough dry-run variable. However the dry run behavior is part of the state of the reconciler, and it shouldn't change within the lifecycle of the reconciler. Decoupling it from the reconciler gives the appearance that the dry run behavior should somehow be separate, and results in a variable that has to be passed around to private package functions.
These functions all take data clumps of internal state of the Reconciler. While decoupling them might be useful if they were exported package functions, the fact that they're unexported means that the Reconciler is their only caller. Given those two facts, it makes more sense to couple the these functions with the Reconciler.
The name checkNamespaceLabel describes what is done mechanically, not why. namespaceIsManaged should hopefully give a clearer sense of what the boolean return value means for this method.
The main exported API on of the vpa package is the Reconciler.ReconcilerNamespace method. However, this method is a bit hard to read because it mixes levels of abstraction quite frequently. This leads to comments being used to convey semantic information. This commit refactors the main reconciliation loop into a series of unexported methods, allowing the reader of the code to get a better sense of why certain operations are taking place.
hjkatz
requested review from
lucasreed,
mgoodhart5 and
sudermanjr
as code owners
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
+ Coverage 44.2% 44.65% +0.45%
==========================================
Files 8 8
Lines 561 571 +10
==========================================
+ Hits 248 255 +7
- Misses 302 304 +2
- Partials 11 12 +1
Continue to review full report at Codecov.
|
|
Initially just reading through the PR description, everything sounds good except for the next two planned steps: In my opinion, these changes push this project outside of the intended scope and should not be included. The intent here is to display, not to change. Especially considering the inability of the VPA to play nicely with HPA (which we recommend heavily). I'm definitely open to further discussion of this, and would love to hear what @rbren and @ejether and @lucasreed think of this. |
| @@ -79,7 +131,7 @@ func (vpa Reconciler) checkDeploymentLabels(deployment *appsv1.Deployment) (bool | |||
| return false, nil | |||
| } | |||
|
|
|||
| func (vpa Reconciler) checkNamespaceLabel(namespace *corev1.Namespace) bool { | |||
| func (r Reconciler) namespaceIsManaged(namespace *corev1.Namespace) bool { | |||
There was a problem hiding this comment.
Love this new name. I was never sure what to call that function.
| @@ -34,6 +34,7 @@ type Reconciler struct { | |||
| KubeClient *kube.ClientInstance | |||
| VPAClient *kube.VPAClientInstance | |||
| OnByDefault bool | |||
| DryRun bool | |||
There was a problem hiding this comment.
Another great change. Dryrun functionality came before this was actually a struct, so I think it just got forgotten.
|
@sudermanjr Thanks for the review. On the note about the The incoming PRs provide an optional way of managing the VPA objects created by Goldilocks that could be presented as an advanced or undocumented feature. While Squarespace may be interested in managing these VPAs through Goldilocks that may not be the goal of every user. Unfortunately a majority of the PRs are built on top of each other as direct changes/improvements to the one before (this is because we were iterating on what would work best for our use-case at the time). Therefore I'd like to offer a halfway point while getting these PRs reviewed that we can come back to visit after larger changes have been merged in (such as the summary and web package updates). In the upcoming PRs I can be sure to remove documentation and direct functionality of the vpa update mode in terms of the controller having any impact on a running system. The code for the update mode and the management of the VPAs I still think will be useful though, and I would like to still include them. However I can be sure to keep the labels, annotations, and other actions of the VPA controller commented out with notes or something similar. Then later on y'all can decide to enable or remove these features as they will be undocumented yet still living in the code, just disabled. Thoughts? |
Here's an xl PR that refactors the reconciler
pkg/vpato allow for more flexible design and incremental improvements in the future. Below is the original PR description cc @while1malloc0Note: This PR is split into 2 parts. The original PR was a refactor + the vpa update mode implementation. I decided to split the PR to help make the second PR easier to digest what feature is being added in the future.
What this PR does/Why
This (the next PR will implement...) PR implements the user specification of the update mode for the vertical pod autoscalers created by Goldilocks. (This PR does:) In order to make this easier, the vpa package has been refactored heavily. The heavy refactoring was justified by the fact that this package is both one of the most cyclomatically complex, and has the heaviest churn of any package in the program. The refactoring commits are as atomic as possible, which should hopefully ease review.
Reorganize Reconciler code ordering
The vpa.Reconciler function read a bit funny to me: a bunch of private
methods were listed before the exported methods. When someone is going
to read that file, they most likely want to see the public API of the
struct before they see the private functions that implement it. This
commit reorders things so that the public API is listed first in the
file.
Add DryRun field to the Reconciler
The vpa.Reconciler currently uses a passthrough dry-run variable.
However the dry run behavior is part of the state of the reconciler, and
it shouldn't change within the lifecycle of the reconciler. Decoupling
it from the reconciler gives the appearance that the dry run behavior
should somehow be separate, and results in a variable that has to be
passed around to private package functions.
Make list/create/deleteVPA Reconciler methods
These functions all take data clumps of internal state of the
Reconciler. While decoupling them might be useful if they were exported
package functions, the fact that they're unexported means that the
Reconciler is their only caller. Given those two facts, it makes more
sense to couple the these functions with the Reconciler.
Rename checkNamespaceLabel to namespaceIsManaged
The name checkNamespaceLabel describes what is done mechanically, not
why. namespaceIsManaged should hopefully give a clearer sense of what
the boolean return value means for this method.
Refactor Reconciler.ReconcileNamespace
The main exported API on of the vpa package is the
Reconciler.ReconcilerNamespace method. However, this method is a bit
hard to read because it mixes levels of abstraction quite frequently.
This leads to comments being used to convey semantic information. This
commit refactors the main reconciliation loop into a series of
unexported methods, allowing the reader of the code to get a better
sense of why certain operations are taking place.
Add a VPAUpdateMode to the vpa.Reconciler (Next PR)
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 (Next PR)
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.
Notes to reviewer
I'm still not super happy with this PR, but it's an improvement. There seems to be some notion of a "snapshot" of a namespace's state that's floating in the ether of this package, and right now that's manifested as lists of strings that are passed around. I'm hoping that this refactor pulls the majority of the package up a level of abstraction so that the best manifestation of that becomes clear.