From 927d5e2deb9d31a48563ea20762a8af05e6e34ef Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Sat, 11 Mar 2023 18:51:00 -0500 Subject: [PATCH 1/6] OCPBUGS-9996: make lvms configuration more robust Instead of requiring the user to configure their system with a "rhel" volume group, look at the volume groups that exist and apply some selection rules: * If there is a "rhel" VG, use it. * If there is only one VG, use it. * Otherwise, disable the CSI driver with a log message. --- pkg/components/storage.go | 6 ++- pkg/config/lvmd/lvmd.go | 102 ++++++++++++++++++++++++++++++++--- pkg/config/lvmd/lvmd_test.go | 99 ++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 9 deletions(-) diff --git a/pkg/components/storage.go b/pkg/components/storage.go index 8a02e68350..6d12e50127 100644 --- a/pkg/components/storage.go +++ b/pkg/components/storage.go @@ -21,7 +21,7 @@ func getCSIPluginConfig() (*lvmd.Lvmd, error) { if _, err := os.Stat(lvmdConfig); !errors.Is(err, os.ErrNotExist) { return lvmd.NewLvmdConfigFromFile(lvmdConfig) } - return (&lvmd.Lvmd{}).WithDefaults(), nil + return lvmd.DefaultLvmdConfig() } func startCSIPlugin(cfg *config.MicroshiftConfig, kubeconfigPath string) error { @@ -81,6 +81,10 @@ func startCSIPlugin(cfg *config.MicroshiftConfig, kubeconfigPath string) error { if err != nil { return err } + if !lvmdCfg.IsEnabled() { + klog.V(2).Info("CSI is disabled. %s", lvmdCfg.DisabledReason) + return nil + } lvmdRenderParams, err := renderLvmdParams(lvmdCfg) if err != nil { return fmt.Errorf("rendering lvmd params: %v", err) diff --git a/pkg/config/lvmd/lvmd.go b/pkg/config/lvmd/lvmd.go index 2bf0e59bc3..7d86819126 100644 --- a/pkg/config/lvmd/lvmd.go +++ b/pkg/config/lvmd/lvmd.go @@ -3,34 +3,120 @@ package lvmd import ( "fmt" "os" + "os/exec" + "strings" "github.com/ghodss/yaml" + "k8s.io/klog/v2" ) const ( LvmdConfigFileName = "lvmd.yaml" defaultSockName = "/run/lvmd/lvmd.socket" defaultRHEL4EdgeVolumeGroup = "rhel" + + errorMessageNoVolumeGroups = "No volume groups found" + errorMessageMultipleVolumeGroups = "Multiple volume groups are available, but no configuration file was provided." ) // Lvmd stores the read-in or defaulted values of the lvmd configuration and provides the topolvm-node process information // about its host's storage environment. type Lvmd struct { - DeviceClasses []*DeviceClass `json:"device-classes"` - SocketName string `json:"socket-name"` + DeviceClasses []*DeviceClass `json:"device-classes"` + SocketName string `json:"socket-name"` + DisabledReason string `json:"-"` +} + +// IsEnabled returns a boolean indicating whether the CSI driver +// should be enabled for this host. +func (l *Lvmd) IsEnabled() bool { + return len(l.DeviceClasses) > 0 } -func (l *Lvmd) WithDefaults() *Lvmd { - l.SocketName = defaultSockName - l.DeviceClasses = []*DeviceClass{ +func uint64Ptr(val uint64) *uint64 { + return &val +} + +func getLvmdConfigForVGs(vgNames []string) (*Lvmd, error) { + response := &Lvmd{ + SocketName: defaultSockName, + } + + vgName := "" + if len(vgNames) == 0 { + + response.DisabledReason = errorMessageNoVolumeGroups + klog.V(2).Info(errorMessageNoVolumeGroups) + return response, nil + + } else if len(vgNames) == 1 { + + vgName = vgNames[0] + klog.V(2).Infof("Using volume group %q", vgName) + + } else { + + for _, name := range vgNames { + if name == defaultRHEL4EdgeVolumeGroup { + klog.V(2).Infof("Using default volume group %q", defaultRHEL4EdgeVolumeGroup) + vgName = name + break + } + } + + // If the default volume group is not found and there are + // multiple volume groups, disable the CSI driver. + if vgName == "" { + klog.V(2).Infof("Multiple volume groups available but no configuration file is present, disabling CSI. %v", vgNames) + response.DisabledReason = errorMessageMultipleVolumeGroups + return response, nil + } + } + + // Fill in the default device class using the selected volume + // group. + response.DeviceClasses = []*DeviceClass{ { Name: "default", - VolumeGroup: defaultRHEL4EdgeVolumeGroup, + VolumeGroup: vgName, Default: true, - SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), + SpareGB: uint64Ptr(defaultSpareGB), }, } - return l + return response, nil +} + +// DefaultLvmdConfig returns a configuration struct for Lvmd with +// default settings based on the current host. If a single volume +// group is found, that value is used. If multiple volume groups are +// available and one is named "rhel", that group is used. Otherwise, +// the configuration returned will report that it is not enabled (see +// IsEnabled()). +func DefaultLvmdConfig() (*Lvmd, error) { + vgNames, err := getVolumeGroups() + if err != nil { + return nil, fmt.Errorf("Failed to discover local volume groups: %s", err) + } + + return getLvmdConfigForVGs(vgNames) + +} + +// getVolumeGroups returns a slice of volume group names. +func getVolumeGroups() ([]string, error) { + cmd := exec.Command("vgs", "--readonly", "--options=name", "--noheadings") + output, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("error running vgs: %s", err) + } + names := []string{} + for _, line := range strings.Split(string(output), "\n") { + newName := strings.Trim(line, " \t\n") + if newName != "" { + names = append(names, newName) + } + } + return names, nil } func NewLvmdConfigFromFile(p string) (*Lvmd, error) { diff --git a/pkg/config/lvmd/lvmd_test.go b/pkg/config/lvmd/lvmd_test.go index 37ad13e84f..9a016130fc 100644 --- a/pkg/config/lvmd/lvmd_test.go +++ b/pkg/config/lvmd/lvmd_test.go @@ -4,8 +4,107 @@ import ( "encoding/json" "reflect" "testing" + + "github.com/stretchr/testify/assert" ) +func TestGetLvmdConfigForVGs(t *testing.T) { + tests := []struct { + name string + vgNames []string + expected *Lvmd + expectedErr error + }{ + { + name: "no groups", + expected: &Lvmd{ + SocketName: defaultSockName, + DisabledReason: errorMessageNoVolumeGroups, + }, + }, + { + name: "one group", + vgNames: []string{"choose-me"}, + expected: &Lvmd{ + SocketName: defaultSockName, + DeviceClasses: []*DeviceClass{ + { + Name: "default", + VolumeGroup: "choose-me", + Default: true, + SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), + }, + }, + }, + }, + { + name: "one group rhel", + vgNames: []string{"rhel"}, + expected: &Lvmd{ + SocketName: defaultSockName, + DeviceClasses: []*DeviceClass{ + { + Name: "default", + VolumeGroup: "rhel", + Default: true, + SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), + }, + }, + }, + }, + { + name: "rhel first", + vgNames: []string{"rhel", "other"}, + expected: &Lvmd{ + SocketName: defaultSockName, + DeviceClasses: []*DeviceClass{ + { + Name: "default", + VolumeGroup: "rhel", + Default: true, + SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), + }, + }, + }, + }, + { + name: "rhel last", + vgNames: []string{"other", "rhel"}, + expected: &Lvmd{ + SocketName: defaultSockName, + DeviceClasses: []*DeviceClass{ + { + Name: "default", + VolumeGroup: "rhel", + Default: true, + SpareGB: uint64Ptr(defaultSpareGB), + }, + }, + }, + }, + { + name: "no rhel", + vgNames: []string{"other", "choose-me"}, + expected: &Lvmd{ + SocketName: defaultSockName, + DisabledReason: errorMessageMultipleVolumeGroups, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual, err := getLvmdConfigForVGs(tt.vgNames) + assert.Equal(t, tt.expected, actual, "names: %v", tt.vgNames) + if tt.expectedErr != nil { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func Test_newLvmdConfigFromFile(t *testing.T) { iToP := func(i int) *uint64 { From b90c2e8f400125816aacefa4454f4550e4582717 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 15 Mar 2023 11:35:27 -0400 Subject: [PATCH 2/6] OCPBUGS-9996: update documentation explaining how the VG is picked --- docs/default_csi_plugin.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/default_csi_plugin.md b/docs/default_csi_plugin.md index c8ff733034..3b02ecec42 100644 --- a/docs/default_csi_plugin.md +++ b/docs/default_csi_plugin.md @@ -40,11 +40,17 @@ is run as. ## System Requirements -### Volume Group Name +### Default Volume Group -The default integration of LVMS assumes a volume-group named `rhel`. LVMS's node-controller expects that volume -group to exist prior to launching the service. If the volume group does not exist, the node-controller will fail to -start and enter a CrashLoopBackoff state. +If there is only one volume group on the system, LVMS uses it by +default. If there are multiple volume groups, and no configuration +file, LVMS looks for a volume group named `rhel`. If there is no +volume group named `rhel`, LVMS is disabled. + +LVMS expects all volume groups to exist prior to launching the +service. If LVMS is configured to use a volume group that does not +exist, the node-controller Pod will fail and enter a CrashLoopBackoff +state. ### Volume Size Increments From 5ad1d4e30984fb46ee0d858432e1ca07d6344407 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 15 Mar 2023 11:35:52 -0400 Subject: [PATCH 3/6] update LVMS documentation about configuration file location --- docs/default_csi_plugin.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/docs/default_csi_plugin.md b/docs/default_csi_plugin.md index 3b02ecec42..28bae4bdf8 100644 --- a/docs/default_csi_plugin.md +++ b/docs/default_csi_plugin.md @@ -31,12 +31,8 @@ Full documentation of the config spec can be found at [github.com/red-hat-storag #### Path -The user provided lvmd config should be written to the same directory as the MicroShift config. If a MicroShift config -doesn't exist, MicroShift will assume default lvmd values. These paths will be checked for the config, depending on the user MicroShift -is run as. - -1. User config dir: `~/.microshift/lvmd.yaml` -2. Global config dir: `/etc/microshift/lvmd.yaml` +The user provided lvmd config should be written to the same directory as the MicroShift config. If an lvmd configuration file +does not exist in `/etc/microshift/lvmd.yaml`, MicroShift will use default values. ## System Requirements From 2115c168bdb6b8639e410477da26e429a18e20cf Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Wed, 15 Mar 2023 11:36:07 -0400 Subject: [PATCH 4/6] our LVMS is not a fork, it is a build of upstream --- docs/default_csi_plugin.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/default_csi_plugin.md b/docs/default_csi_plugin.md index 28bae4bdf8..8a6d30da6c 100644 --- a/docs/default_csi_plugin.md +++ b/docs/default_csi_plugin.md @@ -3,7 +3,7 @@ > **IMPORTANT!** The default LVMS configuration is intended to match the developer environment described in [MicroShift Development Environment](./devenv_setup.md). See section **[Configuring LVMS](#Configuring-LVMS)** for guidance on configuring LVMS for your environment. MicroShift enables dynamic storage provisioning out of the box with the LVMS CSI plugin. This plugin is a downstream -Red Hat fork of TopoLVM. This provisioner will create a new LVM logical volume in the `rhel` volume group for each +Red Hat build of TopoLVM. This provisioner will create a new LVM logical volume in the `rhel` volume group for each PersistenVolumeClaim(PVC), and make these volumes available to pods. For more information on LVMS, visit the repo's [README](https://github.com/red-hat-storage/topolvm). From 1e8f5367646eba4a776f05605631b17fe2dbc59a Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Fri, 17 Mar 2023 10:15:07 -0400 Subject: [PATCH 5/6] OCPBUGS-9996: Change default VG name to "microshift" When there are multiple volume groups, look for one named "microshift" to use, instead of "rhel". --- docs/default_csi_plugin.md | 6 +++--- pkg/config/lvmd/lvmd.go | 2 +- pkg/config/lvmd/lvmd_test.go | 20 ++++++++++---------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/docs/default_csi_plugin.md b/docs/default_csi_plugin.md index 8a6d30da6c..4bd2682f62 100644 --- a/docs/default_csi_plugin.md +++ b/docs/default_csi_plugin.md @@ -32,7 +32,7 @@ Full documentation of the config spec can be found at [github.com/red-hat-storag #### Path The user provided lvmd config should be written to the same directory as the MicroShift config. If an lvmd configuration file -does not exist in `/etc/microshift/lvmd.yaml`, MicroShift will use default values. +does not exist in `/etc/microshift/lvmd.yaml`, MicroShift will use default values. ## System Requirements @@ -40,8 +40,8 @@ does not exist in `/etc/microshift/lvmd.yaml`, MicroShift will use default value If there is only one volume group on the system, LVMS uses it by default. If there are multiple volume groups, and no configuration -file, LVMS looks for a volume group named `rhel`. If there is no -volume group named `rhel`, LVMS is disabled. +file, LVMS looks for a volume group named `microshift`. If there is no +volume group named `microshift`, LVMS is disabled. LVMS expects all volume groups to exist prior to launching the service. If LVMS is configured to use a volume group that does not diff --git a/pkg/config/lvmd/lvmd.go b/pkg/config/lvmd/lvmd.go index 7d86819126..5036013082 100644 --- a/pkg/config/lvmd/lvmd.go +++ b/pkg/config/lvmd/lvmd.go @@ -13,7 +13,7 @@ import ( const ( LvmdConfigFileName = "lvmd.yaml" defaultSockName = "/run/lvmd/lvmd.socket" - defaultRHEL4EdgeVolumeGroup = "rhel" + defaultRHEL4EdgeVolumeGroup = "microshift" errorMessageNoVolumeGroups = "No volume groups found" errorMessageMultipleVolumeGroups = "Multiple volume groups are available, but no configuration file was provided." diff --git a/pkg/config/lvmd/lvmd_test.go b/pkg/config/lvmd/lvmd_test.go index 9a016130fc..667baf8639 100644 --- a/pkg/config/lvmd/lvmd_test.go +++ b/pkg/config/lvmd/lvmd_test.go @@ -38,14 +38,14 @@ func TestGetLvmdConfigForVGs(t *testing.T) { }, }, { - name: "one group rhel", - vgNames: []string{"rhel"}, + name: "one group default", + vgNames: []string{defaultRHEL4EdgeVolumeGroup}, expected: &Lvmd{ SocketName: defaultSockName, DeviceClasses: []*DeviceClass{ { Name: "default", - VolumeGroup: "rhel", + VolumeGroup: defaultRHEL4EdgeVolumeGroup, Default: true, SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), }, @@ -53,14 +53,14 @@ func TestGetLvmdConfigForVGs(t *testing.T) { }, }, { - name: "rhel first", - vgNames: []string{"rhel", "other"}, + name: "default first", + vgNames: []string{defaultRHEL4EdgeVolumeGroup, "other"}, expected: &Lvmd{ SocketName: defaultSockName, DeviceClasses: []*DeviceClass{ { Name: "default", - VolumeGroup: "rhel", + VolumeGroup: defaultRHEL4EdgeVolumeGroup, Default: true, SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), }, @@ -68,14 +68,14 @@ func TestGetLvmdConfigForVGs(t *testing.T) { }, }, { - name: "rhel last", - vgNames: []string{"other", "rhel"}, + name: "default last", + vgNames: []string{"other", defaultRHEL4EdgeVolumeGroup}, expected: &Lvmd{ SocketName: defaultSockName, DeviceClasses: []*DeviceClass{ { Name: "default", - VolumeGroup: "rhel", + VolumeGroup: defaultRHEL4EdgeVolumeGroup, Default: true, SpareGB: uint64Ptr(defaultSpareGB), }, @@ -83,7 +83,7 @@ func TestGetLvmdConfigForVGs(t *testing.T) { }, }, { - name: "no rhel", + name: "no default", vgNames: []string{"other", "choose-me"}, expected: &Lvmd{ SocketName: defaultSockName, From 22c7d0c20f097e64027bbab9a9220b4303dc74b9 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Sun, 19 Mar 2023 10:38:08 -0400 Subject: [PATCH 6/6] OCPBUGS-9996: show where we get the lvmd configuration Include a message in the body of the file added to the config map to explain where the content came from. --- pkg/components/render.go | 6 +++++- pkg/components/storage.go | 2 +- pkg/config/lvmd/lvmd.go | 15 ++++++++++----- pkg/config/lvmd/lvmd_test.go | 13 +++++++++---- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/pkg/components/render.go b/pkg/components/render.go index ce2c26c39d..b24723c5db 100755 --- a/pkg/components/render.go +++ b/pkg/components/render.go @@ -54,7 +54,11 @@ func renderLvmdParams(l *lvmd.Lvmd) (assets.RenderParams, error) { if err != nil { return nil, err } - r["lvmd"] = string(b) + content := string(b) + if l.Message != "" { + content = fmt.Sprintf("# %s\n%s", l.Message, content) + } + r["lvmd"] = content r["SocketName"] = l.SocketName return r, nil } diff --git a/pkg/components/storage.go b/pkg/components/storage.go index 6d12e50127..e55ac423d3 100644 --- a/pkg/components/storage.go +++ b/pkg/components/storage.go @@ -82,7 +82,7 @@ func startCSIPlugin(cfg *config.MicroshiftConfig, kubeconfigPath string) error { return err } if !lvmdCfg.IsEnabled() { - klog.V(2).Info("CSI is disabled. %s", lvmdCfg.DisabledReason) + klog.V(2).Info("CSI is disabled. %s", lvmdCfg.Message) return nil } lvmdRenderParams, err := renderLvmdParams(lvmdCfg) diff --git a/pkg/config/lvmd/lvmd.go b/pkg/config/lvmd/lvmd.go index 5036013082..b0bea33732 100644 --- a/pkg/config/lvmd/lvmd.go +++ b/pkg/config/lvmd/lvmd.go @@ -17,14 +17,16 @@ const ( errorMessageNoVolumeGroups = "No volume groups found" errorMessageMultipleVolumeGroups = "Multiple volume groups are available, but no configuration file was provided." + statusMessageFoundDefault = "Found default volume group \"microshift\"" + statusMessageDefaultAvailable = "Defaulting to the only available volume group" ) // Lvmd stores the read-in or defaulted values of the lvmd configuration and provides the topolvm-node process information // about its host's storage environment. type Lvmd struct { - DeviceClasses []*DeviceClass `json:"device-classes"` - SocketName string `json:"socket-name"` - DisabledReason string `json:"-"` + DeviceClasses []*DeviceClass `json:"device-classes"` + SocketName string `json:"socket-name"` + Message string `json:"-"` } // IsEnabled returns a boolean indicating whether the CSI driver @@ -45,7 +47,7 @@ func getLvmdConfigForVGs(vgNames []string) (*Lvmd, error) { vgName := "" if len(vgNames) == 0 { - response.DisabledReason = errorMessageNoVolumeGroups + response.Message = errorMessageNoVolumeGroups klog.V(2).Info(errorMessageNoVolumeGroups) return response, nil @@ -53,6 +55,7 @@ func getLvmdConfigForVGs(vgNames []string) (*Lvmd, error) { vgName = vgNames[0] klog.V(2).Infof("Using volume group %q", vgName) + response.Message = statusMessageDefaultAvailable } else { @@ -60,6 +63,7 @@ func getLvmdConfigForVGs(vgNames []string) (*Lvmd, error) { if name == defaultRHEL4EdgeVolumeGroup { klog.V(2).Infof("Using default volume group %q", defaultRHEL4EdgeVolumeGroup) vgName = name + response.Message = statusMessageFoundDefault break } } @@ -68,7 +72,7 @@ func getLvmdConfigForVGs(vgNames []string) (*Lvmd, error) { // multiple volume groups, disable the CSI driver. if vgName == "" { klog.V(2).Infof("Multiple volume groups available but no configuration file is present, disabling CSI. %v", vgNames) - response.DisabledReason = errorMessageMultipleVolumeGroups + response.Message = errorMessageMultipleVolumeGroups return response, nil } } @@ -133,5 +137,6 @@ func NewLvmdConfigFromFile(p string) (*Lvmd, error) { if l.SocketName == "" { l.SocketName = defaultSockName } + l.Message = fmt.Sprintf("Read from %s", p) return l, nil } diff --git a/pkg/config/lvmd/lvmd_test.go b/pkg/config/lvmd/lvmd_test.go index 667baf8639..854117e006 100644 --- a/pkg/config/lvmd/lvmd_test.go +++ b/pkg/config/lvmd/lvmd_test.go @@ -18,8 +18,8 @@ func TestGetLvmdConfigForVGs(t *testing.T) { { name: "no groups", expected: &Lvmd{ - SocketName: defaultSockName, - DisabledReason: errorMessageNoVolumeGroups, + SocketName: defaultSockName, + Message: errorMessageNoVolumeGroups, }, }, { @@ -35,6 +35,7 @@ func TestGetLvmdConfigForVGs(t *testing.T) { SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), }, }, + Message: statusMessageDefaultAvailable, }, }, { @@ -50,6 +51,7 @@ func TestGetLvmdConfigForVGs(t *testing.T) { SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), }, }, + Message: statusMessageDefaultAvailable, }, }, { @@ -65,6 +67,7 @@ func TestGetLvmdConfigForVGs(t *testing.T) { SpareGB: func() *uint64 { s := uint64(defaultSpareGB); return &s }(), }, }, + Message: statusMessageFoundDefault, }, }, { @@ -80,14 +83,15 @@ func TestGetLvmdConfigForVGs(t *testing.T) { SpareGB: uint64Ptr(defaultSpareGB), }, }, + Message: statusMessageFoundDefault, }, }, { name: "no default", vgNames: []string{"other", "choose-me"}, expected: &Lvmd{ - SocketName: defaultSockName, - DisabledReason: errorMessageMultipleVolumeGroups, + SocketName: defaultSockName, + Message: errorMessageMultipleVolumeGroups, }, }, } @@ -150,6 +154,7 @@ func Test_newLvmdConfigFromFile(t *testing.T) { SpareGB: iToP(5), }, }, + Message: "Read from ./test/lvmd.yaml", }, wantErr: false, },