Upgrades: use the management address type from old install #41

gthvn1 · 2024-09-23T17:01:01Z

No description provided.

util.py

gthvn1 · 2024-09-24T14:33:28Z

It is really strange. When doing a fresh install using IPv6 static only my installation ends with IPv4. And the /etc/xensource-inventory has:

MANAGEMENT_INTERFACE=''
MANAGEMENT_ADDRESS_TYPE='IPv4'

I will add some logs to understand why it doesn't work...

backend.py

gthvn1 · 2024-09-25T08:11:00Z

It is really strange. When doing a fresh install using IPv6 static only my installation ends with IPv4. And the /etc/xensource-inventory has:
MANAGEMENT_INTERFACE=''
MANAGEMENT_ADDRESS_TYPE='IPv4'
I will add some logs to understand why it doesn't work...

So the issue was as expected because the management address type is always set. By default it is set to IPv4. So we use it only when no modes are selected (and it is the case during an upgrade for example). I tested a fresh install and in this case we are either in IPv4 mode or IPv6 mode. If the mode is IPv6 only we are in IPv6 and if it is set to both we use IPv4. And as said during an upgrade we are using the one set in /etc/xensource-inventory.
I tested:

fresh install in IPv6 (static and autoconf)
fresh install in IPv4 (dhcp)
upgrade from IPv6 static

stormi · 2024-09-25T10:05:08Z

Adding Yann to the review. Please all review this very carefully, as we don't want to introduce a regression at this stage of the release. If we doubt we can guarantee this, then we'll take more time for testing this change and release without. We need to decide today and package the updated installer in the afternoon.

An upstream PR is needed, too.

ydirson

The logic is not obvious at first read, it would be useful to give a few details about the change in the commit message

ydirson · 2024-09-25T11:00:55Z

backend.py

+    # if no modes are configured then use mgmtAddrType
+    if not (admin_config.mode or admin_config.modev6):
+        inv.write("MANAGEMENT_ADDRESS_TYPE='%s'\n" % mgmtAddrType)
    # Default to IPv4 unless we have only got an IPv6 admin interface
-    if ((not admin_config.mode) and admin_config.modev6):
+    elif (not admin_config.mode) and admin_config.modev6:


wouldn't it be more readable to have a single if not admin_config.mode around all of this?

I think that cases are more readable like this but I can modify it no worries.

maybe with the updated logic we can still use elif to get get things more readable?

ydirson

Not really sure to get it:

mgmtAddrType() seems to be used to set a default value, but only for install (would be useful to make it explicit in the code)
prepareUpgrade() states MANAGEMENT_ADDRESS_TYPE must be present in xensource-inventory , but looks at the default value (as well as the uuids!?), even though exceptions are rised if they cannot get overriden by getInventoryValue() calls?

stormi · 2024-10-17T09:54:18Z

Unless I'm wrong, I believe this PR is waiting for @gthvn1 after Yann's comments.

gthvn1 · 2024-10-17T11:06:46Z

Unless I'm wrong, I believe this PR is waiting for @gthvn1 after Yann's comments.

Yes right I missed it. But I'm not sure to understand the remark. As far as I understand it we will always in the case where no mode is selected we must have a value in the inventory from previous install so it will be overwritten with the value found in the inventory. And as the default case is "IPv4" I set it by default but yes maybe it is not needed. I don't remember exactly but I think when I tested it it didn't work without this definition.

gthvn1 · 2024-10-17T11:13:42Z

I will give it a try and keep you in touch. Probably today.

gthvn1 · 2024-10-17T14:53:31Z

I will give it a try and keep you in touch. Probably today.

So when removing def mgmtAddrType(): from util.py I have an error because "module" object has no attribute. So I added it. And without my patch the issue is that when upgrading an IPv6 only host the /etc/xensource-inventory has MANAGEMENT_ADDRESS_TYPE=IPv4...
@ydirson any ideas how to fix it?
In fact I'd like to follow what is done for "controlID" but for some reason it doesn't work. I'm sure I'm doing something wrong...

gthvn1 · 2024-10-17T15:12:25Z

