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

Fix/ub 1675 support fc and iscsi #254

Merged
merged 9 commits into from
Nov 21, 2018

Conversation

olgashtivelman
Copy link
Collaborator

@olgashtivelman olgashtivelman commented Nov 14, 2018

in this PR the idea is to allow the user to use ISCSI + FC on the same kubernetes cluster.
before this PR the user had to define a config parameter : skipRescanIscsI to define if they want to use ISCSI or not in ubiquity.
this PR reomves this config and works under the assumption that if a user defined and installed iscsiadm on the node then this node should be connected to the storage system with ISCSI. also the existance of the iscsiadm command or a login to the storage will not fail the rescan and the mount\umount process!

Note: This PR also related to IBM/ubiquity#267


This change is Reviewable

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)


utils/utils.go, line 41 at r1 (raw file):

	bool, err := strconv.ParseBool(os.Getenv("SCBE_SKIP_RESCAN_ISCSI"))
	if err != nil {
		config.ScbeRemoteConfig.SkipRescanISCSI = false
  1. please rebase the PR from dev, because i pumb in the scale helm support.

  2. you should also remove the skip rescan not only from the golang code, you should remove it from all the helm chart and old installer (values or yml files). please do grep for this rescan thing and make sure you removed it from all the places.

  3. please make sure Dima know about this fix, and has a dedicated ticket to remove any documentation about this param, but also have documentation that explain the new behavius in the UG.

Copy link
Collaborator Author

@olgashtivelman olgashtivelman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @shay-berman)


utils/utils.go, line 41 at r1 (raw file):

Previously, shay-berman wrote…
  1. please rebase the PR from dev, because i pumb in the scale helm support.

  2. you should also remove the skip rescan not only from the golang code, you should remove it from all the helm chart and old installer (values or yml files). please do grep for this rescan thing and make sure you removed it from all the places.

  3. please make sure Dima know about this fix, and has a dedicated ticket to remove any documentation about this param, but also have documentation that explain the new behavius in the UG.

regarding 1+2 see changes.
regarding documentation i will consult Dima.

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Well done.

Merge to dev this Pr (and IBM/ubiquity#267) into dev after you have green light from Staging CI.

Reviewable status: 0 of 11 files reviewed, all discussions resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants