Change in vdsm[master]: net: add a resolv.conf writer

[phoracek]

Petr Horáček has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 4: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[automation]

gerrit-hooks has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 3:

(9 comments)

https://gerrit.ovirt.org/#/c/62705/3/lib/vdsm/network/ip/resolv.py
File lib/vdsm/network/ip/resolv.py:

PS3, Line 33: , 'r'
> 'r' is default, we can drop it.
Done


PS3, Line 38: nameservers = []
: for words in _parse_lines(file_text):
: if words[0] == 'nameserver':
: nameservers.append(words[1])
: return nameservers
> return [words[1] for words in _parse_lines(file_test) if words[0] == 'names
I still have domains in mind and want to generalize this into 
_parse_resolver_configuration, which returns two lists: 
https://gerrit.ovirt.org/#/c/61794/


Line 51: yield words
Line 52: 
Line 53: 
Line 54: def update(nameservers):
Line 55: lines = [_conf_header()]
> header_lines
No need to treat a header in a special way.
Line 56: for ns in nameservers:
Line 57: lines.append('nameserver ' + ns)
Line 58: 
Line 59: for words in _parse_lines(_read_resolv_conf()):


PS3, Line 56: for ns in nameservers:
: lines.append('nameserver ' + ns)
> ns_lines = ['nameserver ' + ns for ns in nameservers]
Done


PS3, Line 59: for words in _parse_lines(_read_resolv_conf()):
: if words[0] != 'nameserver':
: lines.append(' '.join(words))
> other_lines = [' '.join(words) for words in _parse_lines(...) if words[0] !
Done


Line 59: for words in _parse_lines(_read_resolv_conf()):
Line 60: if words[0] != 'nameserver':
Line 61: lines.append(' '.join(words))
Line 62: 
Line 63: with open(DNS_CONF_FILE, 'w') as f:
> lines = + itertools.chain.from_iterable([header_lines, ns_lines, other_line
Lists can be simply concatenated.
Line 64: f.writelines(line + '\n' for line in lines)
Line 65: 
Line 66: 
Line 67: def _conf_header():


Line 63: with open(DNS_CONF_FILE, 'w') as f:
Line 64: f.writelines(line + '\n' for line in lines)
Line 65: 
Line 66: 
Line 67: def _conf_header():
> it can be defined as a constant or memoized.
I went for a constant.


https://gerrit.ovirt.org/#/c/62705/3/tests/network/resolv_test.py
File tests/network/resolv_test.py:

PS3, Line 56: test_update
> Please describe what is being tested: Replacing existing, appending new one
For now, we test replacement, to keep it simple. 

Resolver uses up to MAXNS (3) nameservers and ignores any extra nameservers.

One nameserver is in now way special currently (only zero is, in that case 
current nameservers are used).


In a separate patch, we could support initscripts-style 'update' (like in 
ifup-post), but for 3, not 2 entries:

In addition to defined nameservers, and up to MAXNS, append current nameservers 
which differ from the defined ones (as fallback nameservers). I had this in 
patch set 1 (_extend_with_unique).


PS3, Line 68: [
: resolv._conf_header() + '\n'] +
: ['nameserver {}\n'.format(ns) for ns in 
nameservers] +
: self.RESOLV_CONF[1:3])
> can we define it in another constant?
Done


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[edwardh]

Edward Haas has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 3:

Correcting my last comment: Functional tests will appear in the next patch as 
part of OVS + dns setup.

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[edwardh]

Edward Haas has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 3: Code-Review-1

(1 comment)

We are also missing functional tests for this, please add them.

https://gerrit.ovirt.org/#/c/62705/3/tests/network/resolv_test.py
File tests/network/resolv_test.py:

PS3, Line 56: test_update
Please describe what is being tested: Replacing existing, appending new ones, 
removing?

What happens when adding only one entry?
What happens when we try to add too many entries?


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[phoracek]

Petr Horáček has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 3: Code-Review-1

(9 comments)

https://gerrit.ovirt.org/#/c/62705/3/lib/vdsm/network/ip/resolv.py
File lib/vdsm/network/ip/resolv.py:

PS3, Line 33: , 'r'
'r' is default, we can drop it.


PS3, Line 38: nameservers = []
: for words in _parse_lines(file_text):
: if words[0] == 'nameserver':
: nameservers.append(words[1])
: return nameservers
return [words[1] for words in _parse_lines(file_test) if words[0] == 
'nameserver']


Line 51: yield words
Line 52: 
Line 53: 
Line 54: def update(nameservers):
Line 55: lines = [_conf_header()]
header_lines
Line 56: for ns in nameservers:
Line 57: lines.append('nameserver ' + ns)
Line 58: 
Line 59: for words in _parse_lines(_read_resolv_conf()):


PS3, Line 56: for ns in nameservers:
: lines.append('nameserver ' + ns)
ns_lines = ['nameserver ' + ns for ns in nameservers]


PS3, Line 59: for words in _parse_lines(_read_resolv_conf()):
: if words[0] != 'nameserver':
: lines.append(' '.join(words))
other_lines = [' '.join(words) for words in _parse_lines(...) if words[0] != 
'nameserver']


Line 59: for words in _parse_lines(_read_resolv_conf()):
Line 60: if words[0] != 'nameserver':
Line 61: lines.append(' '.join(words))
Line 62: 
Line 63: with open(DNS_CONF_FILE, 'w') as f:
lines = + itertools.chain.from_iterable([header_lines, ns_lines, other_lines])
Line 64: f.writelines(line + '\n' for line in lines)
Line 65: 
Line 66: 
Line 67: def _conf_header():


Line 63: with open(DNS_CONF_FILE, 'w') as f:
Line 64: f.writelines(line + '\n' for line in lines)
Line 65: 
Line 66: 
Line 67: def _conf_header():
it can be defined as a constant or memoized.


https://gerrit.ovirt.org/#/c/62705/3/tests/network/resolv_test.py
File tests/network/resolv_test.py:

PS3, Line 57: namedTemporaryDir
We should use mock.mock_open() in new tests. see ifacquire_test.py.


PS3, Line 68: [
: resolv._conf_header() + '\n'] +
: ['nameserver {}\n'.format(ns) for ns in 
nameservers] +
: self.RESOLV_CONF[1:3])
can we define it in another constant?


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[automation]

gerrit-hooks has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[automation]

gerrit-hooks has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[osvoboda]

Ondřej Svoboda has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/62705/1/lib/vdsm/network/ip/resolv.py
File lib/vdsm/network/ip/resolv.py:

Line 50: if len(words) >= 2:
Line 51: yield words
Line 52: 
Line 53: 
Line 54: def update(nameservers):
> can you add a unit test for this function?
Yes, but please note I will remove complex handling of "old_nameservers" first. 
It needs rethinking or to be moved to canonicalize.
Line 55: file_text = _read_resolv_conf()
Line 56: old_nameservers = _parse_nameservers(file_text)
Line 57: nameservers = list(nameservers)
Line 58: _extend_with_unique(nameservers, old_nameservers)


PS1, Line 60: Generated by VDSM version
> can this be turned into a constant, importable from ifcfg configurator?
Let us discuss where to put such a constant.

This literal is in ifcfg.py, unified_persistence(_test).py and 
vdsm-restore-net-config. So far, it is found only in network-related code.

Is network/__init__.py a good location, then?


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda 
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 1:

(2 comments)

very very shallow review on my part

https://gerrit.ovirt.org/#/c/62705/1/lib/vdsm/network/ip/resolv.py
File lib/vdsm/network/ip/resolv.py:

Line 50: if len(words) >= 2:
Line 51: yield words
Line 52: 
Line 53: 
Line 54: def update(nameservers):
can you add a unit test for this function?
Line 55: file_text = _read_resolv_conf()
Line 56: old_nameservers = _parse_nameservers(file_text)
Line 57: nameservers = list(nameservers)
Line 58: _extend_with_unique(nameservers, old_nameservers)


PS1, Line 60: Generated by VDSM version
can this be turned into a constant, importable from ifcfg configurator?


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[automation]

gerrit-hooks has posted comments on this change.

Change subject: net: add a resolv.conf writer
..


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: net: add a resolv.conf writer

[osvoboda]

Ondřej Svoboda has uploaded a new change for review.

Change subject: net: add a resolv.conf writer
..

net: add a resolv.conf writer

Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Signed-off-by: Ondřej Svoboda 
---
M lib/vdsm/network/ip/resolv.py
1 file changed, 46 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/05/62705/1

diff --git a/lib/vdsm/network/ip/resolv.py b/lib/vdsm/network/ip/resolv.py
index 5d9ce22..c21b43f 100644
--- a/lib/vdsm/network/ip/resolv.py
+++ b/lib/vdsm/network/ip/resolv.py
@@ -18,22 +18,60 @@
 # Refer to the README and COPYING files for full details of the license
 from __future__ import absolute_import
 
+from vdsm import dsaversion
+
 DNS_CONF_FILE = '/etc/resolv.conf'
+MAXNS = 3  # the resolver suppors no more than this number of nameservers
 
 
 def get_host_nameservers():
-"""Returns a list of nameservers listed in /etc/resolv.conf"""
-with open(DNS_CONF_FILE, 'r') as file_object:
-file_text = file_object.read()
-return _parse_nameservers(file_text)
+"""Return a list of nameservers listed in /etc/resolv.conf"""
+return _parse_nameservers(_read_resolv_conf())
+
+
+def _read_resolv_conf():
+with open(DNS_CONF_FILE, 'r') as f:
+return f.read()
 
 
 def _parse_nameservers(file_text):
 nameservers = []
-for line in file_text.splitlines():
-words = line.strip().split()
-if len(words) < 2:
-continue
+for words in _parse_lines(file_text):
 if words[0] == 'nameserver':
 nameservers.append(words[1])
 return nameservers
+
+
+def _parse_lines(file_text):
+for line in file_text.splitlines():
+if not line or line[0] in ';#':
+continue
+words = line.strip().split()
+if len(words) >= 2:
+yield words
+
+
+def update(nameservers):
+file_text = _read_resolv_conf()
+old_nameservers = _parse_nameservers(file_text)
+nameservers = list(nameservers)
+_extend_with_unique(nameservers, old_nameservers)
+
+lines = ['# Generated by VDSM version ' + dsaversion.raw_version_revision]
+for words in _parse_lines(file_text):
+if words[0] == 'nameserver':
+if nameservers:
+ns = nameservers.pop(0)
+lines.append('nameserver ' + ns)
+else:
+lines.append(' '.join(words))
+
+with open(DNS_CONF_FILE, 'w') as f:
+return f.writelines(line + '\n' for line in lines)
+
+
+def _extend_with_unique(nameservers, old_nameservers):
+while len(nameservers) < MAXNS and old_nameservers:
+ns = old_nameservers.pop(0)
+if ns not in nameservers:
+nameservers.append(ns)


-- 
To view, visit https://gerrit.ovirt.org/62705
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I41b81b04fbe3e3e2398f28cb16fb0dbbc0382249
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org