Change in vdsm[master]: Change iscsiadm operational mode while updating 'node.startu...

Sergey Gotliv has uploaded a new change for review.

Change subject: Change iscsiadm operational mode while updating 'node.startup'
setting
......................................................................

Change iscsiadm operational mode while updating 'node.startup' setting

Iscsiadm operational mode must be 'node' while updating 'node.startup'
setting and not 'discoverydb' as it is today.

Change-Id: Ib34952047367db196733e942416a232d12a4e491
Bug-Url: https://bugzilla.redhat.com/851050
Signed-off-by: Sergey Gotliv <sgotliv(a)redhat.com&gt;
---
M vdsm/storage/iscsi.py
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/17212/1

diff --git a/vdsm/storage/iscsi.py b/vdsm/storage/iscsi.py
index bb09544..a8cba3d 100644
--- a/vdsm/storage/iscsi.py
+++ b/vdsm/storage/iscsi.py
@@ -187,7 +187,7 @@
         #       write the database when the discovery fails, therefore we try
         #       to set the node startup to manual anyway.
         try:
-            iscsiadm.discoverydb_update(discoverType, iface.name, portalStr,
+            iscsiadm.node_update(discoverType, iface.name, portalStr,
                                         "node.startup", "manual")
         except:
             # this is just to be polite we don't really care


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Removing update of 'node.startup' setting in 'addIscsiPortal'

I removed not working code in 'addIscsiPortal' method. It supposed
to update 'node.startup' setting to 'manual', but used
'discoverydb'
operational mode instead of 'node' what caused to exception.

Change-Id: Ib34952047367db196733e942416a232d12a4e491
Bug-Url: https://bugzilla.redhat.com/851050
Signed-off-by: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/17212
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
Reviewed-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Reviewed-by: Ayal Baron <abaron(a)redhat.com&gt;
---
M vdsm/storage/iscsi.py
1 file changed, 0 insertions(+), 10 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Federico Simoncelli: Looks good to me, but someone else must approve
  Sergey Gotliv: Verified
  Dan Kenigsberg: Looks good to me, but someone else must approve

Objections:
  Eduardo: I would prefer that you didn't submit this


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3:

Eduardo, I tend to agree that this patch, with its stated intention of removing log noise,
should not block for further simplification to the function.

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Ayal Baron has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: Looks good to me, approved

So we came round circle on this.
addIscsiPortal is only ever called from discoverSendTargets and is followed by
try..finally and in the finally part deleteIscsiPortal is called, so indeed this code is
redundant.

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Sergey Gotliv has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: Verified

Code removal is safe. All targets discovered during this method will be removed in the
next. Of course there is an option that VDSM will be restarted in the middle, but in this
case user will need to run the flow again to retrieve targets in UI in order to create
storage domain and then they will be deleted(as I mentioned above).

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Ayal Baron has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: No score

After discussing with Sergey, this is to make sure to override the basic iscsiadm
functionality after discovery to *not* automatically login to *all* targets.
Removing the +2 to make sure this is not merged.

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: Looks good to me, but someone else must approve

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Ayal Baron has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
When you move a host to maintenance then that calls disconnectStorageServer which should
remove this definition so in general this shouldn't affect anything negatively.  The
case where this would remain is in case of an error flow where for some reason it
wasn't disconnected properly or something.  Even if that happens, it's not that
interesting as a few leftover connections shouldn't affect the host negatively (think
about FC where the host is always connected).
The advantage is that most of the time (since hosts don't move between DCs very often)
the host would automatically reconnect whenever it reboots (e.g. due to sanlock fencing)
and it would be available to engine almost immediately reducing recovery time.  Imo that
is more important.
See oVirt Bug 881941 - After a reboot of both engine and vdsm domains take a long time (15
minutes) to recover
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(1 inline comment)

Downvoting for visibility to my comment.

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Isn't this a problem if you move an host from a data center to another? (leftovers)
If the host can still reach them it'll be seeing luns that is not supposed to see, if
it  can't, they might timeout (slowing down boot?).

The fact that this wasn't working for 16 months is not an excuse (see lv teardown).

Let's put things in prospective, http://gerrit.ovirt.org/#/c/305 was introduced to
remove the entire code that was overwriting the iscsid.conf file.
Dan nacked the patch because he wanted to maintain node.startup=manual (comment 4 on the
above gerrit link), and that made sense.

So generally I'm fine removing this code, but I think we should make sure that we
really don't care about leftovers (and Dan really changed his mind).
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Ayal Baron has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: Looks good to me, approved

(2 inline comments)

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Ack on removing it.  Having it automatically reconnect on reboot is not a bad thing and
should shorten startup times of a lot of things.
The worst case scenario is we have some leftover connections.
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Line 186:         try:
Indeed, shouldn't mix unrelated issues.
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():
Line 190:                     key = "discovery.sendtargets." + key


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Sergey Gotliv has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Line 186:         try:
Its not related to the patch. This patch resolves a bug, please, see bug-url. All other
changes even if they are make our code better are irrelevant for that issue.
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():
Line 190:                     key = "discovery.sendtargets." + key


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Eduardo has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Line 186:         try:
Please reduce this try-block and check if it is necessary at all.
hideValue is a specially arcane name.
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():
Line 190:                     key = "discovery.sendtargets." + key


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Oh please do not! I like the code removal, hence the +1. I just need Federico's
review.
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

Sergey Gotliv has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
I agree with you. I fixed it in the patch #2, but then I just tempted to remove it, this
code doesn't work 16 month now and nobody doesn't complaint about it.  ???
I'll provide another patch with the fix
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/iscsi.py
Line 181:     portalStr = "%s:%d" % (portal.hostname, portal.port)
Line 182: 
Line 183:     with _iscsiadmTransactionLock:
Line 184:         iscsiadm.discoverydb_new(discoverType, iface.name, portalStr)
Line 185: 
Wouldn't it be better to fix it? Why we don't need this anymore? (beside that it
was failing since some time)
Line 186:         try:
Line 187:             # Push credentials
Line 188:             if credentials is not None:
Line 189:                 for key, value in credentials.getIscsiadmOptions():


-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3: Looks good to me, but someone else must approve

Removing non-functional code (with a silent try-except!) is honorable. However, since this
was introduce by http://gerrit.ovirt.org/#/c/305/6/vdsm/storage/iscsi.py to solve
https://bugzilla.redhat.com/show_bug.cgi?id=499222 I would prefer that Federico acks it.

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Removing update of 'node.startup' setting in 'addIscsiPortal...

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Removing update of 'node.startup' setting in
'addIscsiPortal'
......................................................................


Patch Set 3:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2671/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3478/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3562/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Change iscsiadm operational mode while updating 'node.startup'
setting
......................................................................


Patch Set 1: Fails

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2629/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3436/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3520/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Change iscsiadm operational mode while updating 'node.startup'
setting
......................................................................


Patch Set 2:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2631/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3438/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3522/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/17212
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib34952047367db196733e942416a232d12a4e491
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server