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.

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

stages: filesystems, skip format if filesystem already exists #351

Closed
wants to merge 1 commit into from
Closed

stages: filesystems, skip format if filesystem already exists #351

wants to merge 1 commit into from

Conversation

mcuadros
Copy link
Contributor

@mcuadros mcuadros commented Apr 6, 2017

This PR solve the problem when a filesystems with force equals false is being formatted and a previous filesystem already exists.

When mkfs.ext4 is called with -p (added by 1764829) the command exits with an error, and the boot aborts.

This is fixed checking if a previous filesystem exists at the device with the command blkid and if exists and force is equals to false the format phase is skipped.

If this PR is something that could be merge, I will need guidance to make some tests.

@cgonyeo
Copy link
Contributor

cgonyeo commented Apr 6, 2017

In general the idea with ignition is that it either creates the exact machine you asked for, or the machine doesn't come online. It doesn't produce "mostly-what-you-asked-for" machines.

I might be able to entertain the idea of doing this if the existing filesystem matches the desired filesytem, but then there could still be files on that filesystem the user isn't expecting.

Just my thoughts, let's see what @crawford thinks.

@mcuadros
Copy link
Contributor Author

mcuadros commented Apr 6, 2017

I might be able to entertain the idea of doing this if the existing filesystem matches the desired filesytem,

Converting the Warning into a Error, if the file system doesn't match, solve this problem.

but then there could still be files on that filesystem the user isn't expecting.

This is my goal, the device contains valuable data that I want to preserve.

If you don't want to preserve the information then the user can set force to true and the device will be always empty.

Our machines are booting with iPXE without installing the OS to the HD, via matchbox, the /dev/sda1 is just a partition with the etcd data with the goal of preserve the status of the cluster. If I don't have a way to don't format a this, and continue with the boot, I will require having an alternative ignition config, and detected by hand, if this device was already formatted

I thought that this was the intended use of the force argument based on #270 (comment)

@crawford
Copy link
Contributor

crawford commented Apr 6, 2017

Using a single boolean doesn't cover all of the use cases. The current behavior is:

false - Only create the filesystem if there is no existing data (safe)
true - Always create the filesystem, destroying anything that was previously there

And with your patch it becomes:

false - Reuse the filesystem if it exists
true - Always create the filesystem, destroying anything that was previously there

In practice, this is probably fine though. Let me think about this a bit more. I realize this is something that you need, but I want to make sure we don't paint ourselves into a corner.

@mcuadros
Copy link
Contributor Author

@crawford I guess that will be more useful for everyone.

Forcing an error if is already partitioned, means that you can't apply twice a Igntion configuration to a machine without a manual intervention.

@cgonyeo
Copy link
Contributor

cgonyeo commented Apr 11, 2017

Perhaps adding a reuse flag would be a clearer way to define the behavior?

@crawford
Copy link
Contributor

That's exactly what I proposed in #270. My concern was that the spec was drifting too far from being declarative.

@mcuadros
Copy link
Contributor Author

With this change is still declarative, IMHO more useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants