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

targetcli: 2.1.fb49 -> 2.1.51 #67749

Merged
merged 6 commits into from Nov 20, 2019
Merged

targetcli: 2.1.fb49 -> 2.1.51 #67749

merged 6 commits into from Nov 20, 2019

Conversation

@markuskowa
Copy link
Member

markuskowa commented Aug 30, 2019

Motivation for this change

Regular update. Now the man page is also installed.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@ajs124

This comment has been minimized.

Copy link
Member

ajs124 commented Aug 30, 2019

With the dependencies updated, we arrive at the age old question: did you test this with an actual iSCSI setup?

@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Aug 30, 2019

I did a local test with the the open-iscsi initiator and the targetcli/fileio backend on my laptop (before I updated the libraries), which worked fine. A great thing to have would be a NixOS test, that covers a few basic scenarios.

@ofborg ofborg bot added the 6.topic: nixos label Aug 31, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Aug 31, 2019

@GrahamcOfBorg test iscsi

@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch 3 times, most recently from 85aaa1a to ac85b19 Aug 31, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Sep 7, 2019

The test works just fine when I build it locally. Any ideas why it fails with ofBorg?

@ajs124

This comment has been minimized.

Copy link
Member

ajs124 commented Sep 7, 2019

Seems to work for me locally, as well. @grahamc do you have any insights?

# Check client login
$client->waitForUnit("multi-user.target");
$client->succeed("iscsistart -i ${initiatorName} -t ${targetName} -g 1 -a server");

This comment has been minimized.

Copy link
@Mic92

Mic92 Sep 24, 2019

Contributor

Maybe a race condition?

Suggested change
$client->succeed("iscsistart -i ${initiatorName} -t ${targetName} -g 1 -a server");
$client->waitUntilSucceeds("iscsistart -i ${initiatorName} -t ${targetName} -g 1 -a server");

This comment has been minimized.

Copy link
@markuskowa

markuskowa Sep 25, 2019

Author Member

Thanks that seems to work.

This comment has been minimized.

Copy link
@markuskowa

markuskowa Sep 27, 2019

Author Member

Sorry, I got something wrong here. Still fails with ofBorg.

This comment has been minimized.

Copy link
@Mic92

Mic92 Sep 30, 2019

Contributor

Is the command necessary after all or what does:

iscsistart: initiator reported error (15 - session exists)

mean?

This comment has been minimized.

Copy link
@markuskowa

markuskowa Sep 30, 2019

Author Member

iscsistart connects manually to the server (target). It is required to create new iSCSI provided block devices on the client ("/dev/sda" and "/dev/sdb" in this case). The message session exists means that the connect worked only half way at the first try (it somehow connected to the target but still failed).

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Sep 24, 2019

This is awesome!

I've been trying out the iSCSI infrastructure on NixOS recently and I'm planning to create a test and maybe module for tgt, as it allows declarative configuration. So I'd suggest to rename this test to targetcli.

@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from ac85b19 to 3ba6693 Sep 25, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Sep 25, 2019

@JohnAZoidberg I was also looking at tgt. However, it seems that this package is quite old and the recommended version is now targetcli, which is said to be more performant (uses the kernel space iscsi target). There might be a chance to use declarative configuration with targetcli: the tool stores and reads its config from a json file. Maybe that could be integrated in a nix module somehow?

Copy link
Member

JohnAZoidberg left a comment

Even though tgt is still maintained, it looks like targetcli is more or less a replacement:

Yes, it does look like we can use the JSON config to declaratively configure it. I wasn't aware of that before. I'll try that.
Alright, I'm for having targetcli as NixOS' default iSCSI toolchain.

@markuskowa markuskowa mentioned this pull request Oct 4, 2019
5 of 10 tasks complete
@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 5, 2019

This should be good, even works, when rebased on top of the latest master.

I'm working on the declarative service module but the JSON config seems to be too complicated to generate, completely, ourselves.

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 5, 2019

Oh and by the way, it looks like we've migrated off of Perl and to Python for the tests; and all tests are being rewritten.
So it'd be good to port this to Python. (Looks trivial)

markuskowa added 2 commits Aug 30, 2019
@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from 3ba6693 to 003b9bc Nov 5, 2019
@markuskowa markuskowa requested a review from jonringer as a code owner Nov 5, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 5, 2019

I think we we can leave the test in Perl for now and convert it later when new Python test frame work is more mature.

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 6, 2019

It sure looks like the Python framework is on-par and we're going all in: #72828

I'm quite stumped by this sudden change without a proper RFC or announcement :/

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 6, 2019

I guess there was some discussion at nixcon, and there seemed to be consensus

@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 6, 2019

Yes, the switch to python was quite quick but it I think it is a good thing to move away from perl.

The test is now in python and works fine (only GrahamcOfBorg seems to still have problems with it).

* install man page
@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from baeb01f to 4816ab2 Nov 7, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 7, 2019

@jonringer should we merge it now?

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 7, 2019

