standardize scope local cache #2657
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jackfrancis
force-pushed
the
scope-cache
branch
from
fd22884 to
c0b1a9b
Compare
|
/test pull-cluster-api-provider-azure-e2e-exp |
| } | ||
|
|
||
| // InitMachineCache sets cached information about the machine to be used in the scope. | ||
| func (m *MachineScope) InitMachineCache(ctx context.Context) error { |
There was a problem hiding this comment.
@devigned do you have any concerns about replacing this "do a one-time initiatlization of all cached properties when we instantiate scope" vs implement the property value caching on demand?
My thinking (and this was @CecileRobertMichon 's thinking when we implemented caching in a prior PR) is that not all scope lifecycles are going to require access to all of these properties, and so in the act of caching them we are actually deoptimizing things by fetching data unnecessarily.
|
/retest |
| spec.SKU = m.cache.VMSKU | ||
| spec.Image = m.cache.VMImage | ||
| spec.BootstrapData = m.cache.BootstrapData | ||
| if bootstrapData, err := m.GetBootstrapData(ctx); err == nil { |
There was a problem hiding this comment.
We should be handling the errors here and below
if I remember correctly that was one of the main reasons we decided to go with a separate InitMachineCache func which would be able to handle errors and pass in context without requiring those on the Spec functions.
| } | ||
|
|
||
| // NodeStatus represents the status of a Kubernetes node. | ||
| NodeStatus struct { | ||
| Ready bool | ||
| Version string | ||
| } | ||
|
|
||
| // MachinePoolCache stores common machine information so we don't have to hit the API multiple times within the same reconcile loop. |
There was a problem hiding this comment.
| // MachinePoolCache stores common machine information so we don't have to hit the API multiple times within the same reconcile loop. | |
| // MachinePoolCache stores common machine pool information so we don't have to hit the API multiple times within the same reconcile loop. |
|
/hold Not sure we actually want this change |
k8s-ci-robot
added
the
do-not-merge/hold
Going to close it to remove noise for reviewers in PR queue, please reopen if/when it's ready for review /close |
|
@CecileRobertMichon: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
This PR picks up where this one left off:
#2604
Here we standardize the way we maintain a local scope cache (in other words, we cache values that are retrieved externally so that we only have to fetch them once in any given scope lifecycle [e.g., one reconciliation loop]).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: