feat: upgrade existing filesystem by upils · Pull Request #264 · canonical/chisel

upils · 2026-01-29T16:24:00Z

Have you signed the CLA?

This commit enables Chisel detecting the target directory contains the result
of a previous execution to then operate an upgrade of the content. This initial
simple implementation has the the limitation that content (files, symlinks) is
systematically replaced, even if identical. As a consequences:

user modifications on chisel-managed content is overridden.
in the context of a OCI image build, this "new" content is identified as
different and thus the new OCI layer contains duplicated files.

Signed-off-by: Paul Mars <paul.mars@canonical.com>

github-actions · 2026-01-29T16:28:45Z

Command	Mean [s]	Min [s]	Max [s]	Relative
`BASE`	13.045 ± 0.106	12.934	13.289	1.00
`HEAD`	13.085 ± 0.199	12.766	13.338	1.00 ± 0.02

letFunny

First review. I am missing a couple of files but I wanted to publish it as quickly as possible so we have more time to iterate. This is looking great Paul, many things are looking dead simple which is always an extremely good sign. I have written some comments about how to make the PR shorter and how to make it, hopefully, even simpler. Let me know what you think.

I see some of the comments are similar to past discussions we had in upils#1. We spent some effort there, so please make sure all the comments were carried over to this one.

internal/fsutil/create.go

letFunny · 2026-02-04T15:19:27Z

cmd/chisel/cmd_cut.go

 		}
 	}

+	mfest, err := slicer.SelectValidManifest(cmd.RootDir, release)


In general our philosophy is that we should be able to cut a release from main at any point in time. That is, if we ever do a bug fix we should be able to release a point version. This is changing how Chisel works in folders with a manifest and it is not the correct behavior (yet) so I would prefer if we could gate this functionality over a environmental variable or a debug command or any other mechanism.

letFunny · 2026-02-04T15:21:23Z

internal/fsutil/move.go

+	case 0, fs.ModeSymlink:
+		err = os.Rename(srcPath, dstPath)
+	case fs.ModeDir:
+		err = createDir(&CreateOptions{


This is not moving the directory, it is left behind. It is also unexpected when an API is called Move and it moves the directory without its contents.

You are right, "move" might not be the right name. What about:

"Sync"?

"Install"? I really like this one. Even if our strategy to install the content might be slightly different than the install command, the feature is the same I think.

"Populate"?

internal/fsutil/move.go

internal/manifestutil/manifestutil_test.go

internal/fsutil/remove.go

letFunny · 2026-02-04T16:07:07Z

internal/slicer/slicer.go

 		targetDir = filepath.Join(dir, targetDir)
 	}

+	var originalTargetDir string


A different way of doing it is to extract what used to be the Run function into another one and have Run call into that with different target dir and then upgrade (see extract.go for Run vs extractData). Do you think it will be clearer this way?

I have no strong opinion for one or the other. It is unclear to me what lead extractData to be split into it's own function. Can you share the reason?

It was already like that when I joined :). IMO it is better to split than to carry more data around and "fake" it bypassing the interface by overriding target and copying original.

It is also a cleaner abstraction as it doesn't need the rest of the code and will make the function shorter (even if marginally).

internal/slicer/slicer.go

letFunny

The final set of files. Thanks again Paul for working on this.

letFunny · 2026-02-05T14:24:36Z

tests/recut/task.yaml

