Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bcl merged 1 commit into from
Aug 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions pkg/distro/defs/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ func NewDistroYAML(nameVer string) (*DistroYAML, error) {
if err := v.runTemplates(foundDistro); err != nil {
return nil, err
}
if err := v.setupDefaultFS(foundDistro.DefaultFSType.String()); err != nil {
return nil, err
}

foundDistro.imageTypes[name] = v
}
}
Expand Down Expand Up @@ -462,6 +466,44 @@ func (it *ImageTypeYAML) runTemplates(distro *DistroYAML) error {
return nil
}

func (it *ImageTypeYAML) setupDefaultFS(distroDefaultFS string) error {
subs := func(pts map[string]*disk.PartitionTable) error {
for _, pt := range pts {
err := pt.ForEachMountable(func(mnt disk.Mountable, _ []disk.Entity) error {
elem, ok := mnt.(*disk.Filesystem)
if !ok {
return nil
}
if elem.Type == "" {
if distroDefaultFS == "" {
return fmt.Errorf("mount %q requires a default filesystem for the distribution but none set", mnt.GetMountpoint())
}
elem.Type = distroDefaultFS
}
return nil
})
if err != nil {
return err
}
}
return nil
}
// we need to update both the partition tables and all
// partition tables overrides
if err := subs(it.PartitionTables); err != nil {
return err
}
if it.PartitionTablesOverrides != nil {
for _, cond := range it.PartitionTablesOverrides.Conditions {
if err := subs(cond.Override); err != nil {
return err
}
}
}
Comment on lines +496 to +502
Copy link
Member

@thozza thozza Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Contributor Author

@mvo5 mvo5 Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@thozza thozza Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


return nil
}

type platformsOverride struct {
Conditions map[string]*conditionsPlatforms `yaml:"conditions,omitempty"`
}
Expand Down
121 changes: 121 additions & 0 deletions pkg/distro/defs/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,127 @@ image_types:
}, partTable)
}

func TestDefsPartitionTableFilesystemDistroDefault(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:
test_arch:
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}, "test_arch")
require.NoError(t, err)
assert.Equal(t, &disk.PartitionTable{
Partitions: []disk.Partition{
{
Payload: &disk.Filesystem{
Type: "ext4",
Mountpoint: "/",
},
},
},
}, partTable)
}

func TestDefsPartitionTableFilesystemPartTableOverride(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:
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)
}

func TestDefsPartitionTableFilesystemDistroDefaultErr(t *testing.T) {
fakeDistrosYaml := `
distros:
- name: test-distro-1
defs_path: test-distro
`
fakeImageTypesYaml := `
image_types:
test_type:
filename: test.img
platforms:
- arch: x86_64
partition_table:
test_arch:
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()
_, err := defs.NewDistroYAML("test-distro-1")
assert.EqualError(t, err, `mount "/" requires a default filesystem for the distribution but none set`)
}

var fakeImageTypesYaml = `
image_types:
test_type:
Expand Down
Loading