Handle IPv6 #4
Handle IPv6 #4
Conversation
|
Here's a screenshot displaying an ipv6 address |
|
The way IP6 is handled varies: sometimes the code branches at the top, and at other times on a per-statement basis using if-expressions. It also treats IP6 as a boolean where it is by nature more an enumeration and we could envision more cases (albeit not short term). It's difficult to do this in a systematic and nice way - so I think it's ok. |
benjamreis
force-pushed
the
adapt-for-ipv6
branch
2 times, most recently
from
77ff359 to
dd2b06d
Compare
|
Hey, is something missing for this PR to be merged? It's been approved a while ago. |
xapi-project/xsconsole#4 Signed-off-by: BenjiReis <[email protected]>
xapi-project/xsconsole#4 Signed-off-by: BenjiReis <[email protected]>
bernhardkaindl
dismissed
their stale review
The changes I requested have been addressed
There was a problem hiding this comment.
@benjamreis This PR has commit conflicts which have to be resolved, can you fix them?
benjamreis
force-pushed
the
adapt-for-ipv6
branch
from
f16f4d1 to
d998737
Compare
|
@bernhardkaindl rebased on latest master 👍 |
|
Ping @bernhardkaindl. Is there anything blocking the merge now? |
|
Hi @stormi, I asked about it, and the response was that we shall review it as part of our larger IPv6 project: Then when we'll have the requirements for handling IPv6 finished and the infrastructure for full IPv6 testing in all variants (single-stack, dual-stack and the required IPv6 address configuration variants) is available for testing. |
|
Thanks for asking. I'm disappointed by the answer, to be honest. It makes it sound like XenServer is the only user of xsconsole. XCP-ng 8.3 (development version) already supports IPv6. In fact, we have been adding support in many components, and uncovering and fixing issues for you, which will make your own IPv6 project easier. We've worked on it for several years, have users testing it, are currently adding tests in our CI, uncovering more issues in the process. Here, all we ask is that a rather simple PR is merged so that we don't have to maintain a separate branch of xsconsole (and rebase each time the master branch changes - and there might be a python 3 port on the way), for months if not years. The PR is simple, was reviewed, refined, and moreover should not impact XenServer in any way when IPv6 is not enabled in XAPI. We're already 2 years 1/4 after the initial commits. If any regression is found in your IPv4-based tests that our own tests haven't uncovered, we can fix it, and you can revert the PR while waiting for the fix. When you start working on IPv6 support, you can then have a new look at what has been done, and determine whether it's good enough - as it seemed when merging -, or needs to be improved. |
benjamreis
force-pushed
the
adapt-for-ipv6
branch
4 times, most recently
from
b2d465c to
b990b4e
Compare
benjamreis
force-pushed
the
adapt-for-ipv6
branch
from
2686f9a to
e792a7a
Compare
benjamreis
force-pushed
the
adapt-for-ipv6
branch
3 times, most recently
from
887219a to
63501c8
Compare
Display IPv6 PIF's fields when primary_address_type is IPv6 Reconfigure management interface with appropriate method Support network reset in the IPv6 case Signed-off-by: Benjamin Reis <[email protected]>
benjamreis
force-pushed
the
adapt-for-ipv6
branch
from
63501c8 to
ed1d28c
Compare
|
This should be reviewed by @alexbrett. All in all, I would like this more if it would employ OO to handle IPv4/IPv6 more uniformly. |
| if inPIF['primary_address_type'].lower() == 'ipv4': | ||
| self.session.xenapi.PIF.reconfigure_ip(inPIF['opaqueref'], inMode, inIP, inNetmask, inGateway, FirstValue(inDNS, '')) | ||
| if inPIF['ipv6_configuration_mode'].lower() == 'static': |
There was a problem hiding this comment.
The expressions in the two ifs can raise exceptions when the keys are not defined, please add presence checks before checking the contents.
| @@ -12,7 +12,7 @@ def test_min(self): | |||
| self.assertTrue(IPUtils.ValidateIP('0.0.0.1')) | |||
|
|
|||
| def test_beyond_min(self): | |||
| self.assertFalse(IPUtils.ValidateIP('0.0.0.0')) | |||
| self.assertTrue(IPUtils.ValidateIP('0.0.0.0')) | |||
There was a problem hiding this comment.
Why is this test being changed?
There was a problem hiding this comment.
Because changing the validate method hasa changed how 0.0.0.0 is perceived valid by socket.inet_pton
There was a problem hiding this comment.
Doesn't other code depend on this property?
| # Rewrite firstboot management.conf file, which will be picked it by xcp-networkd on restart (if used) | ||
| f = open(management_conf, 'w') | ||
| try: | ||
| f.write("LABEL='" + self.device + "'\n") | ||
| f.write("MODE='" + self.mode + "'\n") |
There was a problem hiding this comment.
This will get hairy with dual-stack ip, it needs a rethinking when that is introduced so both ipv4 and ipv6 can b written independently. As it is, it will get difficult to read with 3 modes.
Display IPv6 PIF's fields when
primary_address_typeisIPv6Reconfigure management interface with appropriate method
Support network reset in the IPv6 case
Signed-off-by: BenjiReis [email protected]