Skip to content
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

CreateOrUpdate ergonomics in presence of defaults #2733

Open
dtantsur opened this issue Mar 26, 2024 · 2 comments
Open

CreateOrUpdate ergonomics in presence of defaults #2733

dtantsur opened this issue Mar 26, 2024 · 2 comments

Comments

@dtantsur
Copy link

Hi folks.

I'm working on an operator that, absolutely unsurprisingly, creates deployments. Following what seems to be the good controller style, I tried using CreateOrUpdate in roughly this way (simplified):

	deploy := &appsv1.Deployment{
		ObjectMeta: metav1.ObjectMeta{Name: databaseDeploymentName(db), Namespace: db.Namespace},
	}
	result, err := controllerutil.CreateOrUpdate(cctx.Context, cctx.Client, deploy, func() error {
		deploy.Spec.Template = newDatabasePodTemplate(db)
		return controllerutil.SetControllerReference(db, deploy, cctx.Scheme)
	})

As you can guess from the title, this naive approach does not actually work. Because there are certain defaults in the schema (e.g. ImagePullPolicy), the deep comparison inside CreateOrUpdate always fails, and my controller gets stuck in an infinite reconcile loop trying to update the "defaulted" fields to their empty values.

What is a possible way out of this problem?

  1. I've tried only changing fields that I want to set to non-default values. This quickly becomes really tedious.
  2. Apply defaults before comparisons. So something like
	deploy := &appsv1.Deployment{
		ObjectMeta: metav1.ObjectMeta{Name: databaseDeploymentName(db), Namespace: db.Namespace},
	}
	result, err := controllerutil.CreateOrUpdate(cctx.Context, cctx.Client, deploy, func() error {
		deploy.Spec.Template = newDatabasePodTemplate(db)
                schema.Default(deploy)
		return controllerutil.SetControllerReference(db, deploy, cctx.Scheme)
	})

I've seen this solution mentioned on stackoverflow, but I see a large problem here. The local copy of Kubernetes code may not match the server side causing one of three issues:

  1. If the server is older, my code may try sending fields that do not exist in it.
  2. If the server is newer, it may have fields my code doesn't know defaults for, causing the infinite reconcile loop again.
  3. If the server changes any defaults (I hope they don't but who knows), I'm now sending non-default values.

The only possible solution I can imaging is to run comparison on copies with defaults applied. So, change CreateOrUpdate to something like

	existing := obj.DeepCopyObject()
        schema.Default(existing)
	if err := mutate(f, key, obj); err != nil {
		return OperationResultNone, err
	}

        copyObj := obj.DeepCopyObject()
        schema.Default(copyObj)
	if equality.Semantic.DeepEqual(existing, copyObj) {
		return OperationResultNone, nil
	}

but obviously

  1. Another deep copy may be expensive
  2. The function does not have access to Schema now
  3. Does not solve problem (2) with newer servers

No idea if this issue even has solutions but recording it here for any potential ideas.

@dtantsur
Copy link
Author

If the server is newer, it may have fields my code doesn't know defaults for, causing the infinite reconcile loop again.

Hmm, this has to be false: if my client is older than the server, I will not see new fields at all. So at least this is not an issue. Point 3 (changing defaults) is probably a rare case. Now I'm just curious how much of a problem point 1 (older servers) is.

In any case, this stuff could use a lot of documentation.

@dtantsur
Copy link
Author

schema.Default(deploy)

This does not work in my testing :( I guess I'm stuck with carefully updating only the fields I need set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant