Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Always reset SetupReadyCondition to False #550

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Always reset SetupReadyCondition to False #550

merged 1 commit into from
Dec 12, 2023

Conversation

rabi
Copy link
Contributor

@rabi rabi commented Nov 30, 2023

We use this condition to trigger deployments for a nodeset. Without this being reset to false, deployments would tigger before nodeset is reconciled for update/patch as the condition would be true.

We use this condition to trigger deployments for a nodeset.
Without this being reset to false, deployments would tigger
before nodeset is reconciled for update/patch as the condition
would be true.
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://review.rdoproject.org/zuul/buildset/fc1d58b9de074058a5cf0c1e8f4124b7

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 32m 25s
✔️ podified-multinode-edpm-deployment-crc SUCCESS in 1h 07m 58s
cifmw-crc-podified-edpm-baremetal FAILURE in 1h 15m 00s

@rabi
Copy link
Contributor Author

rabi commented Dec 1, 2023

recheck

@slagle
Copy link
Collaborator

slagle commented Dec 4, 2023

I'd like to understand this a little better, and the scenario that's being solved.

deployments would tigger
before nodeset is reconciled for update/patch as the condition
would be true.

Only if the NodeSet is already SetupReady=True, and the deployment was already reconciling right? Specifically, are you trying to address the case where someone patches a NodeSet during an already reconciling Deployment?

It's likely an inconsistent pattern, but I don't see other conditions in other operators working the same as you're proposing here. Example, InputReady getting marked to False on every reconcile loop.

Forcing SetupReady=False on every reconcile seems unnecessary IMO, and may be inaccurate in many cases (we don't actually know that SetupReady is indeed False, granted we don't actually know that it's True either on every reconcile until we've checked).

@rabi
Copy link
Contributor Author

rabi commented Dec 5, 2023

Only if the NodeSet is already SetupReady=True, and the deployment was already reconciling right? Specifically, are you trying to address the case where someone patches a NodeSet during an already reconciling Deployment?

Nah, this is the scale-out case where a nodeset and deployment are both already reconciled. Then you patch the nodeset to add more nodes to the nodeset and create a (new) deployment. Because the nodset status is already SetupReady=True[1], the new deployment moves ahead and uses the old inventory and runs the aee jobs and the scale-out nodes are not deployed.

we don't actually know that SetupReady is indeed False, granted we don't actually know that it's True either on every reconcile until we've checked

During every reconcile we don't not know what is a specific status till reconciliation is gone through that step and updated the status. The assumption is that there won't be any reconcile if there is no spec change and if there is a spec change we don't know any of the statuses.

The other option is to evaluate SetupReady based on all other statuses that should be ready before that and in that case also we've to reset it back at the beginning of the reconciliation.

[1] https://github.com/openstack-k8s-operators/dataplane-operator/blob/main/controllers/openstackdataplanedeployment_controller.go#L148

@slagle
Copy link
Collaborator

slagle commented Dec 5, 2023

Only if the NodeSet is already SetupReady=True, and the deployment was already reconciling right? Specifically, are you trying to address the case where someone patches a NodeSet during an already reconciling Deployment?

Nah, this is the scale-out case where a nodeset and deployment are both already reconciled. Then you patch the nodeset to add more nodes to the nodeset and create a (new) deployment. Because the nodset status is already SetupReady=True[1], the new deployment moves ahead and uses the old inventory and runs the aee jobs and the scale-out nodes are not deployed.

Ok, that makes sense. Sounds good to me.

Copy link
Contributor

openshift-ci bot commented Dec 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, rabi

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

@rabi
Copy link
Contributor Author

rabi commented Dec 12, 2023

error: resource mapping not found for name: "openstack-galera" namespace: "openstack" from "STDIN": no matches for kind "OpenStackControlPlane" in version "core.openstack.org/v1beta1"
ensure CRDs are installed first

@rabi
Copy link
Contributor Author

rabi commented Dec 12, 2023

/test dataplane-operator-build-deploy-kuttl

1 similar comment
@rabi
Copy link
Contributor Author

rabi commented Dec 12, 2023

/test dataplane-operator-build-deploy-kuttl

@openshift-merge-bot openshift-merge-bot bot merged commit c46f8fb into openstack-k8s-operators:main Dec 12, 2023
7 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants