Add cronjob to periodically purge the DB #295
Conversation
|
/retest |
api/v1beta1/cinder_types.go
Outdated
| "github.com/openstack-k8s-operators/lib-common/modules/storage" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| const ( | ||
| // CinderUserID - Kolla's cinder UID |
There was a problem hiding this comment.
nit: it could be good to point to the source code were it is defined https://github.com/openstack/kolla/blob/master/kolla/common/users.py
api/v1beta1/cinder_types.go
Outdated
| @@ -172,6 +185,32 @@ type Cinder struct { | |||
| Status CinderStatus `json:"status,omitempty"` | |||
| } | |||
|
|
|||
| // DBPurge struct is used to model the parameters exposed to the Manila API CronJob | |||
api/v1beta1/cinder_webhook.go
Outdated
| @@ -86,6 +97,11 @@ func (spec *CinderSpec) Default() { | |||
| // This is required, as the loop variable is a by-value copy | |||
| spec.CinderVolumes[index] = cinderVolume | |||
| } | |||
|
|
|||
| if (spec.DBPurge.Age == 0) && (spec.DBPurge.Schedule == "") { | |||
There was a problem hiding this comment.
-1: This seems wrong, shouldn't they be independently checked? What happens when only one of the 2 fields is changed? Or is that not possible?
There was a problem hiding this comment.
I'm not sure if it's possible, but I see no reason why these shouldn't be independently checked, so I'll update the code.
There was a problem hiding this comment.
yeah, I think the main problem we were trying to solve here is when the CR is applied and no dbPurge related section is added. kubebuilder is basically not initializing the fields of the structs with the default values, and itresulted in something like dbPurge: {}.
However, without digging more into the original issue, I think that the latest @ASBishop's PS is good enough, and processing the two params independently works fine (and covers the previous case I mentioned).
I'm going to update the Manila patch the same way, thanks for the great input!
pkg/cindervolume/statefulset.go
Outdated
| @@ -44,8 +44,8 @@ func StatefulSet( | |||
| rootUser := int64(0) | |||
| // Cinder's uid and gid magic numbers come from the 'cinder-user' in | |||
| // https://github.com/openstack/kolla/blob/master/kolla/common/users.py | |||
There was a problem hiding this comment.
-1: Remove these 2 comment lines
pkg/cinderscheduler/statefulset.go
Outdated
| @@ -35,16 +35,16 @@ const ( | |||
|
|
|||
| // StatefulSet func | |||
| func StatefulSet( | |||
| instance *cinderv1beta1.CinderScheduler, | |||
| instance *cinderv1.CinderScheduler, | |||
| configHash string, | |||
| labels map[string]string, | |||
| annotations map[string]string, | |||
| ) *appsv1.StatefulSet { | |||
| rootUser := int64(0) | |||
| // Cinder's uid and gid magic numbers come from the 'cinder-user' in | |||
| // https://github.com/openstack/kolla/blob/master/kolla/common/users.py | |||
There was a problem hiding this comment.
-1: Remove these 2 comment lines
pkg/cinderbackup/statefulset.go
Outdated
| @@ -44,8 +44,8 @@ func StatefulSet( | |||
| rootUser := int64(0) | |||
| // Cinder's uid and gid magic numbers come from the 'cinder-user' in | |||
| // https://github.com/openstack/kolla/blob/master/kolla/common/users.py | |||
There was a problem hiding this comment.
-1: Remove this from here.
nit: Move it to the constant definition as mentioned above.
| cronjobDef := cinder.CronJob(instance, serviceLabels, serviceAnnotations) | ||
| cronjob := cronjob.NewCronJob( | ||
| cronjobDef, | ||
| 5*time.Second, |
There was a problem hiding this comment.
nit: Since this is not a job we need "right now" we can probably set a higher reconciliation timeout value.
There was a problem hiding this comment.
Sure, but what would that value be?
There was a problem hiding this comment.
Good question, I don't know :-P
|
/retest looks like temporary issues with ci infrastructure |
api/v1beta1/cinder_types.go
Outdated
| } | ||
|
|
||
| // CinderDebug indicates whether certain stages of Cinder deployment should | ||
| // pause in debug mode, or if debug is enabled when purging the DB. |
There was a problem hiding this comment.
-1: If we are no longer going to pause on these actions we should rephrase this.
There was a problem hiding this comment.
Um, I thought I did. But I can remove this comment in favor of explicitly describing each flag within the structure.
api/v1beta1/cinder_types.go
Outdated
| type CinderDebug struct { | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default=false | ||
| // dbSync enable debug |
There was a problem hiding this comment.
Mention that this debug action pauses the container.
api/v1beta1/cinder_types.go
Outdated
| type CinderDebug struct { | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default=false | ||
| // DBSync pauses the dbSync container prior to executing the db_sync command. |
There was a problem hiding this comment.
-1: I hate to downvote for this, but the comment is not correct, it's an infinite sleep, so it will never execute the dbsync command.
Maybe it's not the best command, and we should have a command to sleep until a file is removed, like I did in openstack-must-gather, but it's what we have right now.
There was a problem hiding this comment.
Changing "prior to executing" to "instead of executing" per our conversation.
The cron schedule and the age at which records are purged are configurable in the CRD, with the same default values chosen for TripleO based deployments.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Akrog, ASBishop, fmount 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 |
|
/retest |
1 similar comment
|
/retest |
openshift-merge-bot
bot
merged commit 3771599
into
openstack-k8s-operators:main
Add Nova defaulting mechanism
The cron schedule and the age at which records are purged are configurable in the CRD, with the same default values chosen for TripleO based deployments.