Expose DBPurge Age and Schedule #170

fmount · 2023-10-31T08:38:49Z

This patch updates the current manila CR to expose both dbPurgeAge and dbPurgeSchedule as customizable parameters. They were previously hardcoded as const in the code, and it makes sense removing that part as long as the human operator has the ability to customize them.

api/v1beta1/manila_types.go

api/v1beta1/common_types.go

fmount · 2023-11-10T13:32:24Z

Let's see the CI in action with this change

fmount · 2023-11-10T15:05:38Z

From the CI execution we can see that the Manila CR has the dbPurge struct properly initialized [1] and the cronJob has been properly created [2].

[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_manila-operator/170/pull-ci-openstack-k8s-operators-manila-operator-main-manila-operator-build-deploy-tempest/1722971224570597376/artifacts/manila-operator-build-deploy-tempest/openstack-k8s-operators-gather/artifacts/must-gather/quay-io-openstack-k8s-operators-openstack-must-gather-sha256-0812f031e363406238d47a3ba3cfb33412e9a2d143d2b4b9365c9796b80bb8aa/namespaces/openstack/crs/manilas.manila.openstack.org/manila.yaml

[2] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openstack-k8s-operators_manila-operator/170/pull-ci-openstack-k8s-operators-manila-operator-main-manila-operator-build-deploy-tempest/1722971224570597376/artifacts/manila-operator-build-deploy-tempest/openstack-k8s-operators-gather/artifacts/must-gather/quay-io-openstack-k8s-operators-openstack-must-gather-sha256-0812f031e363406238d47a3ba3cfb33412e9a2d143d2b4b9365c9796b80bb8aa/namespaces/openstack/cronjobs/manila-cron.yaml

ASBishop

In addition to some (very minor) comments noted in the code, I have another suggestion that I'll describe here:

As things currently stand:

main.go calls manilav1beta1.SetupDefaults()
But that function is defined in common_types.go, which doesn't feel write to me (it's a function, not a type)
SetupDefaults (in common_types.go) initializes a local variable with the default values, and then calls SetupManilaDefaults()
SetupManilaDefaults is implemented in manila_webhook.go, and it essentially saves a copy of the input parameter to the "manilaDefaults" variable defined in the file

This seems overly convoluted. I suggest:

(Re)move the SetupDefaults() function from common_types.go entirely
In manila_webhook.go, rename SetupManilaDefaults() to SetupDefaults(), and add the initialization code that was previously done in common_types.go. It would look like this:

// SetupDefaults - initialize Manila spec defaults for use with either internal or external webhooks
func SetupDefaults() {
	// Acquire environmental defaults and initialize Manila defaults with them
	manilaDefaults = ManilaDefaults{
		APIContainerImageURL:       util.GetEnvVar("RELATED_IMAGE_MANILA_API_IMAGE_URL_DEFAULT", ManilaAPIContainerImage),
		SchedulerContainerImageURL: util.GetEnvVar("RELATED_IMAGE_MANILA_SCHEDULER_IMAGE_URL_DEFAULT", ManilaSchedulerContainerImage),
		ShareContainerImageURL:     util.GetEnvVar("RELATED_IMAGE_MANILA_SHARE_IMAGE_URL_DEFAULT", ManilaShareContainerImage),
		DBPurgeAge:                 DBPurgeDefaultAge,
		DBPurgeSchedule:            DBPurgeDefaultSchedule,
	}

	manilalog.Info("Manila defaults initialized", "defaults", manilaDefaults)
}

ASBishop · 2023-11-10T18:44:42Z

api/v1beta1/manila_types.go

+	// +kubebuilder:validation:Optional
+	// +kubebuilder:default=false
+	// DBPurge enables debug mode on the DBPurge CronJob
+	DBPurge bool `json:"debug,omitempty"`


I think the json name should be dbPurge:

DBPurge bool `json:"dbPurge,omitempty"`

Otherwise the CRD looks like this:

spec: debug: dbSync: false debug: false <=== spec.debug.debug looks odd to me