when trying to run the tests locally, i got:

client: must succeed: iscsistart -i iqn.2004-01.org.nixos:initiator -t iqn.2004-01.org.nixos:target -g 1 -a server
client # [   84.446956] Loading iSCSI transport class v2.0-870.
client # iscsistart: version 2.0-878
client # [   85.448936] iscsi: registered transport (tcp)
client # iscsistart: initiator reported error (15 - session exists)
client # [   86.409030] scsi host2: iSCSI Initiator over TCP/IP
client # [   86.518122]  connection1:0: detected conn error (1011)
client: output: iscsistart: Logging into iqn.2004-01.org.nixos:target server:3260,1

error: command `iscsistart -i iqn.2004-01.org.nixos:initiator -t iqn.2004-01.org.nixos:target -g 1 -a server` failed (exit code 1)
cleaning up
@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 7, 2019

@GrahamcOfBorg test iscsi

@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 7, 2019

That is strange I can run the test locally.

@ajs124

This comment has been minimized.

Copy link
Member

ajs124 commented Nov 8, 2019

@jonringer should we merge it now?

Upstream seems to have release 2.1.51 in the meantime.

@markuskowa markuskowa changed the title targetcli: 2.1.fb49 -> 2.1.50 targetcli: 2.1.fb49 -> 2.1.51 Nov 8, 2019
@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from d6bc374 to 82c38ea Nov 8, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 10, 2019

I did update all packages to the latest version. The test seems to be more stable now x86 but still is a bit shaky on ARM. Any suggestions?

Making sure that the iSCSI target can only be reached with via a single network interface seems to solve the problem with the unreliable test.

@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from b889afc to b389a83 Nov 13, 2019
@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 15, 2019

Test fails on x64 nixos for me :(

client # [  256.637003] systemd[1]: Reached target Multi-User System.
client # [  256.669568] systemd[1]: Startup finished in 1min 10.998s (kernel) + 3min 5.582s (userspace) = 4min 16.581s.
client: waiting for success: ping -c 1 server
(0.58 seconds)
client: must succeed: iscsistart -i iqn.2004-01.org.nixos:initiator -t iqn.2004-01.org.nixos:target -g 1 -a server
client # [  259.585977] Loading iSCSI transport class v2.0-870.
client # iscsistart: version 2.0-878
client # [  260.640047] iscsi: registered transport (tcp)
client # iscsistart: initiator reported error (15 - session exists)
client # [  261.415787] scsi host2: iSCSI Initiator over TCP/IP
client # [  261.688013]  connection1:0: detected conn error (1011)
client: output: iscsistart: Logging into iqn.2004-01.org.nixos:target server:3260,1

error: command `iscsistart -i iqn.2004-01.org.nixos:initiator -t iqn.2004-01.org.nixos:target -g 1 -a server` failed (exit code 1)
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 15, 2019

@JohnAZoidberg I am almost about to give up on the test. It seems to be reliably unreliable. My guess is that iscsistart does not like it when there are more than two network interfaces available (there are some mentions on google). I can't see a way to eliminate the problem.
Using iscsistart to test the functionality seems just not to be the right approach. Maybe the best way is to implement some basic NixOS modules for targetcli/iscsid first and and then base the test on iscsid/iscsiadm.

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 15, 2019

Maybe the best way is to implement some basic NixOS modules for targetcli/iscsid first and and then base the test on iscsid/iscsiadm.

Yes, that seems reasonable. I'm already working on that.

But for now I suggest we just merge the package updates and you could open another WIP PR for the test, I'd be interested to figure it out.

Are you using iSCSI on NixOS for anything other than testing?

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 15, 2019

According to this my error and the one on ofBorg for ARM means that the initiator is timing out, while connecting to the target.

@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 17, 2019

@JohnAZoidberg I am not using iSCSI at the moment but I want to in the future (hopefully with iSCSI root at some point).
Should we proceed like this: I will remove the the test from this PR. Since you are already working on a NixOS module the test will fit better into your PR. Feel free to just copy the test if you want to use it as a starting point.

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 17, 2019

Alright, sounds good, you can remove it.

Ohh, iSCSI root, that'll be interesting and probably doesn't work out of the box on NixOS right now.

markuskowa added 3 commits Nov 8, 2019
@markuskowa markuskowa force-pushed the markuskowa:upd-targetcli branch from b389a83 to 6b39472 Nov 17, 2019
@markuskowa

This comment has been minimized.

Copy link
Member Author

markuskowa commented Nov 17, 2019

OK, done

Ohh, iSCSI root, that'll be interesting and probably doesn't work out of the box on NixOS right now.

Yeah, that will probably require some more work to make it run smoothly.

@ofborg ofborg bot removed the 6.topic: nixos label Nov 17, 2019
@JohnAZoidberg JohnAZoidberg merged commit 09f9bcd into NixOS:master Nov 20, 2019
16 checks passed
16 checks passed
targetcli on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
targetcli on aarch64-linux Success
Details
targetcli on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.