-
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 |
|---|---|---|
|
|
@@ -2,11 +2,13 @@ package openstack | |
|
|
||
| import ( | ||
| "context" | ||
| "slices" | ||
|
|
||
| "github.com/openstack-k8s-operators/lib-common/modules/common/helper" | ||
| 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 +60,84 @@ func DataplaneNodesetsOVNControllerImagesMatch(version *corev1beta1.OpenStackVer | |
| return true | ||
| } | ||
|
|
||
| // IsDataplaneDeploymentRunningForContainerImage checks whether any in-progress | ||
| // OpenStackDataPlaneDeployment is deploying a service that manages the given | ||
| // containerImageField (e.g. "OvnControllerImage"). It resolves which services | ||
| // each deployment runs (from ServicesOverride or the nodeset's service list) | ||
| // and inspects the service's ContainerImageFields to determine if it manages | ||
| // the specified container image. | ||
| func IsDataplaneDeploymentRunningForContainerImage( | ||
| ctx context.Context, | ||
| h *helper.Helper, | ||
| namespace string, | ||
| dataplaneNodesets *dataplanev1.OpenStackDataPlaneNodeSetList, | ||
| containerImageField 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 | ||
| } | ||
|
|
||
| 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 slices.Contains(svc.Spec.ContainerImageFields, containerImageField) { | ||
| 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.