From 5717a50047184a1cd0bc59a7b3a55b82bff8d6ba Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 25 Oct 2023 11:57:01 -0400 Subject: [PATCH 1/2] stages/disks: retry `sgdisk --zap-all` invocation When `wipeTable` is enabled, we run `sgdisk --zap-all`. But if the table was corrupted in the first place, `sgdisk` will exit with code 2 which then breaks boot. As a workaround, Ignition used to retry the invocation but the context around it was lost in #544 and #1149 and the retry was removed and the error-checking was added. So this patch effectively re-applies 94c98bcb ("sgdisk: retry zap-all operation on failure"), but now with a comment and a test to make sure we don't regress again. Closes: https://github.com/coreos/fedora-coreos-tracker/issues/1596 --- docs/release-notes.md | 1 + internal/exec/stages/disks/partitions.go | 7 ++- tests/filesystem.go | 17 +++++++ tests/positive/partitions/wipe.go | 63 ++++++++++++++++++++++++ tests/types/types.go | 9 ++-- 5 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 tests/positive/partitions/wipe.go diff --git a/docs/release-notes.md b/docs/release-notes.md index 3411ac062..165d5bfc9 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -21,6 +21,7 @@ nav_order: 9 ### Bug fixes - Prevent races with udev after disk editing +- Don't fail to wipe partition table if it's corrupted ## Ignition 2.16.2 (2023-07-12) diff --git a/internal/exec/stages/disks/partitions.go b/internal/exec/stages/disks/partitions.go index a21fc0477..cb55c3765 100644 --- a/internal/exec/stages/disks/partitions.go +++ b/internal/exec/stages/disks/partitions.go @@ -324,7 +324,12 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error { s.Logger.Info("wiping partition table requested on %q", devAlias) op.WipeTable(true) if err := op.Commit(); err != nil { - return err + // `sgdisk --zap-all` will exit code 2 if the table was corrupted; retry it + // https://github.com/coreos/fedora-coreos-tracker/issues/1596 + s.Logger.Info("potential error encountered while wiping table... retrying") + if err := op.Commit(); err != nil { + return err + } } } diff --git a/tests/filesystem.go b/tests/filesystem.go index fa16c44b9..f63365e28 100644 --- a/tests/filesystem.go +++ b/tests/filesystem.go @@ -19,6 +19,7 @@ import ( "bytes" "compress/bzip2" "context" + "crypto/rand" "encoding/base64" "fmt" "io" @@ -226,6 +227,22 @@ func setupDisk(ctx context.Context, disk *types.Disk, diskIndex int, imageSize i return err } } + + if disk.CorruptTable { + bytes := make([]byte, 1536) + if _, err := rand.Read(bytes); err != nil { + return err + } + f, err := os.OpenFile(disk.ImageFile, os.O_WRONLY, 0666) + if err != nil { + return err + } + if _, err := f.Write(bytes); err != nil { + return err + } + defer f.Close() + } + return nil } diff --git a/tests/positive/partitions/wipe.go b/tests/positive/partitions/wipe.go new file mode 100644 index 000000000..d51b5d59f --- /dev/null +++ b/tests/positive/partitions/wipe.go @@ -0,0 +1,63 @@ +// Copyright 2023 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package partitions + +import ( + "github.com/coreos/ignition/v2/tests/register" + "github.com/coreos/ignition/v2/tests/types" +) + +func init() { + // Tests that wipe tables + register.Register(register.PositiveTest, WipeBadTable()) +} + +func WipeBadTable() types.Test { + name := "partition.wipebadtable" + in := append(types.GetBaseDisk(), types.Disk{ + Alignment: types.IgnitionAlignment, + Partitions: types.Partitions{ + { + Label: "deleteme", + Number: 1, + Length: 65536, + }, + }, + CorruptTable: true, + }) + out := append(types.GetBaseDisk(), types.Disk{Alignment: types.IgnitionAlignment}) + config := `{ + "ignition": { + "version": "$version" + }, + "storage": { + "disks": [ + { + "device": "$disk1", + "wipeTable": true + } + ] + } + }` + configMinVersion := "3.0.0" + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + } +} diff --git a/tests/types/types.go b/tests/types/types.go index d5bd3fe78..4405cb6f0 100644 --- a/tests/types/types.go +++ b/tests/types/types.go @@ -57,10 +57,11 @@ type Node struct { } type Disk struct { - ImageFile string - Device string - Alignment int - Partitions Partitions + ImageFile string + Device string + Alignment int + Partitions Partitions + CorruptTable bool // set to true to corrupt the partition table } type Partitions []*Partition From bc737f04b426fbb424a6e22bc7a99304671adfc1 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 25 Oct 2023 12:26:58 -0400 Subject: [PATCH 2/2] tests/blackbox: allow skipping critical logging check In the context of the test added in the previous patch, it's normal for the first `sgdisk --zap-all` invocation to fail. Add a knob to allow the harness to tolerate this and make use of it in the new test. --- tests/blackbox_test.go | 20 ++++++++++---------- tests/filesystem.go | 4 ++-- tests/positive/partitions/wipe.go | 2 ++ tests/types/types.go | 1 + 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/blackbox_test.go b/tests/blackbox_test.go index 88de68fb5..086d65666 100644 --- a/tests/blackbox_test.go +++ b/tests/blackbox_test.go @@ -278,11 +278,11 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error { appendEnv = append(appendEnv, "IGNITION_SYSTEM_CONFIG_DIR="+systemConfigDir) if !negativeTests { - if err := runIgnition(t, ctx, "fetch", "", tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "fetch", "", tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return err } - if err := runIgnition(t, ctx, "disks", "", tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "disks", "", tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return err } @@ -290,12 +290,12 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error { return err } - if err := runIgnition(t, ctx, "mount", rootPartition.MountPath, tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "mount", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return err } - filesErr := runIgnition(t, ctx, "files", rootPartition.MountPath, tmpDirectory, appendEnv) - if err := runIgnition(t, ctx, "umount", rootPartition.MountPath, tmpDirectory, appendEnv); err != nil { + filesErr := runIgnition(t, ctx, "files", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck) + if err := runIgnition(t, ctx, "umount", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return err } if err := umountPartition(rootPartition); err != nil { @@ -318,11 +318,11 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error { } return nil } else { - if err := runIgnition(t, ctx, "fetch", "", tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "fetch", "", tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return nil // error is expected } - if err := runIgnition(t, ctx, "disks", "", tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "disks", "", tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return nil // error is expected } @@ -330,12 +330,12 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error { return err } - if err := runIgnition(t, ctx, "mount", rootPartition.MountPath, tmpDirectory, appendEnv); err != nil { + if err := runIgnition(t, ctx, "mount", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return nil // error is expected } - filesErr := runIgnition(t, ctx, "files", rootPartition.MountPath, tmpDirectory, appendEnv) - if err := runIgnition(t, ctx, "umount", rootPartition.MountPath, tmpDirectory, appendEnv); err != nil { + filesErr := runIgnition(t, ctx, "files", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck) + if err := runIgnition(t, ctx, "umount", rootPartition.MountPath, tmpDirectory, appendEnv, test.SkipCriticalCheck); err != nil { return nil } if err := umountPartition(rootPartition); err != nil { diff --git a/tests/filesystem.go b/tests/filesystem.go index f63365e28..2d9c676bb 100644 --- a/tests/filesystem.go +++ b/tests/filesystem.go @@ -116,7 +116,7 @@ func umountPartition(p *types.Partition) error { } // returns true if no error, false if error -func runIgnition(t *testing.T, ctx context.Context, stage, root, cwd string, appendEnv []string) error { +func runIgnition(t *testing.T, ctx context.Context, stage, root, cwd string, appendEnv []string, skipCriticalCheck bool) error { args := []string{"-platform", "file", "-stage", stage, "-root", root, "-log-to-stdout", "-config-cache", filepath.Join(cwd, "ignition.json"), @@ -141,7 +141,7 @@ func runIgnition(t *testing.T, ctx context.Context, stage, root, cwd string, app if strings.Contains(string(out), "panic") { return fmt.Errorf("ignition panicked") } - if strings.Contains(string(out), "CRITICAL") { + if !skipCriticalCheck && strings.Contains(string(out), "CRITICAL") { return fmt.Errorf("found critical ignition log") } return err diff --git a/tests/positive/partitions/wipe.go b/tests/positive/partitions/wipe.go index d51b5d59f..da8b34bf9 100644 --- a/tests/positive/partitions/wipe.go +++ b/tests/positive/partitions/wipe.go @@ -59,5 +59,7 @@ func WipeBadTable() types.Test { Out: out, Config: config, ConfigMinVersion: configMinVersion, + // the first `sgdisk --zap-all` is expected to fail + SkipCriticalCheck: true, } } diff --git a/tests/types/types.go b/tests/types/types.go index 4405cb6f0..46de2bfa1 100644 --- a/tests/types/types.go +++ b/tests/types/types.go @@ -104,6 +104,7 @@ type Test struct { ConfigMinVersion string ConfigVersion string ConfigShouldBeBad bool // Set to true to skip config validation step + SkipCriticalCheck bool // Set to true to skip critical logging check } func (ps Partitions) GetPartition(label string) *Partition {