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

DO NOT MERGE: Nix 2 out of memory workaround #972

Closed
wants to merge 2 commits into from

Conversation

@nh2
Copy link
Contributor

@nh2 nh2 commented Jun 21, 2018

This is a workaround for the out of memory issue introduced with Nix 2 and present in NixOS 18.03 images (e.g. NixOS 18.03 AMIs).

It includes my fix from NixOS/nix#2206

How to use this workaround

Use this branch and set

deployment.ec2.nixosAmiVersion = "17.09";

for your AWS machines.

That will ensure the machine gets initially deployed with 17.09 which doesn't have the memory problem, will then get nix 2 with my patch deployed, so that in no of the two states is the problem present.


This is probably not intended to be merged, at least not with the hard nix override in the second patch.

However, if we made that override an option, then we could merge this.

Right now this is just to present a publicly visible workaround for people to use, until there's an official nix release with the fix.

@AmineChikhaoui
Copy link
Member

@AmineChikhaoui AmineChikhaoui commented Sep 6, 2018

I was just testing t2.nano/t2.micro instances with nix.package = pkgs.nixUnstable (Nix 2.1) on NixOS 18.03 and it seems to work just fine :)

@nh2 I'm going to close this PR as it seems that it's probably not relevant anymore, please feel free to re-open if you think I missed anything. Thanks

@nh2
Copy link
Contributor Author

@nh2 nh2 commented Sep 9, 2018

@AmineChikhaoui I'd still like to merge the deployment.ec2.nixosAmiVersion feature included in here.

Even though Nix 2.1 fixes the memory issue, I don't think it will help if based on the 18.03 AMI.

Because when you deploy the first time from the 18.03, the nix on the VM will be Nix 2.0, not 2.1. So if in the initial deploy big packages are deploy, it should continue to crash.

please feel free to re-open

Looks like I can't do that, I can only comment.

@edolstra
Copy link
Member

@edolstra edolstra commented Sep 10, 2018

I don't think nixosAmiVersion solves the problem well because it only helps on the initial deployment. Any subsequent deployment will use Nix 2.0 from the new system, so it will have the same OOM issues.

However, I think nixosAmiVersion might be useful for other use cases, such as testing NixOS upgrades.

@AmineChikhaoui
Copy link
Member

@AmineChikhaoui AmineChikhaoui commented Sep 10, 2018

@nh2 Ok right, seems the expression that I have didn't reproduce the initial issue, I was under the impression t2.nano/micro won't work at all even with a minimal expression but I guess I need to add few "large" packages to trigger the issue.

@AmineChikhaoui
Copy link
Member

@AmineChikhaoui AmineChikhaoui commented Sep 10, 2018

and yeah most likely the issue will persist unless Nix is built and activated before copying the closure which doesn't happen.
Most likely we will need to wait for NixOS 18.09 to be officially released as I don't think shipping the ec2 AMIs with nixUnstable would be a good idea.

@nh2
Copy link
Contributor Author

@nh2 nh2 commented Sep 20, 2018

Any subsequent deployment will use Nix 2.0 from the new system, so it will have the same OOM issues.

I don't understand this argument. The idea here is that deployment.ec2.nixosAmiVersion would always be used in combination with specifying a patched version of nix that doesn't have the issue.

The initial boot from AMI will bring up a version of nix that is so old that it doesn't have the problem. The first deployment will then instal whatever version of nix that I specify, which of course would have the patches in to fix the issue.

At no point would a 2.0 version without the issue patched be installed on the target system.

@coretemp
Copy link

@coretemp coretemp commented Oct 20, 2018

Is this now obsolete? If so, please close this.

@nh2
Copy link
Contributor Author

@nh2 nh2 commented Oct 20, 2018

Is this now obsolete?

No (please read the thread, this was already asked and answered above).

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