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

Conversation

markuskowa
Copy link
Member

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

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.

@markuskowa
Copy link
Member Author

markuskowa commented Aug 31, 2019

@GrahamcOfBorg test iscsi

@markuskowa markuskowa force-pushed the upd-targetcli branch 3 times, most recently from 85aaa1a to ac85b19 Compare September 7, 2019 13:00
@markuskowa
Copy link
Member Author

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

@ajs124
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");
Copy link
Member

Choose a reason for hiding this comment

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

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");

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks that seems to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Is the command necessary after all or what does:

iscsistart: initiator reported error (15 - session exists)

mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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
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 JohnAZoidberg left a comment

Choose a reason for hiding this comment

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

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
10 tasks
@JohnAZoidberg
Copy link
Member

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

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

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

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

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

@markuskowa
Copy link
Member Author

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

@jonringer should we merge it now?

@jonringer
Copy link
Contributor

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

@GrahamcOfBorg test iscsi

@markuskowa
Copy link
Member Author

That is strange I can run the test locally.

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

@JohnAZoidberg
Copy link
Member

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

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

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

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

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

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
@markuskowa markuskowa deleted the upd-targetcli branch May 25, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants