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

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 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 commented Jul 5, 2018

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

@HolgerPeters
Copy link

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

@aforemny
Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably would make sense to use upper-case INFECT_PATH.

resources.sshKeyPairs.ssh-key = {};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }|'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Is this PR still being worked on?

@nixos-discourse
Copy link

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 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hetzner Cloud support
9 participants