🐛 Download and store manifests from provider URL #131
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demicev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
|
||
| log := ctrl.LoggerFrom(ctx) | ||
|
|
||
| log.Info("Downloading provider manifests") |
There was a problem hiding this comment.
we could add a verbosity to the logger:
| log.Info("Downloading provider manifests") | |
| log.V(5).Info("Downloading provider manifests") |
| func (p *phaseReconciler) load(ctx context.Context) (reconcile.Result, error) { | ||
| log := ctrl.LoggerFrom(ctx) | ||
|
|
||
| log.Info("Loading provider") |
There was a problem hiding this comment.
| log.Info("Loading provider") | |
| log.V(5).Info("Loading provider") |
There was a problem hiding this comment.
We actually moved these logs to the 0 level some time ago so the operator is not too silent by default
There was a problem hiding this comment.
@alexander-demicev you mean in all cases? Because I spotted
where it is using the log level, but if that is just a left out then you can disregard my commentThere was a problem hiding this comment.
I think setting log level is not strictly related to this PR, so for now we can keep it as-is and address it later.
Fedosin
force-pushed
the
download_manifests
branch
2 times, most recently
from
803636e to
6c819f6
Compare
There was a problem hiding this comment.
/lgtm
/hold
in case you want someone else as well to do another round of reviews before it is merged.
(@Fedosin feel free to unhold)
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
Fedosin
force-pushed
the
download_manifests
branch
from
6c819f6 to
67bed92
Compare
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
4 participants
What this PR does / why we need it:
Currently Cluster API Operator keeps refetching data from the specified URL after successful provider installation. It means that in case of network error Operator can't download manifests again and marks providers as unhealthy.
To prevent this, we fetch manifests only once in the beginning and store them in a configmap. For all the following reconciliations we take data from the configmap instead of downloading it from github/gitlab.
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 #119