Add a check for deltas before saving

[johnbelamaric]

The auto-approval controller was not kicking in, because the PackageRevision creation timestamp kept updating. Which is odd, and probably a bug in Porch. But, we want this anyway. It will avoid saving the package revision resources if there is no delta. This should prevent the forever growing task list, hopefully.

Still testing - one concern is that if we are refreshing claims, we may be stuffing away a timestamp which means this would never find matching hashes.

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from johnbelamaric by writing /assign @johnbelamaric in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johnbelamaric]

This is probably still useful to have, but it is not fixing the problem yet.

What I have found is that when the package variant controller saves the Kptfile to package revision resources, the Kptfile keys (name, description, etc.) and the condition keys (type, message, etc.) are in one order, while when the specializer does it they are in a different order. Both do sort the condition arrays themselves; the issue is the field names come out in different orders.

Suspiciously, they use different versions of kyaml (13.9 in porch, 14.2 in nephio), which is what serializes that. Though I didn't immediately spot differences that would cause this.

I am trying to see if I can build a version of porch-controllers with the newer kyaml, and also with some additional instrumentation. I think there are some fixes to the way existing revisions are processed that may also contribute.

[johnbelamaric]

Ugh. Building porch-controllers with an updated kyaml is a nightmare, it descends into updating k8s API, kustomize api, controller-runtime, etc.

I will just add my instrumentation.

Right now, what this means is that we have to manually approve the NF packages and other packages that go through specialization. Auto-approve won't do it. But we can work around that.

[johnbelamaric]

This is the method that seems to re-order the conditions:

https://github.com/GoogleContainerTools/kpt/blob/main/porch/controllers/packagevariants/pkg/controllers/packagevariant/injection.go#L284

Not 100% sure but checking now.

[henderiw]

I will have a look. We did add something to sort conditions since the tests validations need something otherwise they fail as we check the expected result.

[henderiw]

Here is where we sort conditions.

// SortConditions returns a kptfile with sorted conditions
func (r *kptFile) SortConditions() {
r.m.RLock()
defer r.m.RUnlock()

if r.GetKptFile().Status == nil || len(r.GetKptFile().Status.Conditions) == 0 {
	return
}

sort.SliceStable(r.GetKptFile().Status.Conditions, func(i, j int) bool {
	return r.GetKptFile().Status.Conditions[i].Type < r.GetKptFile().Status.Conditions[j].Type
})

}

[johnbelamaric]

/hold

Still testing this, but may not be needed given #295 - may still also be helpful, but if not needed we may want to defer it.

[nephio-prow]

@johnbelamaric: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
presubmit-nephio-golangci-lint	94be99f	link	true	/test presubmit-nephio-golangci-lint

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[efiacor]

Hi @johnbelamaric , is this still relevant/needed?

[efiacor]

@kispaljr looks like we still have differences in the kptfile rendering. Did the sdk fix not address this?
#295