Skip to content
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

Add method to bump generation in tests #222

Conversation

mrkisaolamb
Copy link
Contributor

In nova we need SimulateDeploymentReplicaReady
call after SimulateMariaDBAccountCompleted(newAccountName) too be able reconcile NovaCr. Changes is needed
because we added observeGeneration into subCrs
and now readines logic depend on generations.

@@ -69,6 +73,7 @@ func (harness *MariaDBTestHarness) Setup(
mariadb *TestHelper,
timeout time.Duration,
interval time.Duration,
runUpdateOptional ...bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we just create a logic in Setup that if UpdateGenerationCR is not provided then we don't run it? We should be able to check if UpdateGenerationCR is a real function or some golang default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if not provided the func var will be nil so we can simply check for nil before running it and we dont need to change the signature of Setup. https://go.dev/play/p/oj808HE7Jj8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 294 to 295
harness.UpdateAccount(newAccountName)
harness.mariaDBHelper.SimulateMariaDBAccountCompleted(newAccountName)

if harness.runUpdate {
harness.UpdateGenerationCR()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to find a better, more generic name for UpdateGenerationCR. As far as I understand the generic logic is that the account is updated and the made ready and then the service CR using the account needs to roll out the usage of the new account and then the finalizer removed from the old account. The impl of rolling out the usage of the new account is now requires extra steps as the service CR can only remove the finalizer if the new generation of the statefulset is ready. So based on that I would name this hook "UseNewAccount" or "SwitchToNewAccount"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

In nova we need SimulateDeploymentReplicaReady
call after SimulateMariaDBAccountCompleted(newAccountName)
too be able reconcile NovaCr. Changes is needed
because we added observeGeneration into subCrs
and now readines logic depend on generations.
@mrkisaolamb mrkisaolamb force-pushed the update_harness_test branch from 0c8cfdd to 0c306d2 Compare May 23, 2024 12:49
@mrkisaolamb mrkisaolamb requested a review from gibizer May 23, 2024 13:48
Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, mrkisaolamb, stuggi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jun 3, 2024
@gibizer
Copy link
Contributor

gibizer commented Jun 3, 2024

/test mariadb-operator-build-deploy-kuttl
seems like a build issue:

+ BASE_BUNDLE=docker.io/rdotripleomirror/openstack-operator-bundle:0c306d231aefe4b73a94-1797642540447436800
+ INDEX_DOCKERFILE=index.Dockerfile
+ DOCKERFILE_PATH_PATCH=(\{\"spec\":\{\"strategy\":\{\"dockerStrategy\":\{\"dockerfilePath\":\""${INDEX_DOCKERFILE}"\"\}\}\}\})
+ [[ openstack-operator == \o\p\e\n\s\t\a\c\k\-\o\p\e\r\a\t\o\r ]]
+ local OPENSTACK_BUNDLES
++ /bin/bash hack/pin-bundle-images.sh
parse error: Invalid numeric literal at line 2, column 0
+ OPENSTACK_BUNDLES=,quay.io/openstack-k8s-operators/barbican-operator-bundle:98dc2399536927dfdf00f8814f8af07befe60965,quay.io/openstack-k8s-operators/cinder-operator-bundle:4afc791bbb829683bbb85ca527f243e75f78de98,quay.io/openstack-k8s-operators/dataplane-operator-bundle:59c96c53e209dc85027a40d9e4b4ffde4b678b2c,quay.io/openstack-k8s-operators/designate-operator-bundle:b8f58a72095b98bc989cfb6ea7fa9b49ace91e3d,quay.io/openstack-k8s-operators/glance-operator-bundle:e5a2d7c523bd86a97eb8624eb7a6d0203412b1a2,quay.io/openstack-k8s-operators/heat-operator-bundle:47eb021b78762d1461c6eaebf661ad5cad5ab451,quay.io/openstack-k8s-operators/horizon-operator-bundle:9d2609bb0015818c7eb07a207f5942817c2273c3,quay.io/openstack-k8s-operators/infra-operator-bundle:715f01bb2987b15df23f0ccafd7f660e9c77b0aa,quay.io/openstack-k8s-operators/ironic-operator-bundle:7a25580a799d3a8daa26c0920a2c82e25fd24028,quay.io/openstack-k8s-operators/keystone-operator-bundle:1e3ee314289c125b074a6a67381568b5e888e355,quay.io/openstack-k8s-operators/manila-operator-bundle:04b02cfe0c97f0d98401455325098198c7edd294,docker.io/rdotripleomirror/mariadb-operator-bundle:0c306d231aefe4b73a94-1797642540447436800,quay.io/openstack-k8s-operators/neutron-operator-bundle:1aeb6862e5235dbdf9812fc7b68fab483302655d,quay.io/openstack-k8s-operators/nova-operator-bundle:ab95f150cbecb6cdf03795cc56d1516914c9e2fd,quay.io/openstack-k8s-operators/octavia-operator-bundle:c80b32fd671c1a9eaffbcb741b52ff81bbcde007,quay.io/openstack-k8s-operators/openstack-ansibleee-operator-bundle:7cd4595fa2d78f447d7eb7eab0cfd4487d8c0ae8,quay.io/openstack-k8s-operators/openstack-baremetal-operator-bundle:c332745390c8eecd74c86462cc0a107a096c48be,quay.io/openstack-k8s-operators/ovn-operator-bundle:77edfa5a3e1d7a7ab6340d89805c705ec405eefd

@openshift-merge-bot openshift-merge-bot bot merged commit b02b9f3 into openstack-k8s-operators:main Jun 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants