Skip to content

Fix operator upgrade issue #6483

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −1
Open

Fix operator upgrade issue #6483

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion operator/pkg/util/apiclient/idempotency.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,16 @@ func CreateOrUpdateDeployment(client clientset.Interface, deployment *appsv1.Dep
return err
}

_, err := client.AppsV1().Deployments(deployment.GetNamespace()).Update(context.TODO(), deployment, metav1.UpdateOptions{})
older, err := client.AppsV1().Deployments(deployment.GetNamespace()).Get(context.TODO(), deployment.GetName(), metav1.GetOptions{})
if err != nil {
return err
}

// The Deployment selector field is immutable. To prevent update failures caused by inconsistent Deployment selectors generated during
// Karmada Operator upgrades, ensure the selector remains unchanged.
deployment.Spec.Selector = older.Spec.Selector
deployment.Spec.Template.ObjectMeta.Labels = labels.Merge(deployment.Spec.Template.ObjectMeta.Labels, older.Spec.Selector.MatchLabels)
Comment on lines +136 to +137
Copy link
Member

@RainbowMango RainbowMango Jun 25, 2025

Choose a reason for hiding this comment

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

deployment.Spec.Selector = older.Spec.Selector

It means always use the original selector, so that it never gets a chance to change it? even when it is required in new version?

Is .spec.template.lables mutable? Why we need to keep the original labels?

Copy link
Contributor Author

@zhzhuang-zju zhzhuang-zju Jun 25, 2025

Choose a reason for hiding this comment

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

It means always use the original selector, so that it never gets a chance to change it?

Yes, the selector field of Deployments/StatefulSets is immutable. Once defined, it cannot be changed. So in the operator upgrade scenarios, it should not be changed.

even when it is required in new version?

In fact, this selector only needs to match .spec.template.labels.

Is .spec.template.lables mutable? Why we need to keep the original labels?

.spec.template.lables is mutable, it can be modified according to requirements. We don't need to keep the original labels. Instead, we should add the selector to the labels of the new version to ensure that the selector can match .spec.template.labels.

Copy link
Member

@jabellard jabellard Jun 26, 2025

Choose a reason for hiding this comment

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

Thanks for explaining.

Copy link
Member

@RainbowMango RainbowMango Jun 26, 2025

Choose a reason for hiding this comment

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

My concern is that if we use merge here, it means we'll always carry selectors that aren't needed, right?

I haven't looked closely. What's the difference between the original selector and the new selector?

Copy link
Contributor Author

@zhzhuang-zju zhzhuang-zju Jun 27, 2025

Choose a reason for hiding this comment

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

My concern is that if we use merge here, it means we'll always carry selectors that aren't needed, right?

Take deployment as an example. deploymentSpec.Selector is used to select pods affected by this deployment. deploymentSpec.Selector cannot be modified. Therefore, podLabels must include deploymentSpec.Selector.
For example:

  selector:
    matchLabels:
      app.kubernetes.io/instance: karmada-demo
      app.kubernetes.io/name: karmada-controller-manager
  template:
      labels:
        app.kubernetes.io/instance: karmada-demo
        app.kubernetes.io/name: karmada-controller-manager
        others...

From v1.13.0 to v1.14.0, the components will be from:

  selector:
    matchLabels:
       karmada-app: {component name}
  template:
    labels:
       karmada-app: {component name}

to

  selector:
    matchLabels:
       app.kubernetes.io/instance: {karmada instance name}
       app.kubernetes.io/name: {component name}
  template:
    labels:
       app.kubernetes.io/instance: {karmada instance name}
       app.kubernetes.io/name: {component name}

The purpose is to support deploying multiple Karmada instances within the same namespace. The original pod labels cannot ensure uniqueness within the same namespace, so the Karmada instance name is newly added to the labels.
Since the selector cannot be modified, the above upgrade will fail.

After modifying:
When upgrading from v1.13.0 to v1.14.1, the components will be from:

  selector:
    matchLabels:
       karmada-app: {component name}
  template:
    labels:
       karmada-app: {component name}

to

  selector:
    matchLabels:
       karmada-app: {component name}
  template:
    labels:
       karmada-app: {component name} # Ensure that the deployment and pod can match
       app.kubernetes.io/instance: {karmada instance name} # Consistent with the new version, such as match the Service in the new version
       app.kubernetes.io/name: {component name}

The selector remains unchanged, and the podLabel is the label of the new version plus the selector of the old version.

Copy link
Member

@RainbowMango RainbowMango Jun 30, 2025

Choose a reason for hiding this comment

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

That's exactly my concern. The label karmada-app has essentially been deprecated since v1.13.0, but during the migration to v1.14.0, we have to keep it, and never get a chance to get rid of it. In other words, this might not be a clean migration, just a tradeoff.

Copy link
Contributor Author

@zhzhuang-zju zhzhuang-zju Jun 30, 2025

Choose a reason for hiding this comment

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

Kubernetes subscribes to the immutable infrastructure paradigm. Immutable infrastructure refers to the concept where deployed infrastructure is never modified in place. If changes are required to a given resource, destroying the current resource and rebuilding that resource with changes is the appropriate workflow.

Therefore, when immutable fields such as selector change due to functional evolution during an upgrade, the common practice is redploying, which means deleting the resource first and then recreating it.

Similar behaviors:
kubectl replace --force: Force replace, delete and then re-create the resource
argo: allows users to specifically annotate argocd.argoproj.io/sync-options: Replace=true,Force=true on resources that they know can't be updated without being deleted first.

What I'm worried about is that if the selector of the component changes during the upgrade process, the downtime of redeploying will be greater than that of updating. This also gives us a lesson: It’s so important to plan our deployments so that we prevent ourselves from making changes to the label selectors once the deployments have been created.

WDYT? @jabellard @RainbowMango

_, err = client.AppsV1().Deployments(deployment.GetNamespace()).Update(context.TODO(), deployment, metav1.UpdateOptions{})
if err != nil {
return err
}
Expand Down Expand Up @@ -246,6 +255,11 @@ func CreateOrUpdateStatefulSet(client clientset.Interface, statefulSet *appsv1.S
}

statefulSet.ResourceVersion = older.ResourceVersion

// The StatefulSet selector field is immutable. To prevent update failures caused by inconsistent StatefulSet selectors generated during
// Karmada Operator upgrades, ensure the selector remains unchanged.
statefulSet.Spec.Selector = older.Spec.Selector
statefulSet.Spec.Template.ObjectMeta.Labels = labels.Merge(statefulSet.Spec.Template.ObjectMeta.Labels, older.Spec.Selector.MatchLabels)
_, err = client.AppsV1().StatefulSets(statefulSet.GetNamespace()).Update(context.TODO(), statefulSet, metav1.UpdateOptions{})
if err != nil {
return err
Expand Down