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

Hetzner partitioning script #948

Closed
wants to merge 3 commits into from
Closed

Conversation

@nh2
Copy link
Contributor

@nh2 nh2 commented May 8, 2018

This PR is based on top of PR #857 and thus includes its first commit; that PR should be merged before this one.

This PR modernises how the hetzner backend gets its inputs (using config instead of custom XML parsing), and then adds 3 options via which you can do custom partitioning. This is needed e.g. when you want to make an ext4 with external journal, using a combination of SSDs and HDDs that are common at Hetzner.

CC @aszlig @cleverca22

@nh2 nh2 force-pushed the nh2:hetzner-partitioning-script branch 2 times, most recently from e5f90bc to 85f720f May 8, 2018
setattr(self, var, xml_expr_to_python(node))

self.main_ipv4 = config["hetzner"]["mainIPv4"]
assert type(self.main_ipv4) is str

This comment has been minimized.

@FRidh

FRidh May 10, 2018
Member

Use descriptors instead? Possibly http://traitlets.readthedocs.io.

This comment has been minimized.

@nh2

nh2 May 10, 2018
Author Contributor

These are nice ideas, but here I'm simply unifying the style to use the same as the other backends (such as the EC2 one,

self.access_key_id = config["ec2"]["accessKeyId"]
).

If we want to push for a change in style in general, we should probably tackle that separately for all backends and in coordination with whoever takes care of the those.

Regarding traitlets, I'm all for more type checking, but as far as I can tell it's an extra dependency so I can't tell whether or not we want to add it to nixops.

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

Apart from that, those types should already be checked by the NixOS module system, so I think most of those asserts aren't really needed.

@nh2
Copy link
Contributor Author

@nh2 nh2 commented May 10, 2018

TODO for myself:

Fix example:

  • mkpart primary is misleading for GPT tables; as per https://www.gnu.org/software/parted/manual/html_node/mkpart.html#mkpart:

    part-type is one of ‘primary’, ‘extended’ or ‘logical’, and may be specified only with ‘msdos’ or ‘dvh’ partition tables

  • Switch to MB instead of MiB, add note
          # Note we use "MB" instead of "MiB" because otherwise `--align optimal` has no effect;
          # as per documentation https://www.gnu.org/software/parted/manual/html_node/unit.html#unit:
          # > Note that as of parted-2.4, when you specify start and/or end values using IEC
          # > binary units like "MiB", "GiB", "TiB", etc., parted treats those values as exact
    
Copy link
Member

@aszlig aszlig left a comment

I'd move most of the type and conflict checking to the module (for example via the assertions option) instead of within the Python part, as we can provide better error messages than a traceback from Python if we got an assertion.

Apart from that, it might be a good idea to fall back using nixos-generate-config (see _detect_hardware) with filesystem detection, so that providing fsInfo is optional.

description = ''
Specify layout of partitions and file systems using Anacondas Kickstart
format. For possible options and commands, please have a look at:
<link xlink:href="http://fedoraproject.org/wiki/Anaconda/Kickstart"/>
If the Kickstart is not sufficient for your partitioning needs,

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

Probably remove the the: If Kickstart is not sufficient...

This comment has been minimized.

@nh2

nh2 May 26, 2018
Author Contributor

Done

Where possible, use the simpler "partitions" option instead of this option.
The "partitions" and "partitioningScript" options are mutually exclusive.

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

<option>partitions</option> and <option>partitioningScript</option>

This comment has been minimized.

@nh2

nh2 May 26, 2018
Author Contributor

Done

filesystemInfo = mkOption {
type = types.nullOr types.attrs;
default = null;
example = {

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

Maybe use literalExample here, otherwise this ends up like this:

{ boot = { loader = { grub = { devices = [ "/dev/sda" "/dev/sdb" "/dev/sdc" "/dev/sdd" ] ; } ; } ; } ; fileSystems = { / = { fsType = "ext4"; label = "root"; options = [ "journal_path=/dev/disk/by-label/rootjournal" "data=journal" "errors=remount-ro" ] ; } ; } ; swapDevices = [ ] ; }

This comment has been minimized.

@nh2

nh2 May 26, 2018
Author Contributor

Done

description = ''
Override the filesystem info obtained from the machine after partitioning.
This option is required when "partitioningScript" is used, but can also

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

Again, <option/>.

This comment has been minimized.

@nh2

nh2 May 26, 2018
Author Contributor

Done

setattr(self, var, xml_expr_to_python(node))

self.main_ipv4 = config["hetzner"]["mainIPv4"]
assert type(self.main_ipv4) is str

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

Apart from that, those types should already be checked by the NixOS module system, so I think most of those asserts aren't really needed.

"""
Bootstrap everything needed in order to get Nix and the partitioner
usable in the rescue system. The keyword arguments are only for
partitioning, see reboot_rescue() for description, if not given we will
only mount based on information provided in self.partitions.
Exactly one of `partitions` and `partitioning_script` must be given as
non-None value.

This comment has been minimized.

@aszlig

aszlig May 15, 2018
Member

I'd check this via the NixOS module system as well.

This comment has been minimized.

@coretemp

coretemp Jun 1, 2018

Yes, but not a reason not to merge, is it?

@nh2 nh2 force-pushed the nh2:hetzner-partitioning-script branch 2 times, most recently from 161d512 to a794a4d May 26, 2018
@nh2
Copy link
Contributor Author

@nh2 nh2 commented May 26, 2018

Addressed a couple comments, some other ones still outstanding

nh2 added 3 commits Jan 27, 2018
The old approach, waiting for the machine to not having an open
port, and then waiting for it to be open again, was insufficient,
because of the race condition that the machine rebooted so quickly
that the port was immediately open again without nixops noticing
that it went down. I experienced this on a Hetzner cloud server.

The new approach checks the `last reboot` on the remote side
to change, which is not racy.
From @aszlig:

> the Hetzner backend was written back then where there was
> no config argument, so it's a good idea to switch to it
This allows for custom partitioning that Anaconda Kickstart / blivet
cannot do.
@nh2 nh2 force-pushed the nh2:hetzner-partitioning-script branch from a794a4d to 3ca62f2 May 26, 2018
self.reboot(hard=hard, reset=False)

self.log_start("waiting for reboot to complete...")
while True:

This comment has been minimized.

@coretemp

coretemp Jun 1, 2018

This doesn't look robust, because perhaps Hetzner did something wrong and it doesn't actually ever come up. I see lots of people who write while True: loops, but in reality, if it hasn't started after 3 minutes, you will start to wonder what's going on and you would have to manual work.

As such, this level of automation is an improvement over having nothing, but there is room for improvement.

break
self.log_continue(".")
time.sleep(1)
self.log_end("done.")

This comment has been minimized.

@coretemp

coretemp Jun 1, 2018

In a log it would be more useful see a line that says what actually was done.

This comment has been minimized.

@aszlig

aszlig Jun 1, 2018
Member

@coretemp: This is done in log_start, which results in something like waiting for reboot to complete.....xxxx....done.. However I guess we could also refactor the logging mechanism to allow for context managers so this becomes more clear.

@grahamc
Copy link
Member

@grahamc grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.