+  chisel_release="./release_${RELEASE}"
+  mkdir -p ${chisel_release}/slices
+
+  ref_chisel_release="ref-chisel-release_${RELEASE}"
+  git clone --depth=1 -b ${OS}-${RELEASE} \
+    https://github.com/canonical/chisel-releases $ref_chisel_release
+
+  cp ${ref_chisel_release}/chisel.yaml ${chisel_release}/chisel.yaml
+
+  cat >${chisel_release}/slices/base-files.yaml <<'EOF'
+    package: base-files
+
+    slices:
+      slice-a:
+        contents:
+          /etc/debian_version:
+      slice-b:
+        contents:
+          /etc/issue:
+      manifest:
+        contents:
+          /chisel/**: {generate: manifest}
+  EOF


In my opinion, this is a bit heavy for what we need to test. We really do not care about testing the different releases as the release itself should not be important. I think you are correct in adding a simplified base-files.yaml manually, but I would take it a step further and add a release similar to pro-archives / unmaintained tests.

This is only a suggestion, I don't have a strong opinion, both work.

letFunny · 2026-02-05T14:29:13Z

tests/recut/task.yaml

+    EXTRA_OPTIONS="--ignore=unmaintained "
+  fi
+
+  # First cut generates manifest and installs slice-a


Suggested change

# First cut generates manifest and installs slice-a

# First cut generates manifest and installs slice-a.

Remember to punctuate comments please :)

letFunny · 2026-02-05T14:31:36Z

tests/recut/task.yaml

+  test -s ${rootfs_folder}/etc/debian_version
+  test -f ${rootfs_folder}/chisel/manifest.wall
+
+  # Remove a file from the first slice


Please add a TODO here saying that this is not expected behavior of the final feature. We should highlight in the test that this is only temporary.

I think what you are trying to test is that the content of pre-installed slices gets updated. What I would do is to change the contents and change the manifest to contain the new hash. The test will then verify that the file was updated. This will work now and in the future as it is highlighting the correct behavior.

letFunny · 2026-02-05T14:36:29Z

internal/slicer/slicer_test.go

 		"/dir/file": "file 0644 cc55e2ec {test-package_third}",
 	},
+}, {
+	summary: "Recut removes obsolete paths when selection shrinks",


I think we are pushing this table too much now. This should really be test for the inner function, i.e. the equivalent of extract_data we discus in the other comments (not saying you should call internal function, I am just talking about conceptually).

I think we need a different test table that verifies re-cut and will take different data. You can have a release (maybe with a default release) and two inputs for the slices installed. The test installation will install version one and then call cut again. Compared to the current version, it is less leaky of the report abstraction as we don't need to build it manually, it is more faithful to what the real execution looks like and it is also far simpler to read and write as we are talking about normal chisel releases file contents, not the intricacies of the report with slice attribution.

I expect the file to change substantially so I will not comment on the rest for now.

letFunny · 2026-02-05T14:40:54Z

internal/slicer/slicer.go

+		hash string
+	}
+	var selected *manifest.Manifest
+	schemaManifest := make(map[string]manifestHash)


There is only one valid schema, why not keep a single variable with the expected hash that starts empty and then, when filled, is used to compare manifests?

letFunny · 2026-02-05T14:41:40Z

internal/slicer/slicer.go

+	var selected *manifest.Manifest
+	schemaManifest := make(map[string]manifestHash)
+	for _, mfestPath := range manifestPaths {
+		err := func() error {


Why do we need this closure?

letFunny · 2026-02-05T14:42:46Z

internal/slicer/slicer.go

+			defer r.Close()
+			mfest, err := manifest.Read(r)
+			if err != nil {
+				return err


This will not work correctly unless you add a custom error for the invalid schema and verify it here, I remember we discussed this a number of times in the other PR. Is there a test with a manifest with a different version that is valid jsonwall? I may have missed it.

letFunny · 2026-02-05T14:46:23Z

internal/slicer/slicer.go

+			if !ok {
+				schemaManifest[mfest.Schema()] = manifestHash{mfestPath, mfestHash}
+			} else if refMfest.hash != mfestHash {
+				return fmt.Errorf("inconsistent manifests: %q and %q", refMfest.path, mfestPath)
+			}
+
+			if selected == nil || manifestutil.CompareSchemas(mfest.Schema(), selected.Schema()) > 0 {
+				selected = mfest
+			}


This is a bit weird, I would remove this logic until we have a need for it (see my other comment). We need to verify schema matches expected and matches hash (if set). This is trying to predict the future, but let's not anticipate small details about code when not needed.

Also, as a side-node, checking the hash for each version is different from:

Consistency with all other manifests with the same schema is verified so the selection is deterministic.

which I read as, verify all manifests of the selected schema. Is that what we want? (we probably don't want to have this discussion and we want to simplify it further instead).

upils added 2 commits January 27, 2026 11:28

feat: search, load and validates manifests

491cd0c

Signed-off-by: Paul Mars <paul.mars@canonical.com>

feat: adapt to revised strategy

a6e57f7

upils added 6 commits January 30, 2026 14:26

feat: implement upgrade

5da0450

fix: deletion

5703751

fix: revert inadvertent change

26bb2b3

fix: check slice collecting error

4a14230

fix: simplify upgrade and improve deletion

865c1b8

refactor: cleaning

0343ddc

upils changed the title ~~feat: update existing filesystem~~ feat: upgrade existing filesystem Feb 2, 2026

refactor: refine upgrade

e116424

upils requested a review from letFunny February 2, 2026 14:35

upils marked this pull request as ready for review February 2, 2026 14:35

upils added 8 commits February 3, 2026 09:31

refactor: fsutil handles moving and removing

5b9d408

refactor: improve consistency

7861c8d

style: fix lint error

27a0d22

tests: Move and Remove

5f377b0

tests: FindPathsInRelease

f74265c

tests: more test cases for Move

799373c

tests: add spread test

dbe3f30

tests: fix recut spread test

becfe42

upils mentioned this pull request Feb 4, 2026

feat: search, load and validates manifests upils/chisel#1

Closed

1 task

upils added 5 commits February 4, 2026 09:44

tests: SelectValidManifest

3c994b8

fix: add missing deps

6ebb893

tests: recut feature

ceac217

fix: avoid duplicates in FindPathsInRelease

7e34e32

ci: rerun

e1d4e17

letFunny suggested changes Feb 4, 2026

View reviewed changes

upils added 2 commits February 5, 2026 08:46

fix: apply PR suggestions

c3db9d4

refactor: simplify FindPaths and FindPathsInRelease

e1e9c43

upils added 3 commits February 5, 2026 09:19

style: lint

da567ba

fix: remove outdated comment

2a6c891

test: fix inaccurate tests

398e032

letFunny suggested changes Feb 5, 2026

View reviewed changes

upils added 2 commits February 6, 2026 14:20

refactor: apply PR suggestions

7c17f18

refactor: revert error message change

7caf3ba

	# First cut generates manifest and installs slice-a
	# First cut generates manifest and installs slice-a.

Conversation

upils commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

letFunny left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

upils commented Jan 29, 2026 •

edited

Loading

github-actions bot commented Jan 29, 2026 •

edited

Loading