+1 it's odd, basically I didn't pay too much attention, I'll fix it in my next PS

api/v1beta1/common_types.go

api/v1beta1/manila_webhook.go

ASBishop · 2023-11-10T19:11:38Z

But this is looking good, so I think you can promote if from a mere WIP!

fmount · 2023-11-11T13:36:15Z

In addition to some (very minor) comments noted in the code, I have another suggestion that I'll describe here:

As things currently stand:

* main.go calls manilav1beta1.SetupDefaults()

* But that function is defined in common_types.go, which doesn't feel write to me (it's a function, not a type)

* SetupDefaults (in common_types.go) initializes a local variable with the default values, and then calls SetupManilaDefaults()

* SetupManilaDefaults is implemented in manila_webhook.go, and it essentially saves a copy of the input parameter to the "manilaDefaults" variable defined in the file

This seems overly convoluted. I suggest:

* (Re)move the SetupDefaults() function from common_types.go entirely

* In manila_webhook.go, rename SetupManilaDefaults() to SetupDefaults(), and add the initialization code that was previously done in common_types.go. It would look like this:

// SetupDefaults - initialize Manila spec defaults for use with either internal or external webhooks
func SetupDefaults() {
	// Acquire environmental defaults and initialize Manila defaults with them
	manilaDefaults = ManilaDefaults{
		APIContainerImageURL:       util.GetEnvVar("RELATED_IMAGE_MANILA_API_IMAGE_URL_DEFAULT", ManilaAPIContainerImage),
		SchedulerContainerImageURL: util.GetEnvVar("RELATED_IMAGE_MANILA_SCHEDULER_IMAGE_URL_DEFAULT", ManilaSchedulerContainerImage),
		ShareContainerImageURL:     util.GetEnvVar("RELATED_IMAGE_MANILA_SHARE_IMAGE_URL_DEFAULT", ManilaShareContainerImage),
		DBPurgeAge:                 DBPurgeDefaultAge,
		DBPurgeSchedule:            DBPurgeDefaultSchedule,
	}

	manilalog.Info("Manila defaults initialized", "defaults", manilaDefaults)
}

This is interesting, thanks for this suggestion that I think is in line w/ the effort around code dedup and help with the whole webhooks related logic.
TBH I didn't pay enough attention at the webhook implementation across the operators (and I tend to disable them in my local testing because they have been used for containerImages only), but from a pure coding perspective I see your concern and this seems actually over engineering work ("it wraps the wrapper") that can be simplified.
Let me try what you're suggesting in the next PS and we can do a second pass over the code to see if it's in line with what you have in mind and can be something used consistently across the operators.

softwarefactory-project-zuul · 2023-11-14T09:40:49Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/647d757cf0c5482485f276037169f8d2

❌ openstack-k8s-operators-content-provider FAILURE in 10m 46s

fmount · 2023-11-14T10:18:03Z

@viroel I think the zuul kuttl test is still not ready to manage changes in the API, and I'm wondering if we have a way to set it as "non-voting" or override the result.
When it will be stable enough I'll remove the Prow based job.

softwarefactory-project-zuul · 2023-11-14T10:28:14Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/b5d87a480e164b108c8ade9edc0a43cc

❌ openstack-k8s-operators-content-provider FAILURE in 10m 42s

viroel · 2023-11-14T13:29:53Z

@viroel I think the zuul kuttl test is still not ready to manage changes in the API, and I'm wondering if we have a way to set it as "non-voting" or override the result. When it will be stable enough I'll remove the Prow based job.

Yes, you just need to set the job to voting: false, you can do this at the pipeline [1].
Another issue is that the job is not running due to parent's files configuration. You should be able to fix this replacing parent's job with cifmw-base-multinode-kuttl instead.

[1] https://github.com/rdo-infra/rdo-jobs/blob/c18955ec6951e50a5cf75b6662c9ab015f42d03c/zuul.d/project-templates.yaml#L206

softwarefactory-project-zuul · 2023-11-14T13:35:43Z

Zuul encountered a syntax error while parsing its configuration in the
repo openstack-k8s-operators/manila-operator on branch main. The
error was:

