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

Does not work on platforms aside from x86_64-linux #12

Closed
khaled opened this issue Jan 26, 2023 · 17 comments
Closed

Does not work on platforms aside from x86_64-linux #12

khaled opened this issue Jan 26, 2023 · 17 comments

Comments

@khaled
Copy link

khaled commented Jan 26, 2023

This line seems to prevent this from working on other systems.

Any reason why it is this way? I tried cloning the repo, uncommenting the line, and building the example on x86_86-darwin. Seemed to work just fine.

@adrian-gierakowski
Copy link
Contributor

It's possibly due to docker images not being able to be built on darwin (at least not without extra work setting up a remote builder). However manifest generation should work just fine (as long as you don't use the docker module).

@khaled
Copy link
Author

khaled commented Jan 26, 2023

It's possibly due to docker images not being able to be built on darwin (at least not without extra work setting up a remote builder). However manifest generation should work just fine (as long as you don't use the docker module).

Agreed. Also what about aarch64-linux systems?

@adrian-gierakowski
Copy link
Contributor

any architecture supported by nix should be ok since manifests can be created with nix eval (nix build is only needed if you want to convert generate json to yaml).

@hall
Copy link
Owner

hall commented Jan 26, 2023

Thanks for the report, @khaled (and for the added details @adrian-gierakowski).

I'm not entirely certain as the commit which introduced that change has no details.

However, the parent commit introduced new checks and failed its CI execution which was then fixed by the commit in question. So, if I had to guess, the x86_64-linux restriction was likely introduced because the GH runners were unable to build those checks for the other platforms.

@khaled
Copy link
Author

khaled commented Jan 26, 2023

Thanks @hall. Sounds like a good guess! With the line un-commented:

x86_64-linux:

❯ nix flake check
warning: unknown flake output 'pkgs'
warning: unknown flake output 'evalModules'
error: a 'aarch64-linux' with features {} is required to build '/nix/store/g4s3cz4r7lwmv8iyb5c3j7fz677yyj9g-yaml-to-json.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test}
(use '--show-trace' to show detailed location information)
❯ error: a 'aarch64-linux' with features {} is required to build '/nix/store/g4s3cz4r7lwmv8iyb5c3j7fz677yyj9g-yaml-to-json.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test}

x86_64-darwin:

❯ nix flake check
warning: input 'flake-utils' has an override for a non-existent input 'nixpkgs'
warning: unknown flake output 'pkgs'
warning: unknown flake output 'evalModules'
error: invalid regular expression '(  |)+'

       at /nix/store/jhj2sgkzbdrn1ywvyf9ck08d8pzwxv4s-source/jobs/generators/istio/default.nix:23:64:

           22|
           23|     removeEmptyLines = str: concatStringsSep "\n" (filter (l: (builtins.match "(  |)+" l) == null) (splitString "\n" str));
             |                                                                ^
           24

Wondered why the x86_64-linux checks are doing anything with aarch64, and found this. Haven't made sense of the darwin failure yet.

@hall
Copy link
Owner

hall commented Jan 27, 2023

Good find. First I'm hearing of IFD but that certainly appears to be the case as ./tests/k8s/imports.nix (evaluated during checks) sets kubernetes.imports which calls importYAML, a wrapper around importJSON, which calls readFile and therefore qualifies as IFD per that wiki page.

Not yet sure what a decent resolution is here.

No idea on the darwin failure though; no surprise that it evaluates fine on linux:

nix-repl> builtins.match "(  |)+" ""
[ "" ]

Presumably it's using a different regex engine but will have to do some work before I can get to a place to test.

@adrian-gierakowski
Copy link
Contributor

adrian-gierakowski commented Jan 27, 2023

./tests/k8s/imports.nix (evaluated during checks) sets kubernetes.imports which calls importYAML, a wrapper around importJSON, which calls readFile and therefore qualifies as IFD per that wiki page.

Just to clarify, the IFD is due to the call to readFile on the output of pkgs.runCommand withi n importYaml. runCommand produces a derivation, and so readFile ends up loading (from the /nix/store) the content of the output of building that derivation.

importJSON itself doesn't cause IFD (as long as it's called with a path to a file which exists at evaluation time, and not a derivation, which needs to be built first by nix).

The way to get rid of IFD here would be to implement a yaml parser in pure nix (or have it implemented as nix builtin). Alternatively you could convert you yaml manifests to json and use importJSON instead of importYaml.

@hall
Copy link
Owner

