Enable Watcher storage collector by default #190
Conversation
|
[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 |
|
/hold |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d952966d65dd4bee82bbe02725da9808 ❌ openstack-meta-content-provider-master FAILURE in 9m 07s |
There was a problem hiding this comment.
General comment, although I think this approach of dynamically configuring storage backend is doable, I'd consider a better approach to manage that in watcher, so that it simply skips storage model collection if there is no cinder service in the cluster.
Actually, I'd say it already does it pretty well as it will just skip running it unless a storage scope is included in an audit
and, in that case, even if the storage plugin is not enable by default, the strategy itself will trigger a storage model collection iirc, so this parameter will be irrelevant
I'd be more inclined to enable storage by default in the watcher-operator and make sure watcher has an acceptable behavior if cinder is not enabled (maybe reporting a warning message) from watcher code.
Said that, this is doable, but I consider adds unneeded complexity. I'm leaving some comments in case we want to follow the dynamic configuration approach.
|
|
||
| isCinderEnabled := isCinderEnabled(ctx, helper, instance.Namespace) | ||
| // store whether cinder is enabled so that it can trigger a restart if it changes | ||
| (*envVars)["CinderEnabled"] = env.SetValue(strconv.FormatBool(isCinderEnabled)) |
There was a problem hiding this comment.
Do we need this envVar anywhere?
There was a problem hiding this comment.
this is passed later to createHashOfInputHashes to create a hash to check if we need to trigger a reconcile
| Watches(&keystonev1.KeystoneEndpoint{}, | ||
| handler.EnqueueRequestsFromMapFunc(r.findObjectsWithAppSelectorLabelInNamespace), | ||
| builder.WithPredicates(keystonev1.KeystoneEndpointStatusChangedPredicate), | ||
| ). |
There was a problem hiding this comment.
If i understand properly, the logic implemented in EnqueueRequestsFromMapFunc, this will trigger reconcile loop on any change in any of the keystone endpoints.
I think this may be refined to trigger the reconcile only if the changed endpoint is a cinder one. An example:
as discussed somewhere else, I'll revert back to the original commit and enable both the storage and compute models since as @amoralej points out, enabling the storage model without cinder should be safe as long as the user does not create any audit that involves storage migration |
cescgina
force-pushed
the
enable_storage_collector
branch
from
7021758 to
a519975
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5709c5a8aa05454b90accc59158f8092 ✔️ openstack-meta-content-provider-master SUCCESS in 3h 09m 55s |
Enable both the compute and storage model collector by default in the watcher-operator.
cescgina
force-pushed
the
enable_storage_collector
branch
from
a519975 to
58921c6
Compare
Enable both the compute and storage model collector by default in the
watcher-operator.