expected str for dictionary value @ data['github-check']['jobs'][1]['voting']

The error appears in the following project stanza:

project:
name: openstack-k8s-operators/manila-operator
github-check:
jobs:
- openstack-k8s-operators-content-provider
- manila-operator-kuttl:
dependencies:
- openstack-k8s-operators-content-provider
voting: false

in "openstack-k8s-operators/manila-operator/zuul.d/projects.yaml@main", line 2, column 3

fmount · 2023-11-14T15:31:11Z

/test manila-operator-build-deploy-tempest

ASBishop · 2023-11-14T18:23:00Z

api/v1beta1/manila_webhook.go

@@ -82,6 +94,11 @@ func (spec *ManilaSpec) Default() {
 			spec.ManilaShares[key] = manilaShare
 		}
 	}
+
+	if (spec.DBPurge.Age == 0) && (spec.DBPurge.Schedule == "") {


See Gorka's comment for the corresponding code in openstack-k8s-operators/cinder-operator#295

+1 and thanks for sharing, just commented and updated the patch. I think it's ok to process these two struct fields independently.

ASBishop · 2023-11-14T18:25:31Z

api/v1beta1/manila_types.go

+	// +kubebuilder:validation:Optional
+	// +kubebuilder:default=30
+	// Age is the DBPurgeAge parameter and indicates the number of days of purging DB records
+	Age int `json:"age"`


Will you add the "+kubebuilder:validation:Minimum=1" in this PR (after CI issues are resolved) or in a follow-up patch?

I'm going to add this line in a follow up patch because I have that one line change that allows me to focus more on the CI (zuul based) execution rather than the change introduced by the patch.

softwarefactory-project-zuul · 2023-11-14T20:25:11Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/e59184c3a0ee459da818438c95693abc

❌ openstack-k8s-operators-content-provider FAILURE in 8m 49s
⚠️ manila-operator-kuttl SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider (non-voting)

fmount · 2023-11-15T07:11:24Z

/retest

fmount · 2023-11-15T08:54:40Z

/override rdoproject.org/github-check

openshift-ci · 2023-11-15T08:54:45Z

@fmount: Overrode contexts on behalf of fmount: rdoproject.org/github-check

In response to this:

/override rdoproject.org/github-check

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.

This patch updates the current manila CR to expose both dbPurgeAge and dbPurgeSchedule as customizable parameters. They were previously hardcoded as constants in the code, and it makes sense removing that part as long as the human operator has the ability to customize them. Signed-off-by: Francesco Pantano <[email protected]>

ASBishop

/lgtm

openshift-ci · 2023-11-27T21:00:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [fmount]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Oct 31, 2023

openshift-ci bot requested review from abays and stuggi October 31, 2023 08:38

openshift-ci bot added the approved label Oct 31, 2023

fmount changed the title ~~WIP - Expose dbPurgeAge and dbPurgeSchedule to the top-level manila CR~~ WIP - Expose dbPurgeAge and dbPurgeSchedule Oct 31, 2023

fmount requested review from ASBishop and gouthampacha and removed request for abays and stuggi October 31, 2023 08:39

fmount force-pushed the config_cron branch from d72bb95 to d484fff Compare October 31, 2023 08:46

fmount changed the title ~~WIP - Expose dbPurgeAge and dbPurgeSchedule~~ WIP - Expose DBPurge Age and Schedule Oct 31, 2023

fmount force-pushed the config_cron branch 3 times, most recently from 651645a to dd80992 Compare October 31, 2023 14:29

fmount commented Oct 31, 2023

View reviewed changes

api/v1beta1/manila_types.go Outdated Show resolved Hide resolved

fmount force-pushed the config_cron branch from dd80992 to 4fbdfb6 Compare November 1, 2023 08:18

ASBishop reviewed Nov 1, 2023

View reviewed changes

api/v1beta1/manila_types.go Outdated Show resolved Hide resolved

api/v1beta1/manila_types.go Outdated Show resolved Hide resolved

ASBishop requested changes Nov 1, 2023

View reviewed changes

api/v1beta1/manila_types.go Show resolved Hide resolved

openshift-ci bot assigned ASBishop Nov 1, 2023

fmount force-pushed the config_cron branch from 4fbdfb6 to 0efd8de Compare November 10, 2023 13:27

fmount commented Nov 10, 2023

View reviewed changes

api/v1beta1/common_types.go Outdated Show resolved Hide resolved

fmount force-pushed the config_cron branch from 0efd8de to 9e4c240 Compare November 10, 2023 13:31

ASBishop requested changes Nov 10, 2023

View reviewed changes

fmount force-pushed the config_cron branch from 9e4c240 to b7e20ad Compare November 11, 2023 13:53

fmount changed the title ~~WIP - Expose DBPurge Age and Schedule~~ Expose DBPurge Age and Schedule Nov 11, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 11, 2023

fmount force-pushed the config_cron branch from faaef11 to 9eddd3e Compare November 14, 2023 10:15

fmount mentioned this pull request Nov 14, 2023

Add zuul based kuttl test #179

Merged

fmount force-pushed the config_cron branch 3 times, most recently from d5f7a85 to 9e7b87e Compare November 14, 2023 12:48

fmount requested a review from ASBishop November 14, 2023 12:48

fmount force-pushed the config_cron branch from 9e7b87e to 5cc2cef Compare November 14, 2023 13:35

fmount force-pushed the config_cron branch from 5cc2cef to 1400c8a Compare November 14, 2023 13:36

ASBishop reviewed Nov 14, 2023

View reviewed changes

fmount force-pushed the config_cron branch from 1400c8a to 3058973 Compare November 14, 2023 20:14

fmount requested a review from ASBishop November 14, 2023 20:18

fmount removed the do-not-merge/hold label Nov 16, 2023

fmount force-pushed the config_cron branch from 3058973 to f3447c6 Compare November 24, 2023 20:40

fmount force-pushed the config_cron branch from f3447c6 to a1ed778 Compare November 24, 2023 20:46

ASBishop approved these changes Nov 27, 2023

View reviewed changes

openshift-ci bot added the lgtm label Nov 27, 2023

openshift-merge-bot bot merged commit f465db7 into openstack-k8s-operators:main Nov 27, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose DBPurge Age and Schedule #170

Expose DBPurge Age and Schedule #170

fmount commented Oct 31, 2023

fmount commented Nov 10, 2023

fmount commented Nov 10, 2023

ASBishop left a comment

ASBishop Nov 10, 2023

fmount Nov 11, 2023

ASBishop commented Nov 10, 2023

fmount commented Nov 11, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 14, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

viroel commented Nov 14, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 14, 2023

ASBishop Nov 14, 2023

fmount Nov 14, 2023

ASBishop Nov 14, 2023

fmount Nov 14, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 15, 2023

fmount commented Nov 15, 2023

openshift-ci bot commented Nov 15, 2023

ASBishop left a comment

openshift-ci bot commented Nov 27, 2023

Expose DBPurge Age and Schedule #170

Expose DBPurge Age and Schedule #170

Conversation

fmount commented Oct 31, 2023

fmount commented Nov 10, 2023

fmount commented Nov 10, 2023

ASBishop left a comment

Choose a reason for hiding this comment

ASBishop Nov 10, 2023

Choose a reason for hiding this comment

fmount Nov 11, 2023

Choose a reason for hiding this comment

ASBishop commented Nov 10, 2023

fmount commented Nov 11, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 14, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

viroel commented Nov 14, 2023

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 14, 2023

ASBishop Nov 14, 2023

Choose a reason for hiding this comment

fmount Nov 14, 2023

Choose a reason for hiding this comment

ASBishop Nov 14, 2023

Choose a reason for hiding this comment

fmount Nov 14, 2023

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Nov 14, 2023

fmount commented Nov 15, 2023

fmount commented Nov 15, 2023

openshift-ci bot commented Nov 15, 2023

ASBishop left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 27, 2023