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

Add initial support for hetzner cloud #944

Closed
wants to merge 1 commit into from

Conversation

@goodraven
Copy link

@goodraven goodraven commented May 5, 2018

This is based on the digital ocean backend. It also uses nixos-infect. I extended nixos-infect to be generic
for both backends.

Fixes #855

--

I see @nh2 has some work at https://github.com/nh2/nixops/tree/hetznercloud but I'm not sure what the status of that is.

This is based on the digital ocean backend. It also uses nixos-infect. I extended nixos-infect to be generic
for both backends.

Fixes #855
@nh2
Copy link
Contributor

@nh2 nh2 commented May 5, 2018

I see @nh2 has some work at https://github.com/nh2/nixops/tree/hetznercloud but I'm not sure what the status of that is.

It's working, almost done and the only thing missing is a NixOS-infect style thing.

I was planning of using @cleverca22's kexec-based approach to that, which might be the fastest way to do it and allow to boot straight into the target nixos system.

A difference I can see between our approaches from skimming the PR for 1 minute is that you use the REST API directly while I use a Python package that does it.

@goodraven Are you in a hurry getting this merged? Otherwise I recommend we take some time to read each other's PRs in detail and then unify them.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 5, 2018

awesome! Is there any chance to get this merged in the next time?

@HolgerPeters
Copy link

@HolgerPeters HolgerPeters commented Jul 25, 2018

Is this PR in a state, so that I could test it?

@aforemny
Copy link

@aforemny aforemny commented Aug 4, 2018

@goodraven I just tested your PR. Thank you for this amazing work! Let's merge this! :-)

Copy link
Member

@aszlig aszlig left a comment

Thanks for the implementation. Apart from the commented points, running flake8 on this generates a lot of errors. So if possible we should aim to be PEP8-compliant for new code.


def destroy(self, wipe=False):
if not self.server_id:
self.log('server {} was never made'.format(self.name))

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

s/made/created/ maybe?

import nixops.util
import nixops.known_hosts

infect_path = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'data', 'nixos-infect'))

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

Probably would make sense to use upper-case INFECT_PATH.

resources.sshKeyPairs.ssh-key = {};

machine = { config, pkgs, ... }: {
services.openssh.enable = true;

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

This is shouldn't needed, as it's already enabled in nix/hetzner-cloud.nix.

deployment.targetEnv = "hetznerCloud";
deployment.hetznerCloud.serverType = "cx11";

networking.firewall.allowedTCPPorts = [ 22 ];

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

Same here, as it's done by the sshd module (openFirewall is enabled by default).

the infect itself, another after we pushed the nixos image.
"""
import os
import os.path

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

Not needed when we already have an import for os.

ssh_key_id = self._create_ssh_key(ssh_key.public_key)

req = {
'name': self.name,

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

Indentation: 4 spaces.


self.wait_for_ssh()
self.log_start("running nixos-infect")
self.run_command('bash </dev/stdin 2>&1', stdin=open(infect_path))

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

It should not be needed to redirect /dev/stdin here as bash reads from stdin by default if not connected to a terminal.


for interface in ${interfaces[@]}; do
ip4s=($(ip address show dev $interface | grep 'inet ' | sed -r 's|.*inet ([0-9.]+)/([0-9]+).*|{ address="\1"; prefixLength=\2; }|'))
ip6s=($(ip address show dev $interface | grep 'inet6 .*global' | sed -r 's|.*inet6 ([0-9a-f:]+)/([0-9]+).*|{ address="\1"; prefixLength=\2; }|'))

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

grep should not be needed here because sed has a -n flag.

];
};
ens4 = {
ip4 = [$(for a in ${ens4_ip4s[@]}; do echo -n "

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

Did you test other backends using nixos-infect whether they still work?

})

def get_ssh_private_key_file(self):
return self.write_ssh_private_key(self.depl.active_resources.get('ssh-key').private_key)

This comment has been minimized.

@aszlig

aszlig Aug 31, 2018
Member

I'd either internalize this for every machine state or allow to specify a specific resource rather than a hardcoded one. The former would have the advantage that it would require less options and later on we can still add an option so users can use an ssh key resource instead of the internal one.

@asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Apr 14, 2019

Is this PR still being worked on?

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Apr 19, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/help-customizing-nixops-version/2718/1

@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.

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