Skip to content
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

console: validate disk size earlier, update error wording #596

Merged
merged 1 commit into from
Nov 27, 2023
Merged

console: validate disk size earlier, update error wording #596

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
8 changes: 4 additions & 4 deletions pkg/config/cos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,22 @@ func TestCalcCosPersistentPartSize(t *testing.T) {
{
diskSize: 150,
partitionSize: "100Gi",
err: "Disk size is too small. Minimum 250Gi is required",
err: "Installation disk size is too small. Minimum 250Gi is required",
},
{
diskSize: 300,
partitionSize: "153600Ki",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 2000,
partitionSize: "1.5Ti",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 500,
partitionSize: "abcd",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
}

Expand Down
49 changes: 33 additions & 16 deletions pkg/console/install_panels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,360 +282,377 @@
return options, nil
}

func addDiskPanel(c *Console) error {
diskConfirmed = false

setLocation := createVerticalLocator(c)
diskOpts, err := getDiskOptions()
if err != nil {
return err
}

// Select device panel
diskV, err := widgets.NewDropDown(c.Gui, diskPanel, diskLabel, func() ([]widgets.Option, error) {
return diskOpts, nil
})
if err != nil {
return err
}
diskV.PreShow = func() error {
if c.config.Install.Device == "" {
c.config.Install.Device = diskOpts[0].Value
}
if err := diskV.SetData(c.config.Install.Device); err != nil {
return err
}

if err := c.setContentByName(diskNotePanel, ""); err != nil {
return err
}
return c.setContentByName(titlePanel, "Choose installation target and data disk. Device will be formatted")
}
setLocation(diskV.Panel, 3)
c.AddElement(diskPanel, diskV)

dataDiskV, err := widgets.NewDropDown(c.Gui, dataDiskPanel, dataDiskLabel, func() ([]widgets.Option, error) {
return getDataDiskOptions(c.config)
})
if err != nil {
return err
}

dataDiskV.PreShow = func() error {
if c.config.Install.DataDisk == "" {
c.config.Install.DataDisk = c.config.Install.Device
}
return dataDiskV.SetData(c.config.Install.DataDisk)
}
setLocation(dataDiskV.Panel, 3)
c.AddElement(dataDiskPanel, dataDiskV)

// Persistent partition size panel
persistentSizeV, err := widgets.NewInput(c.Gui, persistentSizePanel, persistentSizeLabel, false)
if err != nil {
return err
}
persistentSizeV.PreShow = func() error {
c.Cursor = true

device := c.config.Install.Device
if device == "" {
device = diskOpts[0].Value
}

//If the user has already set a persistent partition size, use that
if persistentSizeV.Value != "" {
if c.config.Install.PersistentPartitionSize != "" {
persistentSizeV.Value = c.config.Install.PersistentPartitionSize
} else {
defaultValue, err := calculateDefaultPersistentSize(device)
if err != nil {
return err
}
persistentSizeV.Value = defaultValue
}
} else {
defaultValue, err := calculateDefaultPersistentSize(device)
if err != nil {
return err
}
persistentSizeV.Value = defaultValue
}
return nil
}
setLocation(persistentSizeV, 3)
c.AddElement(persistentSizePanel, persistentSizeV)

// Asking force MBR title
askForceMBRTitleV := widgets.NewPanel(c.Gui, askForceMBRTitlePanel)
askForceMBRTitleV.SetContent("Use MBR partitioning scheme")
setLocation(askForceMBRTitleV, 3)
c.AddElement(askForceMBRTitlePanel, askForceMBRTitleV)

// Asking force MBR DropDown
askForceMBRV, err := widgets.NewDropDown(c.Gui, askForceMBRPanel, "", func() ([]widgets.Option, error) {
return []widgets.Option{{Value: "no", Text: "No"}, {Value: "yes", Text: "Yes"}}, nil
})
if err != nil {
return err
}
askForceMBRV.PreShow = func() error {
c.Cursor = true
if c.config.ForceMBR {
return askForceMBRV.SetData("yes")
}
return askForceMBRV.SetData("no")
}
setLocation(askForceMBRV.Panel, 3)
c.AddElement(askForceMBRPanel, askForceMBRV)

// Note panel for ForceMBR and persistent partition size
diskNoteV := widgets.NewPanel(c.Gui, diskNotePanel)
diskNoteV.Wrap = true
setLocation(diskNoteV, 3)
c.AddElement(diskNotePanel, diskNoteV)

// Panel for showing validator message
diskValidatorV := widgets.NewPanel(c.Gui, diskValidatorPanel)
diskValidatorV.FgColor = gocui.ColorRed
diskValidatorV.Wrap = true
updateValidatorMessage := func(msg string) error {
diskValidatorV.Focus = false
return c.setContentByName(diskValidatorPanel, msg)
}
setLocation(diskValidatorV, 3)
c.AddElement(diskValidatorPanel, diskValidatorV)

// Helper functions
validateAllDiskSizes := func() (bool, error) {
installDisk := c.config.Install.Device
dataDisk := c.config.Install.DataDisk

if dataDisk == "" || installDisk == dataDisk {
if err := validateDiskSize(installDisk, true); err != nil {
return false, updateValidatorMessage(err.Error())
}
} else {
if err := validateDiskSize(installDisk, false); err != nil {
return false, updateValidatorMessage(err.Error())
}
if err := validateDataDiskSize(dataDisk); err != nil {
return false, updateValidatorMessage(err.Error())
}
}
return true, nil;
}
closeThisPage := func() {
c.CloseElements(
diskPanel,
dataDiskPanel,
askForceMBRPanel,
diskValidatorPanel,
diskNotePanel,
askForceMBRTitlePanel,
persistentSizePanel,
)
}
gotoPrevPage := func(g *gocui.Gui, v *gocui.View) error {
closeThisPage()
diskConfirmed = false
return showNext(c, askCreatePanel)
}
gotoNextPage := func(g *gocui.Gui, v *gocui.View) error {
// Don't proceed to the next page if disk size validation fails
if valid, err := validateAllDiskSizes(); !valid || err != nil {
return err;
}

installDisk := c.config.Install.Device
dataDisk := c.config.Install.DataDisk
persistentSize := c.config.Install.PersistentPartitionSize

if dataDisk == "" || installDisk == dataDisk {
if err := validateDiskSize(installDisk, true); err != nil {
return updateValidatorMessage(err.Error())
}

diskSize, err := util.GetDiskSizeBytes(c.config.Install.Device)
if err != nil {
return err
}
if _, err := util.ParsePartitionSize(diskSize, persistentSize); err != nil {
return updateValidatorMessage(err.Error())
}
} else {
if err := validateDiskSize(installDisk, false); err != nil {
return updateValidatorMessage(err.Error())
}
if err := validateDataDiskSize(dataDisk); err != nil {
return updateValidatorMessage(err.Error())
}
}

if !diskConfirmed {
diskConfirmed = true
return nil
}

closeThisPage()
//TODO: When Install modeonly.. we need to decide that this
// network page is not shown and skip straight to the password page
if installModeOnly {
return showNext(c, passwordConfirmPanel, passwordPanel)
}
return showHostnamePage(c)
}

diskConfirm := func(g *gocui.Gui, v *gocui.View) error {
device, err := diskV.GetData()
if err != nil {
return err
}
dataDisk, err := dataDiskV.GetData()
if err != nil {
return err
}

if err := updateValidatorMessage(""); err != nil {
return err
}
c.config.Install.Device = device

if len(diskOpts) > 1 {
if err := updateValidatorMessage(""); err != nil {
return err
// Show error if disk size validation fails, but allow proceeding to next field
if _, err := validateAllDiskSizes(); err != nil {
return err;
}
if device == dataDisk {
return showNext(c, persistentSizePanel, dataDiskPanel)
}
return showNext(c, dataDiskPanel)
}

if err := c.setContentByName(diskNotePanel, persistentSizeNote); err != nil {
return err
}
if err := updateValidatorMessage(""); err != nil {
return err
// Show error if disk size validation fails, but allow proceeding to next field
if _, err := validateAllDiskSizes(); err != nil {
return err;
}
return showNext(c, persistentSizePanel)
}
// Keybindings
diskV.KeyBindings = map[gocui.Key]func(*gocui.Gui, *gocui.View) error{
gocui.KeyEnter: diskConfirm,
gocui.KeyArrowDown: diskConfirm,
gocui.KeyArrowUp: func(g *gocui.Gui, v *gocui.View) error {
return updateValidatorMessage("")
},
gocui.KeyEsc: gotoPrevPage,
}

dataDiskConfirm := func(g *gocui.Gui, v *gocui.View) error {
dataDisk, err := dataDiskV.GetData()
if err != nil {
return err
}

if err := updateValidatorMessage(""); err != nil {
return err
}
c.config.Install.DataDisk = dataDisk

installDisk, err := diskV.GetData()
if err != nil {
return err
}
if installDisk == dataDisk {
if err := c.setContentByName(diskNotePanel, persistentSizeNote); err != nil {
return err
}
// Show error if disk size validation fails, but allow proceeding to next field
if _, err := validateAllDiskSizes(); err != nil {
return err;
}
return showNext(c, persistentSizePanel)
}

c.CloseElements(persistentSizePanel)
if systemIsBIOS() {
if err := c.setContentByName(diskNotePanel, forceMBRNote); err != nil {
return err
}
return showNext(c, askForceMBRPanel)
}
return gotoNextPage(g, v)
}
dataDiskV.KeyBindings = map[gocui.Key]func(*gocui.Gui, *gocui.View) error{
gocui.KeyEnter: dataDiskConfirm,
gocui.KeyArrowDown: dataDiskConfirm,
gocui.KeyArrowUp: func(g *gocui.Gui, v *gocui.View) error {
if err := updateValidatorMessage(""); err != nil {
return err
}
diskConfirmed = false
return showNext(c, diskPanel)
},
gocui.KeyEsc: gotoPrevPage,
}

persistentSizeConfirm := func(g *gocui.Gui, v *gocui.View) error {
persistentSize, err := persistentSizeV.GetData()
if err != nil {
return err
}
c.config.Install.PersistentPartitionSize = persistentSize

if systemIsBIOS() {
if err := c.setContentByName(diskNotePanel, forceMBRNote); err != nil {
return err
}
return showNext(c, askForceMBRPanel)
}
return gotoNextPage(g, v)
}
persistentSizeV.KeyBindings = map[gocui.Key]func(*gocui.Gui, *gocui.View) error{
gocui.KeyEnter: persistentSizeConfirm,
gocui.KeyArrowUp: func(g *gocui.Gui, v *gocui.View) error {
diskConfirmed = false
if len(diskOpts) > 1 {
if err := updateValidatorMessage(""); err != nil {
return err
}
if err := c.setContentByName(diskNotePanel, ""); err != nil {
return err
}
return showNext(c, dataDiskPanel)
}
if err := updateValidatorMessage(""); err != nil {
return err
}
if err := c.setContentByName(diskNotePanel, ""); err != nil {
return err
}
return showNext(c, diskPanel)
},
gocui.KeyArrowDown: persistentSizeConfirm,
gocui.KeyEsc: gotoPrevPage,
}

mbrConfirm := func(g *gocui.Gui, v *gocui.View) error {
forceMBR, err := askForceMBRV.GetData()
if err != nil {
return err
}
if forceMBR == "yes" {
diskTooLargeForMBR, err := diskExceedsMBRLimit(c.config.Device)
if err != nil {
return err
}
if diskTooLargeForMBR {
return updateValidatorMessage("Disk too large for MBR. Must be less than 2TiB")
}
}

c.config.ForceMBR = forceMBR == "yes"
return gotoNextPage(g, v)
}
askForceMBRV.KeyBindings = map[gocui.Key]func(*gocui.Gui, *gocui.View) error{
gocui.KeyEnter: mbrConfirm,
gocui.KeyArrowUp: func(g *gocui.Gui, v *gocui.View) error {
diskConfirmed = false

disk, err := diskV.GetData()
if err != nil {
return err
}
dataDisk, err := dataDiskV.GetData()
if err != nil {
return err
}

if len(diskOpts) > 1 && disk != dataDisk {
return showNext(c, dataDiskPanel)
}
if err := c.setContentByName(diskNotePanel, persistentSizeNote); err != nil {
return err
}
return showNext(c, persistentSizePanel)
},
gocui.KeyArrowDown: mbrConfirm,
gocui.KeyEsc: gotoPrevPage,
}
return nil
}

Check warning on line 655 in pkg/console/install_panels.go

View check run for this annotation

codefactor.io / CodeFactor

pkg/console/install_panels.go#L285-L655 

Very Complex Method
func addAskCreatePanel(c *Console) error {
askOptionsFunc := func() ([]widgets.Option, error) {
options := []widgets.Option{
Expand Down
4 changes: 2 additions & 2 deletions pkg/console/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ func validateDiskSize(devPath string, single bool) error {
limit = config.MultipleDiskMinSizeGiB
}
if util.ByteToGi(diskSizeBytes) < uint64(limit) {
return fmt.Errorf("Disk size is too small. Minimum %dGi is required", limit)
return fmt.Errorf("Installation disk size is too small. Minimum %dGi is required", limit)
}

return nil
Expand All @@ -650,7 +650,7 @@ func validateDataDiskSize(devPath string) error {
return err
}
if util.ByteToGi(diskSizeBytes) < config.HardMinDataDiskSizeGiB {
return fmt.Errorf("Disk size is too small. Minimum %dGi is required", config.HardMinDataDiskSizeGiB)
return fmt.Errorf("Data disk size is too small. Minimum %dGi is required", config.HardMinDataDiskSizeGiB)
}

return nil
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ const (

func ParsePartitionSize(diskSizeBytes uint64, partitionSize string) (uint64, error) {
if diskSizeBytes < MinDiskSize {
return 0, fmt.Errorf("Disk size is too small. Minimum %dGi is required", ByteToGi(MinDiskSize))
return 0, fmt.Errorf("Installation disk size is too small. Minimum %dGi is required", ByteToGi(MinDiskSize))
Copy link
Member

Choose a reason for hiding this comment

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

nit: this makes the function only useful for the installation disk. Although we don't use the function in the data disk path, can we make the function more general like supporting passing a disk type?

The other parts of the PR work great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although we don't use the function in the data disk path, can we make the function more general like supporting passing a disk type?

I'm not sure. I mean, yeah, we could add a parameter to specify disk type :-) but looking at the rest of the function, it's specifically testing sizes against MinDiskSize and MinPersistentSize, so ISTM the way it's implemented now is really geared towards looking at the installation disk only. Maybe this is something we can look at refactoring separately?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, no need to be in this PR!

}
actualDiskSizeBytes := diskSizeBytes - fixedOccupiedSize

if !sizeRegexp.MatchString(partitionSize) {
return 0, fmt.Errorf("Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed")
return 0, fmt.Errorf("Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.")
}

size, err := strconv.ParseUint(partitionSize[:len(partitionSize)-2], 10, 64)
Expand Down
12 changes: 6 additions & 6 deletions pkg/util/disk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,32 +41,32 @@ func TestParsePartitionSize(t *testing.T) {
{
diskSize: 100 * GiByteMultiplier,
partitionSize: "50Gi",
err: "Disk size is too small. Minimum 250Gi is required",
err: "Installation disk size is too small. Minimum 250Gi is required",
},
{
diskSize: 249 * GiByteMultiplier,
partitionSize: "50Gi",
err: "Disk size is too small. Minimum 250Gi is required",
err: "Installation disk size is too small. Minimum 250Gi is required",
},
{
diskSize: 2000 * GiByteMultiplier,
partitionSize: "abcd",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 2000 * GiByteMultiplier,
partitionSize: "1Ti",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 2000 * GiByteMultiplier,
partitionSize: "50Ki",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 2000 * GiByteMultiplier,
partitionSize: "5.5",
err: "Partition size should be ended with 'Mi', 'Gi', and no dot and negative is allowed",
err: "Partition size must end with 'Mi' or 'Gi'. Decimals and negatives are not allowed.",
},
{
diskSize: 400 * GiByteMultiplier,
Expand Down