distro: add support for filesystem from default_fs_type (HMS-8844)

[mvo5]

[splitout of https://github.com//pull/1656]

When we move to bootc we need a way to declare that certain
partitions use the distribution default filesystem as the
partition table itself is generic. This commit adds support
in the YAML to omit the filesystem type, i.e. to just write:

    partition_table:
      test_arch:
        partitions:
          - payload_type: filesystem
            payload:
              mountpoint: "/"

which will then translated at YAML load time to the default
filesystem declared by the distribution. This allows us to
write a generic YAML based partition table in images that
is the same as the partition table in bootc-image-builder.

/jira-epic HMS-8844

[achilleas-k]

I'm not sure about the magic value distro-default in the yaml configs though. Somehow to me it feels more natural to just use an empty value, implicitly falling back to the default. I think distro-default feels to me like we're inching close to doing variables in our yaml configs.

[mvo5]

I'm not sure about the magic value distro-default in the yaml configs though. Somehow to me it feels more natural to just use an empty value, implicitly falling back to the default. I think distro-default feels to me like we're inching close to doing variables in our yaml configs.

Thanks, yeah, empty value is fine with me as well, I updated the code.

[achilleas-k]

Splendid. Ship it!

[supakeen]

This got removed from the merge queue due to (I think) the recent changes to distro.ID.

[achilleas-k]

diff --git a/pkg/distro/defs/loader_test.go b/pkg/distro/defs/loader_test.go
index 0b5229e57..28faf92d3 100644
--- a/pkg/distro/defs/loader_test.go
+++ b/pkg/distro/defs/loader_test.go
@@ -339,12 +339,12 @@ image_types:
 	baseDir := makeFakeDistrosYAML(t, fakeDistrosYaml, fakeImageTypesYaml)
 	restore := defs.MockDataFS(baseDir)
 	defer restore()
-	distro, err := defs.NewDistroYAML("test-distro-1")
+	td, err := defs.NewDistroYAML("test-distro-1")
 	require.NoError(t, err)
-	it := distro.ImageTypes()["test_type"]
+	it := td.ImageTypes()["test_type"]
 	require.NotNil(t, it)
 
-	partTable, err := it.PartitionTable("test-distro-1", "test_arch")
+	partTable, err := it.PartitionTable(distro.ID{Name: "test-distro", MajorVersion: 1}, "test_arch")
 	require.NoError(t, err)
 	assert.Equal(t, &disk.PartitionTable{
 		Partitions: []disk.Partition{
@@ -390,12 +390,12 @@ image_types:
 	baseDir := makeFakeDistrosYAML(t, fakeDistrosYaml, fakeImageTypesYaml)
 	restore := defs.MockDataFS(baseDir)
 	defer restore()
-	distro, err := defs.NewDistroYAML("test-distro-1")
+	td, err := defs.NewDistroYAML("test-distro-1")
 	require.NoError(t, err)
-	it := distro.ImageTypes()["test_type"]
+	it := td.ImageTypes()["test_type"]
 	require.NotNil(t, it)
 
-	partTable, err := it.PartitionTable("test-distro-1", "x86_64")
+	partTable, err := it.PartitionTable(distro.ID{Name: "test-distro", MajorVersion: 1}, "x86_64")
 	require.NoError(t, err)
 	assert.Equal(t, &disk.PartitionTable{
 		Partitions: []disk.Partition{

[achilleas-k]

Since Michael's out this week, I rebased and fixed this so we can get it merged.

[bcl]

I'd also like to see it test:

• not overriding an existing type
• not overriding an existing type in an override

But I don't think not having those should block merging this if all the slots line up finally...

[supakeen]

Re-approval now that conflicts have been fixed.

[thozza]

LGTM

[thozza]

This also has a side effect that if the partition table explicitly specifies the FS type for a mountpoint, which is then overridden by the condition that does not define it, it would use the default FS type.

This is expected, given the implementation. However, it is not tested by a unit test.

What I mean is

func TestDefsPartitionTableFilesystemPartTableOverrideDefault(t *testing.T) {
	fakeDistrosYaml := `
distros:
  - name: test-distro-1
    defs_path: test-distro
    default_fs_type: ext4
`
	fakeImageTypesYaml := `
image_types:
  test_type:
    filename: test.img
    platforms:
      - arch: x86_64
    partition_table:
      x86_64:
        partitions:
          - payload_type: filesystem
            payload:
              type: xfs
              mountpoint: "/"
    partition_tables_override:
      conditions:
        "test condition":
          when:
            distro_name: test-distro
          override:
            x86_64:
              partitions:
                - payload_type: filesystem
                  payload:
                    # note that no "type: <fstype>" is set here
                    mountpoint: "/"
`
	baseDir := makeFakeDistrosYAML(t, fakeDistrosYaml, fakeImageTypesYaml)
	restore := defs.MockDataFS(baseDir)
	defer restore()
	td, err := defs.NewDistroYAML("test-distro-1")
	require.NoError(t, err)
	it := td.ImageTypes()["test_type"]
	require.NotNil(t, it)

	partTable, err := it.PartitionTable(distro.ID{Name: "test-distro", MajorVersion: 1}, "x86_64")
	require.NoError(t, err)
	assert.Equal(t, &disk.PartitionTable{
		Partitions: []disk.Partition{
			{
				Payload: &disk.Filesystem{
					Type:       "ext4",
					Mountpoint: "/",
				},
			},
		},
	}, partTable)
}

[mvo5]

Thanks, I put this into a commit and it will be part of a followup. I think the behavior is okay so just testing for it to ensure we are consistent should be enough, right?

[thozza]

It is to be expected given the implementation. I can imagine someone being surprised or bitten by the behavior. However, in an ideal case, we should have unit tests that specify which FS type should be used for a specific mountpoint on a given Distro x Release x Arch, if there is an override for it (to not be surprised).