Clean conductors from db during rescaling #620

mrkisaolamb · 2023-12-04T15:37:12Z

No description provided.

gibizer · 2023-12-08T12:31:40Z

controllers/novaconductor_controller.go

@@ -446,6 +453,41 @@ func (r *NovaConductorReconciler) ensureDeployment(
 	return ctrl.Result{}, nil
 }

+func (r *NovaConductorReconciler) cleanServiceFromNovaDb(


This can be generalized now as it only differs from the scheduler one by Binary: "nova-conductor" so that can be passed as a parameter and also the keystoneAuthURL can be taken as a parameter.

done a68cc8c

gibizer · 2023-12-08T12:33:20Z

test/functional/novaconductor_controller_test.go

+				condition.DeploymentReadyCondition,
+				corev1.ConditionTrue,
+			)
+			Expect(novaAPIServer.FindRequest("DELETE", "/compute/os-services/3", "")).To(BeTrue())


Now that fixture records the query param please assert that the code queried a list of conductor services.

done a68cc8c

cool. As a follow up add the same to the schedule test

softwarefactory-project-zuul · 2023-12-08T13:06:45Z

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/14f8c0fdb2054343b690b3a735ca0290

✔️ nova-operator-content-provider SUCCESS in 2h 26m 29s
✔️ nova-operator-kuttl SUCCESS in 36m 34s
❌ nova-operator-tempest-multinode FAILURE in 2h 07m 00s

gibizer

I see a list of code cleanup possibilities. I did them in a81aba2 If you like it feel free to squash it in.

gibizer · 2023-12-08T13:52:32Z

test/functional/novaconductor_controller_test.go

+				condition.DeploymentReadyCondition,
+				corev1.ConditionTrue,
+			)
+			Expect(novaAPIServer.FindRequest("DELETE", "/compute/os-services/3", "")).To(BeTrue())


cool. As a follow up add the same to the schedule test

gibizer · 2023-12-08T13:58:15Z

controllers/common.go

@@ -102,6 +103,35 @@ type conditionsGetter interface {
 	GetConditions() condition.Conditions
 }

+func cleanNovaServiceFromNovaDb(ctx context.Context,
+	h *helper.Helper, authURL string, adminUser string,
+	authPassword string, timeout time.Duration, l logr.Logger, serviceName string) error {


timeout parameter is unused and therefore can be removed

I would move the ) error { to the next line for readability

gibizer · 2023-12-08T14:00:23Z

controllers/common.go

+func cleanNovaServiceFromNovaDb(ctx context.Context,
+	h *helper.Helper, authURL string, adminUser string,
+	authPassword string, timeout time.Duration, l logr.Logger, serviceName string) error {
+	computeClient, _, err := getNovaClient(ctx, h, authURL, adminUser, authPassword, defaultRequestTimeout, l)


It seems that the result return value of getNovaClient is unused so that can also be removed.

also as far as I see the defaultRequestTimeout is also unused in getNovaClient so the whole constant can be removed

gibizer · 2023-12-08T14:29:38Z

controllers/novaconductor_controller.go

+	err := cleanNovaServiceFromNovaDb(ctx, h, instance.Spec.KeystoneAuthURL,
+		instance.Spec.ServiceUser, authPassword, defaultRequestTimeout, l, "nova-conductor")


I think we can express that the authURL and the user name is coming from the same instance with some refactoring and shrink the parameter list.

softwarefactory-project-zuul · 2023-12-08T16:02:41Z

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/24ab078f910d428bb75e62abdf90fa15

✔️ nova-operator-content-provider SUCCESS in 2h 39m 16s
✔️ nova-operator-kuttl SUCCESS in 39m 08s
❌ nova-operator-tempest-multinode FAILURE in 2h 21m 48s

mrkisaolamb · 2023-12-11T09:01:08Z

I see a list of code cleanup possibilities. I did them in a81aba2 If you like it feel free to squash it in.

I cherry-pick commit

mrkisaolamb · 2023-12-11T10:28:32Z

/retest

openshift-ci · 2023-12-12T14:59:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [gibizer,mrkisaolamb]

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 Dec 4, 2023

openshift-ci bot requested review from abays and bogdando December 4, 2023 15:37

openshift-ci bot added the approved label Dec 4, 2023

mrkisaolamb force-pushed the conductor-cleaning branch from 6997dc7 to 074fc0b Compare December 8, 2023 10:38

mrkisaolamb changed the title ~~WIP: Clean conductors from db during rescaling~~ Clean conductors from db during rescaling Dec 8, 2023

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

mrkisaolamb requested a review from gibizer December 8, 2023 10:39

gibizer reviewed Dec 8, 2023

View reviewed changes

mrkisaolamb requested a review from gibizer December 8, 2023 13:22

gibizer reviewed Dec 8, 2023

View reviewed changes

mrkisaolamb requested a review from gibizer December 11, 2023 10:11

mrkisaolamb and others added 3 commits December 11, 2023 11:11

Clean conductors from db during rescaling

039f2ac

Refactor deleting service from api

d22e609

Refactor service cleanup

79401a4

mrkisaolamb force-pushed the conductor-cleaning branch from 196a029 to 79401a4 Compare December 11, 2023 10:11

gibizer approved these changes Dec 12, 2023

View reviewed changes

openshift-ci bot assigned gibizer Dec 12, 2023

openshift-ci bot added the lgtm label Dec 12, 2023

gibizer mentioned this pull request Dec 12, 2023

DNM: refactor sevice cleanup #623

Closed

openshift-merge-bot bot merged commit 455707a into openstack-k8s-operators:main Dec 12, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean conductors from db during rescaling #620

Clean conductors from db during rescaling #620

mrkisaolamb commented Dec 4, 2023

gibizer Dec 8, 2023

mrkisaolamb Dec 8, 2023 •

edited

Loading

gibizer Dec 8, 2023

mrkisaolamb Dec 8, 2023

gibizer Dec 8, 2023

softwarefactory-project-zuul bot commented Dec 8, 2023

gibizer left a comment

gibizer Dec 8, 2023

gibizer Dec 8, 2023

gibizer Dec 8, 2023

gibizer Dec 8, 2023

gibizer Dec 8, 2023

softwarefactory-project-zuul bot commented Dec 8, 2023

mrkisaolamb commented Dec 11, 2023

mrkisaolamb commented Dec 11, 2023

openshift-ci bot commented Dec 12, 2023

		err := cleanNovaServiceFromNovaDb(ctx, h, instance.Spec.KeystoneAuthURL,
		instance.Spec.ServiceUser, authPassword, defaultRequestTimeout, l, "nova-conductor")

Clean conductors from db during rescaling #620

Clean conductors from db during rescaling #620

Conversation

mrkisaolamb commented Dec 4, 2023

Choose a reason for hiding this comment

mrkisaolamb Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 8, 2023

gibizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 8, 2023

mrkisaolamb commented Dec 11, 2023

mrkisaolamb commented Dec 11, 2023

openshift-ci bot commented Dec 12, 2023

mrkisaolamb Dec 8, 2023 •

edited

Loading