hall commented Jan 28, 2023

Ah! Thanks, that makes sense.

A quick search doesn't reveal much in the way of complete parsers in pure nix (and I'm not particularly itching to try and write one). I don't use either of the import functions myself but I can see importYaml being useful (instead of requiring users to convert to json beforehand, e.g.).

There is a recent MR inflight to implement fromYAML. So maybe a proper solution isn't far off.

I'm tempted to just disable the yaml test for the time-being so users on other platforms can at least still use the project.

@adrian-gierakowski
Copy link
Contributor

I'm tempted to just disable the yaml test for the time-being so users on other platforms can at least still use the project.

👍

@hall
Copy link
Owner

hall commented Jan 28, 2023

Cool -- thanks for the sanity check. I've disabled the import test (didn't help to only disable yaml without json) in 7201c32 and switched to eachDefaultSystem in 7af982c.

@khaled, closing as this should be complete but let us know if it doesn't work as expected.

@hall hall closed this as completed Jan 28, 2023
@khaled
Copy link
Author

khaled commented Jan 28, 2023

Thanks @hall and @adrian-gierakowski! nix flake check now succeeds on x86_64-linux. The regex failure is still a problem on darwin, but I guess that's a separate bug :)

@hall
Copy link
Owner

hall commented Jan 28, 2023

Yeah, feel free to open another issue for that. Best I can do is help you debug on darwin. The idea is that regex is removing "empty" lines (where that includes whitespace characters). It appears to be a known issue that there are inconsistencies between platforms.

You might try the following diff 🤷 (this line also appears in the istio generator in the same parent directory)

❯ git diff jobs/generators/k8s/
diff --git a/jobs/generators/k8s/default.nix b/jobs/generators/k8s/default.nix
index 43ec4b9..e712742 100644
--- a/jobs/generators/k8s/default.nix
+++ b/jobs/generators/k8s/default.nix
@@ -21,7 +21,7 @@ with lib; let
       then "null"
       else builtins.toString value;
 
-    removeEmptyLines = str: concatStringsSep "\n" (filter (l: (builtins.match "(  |)+" l) == null) (splitString "\n" str));
+    removeEmptyLines = str: concatStringsSep "\n" (filter (l: (builtins.match "[[:space:]]*" l) != []) (splitString "\n" str));
 
     mkOption = {
       description ? null,

You can either test in nix repl (e.g., builtins.match "[[:space:]]*" "") or execute nix run '.#generate' and if there's no diff under ./modules/generated, then it works as expected.

@felixscheinost
Copy link
Contributor

I am trying to implement a change to the Helm module we discussed (implement filtering/modifying the objects).

I am trying to get the currently commented out test working on my machine, a aarch64-darwin but with x86_64-darwin support through Rosetta and also x86_64-linux and aarch64-linux remote builders configured.

If I understand correctly the commented out tests prevent running the checks even on aarch64-linux, right? That doesn't seem right, I agree.

What I am unsure about though is what the expectation is about Darwin:

  • Do you expect that Darwin users without a Linux remote builder configured are able to execute all tests?
    • I think that shouldn't be the case as then integration tests, like are already present, are out of the picture right?
  • nix flake check executes checks for the current arch, right? So would it make sense that on Darwin nix flake check only does the manifest tests but on Linux additional integration tests are executed?
    • But: How could developers on Darwin with remote builders configured run the Linux tests then? Does nix flake check allow to specify the system? I tried nix flake check .#checks.x86_64-linux and some variations of that but that doesn't seem to work.

@hall
Copy link
Owner

hall commented Feb 15, 2023

If I understand correctly the commented out tests prevent running the checks even on aarch64-linux, right?

Since the commented out tests use IFD, it forces evaluation on every platform the flake supports (i.e., the ones defined by eachDefaultSystem). That means whenever running nix flake check, access must be available to a set of machines which cover every system. So anyone without access to at least one builder in eachDefaultSystem will fail the checks.

Ideally, we'd remove the use of IFD and nix flake check would execute only for the local platform.

You're saying that existing integration tests don't support darwin? It's unclear to me in general how well nix is supported on darwin. Which tests? Can you provide an example or links to docs?

I haven't tried running the checks for a specific platform; maybe nix flake check --system x86_64-linux?

@felixscheinost
Copy link
Contributor

Since the commented out tests use IFD, it forces evaluation on every platform the flake supports

Hmm, are you sure that this is really true? Wouldn't it also mean that it would try to evaluate x86_64-darwin on x86_64-linux? Even on the revision of flake-utils currently used by this project the default systems contained the darwin systems (see flake-utils)

Ideally, we'd remove the use of IFD and nix flake check would execute only for the local platform.

I think that sounds like a good idea as it seems that IFD is sort of a discouraged. Maybe this might also reduce resource usage of kubenix? I previously tried running nix flake check on a small Linux VM and nix flake check got killled as it quickly used >2GB RAM.

You're saying that existing integration tests don't support darwin?

I don't know to be honest. But I assume so? Because the integration tests actually test against a real Kubernetes cluster running in a VM I assume? Using NixOS VM testing capability? Or is that assumption wrong?

NixOS VM tests currently don't work properly on macOS as they are optimized for speed: they share the /nix store with the host which is hard or impossible to implement on macOS AFAIK.

I haven't looked through all tests but those that are currently commented out due to IFD are definitely integration test, right? Are there any other integration tests that aren't commented out?

As of right now nix flake check succeeds on my Darwin machine, with Linux builders configured.

It's unclear to me in general how well nix is supported on darwin.

I would say Darwin support is very good already and only getting better (partly due to e.g. https://opencollective.com/nix-macos)

We use it as an alternative to brew. Except for e.g. heavy GUI tools like Slack, Docker Desktop, ... all the CLI tools come from Nix. Most of nixpkgs is also available on Darwin and most of it is also built and cached by Hydra.

If I stumble into a random nix-shell out there most of the time it also works on Darwin, which is great!

For working with NixOS servers though you most of the time need to have a Linux remote builder configured. In my case this was a container running in Docker for Desktops VM but I am in the process of migrating that into the "official" darwin.builder that nixpkgs recently added.


nix flake check is only supposed to be executed by people working on the kubenix project itself, right? Not downstream users of the library? In this case I think it would be okay to assume someone to need a Linux builder configured for that on Darwin. This way developers on all platforms would only need to execute nix flake build to execute the whole test suit.

@adrian-gierakowski
Copy link
Contributor

Since the commented out tests use IFD, it forces evaluation on every platform the flake supports

Hmm, are you sure that this is really true? Wouldn't it also mean that it would try to evaluate x86_64-darwin on x86_64-linux?

see: NixOS/nix#4265
although there seems to be a fix in the works: NixOS/nix#7759

Ideally, we'd remove the use of IFD and nix flake check would execute only for the local platform.

Helm module does IFD, there is no other way to do what it does and we definitely want tests to cover it.

The other place I'm aware of IFD being used is converting json to yaml (as mentioned above). This is not an essential feature imho.

Anyway, with the fix mentioned above we can stop worrying about IFD

nix flake check got killled as it quickly used >2GB RAM.

I don't think that is related to IFD, but rather the fact that it spins up nixos tests which run in a VM

You're saying that existing integration tests don't support darwin?

I don't know to be honest. But I assume so? Because the integration tests actually test against a real Kubernetes cluster running in a VM I assume? Using NixOS VM testing capability? Or is that assumption wrong?

That is correct.

NixOS VM tests currently don't work properly on macOS as they are optimized for speed: they share the /nix store with the host which is hard or impossible to implement on macOS AFAIK.

I believe this has been solved recently.

nix flake check is only supposed to be executed by people working on the kubenix project itself, right? Not downstream users of the library?

yes

@felixscheinost
Copy link
Contributor

Helm module does IFD, there is no other way to do what it does and we definitely want tests to cover it.

Makes sense!

I don't think that is related to IFD, but rather the fact that it spins up nixos tests which run in a VM

True, that is more likely to be the reason. I just remember playing around with IFD myself sometime ago and running into very slow evaluation times as well but I may have jumped to conclusions here 😄

I believe this has been solved recently.

Ah yeah, that's right. Hmm, I think I remember though that building the SquashFS for the root partition can be quite slow. If a Linux builder is available, the edit-test-cycle is probably quicker using that, compared to always having to build a whole new image.

I will try to get the Helm integration test working on Darwin when I have time 😃

I'm tempted to just disable the yaml test for the time-being so users on other platforms can at least still use the project.

Cool -- thanks for the sanity check. I've disabled the import test (didn't help to only disable yaml without json) in 7201c32 and switched to eachDefaultSystem in 7af982c.

But a prerequisite for re-enabling that test would be to find a different workaround than disabling the test, right? I'll play around with that as well.

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

No branches or pull requests

4 participants