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

Delete NovaCell cr only when deletion job pass #917

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrkisaolamb
Copy link
Contributor

@openshift-ci openshift-ci bot requested review from gibizer and lewisdenny January 15, 2025 15:13
Copy link
Contributor

openshift-ci bot commented Jan 15, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Comment on lines +612 to +615
if result == nova.CellDeleteComplete {
Log.Info("Cell deleted", "cell", cr.Spec.CellName)
delete(instance.Status.RegisteredCells, cr.Name)
}
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 the result is not CellDeleteComplete then we return success from the reconciler below and allowing the NovaCell object to be deleted, but we keep the nova.Status.RegisteredCells entry. However when the job finally finishes and a new reconciler starts on nova controller the novaCellList above will not containt the NovaCell CR any more so the nova controller will not cleanup the RegisteredCells entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I added additional loop to check if registered cell job was sucess to ensure that we have correct status

@@ -718,22 +720,22 @@ func (r *NovaReconciler) ensureCellDeleted(

_, err = job.DoJob(ctx, h)
if err != nil {
return err
return nova.CellDeleteFailed, err
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check the result of the DoJob to see if the deletion job is still running and return to the caller if so.

@@ -96,6 +96,12 @@ const (
// CellComputeDiscoveryReady indicates that all NovaComputes in cell reached the Ready status and
// the hosts in the cell have been successfully discovered
CellComputeDiscoveryReady CellDeploymentStatus = iota
// CellDeleteInProgress indicates that the NovaCell deletion is in progress
CellDeleteInProgress CellDeploymentStatus = iota
Copy link
Contributor

Choose a reason for hiding this comment

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

You are right we need in progress indication. You however missed to use this status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I missed this somehow, now value is used in jobs checks

Copy link

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://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/3e56241f2d694cbfbd56fc77a8ec26aa

✔️ openstack-meta-content-provider SUCCESS in 2h 42m 09s
nova-operator-kuttl RETRY_LIMIT in 28m 08s
✔️ nova-operator-tempest-multinode SUCCESS in 1h 59m 18s
✔️ nova-operator-tempest-multinode-ceph SUCCESS in 2h 25m 27s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants