Ensure task list is single element, introduce upgrade task by mozesl-nokia · Pull Request #252 · nephio-project/porch

mozesl-nokia · 2025-05-14T16:40:12Z

Closes #725

Quite a big PR, co-developed with @dgyorgy-nokia, implementing @kispaljr suggestions in #725.

Done in this PR:

Reduce task list size to 1 element (init, edit, clone or the new upgrade)
Effectively deprecate all other non-initial task types (eval/render, patch, update)
Deprecate the porchctl rpkg update command, add a new porchctl rpkg upgrade command with basic testing
Introduce a new upgrade initial task, which performs a semi-custom 3-way merge on the input PackageRevision(Resource)s if the strategy is resource-merge

Known limitations:

The current 3-way-merge implementation will always choose the new upstream version when there is a conflict
CRDs must be part of the package itself in order for them to be picked up into the schema. This is relevant when merging associative lists. However, associative list inference is enabled, but it is pretty basic and only works if the associative key is a name field.
Upgrade only work or published PackageRevisions (but this is artificially limited)
Remove reclone and replay.

Not done in this PR:

Remove the deprecated task types
Extensively test new merging and upgrade
Configurability of the 3-way-merge
Clean up unnecessary task/mutation-related code
Remove deprecated test cases

Other changes:

Make targets for reloading specific components on the cluster (rebuild image, load image, restart deployment)
Support authentication for porchctl rpkg clone
Pass namespace correctly in the clone mutation
Utility test file to run a custom cli e2e test based on folder name (very hacky, just meant for debugging). The repository related params are no longer necessary since E2E testing : Test to scale up SMF ( Instantiating a new POD with more resources) nephio#240 is merged.
Utility function to add mutators to the Kptfile in the E2E tests, since eval is deprecated.
Smaller enhancements to kpt-function-sdk

linux-foundation-easycla · 2025-05-14T16:40:17Z

The committers listed above are authorized under a signed CLA.

✅ login: mozesl-nokia (264cddf, 409ce57, b563445, 11a48d5, 15a9ac5, 2faf68d, bda1dab, b0026b6, fe839d0, 9956aee, ccade70, 43a6b5d, baf818c, 90286a6, 0da86f2, dcdfa3a, 40dfe00, 4600a6e, cd61ca2, 71b3eaf, 7042f17, 15e794b, 0fb8044, a737a52, 550f3bb, 726e931, f8de65c, b767637, 7185187, 3a4182e, adf40b7, 678675d, 8d490db)
✅ login: dgyorgy-nokia (ebdf00a, 9a53293, eecd00d, 6877956)

nagygergo · 2025-05-14T17:07:52Z

+
+func Merge(original, updated, destination fn.KubeObjects, additionalSchemas []byte) (fn.KubeObjects, error) {
+	if additionalSchemas != nil {
+		if err := openapi.AddSchema(additionalSchemas); err != nil {


Is the openapi destroyed after every merge operation, or this leaks memory with the increasing number of CRDs that a porch server has seen?

You can check the kyaml code here. It looks like it will not re-add the same schema for the same kind, but yes, I think it persists between merges.

If this is a concern, we can call openapi.ResetOpenAPI() after every merge, but I assume that also axes all the built-in schemas.

Was reading through the kyaml code + readme a bit.
The compiled-in k8s api version stayed at 1.21, while the docs describe that this should be bumped, noone really bothered to do that.
Either Porch should onboard openapis for supported k8s versions here, or give a way for schemas that don't have an associated CRD be onboarded (for example ValidatingAdmissionPolicies as a recent addition to the default k8s API).
I think that this part of the kyaml implementation could use some patching up, i.e.: the openapi.ResetOpenAPI() should also reset the globalschema.schemaInitso that the defaults are re-loaded the next time some function runs SchemaForResourceType

Also I find it troublesome that the kyaml implementation maintains a single schemalock RWmutex, so only a single routine can execute.

My 2 cents out of this is that probably we need to update the compiled-in version from 1.21, the openapi indeed should not be reset, because it operates on global variables and can cause race conditions or pancis between concurrent processes and that we probably need to refactor the kyaml so that it allows for creating local openapi schema registries so that porch won't leak memory by storing more and more CRDs.

nagygergo · 2025-05-15T07:31:42Z

General comment/question about CRD usage in packages.
Would it make sense to have a special annotation that tells Porch to start consuming the CRD for 3way merge, so this is an opt-in behavior. Also, when writing documentation for this, it would be important to highlight that CRDs that are only in the package for porch 3way merge should be marked with config.kubernetes.io/local-config: "true" for ConfigSync/Flux and whatever the equivalent of that is for Argo.

nagygergo · 2025-05-15T08:03:19Z

This is a question I should've asked before this went into implementation, but if porchctl rpkg upgrade is always creating a new packageRevision, is it possible that packageVariant controller that doesn't restrict on a specific version of the upstream package would create a new draft packageRevision each time it sees a change? Is that even a problem, or intended behavior?

mozesl-nokia · 2025-05-15T08:36:35Z

Would it make sense to have a special annotation that tells Porch to start consuming the CRD for 3way merge, so this is an opt-in behavior.

Not a bad idea, imo.

...is it possible that packageVariant controller that doesn't restrict on a specific version of the upstream package would create a new draft packageRevision each time it sees a change?

If there is a change in the upstream it has to create a new PR with the upgrade task, so yes, if there is a constantly changing upstream, it will keep making new downstream drafts (but only if the previous downstream got published, since the upgrade inputs must be published).

nagygergo · 2025-05-15T10:08:34Z

+}
+
+func loadResourcesFromDirectory(directoryPath string, mergeSourceAnnotation string) (fn.KubeObjects, error) {
+	exclusions, err := findExclusions(directoryPath)


What resources are getting excluded?

Resources in subpackages

Hm... It sounds like it has a lot of implications on how subpackages need to behave, and I won't have time to think it through today, will come up with something over the weekend if that's ok.

This is a fully undocumented territory of Porch, but as I understand how subpackages work in Porch:

You can create packages with subpacakges. The rendering works the same as it works with KPT, with a depth-first render of all subpackages.

If a package with subpackages is not merged to main, the subpackages don't appear on the API.

If a package with subpackages is merged to main, then eventually subpackages will be discovered on main.

For the subpackages, no separate versioned packages will be created.

If a package with subpackages is changed (updated either via update, or manually), then the subpackages are re-rendered.

From this I think No.1 and No.5 is important to keep.

Would it be possible that the 3way Merge also considers the sub-packages as well? Is there any disadvantages of that?

Previously, this was hardcoded to false here so we decided to keep it.

porch/internal/kpt/util/update/resource-merge.go

Line 160 in 55f5a64

IncludeSubPackages: false,

Curiously though, here it looks like we are only updating subpackages...

porch/internal/kpt/util/update/resource-merge.go

Lines 57 to 81 in 55f5a64

// Find all subpackages in local, upstream and original. They are sorted

// in increasing order based on the depth of the subpackage relative to the

// root package.

subPkgPaths, err := pkgutil.FindSubpackagesForPaths(pkg.Local, true,

options.LocalPath, options.UpdatedPath, options.OriginPath)

if err != nil {

return errors.E(op, types.UniquePath(options.LocalPath), err)

}

// Update each package and subpackage. Parent package is updated before

// subpackages to make sure auto-setters can work correctly.

for _, subPkgPath := range append([]string{"."}, subPkgPaths...) {

isRootPkg := false

if subPkgPath == "." && options.IsRoot {

isRootPkg = true

}

localSubPkgPath := filepath.Join(options.LocalPath, subPkgPath)

updatedSubPkgPath := filepath.Join(options.UpdatedPath, subPkgPath)

originalSubPkgPath := filepath.Join(options.OriginPath, subPkgPath)

err := u.updatePackage(subPkgPath, localSubPkgPath, updatedSubPkgPath, originalSubPkgPath, isRootPkg)

if err != nil {

return errors.E(op, types.UniquePath(localSubPkgPath), err)

}

}

Of course, we can just set this exclusion to empty and see what happens if we want to include subpackages.

I was testing locally, setting the exclusions to an empty array exclusions := []string{} and IncludeSubpackages: true, the subfolders seems to get the upgrade treatment alright.

I think this behaviour would be more aligned to what a user would expect, especially if we are planning to deprecate the update with copy-merge which was the most simple strategy to handle conflicts, and it handled sub-folders.

kispaljr · 2025-05-16T12:38:20Z

is it possible that packageVariant controller that doesn't restrict on a specific version of the upstream package would create a new draft packageRevision each time it sees a change? Is that even a problem, or intended behavior?

The upstream package revision is a mandatory field of the PackageVariant, so there is no such PV that "doesn't restrict on a specific version of the upstream package". You explicitly have to change the upstream package revision in the PV so that an upgrade happens, and that will create a new draft. This is exactly how PV worked before, and there is no change is this behvior now, with the only exception that upgrade will be rejected until a draft pkgrev is present in the downstream package.

nagygergo · 2025-05-17T11:59:45Z

You explicitly have to change the upstream package revision in the PV so that an upgrade happens, and that will create a new draft

Sorry, worked from memory instead of testing recent versions. I remember that when the revision main was specified before, packageVariant was tacking it through updates to the main packageRevision. Tested it, it's not doing the same anymore, but I couldn't roll back far enough in history to find where that was dropped off (or my memory is faulty).

mozesl-nokia · 2025-05-19T14:30:59Z

/retest

mozesl-nokia · 2025-06-18T10:36:28Z

Which of these threads can be resolved?

@nagygergo
#252 (comment)
#252 (comment)
#252 (comment)
#252 (comment)

@lapentad
#252 (comment)
#252 (comment)

Also, what else needs to be addressed in this PR? Should I try fixing the sonar issues? The duplication seems unavoidable unless I remove the update command altogether or refactor discover into a common place.

mozesl-nokia · 2025-06-18T16:07:57Z

/retest

efiacor

Big PR. Should really be 2. Task list and 3way merge.
Haven't tested the 3way merge but I did run the chnages through our test-infra e2e and as discussed there are issues with infinite loops of commits on the packageRevision(Resources)

efiacor · 2025-07-09T08:46:10Z

-				downstream, err = r.copyPublished(ctx, downstream, pv, prList)
+				downstream, err = r.createEditDraft(ctx, downstream, pv, prList)
 				if err != nil {
 					klog.Errorf("package variant %q failed to copy %q: %s", pv.Name, oldDS.Name, err.Error())


Update the error msg to reflect the Task type?

efiacor

Still quite a few unresolved comments. The github UI sometimes "collapses" review comments so you need to expand them.

efiacor · 2025-07-09T14:54:20Z

+				LocalPackageRevisionRef: porchapi.PackageRevisionRef{
+					Name: source.Name,
+				},
+				Strategy: porchapi.ResourceMerge,


Not sure. Does it mean if the user uses a PV it will always use ResourceMerge?

mozesl-nokia · 2025-07-11T12:07:56Z

Anybody else's opinion on this? #252 (comment)
@nagygergo @efiacor @Catalin-Stratulat-Ericsson

If nobody I'll revert it.

liamfallon · 2025-07-29T16:14:49Z

@mozesl-nokia I wonder can we get a rebase on this PR. It's very nearly ready to go in now. Also if @efiacor @nagygergo and @lapentad could check the outstanding comments, that would be great.

sonarqubecloud · 2025-07-30T12:54:37Z

Quality Gate failed

Failed conditions
72.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

efiacor · 2025-07-30T14:18:40Z

/approve
/lgtm

nephio-prow · 2025-07-30T14:18:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, mozesl-nokia

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [efiacor]

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

mozesl-nokia · 2025-07-30T14:30:50Z

PS: My attempt at handling/mitigating the infinite commit loop discovered in the free5gc test can be found in these branches:
https://github.com/nokia/nephio-nephio/tree/issue-892-inprogress
https://github.com/nokia/nephio-porch/tree/issue-892-inprogress

My fix attempt focused on preventing porch from creating or pushing a commit if the file contents are unchanged.

mozesl-nokia and others added 4 commits May 14, 2025 17:44

implement 3 way merge without kio.Pipeline

11a48d5

introduce upgrade task in porch

eecd00d

ensure task list has one element, porchctl upgrade

15e794b

fix upgrade/update typo

726e931

nephio-prow Bot requested review from henderiw and kispaljr May 14, 2025 16:40

mozesl-nokia requested review from Catalin-Stratulat-Ericsson, efiacor, liamfallon and nagygergo May 14, 2025 16:41

fix lint

43a6b5d

nagygergo reviewed May 14, 2025

View reviewed changes

adapt to new cli e2e

bda1dab

fix cli upgrade, add pack gcr prefix override to custom_test.go

a737a52

improve coverage and duplicate code

fe839d0

nagygergo reviewed May 15, 2025

View reviewed changes

dgyorgy-nokia and others added 5 commits May 15, 2025 18:31

additional tests for upgrade task

9a53293

generictaskhandler.go coverage

ccade70

address some review comments

0fb8044

SOURCE_PACKAGE -> SOURCE_PACKAGE_REVISION

f8de65c

remove tests for the removed findUpgradeTask

cd61ca2

mozesl-nokia added 2 commits May 19, 2025 14:49

merge main into issue-892

90286a6

rename concurrent edit test repo

9956aee

only pass CRDs to SchemasFromCrdKubeObjects

baf818c

dgyorgy-nokia and others added 3 commits July 3, 2025 10:08

Remove unused patch diff generation

ebdf00a

Remove all deprecated task types from porch

6877956

merge main into issue-892

2faf68d

efiacor reviewed Jul 9, 2025

View reviewed changes

mozesl-nokia added 2 commits July 9, 2025 16:17

address review comments

15a9ac5

merge main into issue-892

40dfe00

efiacor reviewed Jul 9, 2025

View reviewed changes

kushnaidu self-requested a review July 18, 2025 10:18

mozesl-nokia added 7 commits July 30, 2025 11:38

update how task types work around commit messages

550f3bb

fix infinite render task addition

678675d

fix unit test

b767637

revert Revision2Str changes

3a4182e

Merge main into issue-892

8d490db

update TestUpdateResourcesEmptyPatch

adf40b7

fix task list for db cache

7185187

nephio-prow Bot assigned efiacor Jul 30, 2025

nephio-prow Bot added the lgtm #ededed label Jul 30, 2025

nephio-prow Bot added the approved #ededed label Jul 30, 2025

nephio-prow Bot merged commit a7c09d4 into nephio-project:main Jul 30, 2025
10 checks passed

liamfallon mentioned this pull request Aug 29, 2025

porch: packagevariant bug "Internal error occurred: error applying patch: conflict: fragment line does not match src line" #585

Closed

liamfallon mentioned this pull request Sep 15, 2025

PackageRevision tasks continuously grows in size #623

Closed

	// Find all subpackages in local, upstream and original. They are sorted
	// in increasing order based on the depth of the subpackage relative to the
	// root package.
	subPkgPaths, err := pkgutil.FindSubpackagesForPaths(pkg.Local, true,
	options.LocalPath, options.UpdatedPath, options.OriginPath)
	if err != nil {
	return errors.E(op, types.UniquePath(options.LocalPath), err)
	}

	// Update each package and subpackage. Parent package is updated before
	// subpackages to make sure auto-setters can work correctly.
	for _, subPkgPath := range append([]string{"."}, subPkgPaths...) {
	isRootPkg := false
	if subPkgPath == "." && options.IsRoot {
	isRootPkg = true
	}
	localSubPkgPath := filepath.Join(options.LocalPath, subPkgPath)
	updatedSubPkgPath := filepath.Join(options.UpdatedPath, subPkgPath)
	originalSubPkgPath := filepath.Join(options.OriginPath, subPkgPath)

	err := u.updatePackage(subPkgPath, localSubPkgPath, updatedSubPkgPath, originalSubPkgPath, isRootPkg)
	if err != nil {
	return errors.E(op, types.UniquePath(localSubPkgPath), err)
	}
	}

Conversation

mozesl-nokia commented May 14, 2025

Uh oh!

linux-foundation-easycla Bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagygergo May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagygergo May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagygergo commented May 15, 2025

Uh oh!

nagygergo commented May 15, 2025

Uh oh!

mozesl-nokia commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kispaljr commented May 16, 2025

Uh oh!

nagygergo commented May 17, 2025

Uh oh!

mozesl-nokia commented May 19, 2025

Uh oh!

mozesl-nokia commented Jun 18, 2025

Uh oh!

mozesl-nokia commented Jun 18, 2025

Uh oh!

efiacor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efiacor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mozesl-nokia commented Jul 11, 2025

Uh oh!

liamfallon commented Jul 29, 2025

Uh oh!

sonarqubecloud Bot commented Jul 30, 2025

Quality Gate failed

linux-foundation-easycla Bot commented May 14, 2025 •

edited

Loading

nagygergo May 14, 2025 •

edited

Loading

nagygergo May 16, 2025 •

edited

Loading