Consider changing controllerutil#CreateOrUpdate to use equality.Semantic.DeepEqual instead of reflect.DeepEqual

[RoeeShlomo]

I tried to use CreateOrUpdate with a NetworkPolicy object and it kept resulting in OperationResultUpdated even though nothing has changed. It seems like using reflect.DeepEqual sometimes gives false positives.

I have created a copy of CreateOrUpdate that uses equality.Semantic.DeepEqual from the apimachinery package (Semantic can do semantic deep equality checks for api objects). It seems to resolve the issue for me. Should this be changed upstream?

[DirectXMan12]

yeah, can you submit a PR to fix this?

[DirectXMan12]

/kind bug
/priority important-longterm

[zegelin]

Is equality.Semantic.DeepEqual enough to prevent false positives due to optional fields with default values?

For example, the following always results in OperationResultUpdated:

	nodesService := &corev1.Service{ObjectMeta: DataCenterResourceMetadata(rctx.cdc, "nodes")}

	opresult, err := controllerutil.CreateOrUpdate(context.TODO(), rctx.client, nodesService, func(_ runtime.Object) error {
		nodesService.Spec = corev1.ServiceSpec{
			ClusterIP: "None",
			Ports: []corev1.ServicePort{
				{Name: "cql", Port: 9042},
				{Name: "jmx", Port: 7199},
			},
			Selector: DataCenterLabels(rctx.cdc),
		}

		if rctx.cdc.Spec.PrometheusSupport {
			nodesService.Spec.Ports = append(nodesService.Spec.Ports, corev1.ServicePort{Name: "prometheus", Port: 9500})
		}

		if err := controllerutil.SetControllerReference(rctx.cdc, nodesService, rctx.scheme); err != nil {
			return err
		}

		return nil
	})

If I dump the server object and the locally generated object, I see the following are always different:

• Spec.Ports[].Protocol -- the client-side default is "", the server default is "TCP"
• Spec.Ports[].TargetPort.IntVal -- the client-side default is 0, the server default is the value of Spec.Ports[].Port
• Spec.Type -- the client-side default is "", the server default is "ClusterIP"
• Spec.SessionAffinity -- the client-side default is "", the server default is "None"

For a simple object such as a Service, it's possible to fill in every struct field to supply the same defaults as what the server calculates, but for more complex objects such as a StatefulSet it becomes quite unwieldy to fill in every optional field (see this diff). Add to that, the code would need to be kept up-to-date with changes on the K8s side.

[DirectXMan12]

No, deepequal won't prevent issues from defaulting, IIRC. For that, you need some pretty complex logic. Server-side apply provides that logic, otherwise you have to just be careful how you code things.

[RoeeShlomo]

Server side apply is tracked in #347.
Does it also allow to ensure that all unspecified fields are at their server defaults? Or would I need to know what the server defaults are anyway if my use cases require that unspecified fields are at their defaults?

Knative takes the approach of calling Update and then comparing the result from Get with the result from Update. If it's the same then it implies no real change happened. See
https://github.com/knative/serving/blob/master/pkg/reconciler/revision/cruds.go#L86

Would something like that work?

func CreateOrUpdate(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) {
	key, err := client.ObjectKeyFromObject(obj)
	if err != nil {
		return OperationResultNone, err
	}

	if err := c.Get(ctx, key, obj); err != nil {
		if !errors.IsNotFound(err) {
			return OperationResultNone, err
		}
		if err := mutate(f, key, obj); err != nil {
			return OperationResultNone, err
		}
		if err := c.Create(ctx, obj); err != nil {
			return OperationResultNone, err
		}
		return OperationResultCreated, nil
	}

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

       // CHANGE #1
	if equality.Semantic.DeepEqual(existing, obj) {
		return OperationResultNone, nil
	}

	if err := c.Update(ctx, obj); err != nil {
		return OperationResultNone, err
	} else if equality.Semantic.DeepEqual(existing, obj) { // CHANGE #2
		// If what comes back from the update (with defaults applied by the API server) is the same as what we have
		// then nothing changed
		return OperationResultNone, nil
	}
	return OperationResultUpdated, nil
}

If so, maybe we can add logging with warning level there so the developer would know that he should probably add these specific fields and their server-side default values to its mutated object to avoid unnecessary Update calls.

[DirectXMan12]

Does it also allow to ensure that all unspecified fields are at their server defaults

Server-side apply works by tracking "ownership" of a field, so if you don't set a field, and you don't own that field, the server won't try to change anything. It's only when you "own" a field that the server will change the value. You take ownership of fields by setting them. All of that is a long-winded way of saying that with server-side apply, you only have to care about the fields that you actually want to set, not about defaults set by the server, or fields set by other compilers.

Would something like that work?

Yeah, that'd help the result. You'd still have to be more careful than with server-side apply, though, since if you unset something that's defaulted, you can get yourself into a reconcile loop.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[DirectXMan12]

/lifecycle frozen

[DirectXMan12]

/good-first-issue

[k8s-ci-robot]

@DirectXMan12:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.