Adding the iSCSI APIs #99

jmpfar · 2020-10-20T09:16:31Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Adding the iSCSI API group to csi-proxy.
This includes:

CHAP support
Limited Mutual CHAP support (secret needs to be set by the user on the node, not through CSIProxy)
Limited Multipath support (needs to be configured and installed by the user on the node not through CSI Proxy)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The tests are enabled through an environment variable ENABLE_ISCSI_TESTS which needs to be set to "TRUE" in order to run the iSCSI tests. This was done as a fail safe mechanism as the integration tests install an iSCSI Target on the local machine and format an empty iSCSI drive. So it's advisable to run this on a clean VM if possible.
The tests require a Windows Server machine in order to use the iSCSI target feature.

Does this PR introduce a user-facing change?:

Added the iSCSI api group with CHAP support

k8s-ci-robot · 2020-10-20T09:16:41Z

Hi @jmpfar. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jmpfar · 2020-10-20T09:44:55Z

I've currently disabled the iSCSI integration tests on github workflow as they do not work on that environment.
A possible long term solution is using an image that already has an iSCSI target installed.

Notes to reviewer:

The integration tests can be tested on a clean VM without any required prerequisites except for the env var above
Windows iSCSI target has some issues with long computer names, if you see issues try a VM with a short computer name (<= 15 chars)

ddebroy · 2020-10-20T14:00:44Z

/ok-to-test

1. Currently has a very limited support for Multipath 2. Does not contain node level APIs to set reverse CHAP or node IQN 3. Tests format a disk, so special flag is used to enable the iSCSI tests. Take care to use disposable clean VMs for tests. Change-Id: I0fe8431fed00715883ad0431671ec6a060718e0e

Change-Id: Ib669ed4aafad4a7ce6fa4821998e24264072c049

I've tried the test on a clean GCP Windows 2016/2019 VM and this works. This might be related to the Server Target feature install on the github workflow environment. Might be worth fixing with the other disabled integration tests. Change-Id: If76902118f1a2887caf5c554d339ad1dc13f04a8

jmpfar · 2020-10-20T20:08:20Z

/retest

jmpfar · 2020-10-20T20:15:28Z

/test all

jmpfar · 2020-10-20T20:22:47Z

Hey,
I'm trying to rerun the integration tests as they seemed to flake on the last run, any idea how do I trigger those on my pull request?

ddebroy · 2020-10-21T05:27:25Z

@jmpfar invoked rerun from the github actions UI linked from the details link.

ddebroy

Looks pretty good from a first scan. A few minor comments to clarify code comments.

ddebroy · 2020-10-22T06:28:47Z

client/api/iscsi/v1alpha1/api.proto

