feat: Avoid Redundant SSA for Manifest Patching

[LeelaChacha]

Closes #1456

[kyma-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[LeelaChacha]

NOTE: One integration test is failing as it is invalidated now. It is supposed to check that a manifest is reset if manually overwritten. However, now manifest patching only works when new template version, channel, or kyma module status is present.
How to proceed with the test? Should we reverse the check to show that manifest ssa is not?

[LeelaChacha]

/test all

[c-pius]

Thanks for the re-work! Can confirm that the flickering is now gone 👍🏻

NOTE: One integration test is failing as it is invalidated now. It is supposed to check that a manifest is reset if manually overwritten. However, now manifest patching only works when new template version, channel, or kyma module status is present.
How to proceed with the test? Should we reverse the check to show that manifest ssa is not?

This is an important finding. We actually changed parts of our handling, hence the test fails. I also tested this manually. Before, when e.g. changing Spec.Install.Source.Repo of a ManifestCR, we reconciled it back to the original state. Now this is not happening anymore except we also change the version, or channel. I guess this would be acceptable as ManifestCRs reside in KCP and can only be changed by us or SRE. I still want to double check... @ruanxin is this something we explicitly accept?

[ruanxin]

Thanks for the re-work! Can confirm that the flickering is now gone 👍🏻

NOTE: One integration test is failing as it is invalidated now. It is supposed to check that a manifest is reset if manually overwritten. However, now manifest patching only works when new template version, channel, or kyma module status is present.
How to proceed with the test? Should we reverse the check to show that manifest ssa is not?

This is an important finding. We actually changed parts of our handling, hence the test fails. I also tested this manually. Before, when e.g. changing Spec.Install.Source.Repo of a ManifestCR, we reconciled it back to the original state. Now this is not happening anymore except we also change the version, or channel. I guess this would be acceptable as ManifestCRs reside in KCP and can only be changed by us or SRE. I still want to double check... @ruanxin is this something we explicitly accept?

Yes, normally modifying manifest CR spec is controlled by us, it's fine to skip this overwritten, however, it still has benefits that allow this overwritten.
one use case:

1. add skip reconciliation label for one Kyma CR
2. modify related manifest CR spec for testing or troubleshooting
3. afterwards, remove skip reconciliation label
4. manifest CR get reset.

And it's possible to support this feature, just need to do a client diff check between the spec of manifest in cluster (manifestInCluster) and converted from moduletemplate (manifestObj), but we should monitor the kyma reconciliation time after release, to see how expensive this diff check.

[c-pius]

Thanks for chiming in! Just not sure if I understood you correctly since you explained both... Are you suggesting to accept it as proposed in this PR (don't overwrite manual changes), or are you suggesting to add the diff check as an additional evaluation in NeedToUpdate (and overwrite manual changes)?

[ruanxin]

Thanks for chiming in! Just not sure if I understood you correctly since you explained both... Are you suggesting to accept it as proposed in this PR (don't overwrite manual changes), or are you suggesting to add the diff check as an additional evaluation in NeedToUpdate (and overwrite manual changes)?

I suggest adding this diff check in this PR, otherwise, we lose the support for that use case.

[c-pius]

Thanks for chiming in! Just not sure if I understood you correctly since you explained both... Are you suggesting to accept it as proposed in this PR (don't overwrite manual changes), or are you suggesting to add the diff check as an additional evaluation in NeedToUpdate (and overwrite manual changes)?

I suggest adding this diff check in this PR, otherwise, we lose the support for that use case.

Thanks for clarifying!

@LeelaChacha would you mind checking if this can be added to the NeedToUpdate?

[LeelaChacha]

@ruanxin @c-pius does a diff of the spec suffice?

Test is now green, btw

[c-pius]

@ruanxin @c-pius does a diff of the spec suffice?

Test is now green, btw

I guess checking the spec part should suffice. However, I am not sure if reflect.DeepEqual is the way to go. As mentioned here, K8s may provide default values for unset fields. When doing so, it will be unequal all the time for certain manifests. Maybe this equality.Semantic.DeepDerivative is a good alternative. Would you mind checking with some debug comments whether it works?

And one other thing for @ruanxin. I vaguely remember that somebody mentioned that we transitioned to using Server Side Apply to transfer the compute load of comparing the resources from our controller to the API server (not sure if I remember this correctly). We are now re-adding this load on the controller. Are we sure we want to do that?