Improve partition semantics #544

ajeddeloh · 2018-05-16T17:30:54Z

Supercedes #442

Add filesystem-reuse-like semantics for partitions. This allows for

Backwards compatibility with the current implementation
Resizing and otherwise modifying existing partitions
Matching partitions to avoid recreation when possible
The ability to match on the start/size 0 (as interpreted by sgdisk)

Worth noting that this does not handle every possible case, since I didn't feel like Ignition should solve the bin-packing problem on first boot.

I'll push another commit to the docs with some examples, but wanted to open this up first.

cc
@dgonyeo for implemention
@bgilbert for docs clarity
@arithx for tests
@dustymabe and @cgwalters since they expressed interest in reviewing Ignition PRs

Addresses coreos/bugs#2388

ajeddeloh · 2018-05-16T17:44:20Z

retest this please

cgwalters · 2018-05-16T18:52:24Z

Is there a high level description of the issue we're solving here? When would I want to reuse a partition? I started looking back through the issues but it wasn't immediately clear. Something to do with RAID?

cgwalters · 2018-05-16T18:54:44Z

Nevermind I saw coreos/bugs#2388

arithx · 2018-05-16T18:44:11Z

tests/negative/partitions/simple.go

+}
+
+func NotThereAndDoesNotMatchNoWipeEntry() types.Test {
+	name := "Partition does not match and wipePartitionEntry is false but the first partition matches"


This shares the same name as ValidAndDoesNotMatchNoWipeEntry, is this intentional? In the test output you'll see a result for both TestIgnitionBlackBoxNegative/Partition_does_not_match_and_wipePartitionEntry_is_false_but_the_first_partition_matches & TestIgnitionBlackBoxNegative/Partition_does_not_match_and_wipePartitionEntry_is_false_but_the_first_partition_matches#01

Nope, that's a copy-paste error

arithx · 2018-05-16T18:48:12Z

tests/negative/partitions/zeroes.go

+}
+
+func Partition9DoesNotStartCorrectly() types.Test {
+	name := "Partition 9 is does not start at the largest chunk"


arithx · 2018-05-16T18:54:05Z

tests/positive/partitions/complex.go

+)
+
+func init() {
+	// Everything and the kitchen sunk


s/sunk/sink/g

arithx · 2018-05-16T18:58:12Z

tests/positive/partitions/creation.go

@@ -183,46 +245,31 @@ func AppendPartition() types.Test {
 	}
 }