+}
+
+message GetTargetDisksRequest {
+  // Target portal to which the initiator will connect.


Please fix the comment to mention something along the lines of: Target portal whose disks will be querried

ddebroy · 2020-10-22T06:30:09Z

client/api/iscsi/v1alpha1/api.proto

+}
+
+message DisconnectTargetRequest {
+  // Target portal to which the initiator will connect.


Please fix the comment to mention something along the lines of Target portal from which initiator will disconnect.

ddebroy · 2020-10-22T06:31:06Z

client/api/iscsi/v1alpha1/api.proto

+message GetTargetDisksResponse {
+  // List composed of disk ids (numbers) that are associated with the
+  // iSCSI target
+  repeated string diskIds = 1;


Can we use diskIDs instead of diskIds please? See https://github.com/kubernetes-csi/csi-proxy/blob/master/client/api/disk/v1beta2/api.proto#L76

ddebroy · 2020-10-22T07:11:20Z

internal/os/iscsi/api.go

+
+func (APIImplementor) AddTargetPortal(portal *TargetPortal) error {
+	cmdLine := fmt.Sprintf(
+		`New-IscsiTargetPortal -TargetPortalAddress ${Env:iscsi_tp_address} ` +


The lack of -InitiatorInstanceName parameter assumes csi-proxy will always use the software iSCSI initiator in Windows rather any hardware NICs with iSCSI offload/support - which is fine. If you can add a comment to specify this, that would be great!

I've added clarifying comments (in the proto), I think this is a good idea to add this in later versions.
According to Windows documentation, if you don't specify an initiator:

"iSCSI initiator service will use any initiator that can reach the target portal."

(AddIScsiSendTargetPortalW)

So, at least by wording, this might suggest that the hardware initiator may be used

ddebroy · 2020-10-22T07:49:22Z

internal/os/iscsi/api.go

+		`Connect-IscsiTarget -TargetPortalAddress ${Env:iscsi_tp_address}` +
+			` -TargetPortalPortNumber ${Env:iscsi_tp_port} -NodeAddress ${Env:iscsi_target_iqn}` +
+			` -AuthenticationType ${Env:iscsi_auth_type}` +
+			` -IsMultipathEnabled $([System.Convert]::ToBoolean(${Env:iscsi_is_multipath}))`)


If IsMultipathEnabled is true then Connect-IscsiTarget will need to be invoked with InitiatorPortalAddress set to the IP address of each network adapter that wants to participate in the multi-path configuration. I think it will probably be better to block specification of IsMultipathEnabled for now?

I didn't consider the multipath scenario where you use two initiators to the same target, what I was thinking about was the other direction
e.g. I have one NIC that connects to two IPs of the storage array, each one exposing the same IQN.
This scenario will work with the current API
I guess this is more of a scenario for high availability rather than performance, and it was at least what we used in a product I've worked on (although it was far from the normal storage product)

WDYT? The multipath option here was added really quickly just to give an option for users with an already well configured node to use multipath. Full support will probably require more effort anyway, so I can remove the multipath bool and save it for a later version

Interesting - most production multipath configurations I have seen involve redundant paths all the way from the initiator NICs to the target.

I would say let’s remove the multipath option for now in v1alpha1 and add it later if we hear requests/feedback around it.

Once the multipath option is removed for now, I will lgtm/approve the PR. Thanks for getting basic iSCSI support going!

Thanks!
Removed the multipath option

ddebroy · 2020-10-22T07:59:38Z

client/api/iscsi/v1alpha1/api.proto

+  MUTUAL_CHAP = 2;
+}
+
+message ConnectTargetRequest {


Any thoughts on adding a initiator_address parameter to ConnectTargetRequest ? Can be added in a later version but was wondering if it would be useful in nodes with multiple NICs. We will probably need to add another API group potentially as part of the system group to query the set of host IP addresses as well.

See the MPIO comments, but I agree, this is a good feature for a later version
This is a mammoth commit as it is, I think I'll start adding features in separate PRs, where the first one is mutual chap support (the node-level set chap function)

Change-Id: Iff20646f3837b952abe10d81c86bdcab799d5d53

1. Fixing errors in proto comments 2. Adding missing comments for AuthenticationType enum 3. Renaming DiskIds to DiskIDs to better conform to other apis 4. Documenting lack of initiator selection features in AddTargetPortal 5. Improving multipath related documentation Change-Id: Ic9ec4387a140f522fa2e3a3ad781a439bc2cb918

This was decided as currently we don't have a way of specifying different initiators. Effectively making multipath very limited in usage, as well as probably confusing Change-Id: I0cd9b7fa7cdb309dbc29ab1a5a5c64909a429f1e

ddebroy · 2020-10-23T22:14:25Z

Thanks! One more request: can you please squash the commits please? Would like the commit history to not have “address cr comments” commits.

ddebroy · 2020-10-24T06:37:53Z

/lgtm
/approve
/hold

k8s-ci-robot · 2020-10-24T06:37:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, jmpfar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ddebroy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ddebroy · 2020-10-24T06:41:59Z

I squashed and merged this manually. So please ignore #99 (comment) for this PR.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 20, 2020

k8s-ci-robot requested review from jingxu97 and PatrickLang October 20, 2020 09:16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 20, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 20, 2020

jmpfar force-pushed the iscsi-api branch from 3f1f653 to e964409 Compare October 20, 2020 09:44

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 20, 2020

jmpfar added 3 commits October 20, 2020 19:54

Adding newly added vendor files

c1ce756

Change-Id: Ib669ed4aafad4a7ce6fa4821998e24264072c049

jmpfar force-pushed the iscsi-api branch from e964409 to f6c1e5c Compare October 20, 2020 19:56

ddebroy reviewed Oct 22, 2020

View reviewed changes

jmpfar and others added 3 commits October 22, 2020 11:42

Merge remote-tracking branch 'upstream/master' into iscsi-api

e278606

Change-Id: Iff20646f3837b952abe10d81c86bdcab799d5d53

Remove multipath option from API

37b254e

This was decided as currently we don't have a way of specifying different initiators. Effectively making multipath very limited in usage, as well as probably confusing Change-Id: I0cd9b7fa7cdb309dbc29ab1a5a5c64909a429f1e

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2020

k8s-ci-robot assigned ddebroy Oct 24, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2020

ddebroy merged commit dc98fe9 into kubernetes-csi:master Oct 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the iSCSI APIs #99

Adding the iSCSI APIs #99

jmpfar commented Oct 20, 2020 •

edited

Loading

k8s-ci-robot commented Oct 20, 2020

jmpfar commented Oct 20, 2020

ddebroy commented Oct 20, 2020

jmpfar commented Oct 20, 2020

jmpfar commented Oct 20, 2020

jmpfar commented Oct 20, 2020

ddebroy commented Oct 21, 2020

ddebroy left a comment

ddebroy Oct 22, 2020

jmpfar Oct 22, 2020

ddebroy Oct 22, 2020

jmpfar Oct 22, 2020

ddebroy Oct 22, 2020

jmpfar Oct 22, 2020

ddebroy Oct 22, 2020

jmpfar Oct 22, 2020

ddebroy Oct 22, 2020

jmpfar Oct 22, 2020

ddebroy Oct 23, 2020

ddebroy Oct 23, 2020

jmpfar Oct 23, 2020

ddebroy Oct 22, 2020 •

edited

Loading

jmpfar Oct 22, 2020

ddebroy commented Oct 23, 2020

ddebroy commented Oct 24, 2020

k8s-ci-robot commented Oct 24, 2020

ddebroy commented Oct 24, 2020

Adding the iSCSI APIs #99

Adding the iSCSI APIs #99

Conversation

jmpfar commented Oct 20, 2020 • edited Loading

k8s-ci-robot commented Oct 20, 2020

jmpfar commented Oct 20, 2020

ddebroy commented Oct 20, 2020

jmpfar commented Oct 20, 2020

jmpfar commented Oct 20, 2020

jmpfar commented Oct 20, 2020

ddebroy commented Oct 21, 2020

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy commented Oct 23, 2020

ddebroy commented Oct 24, 2020

k8s-ci-robot commented Oct 24, 2020

ddebroy commented Oct 24, 2020

jmpfar commented Oct 20, 2020 •

edited

Loading

ddebroy Oct 22, 2020 •

edited

Loading