-
Notifications
You must be signed in to change notification settings - Fork 106
Fix MinorUpdateOVNDataplane race when OVN image unchanged between versions #1864
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import ( | |
| corev1beta1 "github.com/openstack-k8s-operators/openstack-operator/api/core/v1beta1" | ||
|
|
||
| dataplanev1 "github.com/openstack-k8s-operators/openstack-operator/api/dataplane/v1beta1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
|
|
@@ -58,6 +59,83 @@ func DataplaneNodesetsOVNControllerImagesMatch(version *corev1beta1.OpenStackVer | |
| return true | ||
| } | ||
|
|
||
| // IsDataplaneDeploymentRunningForServiceType checks whether any in-progress | ||
| // OpenStackDataPlaneDeployment is deploying a service with the given | ||
| // EDPMServiceType (e.g. "ovn"). It resolves which services each deployment | ||
| // runs (from ServicesOverride or the nodeset's service list) and inspects | ||
| // the service's EDPMServiceType to determine if it matches. | ||
| func IsDataplaneDeploymentRunningForServiceType( | ||
| ctx context.Context, | ||
| h *helper.Helper, | ||
| namespace string, | ||
| dataplaneNodesets *dataplanev1.OpenStackDataPlaneNodeSetList, | ||
| serviceType string, | ||
| ) (bool, error) { | ||
| // List all deployments in the namespace | ||
| deployments := &dataplanev1.OpenStackDataPlaneDeploymentList{} | ||
| opts := []client.ListOption{ | ||
| client.InNamespace(namespace), | ||
| } | ||
| err := h.GetClient().List(ctx, deployments, opts...) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // Build a map of nodeset name -> nodeset for quick lookup | ||
| nodesetMap := make(map[string]*dataplanev1.OpenStackDataPlaneNodeSet, len(dataplaneNodesets.Items)) | ||
| for i := range dataplaneNodesets.Items { | ||
| nodesetMap[dataplaneNodesets.Items[i].Name] = &dataplaneNodesets.Items[i] | ||
| } | ||
|
|
||
| // Cache service lookups to avoid repeated API calls | ||
| serviceCache := make(map[string]*dataplanev1.OpenStackDataPlaneService) | ||
|
|
||
| for _, deployment := range deployments.Items { | ||
| // Skip completed deployments | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be earlier failed deployments containing ovn. We should check if there is a running deployment. Also, I think probably a would still be a race here. |
||
| if deployment.Status.Deployed { | ||
| continue | ||
| } | ||
|
|
||
| // Determine which services this deployment runs for each of its nodesets | ||
| for _, nodesetName := range deployment.Spec.NodeSets { | ||
| nodeset, exists := nodesetMap[nodesetName] | ||
| if !exists || len(nodeset.Spec.Nodes) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| var services []string | ||
| if len(deployment.Spec.ServicesOverride) != 0 { | ||
| services = deployment.Spec.ServicesOverride | ||
| } else { | ||
| services = nodeset.Spec.Services | ||
| } | ||
|
Comment on lines
+106
to
+111
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tangential, but @rabi I wonder if this should be propagated into the In that first one, you could set it to |
||
|
|
||
| for _, serviceName := range services { | ||
| svc, cached := serviceCache[serviceName] | ||
| if !cached { | ||
| foundService := &dataplanev1.OpenStackDataPlaneService{} | ||
| err := h.GetClient().Get(ctx, types.NamespacedName{ | ||
| Name: serviceName, | ||
| Namespace: namespace, | ||
| }, foundService) | ||
| if err != nil { | ||
| // Service not found — skip it | ||
| continue | ||
| } | ||
| svc = foundService | ||
| serviceCache[serviceName] = svc | ||
| } | ||
|
|
||
| if svc.Spec.EDPMServiceType == serviceType { | ||
| return true, nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably just check serviceType 'ovn' and avoid the harcoded check for the image name.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there always a service type, which is stable and not custom?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, serviceType is always fixed and the service name can change. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| // DataplaneNodesetsDeployed returns true if all nodesets are deployed with the latest version | ||
| func DataplaneNodesetsDeployed(version *corev1beta1.OpenStackVersion, dataplaneNodesets *dataplanev1.OpenStackDataPlaneNodeSetList) bool { | ||
| for _, nodeset := range dataplaneNodesets.Items { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.