@ydirson Ok I found why I added the def mgmtAddrType. It is because of Task(util.mgmtAddrType, As(ans), ['management-address-type']),. And I did that because 'management-address-type' was added to Task(writeInventory... So I followed what is done for 'control-domain-uuid' but as I cannot use the same Task(util.getUUID, As(ans), ['control-domain-uuid']), I created one for management address...

The two parameters are read from the xensource inventory so no need to pass them as parameters of the prepareUpgrade function. Signed-off-by: Guillaume <[email protected]>

During an upgrade if no modes are selected we need to use the management address from the previous installation read from /etc/xensource-inventory. By default it is set to IPv4. If a mode is selected then we are using it. It both modes are selected (IPv4 abd IPv6) then we are using IPv4. Signed-off-by: Guillaume <[email protected]>

gthvn1 · 2024-10-18T16:44:44Z

I have splitted the PR in two commits. The first commit can be upstreamed.

gthvn1 · 2024-10-18T16:46:10Z

testing upgrade from IPv6 only
testing upgrade from IPv4 only
test a fresh install with IPv6
- Only tested using autoconf. Tried with dhcp but as our dhc6p server is not setup properly I'm not sure if the error is caused by our setup...
test a fresh install with IPv4

stormi · 2024-10-29T11:08:17Z

I have split the PR in two commits. The first commit can be upstreamed.

The second commit can also be upstreamed as part of Benjamin's IPv6 PR, can't it?

gthvn1 · 2024-10-29T12:40:41Z

Ah yes maybe 👍

gthvn1 changed the base branch from master to 10.10.19-8.3 September 23, 2024 17:01

gthvn1 changed the base branch from 10.10.19-8.3 to 10.10.16-8.3 September 23, 2024 17:02

benjamreis reviewed Sep 24, 2024

View reviewed changes

util.py Outdated Show resolved Hide resolved

gthvn1 marked this pull request as draft September 24, 2024 14:30

gthvn1 commented Sep 24, 2024

View reviewed changes

backend.py Show resolved Hide resolved

gthvn1 force-pushed the gtn-use-former-mgmtaddrtype branch from ecea49a to 21aa5cf Compare September 24, 2024 15:47

gthvn1 marked this pull request as ready for review September 25, 2024 08:06

gthvn1 requested a review from stormi September 25, 2024 08:07

stormi requested a review from ydirson September 25, 2024 10:01

stormi changed the title ~~use former mgmtaddrtype~~ Upgrades: use the management address type from old install Sep 25, 2024

ydirson requested changes Sep 25, 2024

View reviewed changes

gthvn1 force-pushed the gtn-use-former-mgmtaddrtype branch 2 times, most recently from 9c29926 to 265ed12 Compare September 25, 2024 12:07

ydirson reviewed Oct 7, 2024

View reviewed changes

gthvn1 changed the base branch from 10.10.16-8.3 to 10.10.19-8.3 October 17, 2024 11:36

gthvn1 force-pushed the gtn-use-former-mgmtaddrtype branch from 265ed12 to fd143d1 Compare October 17, 2024 11:36

gthvn1 added 2 commits October 18, 2024 17:41

Remove unused parameters passed to prepareUpgrade

e9082a1

The two parameters are read from the xensource inventory so no need to pass them as parameters of the prepareUpgrade function. Signed-off-by: Guillaume <[email protected]>

gthvn1 force-pushed the gtn-use-former-mgmtaddrtype branch from fd143d1 to f1169e6 Compare October 18, 2024 16:41

gthvn1 requested a review from ydirson October 18, 2024 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrades: use the management address type from old install #41

Upgrades: use the management address type from old install #41

gthvn1 commented Sep 23, 2024

gthvn1 commented Sep 24, 2024

gthvn1 commented Sep 25, 2024 •

edited

Loading

stormi commented Sep 25, 2024

ydirson left a comment

ydirson Sep 25, 2024

gthvn1 Sep 25, 2024

ydirson Oct 7, 2024

ydirson left a comment •

edited

Loading

stormi commented Oct 17, 2024

gthvn1 commented Oct 17, 2024

gthvn1 commented Oct 17, 2024

gthvn1 commented Oct 17, 2024 •

edited

Loading

gthvn1 commented Oct 17, 2024 •

edited

Loading

gthvn1 commented Oct 18, 2024

gthvn1 commented Oct 18, 2024 •

edited

Loading

stormi commented Oct 29, 2024

gthvn1 commented Oct 29, 2024

Upgrades: use the management address type from old install #41

Are you sure you want to change the base?

Upgrades: use the management address type from old install #41

Conversation

gthvn1 commented Sep 23, 2024

gthvn1 commented Sep 24, 2024

gthvn1 commented Sep 25, 2024 • edited Loading

stormi commented Sep 25, 2024

ydirson left a comment

Choose a reason for hiding this comment

ydirson Sep 25, 2024

Choose a reason for hiding this comment

gthvn1 Sep 25, 2024

Choose a reason for hiding this comment

ydirson Oct 7, 2024

Choose a reason for hiding this comment

ydirson left a comment • edited Loading

Choose a reason for hiding this comment

stormi commented Oct 17, 2024

gthvn1 commented Oct 17, 2024

gthvn1 commented Oct 17, 2024

gthvn1 commented Oct 17, 2024 • edited Loading

gthvn1 commented Oct 17, 2024 • edited Loading

gthvn1 commented Oct 18, 2024

gthvn1 commented Oct 18, 2024 • edited Loading

stormi commented Oct 29, 2024

gthvn1 commented Oct 29, 2024

gthvn1 commented Sep 25, 2024 •

edited

Loading

ydirson left a comment •

edited

Loading

gthvn1 commented Oct 17, 2024 •

edited

Loading

gthvn1 commented Oct 17, 2024 •

edited

Loading

gthvn1 commented Oct 18, 2024 •

edited

Loading