-
Notifications
You must be signed in to change notification settings - Fork 102
[OSPRH-25851] Ignore empty node sets during data plane OVN minor update #1786
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
base: main
Are you sure you want to change the base?
Conversation
| "ansibleSSHPrivateKeySecret": "dataplane-ansible-ssh-private-key-secret", | ||
| }, | ||
| "nodes": map[string]interface{}{}, | ||
| "servicesOverride": []string{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this because the servicesOverride field doesn't actually exist in OpenStackDataPlaneNodeSet. It's found in OpenStackDataPlaneDeployment instead, so this line is misleading.
|
looks good to me, but would let someone with better edpm knowledge approve to land it. |
rabi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed it. LGTM. Did you face the issue with some testing. In real-life we don't expect users to have empty nodesets 😄
| // After considering generation (to make sure reconciliation has quiesced for | ||
| // the current nodeset spec), we only care about deployed status if the nodeset | ||
| // has nodes | ||
| if len(nodeset.Spec.Nodes) < 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think general pattern is to use == 0 ?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abays, rabi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
During a minor update, we currently mistakenly consider empty
OpenStackDataPlaneNodeSets and wait for their non-existent OVN deployments to update. We should ignore them in this context instead, in order to prevent unnecessarily blocking the update and thus obviate the need for manual intervention to unblock it (which can be done by deleting the emptyOpenStackDataPlaneNodeSets).Tested locally in a CIFMW dev-scripts environment with a minor update of VA1:
OpenStackDataPlaneNodeSetafter initial deploymentJira: https://issues.redhat.com/browse/OSPRH-25851