-func PartitionSizeStart0() types.Test {


Would there be any value in leaving this test case to test using size & start 0 in older versions of the API to ensure that we don't break the old contract?

This was moved, not deleted. There should still be some tests in positive/zeros that do the same thing

arithx · 2018-05-16T19:05:00Z

tests/positive/partitions/verification.go

+	// one and fail the test.
+	config := `{
+		"ignition": {
+			"version": "2.1.0"


wipePartitionEntry doesn't exist in the 2.1.0 spec and will just throw warnings. Is this intentional?

Nope, that's a typo

arithx · 2018-05-16T19:07:17Z

tests/positive/partitions/verification.go

+}
+
+func VerifyBaseDiskWithWipe() types.Test {
+	name := "Verify the base disk does not change with a matching ignition spec"


As per the comment in tests/negative/partitions/simple.go this matches the same test name as VerifyBaseDisk

arithx · 2018-05-16T19:09:23Z

tests/positive/partitions/zeros.go

+}
+
+func PartitionStartNumber0() types.Test {
+	name := "Create a partition with size and start 0"


This test name implies to me that the size & start are both 0 but size is specified for each partition as 65536

Oops, that's supposed to just be start 0, not size as well

arithx · 2018-05-16T19:11:12Z

tests/positive/partitions/zeros.go

+	out := types.GetBaseDisk()
+	config := `{
+		"ignition": {
+			"version": "2.1.0"


Is a 2.1.0 config intentional?

In this case, yes, since the config uses only old keys.

cgwalters

I started on this, just a few comments so far, but I need to context switch.

cgwalters · 2018-05-16T19:00:39Z

config/v2_3_experimental/types/schema.go

+	Label              *string `json:"label,omitempty"`
+	Number             int     `json:"number,omitempty"`
+	ShouldExist        *bool   `json:"shouldExist,omitempty"`
+	Size               *int    `json:"size,omitempty"`


When I do things like this I add a comment like coreos/rpm-ostree#1368 (comment)

This is generated from schema/ignition.json

cgwalters · 2018-05-16T19:04:00Z

.travis.yml

@@ -24,11 +24,22 @@ addons:
 install:
  # since libblkid-dev:arm64 cannot be installed, spoof it


Wow...am curious about the history here. Since aarch64 isn't a CL deliverable today, maybe just nuke this?

We do ship arm, just only alpha and beta (no stable)

cgwalters · 2018-05-16T19:07:19Z

internal/exec/util/blkid.c

+
+	blkid_parttable table = blkid_partlist_get_table(list);
+	if (!table) {
+		blkid_free_probe(pr);


It probably wouldn't be hard to use attribute(cleanup) for this; it's a bit like defer for C (or RAII in C++).

While I don't think we care about MSVC, we might care about clang. Does Clang support it as well?

Yep! A better link is https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization#Clang_and_GCC_%22cleanup%22_extension_for_C

It's also heavily used by systemd today, as well as ostree, etc. (And ostree's CI tests use clang in some cases)

cgwalters · 2018-05-16T19:15:26Z

internal/exec/util/blkid.c

+//   RESULT_LOOKUP_FAILED if src is null
+//   RESULT_OVERFLOW if src is too big
+//   0 on success
+static result_t checked_copy(char *dest, const char *src, size_t len) {


Since we're returning to golang anyways, any reason not to just pass back strdup()'d data? I understand with partition tables we have fixed buffers from some of the formats, but it feels more annoying to deal with perhaps?

Mostly because dealing with any sort of dynamically allocated data with CGO is a giant pain. We only care about gpt which is limited to 36 UTF16 characters, so the statically allocated buffers are fine.

ajeddeloh · 2018-05-16T19:57:06Z

@cgwalters To give more background: in addition to the linked bug, this allows you do things like blow away the root partition (if you want to do root on raid or something and reclaim the space for something else). Currently there's an awkward dance where you have to wipe the filesystem and just sacrafice that disk space. This is a solution to that problem built out in a generic, declarative way.

ajeddeloh · 2018-05-16T21:03:06Z

@arithx updates to address your comments.

I'll add in the cleanup macro once I get more feedback on the internals.

This also passes a full kola qemu run.

cgonyeo · 2018-05-18T20:03:15Z

doc/operator-notes.md

+A partition matches if all of the specified attributes (`label`, `start`, `size`, `uuid`, and `typeGuid`) are the same. Specifying `uuid` or `typeGuid` as an empty string is the same as not specifying them. When 0 is specified for start or size, Ignition checks if the existing partition's start / size match what they would be if all of the partitions specified were to be deleted (if allowed by wipPartitionEntry), then recreated if `shouldExist` is true.
+
+## Partition number 0
+Specifying `Number` as 0 will use the next available partition number *after* all deletions occur. This means if partition 9 is specified with `shouldExist` as `false` and `wipePartitionEntry` as `true`, and another partition is specified to be created with `number` as `0`, it will get partition number 9. Partition number 0 is disallowed on disks with partitions that specify `shouldExist` as 0. If `number` is not specified it will be treated as 0.


nit: the first number shouldn't be capitalized.

partition number 0 is disallowed on disks with partitions that specify shouldExist as 0

Does this mean in the following example the partition with number: 0 should have shouldExist: false? Doesn't that mean that the partition won't be created? Or does that mean that I should not set shouldExist at all and let it be nil? If nil has a different behaviour than true and false, can we get it added to the above table?

if partition 9 is specified with shouldExist as false and wipePartitionEntry as true, and another partition is specified to be created with number as 0, it will get partition number 9

also shouldExist is a boolean and this can't be set as 0.

Oops, I meant to drop this example. I realized I could just disallow the combination instead of having unexpected behavior

cgonyeo · 2018-05-18T20:10:11Z

doc/operator-notes.md

+Specifying `Number` as 0 will use the next available partition number *after* all deletions occur. This means if partition 9 is specified with `shouldExist` as `false` and `wipePartitionEntry` as `true`, and another partition is specified to be created with `number` as `0`, it will get partition number 9. Partition number 0 is disallowed on disks with partitions that specify `shouldExist` as 0. If `number` is not specified it will be treated as 0.
+
+## Partition start 0
+Specifying `start` as 0 will use the starting sector of the largest available block. It will *not* use the first available block large enough. If `start` is not specified and there is no existing partition, start will be treated as 0, otherwise it will be treated as the existing partition's starting sector.


perhaps "no existing partition with a matching number" ?

cgonyeo · 2018-05-18T20:11:07Z

doc/operator-notes.md

+Specifying `start` as 0 will use the starting sector of the largest available block. It will *not* use the first available block large enough. If `start` is not specified and there is no existing partition, start will be treated as 0, otherwise it will be treated as the existing partition's starting sector.
+
+## Partition size 0
+Specifying `size` specifies the partition should span to the end of the largest available block. If `size` is not specified and there is no existing partition, it will be treated as 0, otherwise it will be treated as the existing partition's size.


Did you mean "specifying size as 0?

nit: "Specifying size specifies" uses the word "specify" too much.

cgonyeo · 2018-05-18T20:15:53Z

internal/exec/util/blkid.c

+}
+
+// blkid_get_partition_list returns the partition list and the probe used to get it (so
+// we can free it). It also performs a sanity check and ensures its is GPT formatted. In the case


nit: s/its/it/

cgonyeo · 2018-05-18T20:17:56Z

internal/exec/util/blkid.c

+// Returns:
+//   RESULT_LOOKUP_FAILED if src is null
+//   RESULT_OVERFLOW if src is too big
+//   0 on success


nit: RESULT_OK on success

cgonyeo · 2018-05-18T20:57:42Z

internal/exec/stages/disks/partitions.go

+	return nil
+}
+
+// getReadStartAnsSize returns a map of partition numbers to a struct that contains what their real start


s/Read/Real/

cgonyeo · 2018-05-18T21:01:24Z

internal/exec/stages/disks/partitions.go

+	size  int
+}
+
+func parseLine(r *regexp.Regexp, line string) (int, error) {


It would be nice to have a comment on this saying what it does

cgonyeo · 2018-05-18T21:02:15Z

internal/exec/stages/disks/partitions.go

+// output was from running `sgdisk --pretend <commands> --info=1 --info=4 --info=5`. It assumes the the
+// partition labels are well behaved (i.e. contain no control characters). It returns a list of partitions
+// matching the partition numbers specified, but with the start and size information as determined by sgdisk.
+// The partition numbers need to passed in because sgdisk include them in it's output.


s/include/includes/
s/it's/its/

cgonyeo · 2018-05-18T21:08:38Z

internal/exec/stages/disks/partitions.go

+
+// partitionDisk partitions devAlias according to the spec given by dev
+func (s stage) partitionDisk(dev types.Disk, devAlias string) error {
+	if dev.WipeTable {


What's the reason to not wipe during the following operation?

getPartitionMap reads the disk and needs to see that it's empty

cgonyeo · 2018-05-18T21:14:00Z

internal/config/translate.go

+				Start:              x.Start,
+				TypeGUID:           x.TypeGUID,
+				ShouldExist:        x.ShouldExist,
+				WipePartitionEntry: x.WipePartitionEntry,


Can you also add these fields somewhere in internal/config/translate_test.go?

arithx · 2018-05-21T20:37:23Z

tests/positive/partitions/zeros.go

+}
+
+func VerifyUnspecifiedIsDoNotCare() types.Test {
+	name := "Verify the ROOT partition to fills the default disk"


Same test name as VerifyRootFillsDisk

ajeddeloh · 2018-05-21T22:39:12Z

Updated with feedback. @cgwalters the cleanup extension stuff made the cgo so much cleaner and also fixed a few existing memory leaks I found in the process.

Ran it through kola again and it passed.

arithx

Tests LGTM

arithx · 2018-05-22T22:35:05Z

tests/positive/partitions/creation.go

+	}
+}
+
+func WipeAndCreateNewPartitions() types.Test {


Should we update the test.Name to note the Wipe occurring?

arithx · 2018-05-22T22:36:14Z

tests/positive/partitions/verification.go

+}
+
+func VerifyBaseDiskWithWipe() types.Test {
+	name := "Verify the base disk does not change with a matching ignition spec with wipePartitionEntry as true"


s,ignition,Ignition,g

arithx · 2018-05-22T22:38:10Z

tests/positive/partitions/verification.go

+}
+
+func VerifyBaseDisk() types.Test {
+	name := "Verify the base disk does not change with a matching ignition spec"


s,ignition,Ignition,g

lucab · 2018-05-23T11:49:20Z

internal/config/types/schema.go

-	TypeGUID string `json:"typeGuid,omitempty"`
+	GUID               string  `json:"guid,omitempty"`
+	Label              *string `json:"label,omitempty"`
+	Number             int     `json:"number,omitempty"`


It looks like this (and most of the other ints) is really a uint. Does schematyper support specifying that?

Nope, which is unfortunate.

cgonyeo

LGTM, pending nits

cgonyeo · 2018-05-30T20:14:41Z

doc/operator-notes.md

+
+The `wipePartitionEntry` and `shouldExist` flags control what Ignition will do when it encounters an existing partition. `wipePartitionEntry` specifies whether Ignition is permitted to delete partition entries in the partition table.  `shouldExist` specifies whether a partition with that number should exist or not (it is invalid to specify a partition should not exist and specify its attributes, such as `size` or `label`).
+
+The following table shows the possible combinations of `shouldExist`, `wipePartitionEntry`, whether or not a partition with the specified number is present, and the action Ignition will take


silly nit: I'd like the list of items here to follow the same ordering as the table headers

cgonyeo · 2018-05-30T20:19:09Z

doc/operator-notes.md

+A partition matches if all of the specified attributes (`label`, `start`, `size`, `uuid`, and `typeGuid`) are the same. Specifying `uuid` or `typeGuid` as an empty string is the same as not specifying them. When 0 is specified for start or size, Ignition checks if the existing partition's start / size match what they would be if all of the partitions specified were to be deleted (if allowed by wipPartitionEntry), then recreated if `shouldExist` is true.
+
+### Partition number 0
+Specifying `number` as 0 will use the next available partition number. Partition number 0 is disallowed on disks with partitions that specify `shouldExist` as 0. If `number` is not specified it will be treated as 0.


I'm having problems following this. How can a partition specify shouldExist as 0? shouldExist is a boolean.

cgonyeo · 2018-05-30T20:38:08Z

config/v2_3_experimental/types/partition_test.go

@@ -22,9 +22,14 @@ import (
 	"github.com/coreos/ignition/config/validate/report"
 )

+// redfined here to avoid import cycles


nit: redefined

cgonyeo · 2018-05-30T20:45:31Z

internal/exec/stages/disks/partitions.go

+}
+
+// partitionMatches determines if the existing partition matches the spec given. See doc/operator notes for what
+// what it means for an existing partition to match the spec. spec must have non-zero Start an Size. existing must


nit: s/an/and/

cgonyeo · 2018-05-30T20:46:14Z

internal/exec/stages/disks/partitions.go

+
+// parseLine takes a regexp that captures an int and a string to match on. On success it returns
+// the captured int and nil. If the regexp does not match it returns -1 and nil. If it encountered
+// and error it returns 0 and the error.


nit: s/and/an/

Change the partitioning semantics to be more similar to the filesystem reuse semantics. Support deletion and recreation of partitions.

Add wipePartition flag to storage.disks.partitions. Add shouldExist flag to storage.disks.partitions. Make partition label, size, and start a pointer type so not specifying it is distinguishable from the go zero value.

Handle start, size, and label being nil-able.

Some fields became pointers. Update the 2.2.0 -> 2.3.0-exp translation to handle that.

Fix the disks stage to handle label, start, and size being pointers. This is be removed by later commits, but I wanted to have a testable point first.

Change the blkid probing to probe for partition information as well. Add new stub definitions to .travis.yml

- Refactor sgdisk to reuse types.Partition instead of its own struct. - Add the ability for sgdisk to delete partitions. - Add the ability for sgdisk to read partition info resulting from running in pretend mode.

The disks.go file is quite large. Refactor into multiple files for common functionality, partitioning, raid, and filesystems.

Add filesystem reuse like semantics to partitioning. This will allow partitions to be specified as non-existent, prevent them from being recreated if they already exist and a bunch of other goodies like that.

Fix partition validation to be cAsE insensitive whn comparing type guids and correctly skip blank partitions.

bgilbert

Drive-by copyedit!

bgilbert · 2018-05-31T23:55:09Z

doc/configuration-v2_3-experimental.md

@@ -28,13 +28,16 @@ The Ignition configuration is a JSON document conforming to the following specif
  * **_disks_** (list of objects): the list of disks to be configured and their options.
    * **device** (string): the absolute path to the device. Devices are typically referenced by the `/dev/disk/by-*` symlinks.
    * **_wipeTable_** (boolean): whether or not the partition tables shall be wiped. When true, the partition tables are erased before any further manipulation. Otherwise, the existing entries are left intact.
+    * **disallowUnspecifiedPartitions** (boolean): whether or not to fail if a disk has partitions not described by Ignition. Has no effect if "wipeTable" is true.


Missing italics.

I think partitions not described in the config is what this is implying?

wipeTable should be monospace I think.

this was meant to be dropped from the docs since its not implemented yet (it was seperable from the rest). Will just drop

bgilbert · 2018-06-01T00:01:42Z

doc/operator-notes.md

+
+## Partition Reuse Semantics
+
+The `wipePartitionEntry` and `shouldExist` flags control what Ignition will do when it encounters an existing partition. `wipePartitionEntry` specifies whether Ignition is permitted to delete partition entries in the partition table.  `shouldExist` specifies whether a partition with that number should exist or not (it is invalid to specify a partition should not exist and specify its attributes, such as `size` or `label`).


to specify that a partition should not exist and also to specify

bgilbert · 2018-06-01T00:01:58Z

doc/operator-notes.md

+
+The `wipePartitionEntry` and `shouldExist` flags control what Ignition will do when it encounters an existing partition. `wipePartitionEntry` specifies whether Ignition is permitted to delete partition entries in the partition table.  `shouldExist` specifies whether a partition with that number should exist or not (it is invalid to specify a partition should not exist and specify its attributes, such as `size` or `label`).
+
+The following table shows the possible combinations of whether or not a partition with the specified number is present, `shouldExist`, and `wipePartitionEntry`, and the action Ignition will take


trailing colon:

bgilbert · 2018-06-01T00:08:14Z

doc/configuration-v2_3-experimental.md

    * **_partitions_** (list of objects): the list of partitions and their configuration for this particular disk.
      * **_label_** (string): the PARTLABEL for the partition.
      * **_number_** (integer): the partition number, which dictates it's position in the partition table (one-indexed). If zero, use the next available partition slot.
      * **_size_** (integer): the size of the partition (in device logical sectors, 512 or 4096 bytes). If zero, the partition will be made as large as possible.
      * **_start_** (integer): the start of the partition (in device logical sectors). If zero, the partition will be positioned at the start of the largest block available.
      * **_typeGuid_** (string): the GPT [partition type GUID][part-types]. If omitted, the default will be 0FC63DAF-8483-4772-8E79-3D69D8477DE4 (Linux filesystem data).
      * **_guid_** (string): the GPT unique partition GUID.
+      * **_wipePartitionEntry_** (boolean) whether or not Ignition is allowed to wipe an existing partition entry for this partition number.


This isn't very clear. I think the important point is that true => Ignition will clobber an existing partition if necessary to make the disk match the config; false => Ignition will fail instead.

bgilbert · 2018-06-01T00:09:52Z

doc/configuration-v2_3-experimental.md

    * **_partitions_** (list of objects): the list of partitions and their configuration for this particular disk.
      * **_label_** (string): the PARTLABEL for the partition.
      * **_number_** (integer): the partition number, which dictates it's position in the partition table (one-indexed). If zero, use the next available partition slot.
      * **_size_** (integer): the size of the partition (in device logical sectors, 512 or 4096 bytes). If zero, the partition will be made as large as possible.
      * **_start_** (integer): the start of the partition (in device logical sectors). If zero, the partition will be positioned at the start of the largest block available.
      * **_typeGuid_** (string): the GPT [partition type GUID][part-types]. If omitted, the default will be 0FC63DAF-8483-4772-8E79-3D69D8477DE4 (Linux filesystem data).
      * **_guid_** (string): the GPT unique partition GUID.
+      * **_wipePartitionEntry_** (boolean) whether or not Ignition is allowed to wipe an existing partition entry for this partition number.
+      * **_shouldExist_** (boolean) whether or not a partition `number` should exist. If ommited, it defaults to true. If false, `number` must be specified and non-zero and `label`, `start`, `size`, `guid`, and `typeGuid` must all be omitted.


"whether or not the partition with the specified number"
omitted
The consequences of false are implied, but maybe should be explicit: Ignition will either delete the partition or fail, depending on wipePartitionEntry.

bgilbert · 2018-06-01T01:49:25Z

doc/operator-notes.md

+Specifying `start` as 0 will use the starting sector of the largest available block. It will *not* use the first available block large enough.
+
+### Unspecified partition start
+If `start` is not specified and a partition with the same number exists, it will use the value of the existing partition, unless wipePartitionEntry is set.


Ignition will use the start of the existing partition

bgilbert · 2018-06-01T01:51:12Z

doc/operator-notes.md

+
+### Unspecified partition start
+If `start` is not specified and a partition with the same number exists, it will use the value of the existing partition, unless wipePartitionEntry is set.
+If `start` is not specified and there is no existing partition, or wipePartitionEntry is set, `start` act as if it were set to 0 and use the starting sector of the largest available block.


or `wipePartitionEntry` is set, Ignition will use the starting sector of the largest available block, as if `start` were set to 0.

bgilbert · 2018-06-01T01:53:33Z

doc/operator-notes.md

+If `start` is not specified and there is no existing partition, or wipePartitionEntry is set, `start` act as if it were set to 0 and use the starting sector of the largest available block.
+
+### Partition size 0
+Specifying `size` as 0 means the partition should span to the end of the largest available block. It will *not* use the size of first available block.


Maybe something like: If `size` is specified as 0, the partition will span to the end of the largest available block. If the starting sector is not within the largest available block, Ignition will fail.

bgilbert · 2018-06-01T01:54:27Z

doc/operator-notes.md

+Specifying `size` as 0 means the partition should span to the end of the largest available block. It will *not* use the size of first available block.
+
+### Unspecified partition size
+If `size` is not specified and a partition with the same number exists, it will use the value of the existing partition, unless wipePartitionEntry is set.


Ignition will use the size of the existing partition

bgilbert · 2018-06-01T01:56:36Z

doc/operator-notes.md

+
+### Unspecified partition size
+If `size` is not specified and a partition with the same number exists, it will use the value of the existing partition, unless wipePartitionEntry is set.
+If `size` is not specified and there is no existing partition, or wipePartitionEntry is set, `size` act as if it were set to 0 and use the size of the largest block.


or `wipePartitionEntry` is set, the partition will span to the end of the largest available block, as if `size` were set to 0.

Improve partition semantics

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 coreos#544 and coreos#1149 and the retry was removed and the error-checking was added. So this patch effectively re-applies 94c98bc ("sgdisk: retry zap-all operation on failure"), but now with a comment and a test to make sure we don't regress again. Closes: coreos/fedora-coreos-tracker#1596

arithx reviewed May 16, 2018

View reviewed changes

cgwalters reviewed May 16, 2018

View reviewed changes

ajeddeloh force-pushed the parition-230-2 branch from a9782ce to c7a813a Compare May 16, 2018 21:01

cgonyeo suggested changes May 18, 2018

View reviewed changes

ajeddeloh force-pushed the parition-230-2 branch from c7a813a to 3d44d10 Compare May 18, 2018 23:22

arithx reviewed May 21, 2018

View reviewed changes

ajeddeloh force-pushed the parition-230-2 branch 2 times, most recently from c67e52b to 0b9854a Compare May 21, 2018 22:18

arithx reviewed May 22, 2018

View reviewed changes

lucab reviewed May 23, 2018

View reviewed changes

cgonyeo approved these changes May 30, 2018

View reviewed changes

Andrew Jeddeloh added 12 commits May 31, 2018 13:31

docs: change partitioning to be more powerful

67eec80

Change the partitioning semantics to be more similar to the filesystem reuse semantics. Support deletion and recreation of partitions.

schema: add partitioning flags, make some nullable

9a03084

Add wipePartition flag to storage.disks.partitions. Add shouldExist flag to storage.disks.partitions. Make partition label, size, and start a pointer type so not specifying it is distinguishable from the go zero value.

config/v2_3_exp: update partitioning validation

9cfea99

Handle start, size, and label being nil-able.

config/v2_3_exp/translate: fix translate for ptrs

8ce8e53

Some fields became pointers. Update the 2.2.0 -> 2.3.0-exp translation to handle that.

internal/config: add translation code for shouldexist

352f152

stages/disks: handle fields being pointers

1b5fda8

Fix the disks stage to handle label, start, and size being pointers. This is be removed by later commits, but I wanted to have a testable point first.

exec/util: allow blkid to read partition info

e913593

Change the blkid probing to probe for partition information as well. Add new stub definitions to .travis.yml

internal/sgdisk: refactor, add deletions, info

81bb228

- Refactor sgdisk to reuse types.Partition instead of its own struct. - Add the ability for sgdisk to delete partitions. - Add the ability for sgdisk to read partition info resulting from running in pretend mode.

exec/stages/disks: refactor into seperate files

5973263

The disks.go file is quite large. Refactor into multiple files for common functionality, partitioning, raid, and filesystems.

exec/stages/disks: enhance partitioning

7b49d00

Add filesystem reuse like semantics to partitioning. This will allow partitions to be specified as non-existent, prevent them from being recreated if they already exist and a bunch of other goodies like that.

tests: fix partition validation

68e9c10

Fix partition validation to be cAsE insensitive whn comparing type guids and correctly skip blank partitions.

tests: add more tests for partitioning

4241d1c

ajeddeloh force-pushed the parition-230-2 branch from 0b9854a to 4241d1c Compare May 31, 2018 20:35

ajeddeloh merged commit 2d7f17c into coreos:master May 31, 2018

bgilbert reviewed Jun 1, 2018

View reviewed changes

ajeddeloh mentioned this pull request Jun 8, 2018

docs: improve partitioning docs clarity #559

Merged

Rahuls0720 pushed a commit to Rahuls0720/ignition that referenced this pull request Jun 13, 2018

Merge pull request coreos#544 from ajeddeloh/parition-230-2

cf19474

Improve partition semantics

This was referenced Oct 25, 2023

stages/disks: retry sgdisk --zap-all invocation #1735

Merged

Ignition fails to wipe disk if partition table is corrupted coreos/fedora-coreos-tracker#1596

Closed

		@@ -24,11 +24,22 @@ addons:
		install:
		# since libblkid-dev:arm64 cannot be installed, spoof it


		The `wipePartitionEntry` and `shouldExist` flags control what Ignition will do when it encounters an existing partition. `wipePartitionEntry` specifies whether Ignition is permitted to delete partition entries in the partition table. `shouldExist` specifies whether a partition with that number should exist or not (it is invalid to specify a partition should not exist and specify its attributes, such as `size` or `label`).

		The following table shows the possible combinations of `shouldExist`, `wipePartitionEntry`, whether or not a partition with the specified number is present, and the action Ignition will take


		## Partition Reuse Semantics

		The `wipePartitionEntry` and `shouldExist` flags control what Ignition will do when it encounters an existing partition. `wipePartitionEntry` specifies whether Ignition is permitted to delete partition entries in the partition table. `shouldExist` specifies whether a partition with that number should exist or not (it is invalid to specify a partition should not exist and specify its attributes, such as `size` or `label`).

Improve partition semantics #544

Improve partition semantics #544

Conversation

ajeddeloh commented May 16, 2018 • edited

ajeddeloh commented May 16, 2018

cgwalters commented May 16, 2018

cgwalters commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeddeloh commented May 16, 2018

ajeddeloh commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeddeloh commented May 21, 2018

arithx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab May 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgonyeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgilbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeddeloh commented May 16, 2018 •

edited

lucab May 23, 2018 •

edited