-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable caching using memcached #286
Conversation
/hold I need to verify something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but it is making the deployment of memcached a hard requirement.
I would prefer an approach that would make it optional, for example when the memcachedInstance
is set to an empty string the enabled
in the global-defaults is set to false
.
controllers/cinder_controller.go
Outdated
@@ -780,6 +851,7 @@ func (r *CinderReconciler) generateServiceConfigs( | |||
instance *cinderv1beta1.Cinder, | |||
envVars *map[string]env.Setter, | |||
serviceLabels map[string]string, | |||
mc *memcachedv1.Memcached, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Wouldn't it make it more readable using memcached
instead of mc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
Jira: OSP-26501
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't know exactly where the discussion took place, my understanding is memcached is a required component (just as it is in tripleo). I recall seeing (but cannot find) a comment in another operator's PR that it is not necessary, at least at this juncture, to provide support for conditionally configuring memcached. See also https://issues.redhat.com/browse/OSP-26273
@@ -30,6 +30,11 @@ osapi_volume_workers = 4 | |||
control_exchange = openstack | |||
api_paste_config = /etc/cinder/api-paste.ini | |||
|
|||
[cache] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cinder and other storage services do not configure the [cache] section in tripleo deployments. The initial goal is feature parity, so I'll remove this section. Whether it actually should be present can be discussed as a future enhancement.
controllers/cinder_controller.go
Outdated
@@ -780,6 +851,7 @@ func (r *CinderReconciler) generateServiceConfigs( | |||
instance *cinderv1beta1.Cinder, | |||
envVars *map[string]env.Setter, | |||
serviceLabels map[string]string, | |||
mc *memcachedv1.Memcached, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
/unhold |
/retest |
/retest '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate in certificate chain (_ssl.c:1129)' |
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Akrog, ASBishop 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 |
Follows openstack-k8s-operators/cinder-operator#286 Jira: OSPRH-1524 Signed-off-by: John Fulton <[email protected]>
Follows openstack-k8s-operators/cinder-operator#286 Jira: OSPRH-1524 Signed-off-by: John Fulton <[email protected]>
Add openstack-baremetal-operator to openstack-operator bundle
Jira: OSP-26501