Skip to content

Keep user multipath configuration upon upgrade #52

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.

Sign up for GitHub

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

Jump to bottom
Open
benjamreis wants to merge 99 commits into
base: master
Choose a base branch
from
Open

Keep user multipath configuration upon upgrade #52

benjamreis wants to merge 99 commits into from

Conversation

@benjamreis
Copy link
Contributor

@benjamreis benjamreis commented Jul 17, 2023 •
edited
Loading

All files matchin custom.*\.conf will be kept upon upgrade.

See: xapi-project/sm#600

@stormi
Copy link
Contributor

stormi commented Jul 17, 2023

Would be better with a word of context: since PR xapi-project/sm#600, there is text in the multipath/custom.conf file which says:

# Changes made to this file will not be overwritten by future system updates.
# They will also be retained through system upgrades to newer releases.

And it was agreed upon that we would make it become true in the installer.

The current PR does this.

stormi
stormi reviewed Jul 17, 2023
View reviewed changes
upgrade.py Outdated
self.restore_list += ['etc/nagios/nrpe.cfg', {'dir': 'etc/nrpe.d'}]

# Keep user multipath configuration
self.restore_list += [{'dir': 'etc/multipath/conf.d'}]
Copy link
Contributor

@stormi stormi Jul 17, 2023

Choose a reason for hiding this comment

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

Here we need to choose between either saving the whole directory, or just custom.conf. Saving the whole directory may be a perk when/if vendors start taking the habit of dropping a file of theirs into this directory when needed.

On the other hand, if we want to keep the liberty of packaging configuration files in /etc/multipath/conf.d, then it may be safer to just add etc/multipath/conf.d/custom.conf to the list.

Copy link
Contributor

@ydirson ydirson Oct 13, 2023

Choose a reason for hiding this comment

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

I'd say vendors should deploy such files using a RPM anyway, no?

Copy link
Contributor

@ydirson ydirson Oct 13, 2023

Choose a reason for hiding this comment

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

what about saving custom*.conf to give users more flexibility and still avoid issues with proper RPMs?

@benjamreis benjamreis force-pushed the restore-multipath-conf branch 2 times, most recently from cb14751 to 08059cf Compare October 16, 2023 08:20
@benjamreis
Copy link
Contributor Author

benjamreis commented Oct 16, 2023

Hi, we reduced to a specific regex to only save custom config files to avoid conflic with RPM installed files.

What do you guys think? :)

@benjamreis benjamreis mentioned this pull request Oct 16, 2023
Merged
@alexbrett
Copy link
Contributor

alexbrett commented Oct 24, 2023

From the sm PR, it seems we state in multipath.conf:

# For custom multipath configuration, create a separate .conf file in the
# /etc/multipath/conf.d/ directory.

However, this change only preserves files that match custom*.conf, which is not quite the same. If people have found the custom.conf file they may see the text in that which says it'll be preserved, but if they have only looked at multipath.conf, and then created e.g. /etc/multipath/conf.d/my_new_file.conf, they might still expect that to be preserved based on the text in multipath.conf.

As such it seems to me the text in multipath.conf should either say e.g. create a separate .conf file in the /etc/multipath/conf.d/ directory. Note filenames beginning with the name custom, will be preserved during system updates/upgrades (or something worded slightly better to that effect), or we should preserve everything, otherwise we risk confusing people?

@ydirson
Copy link
Contributor

ydirson commented Oct 24, 2023

@alexbrett the question would be, would it be valid for eg. an OEM to install files there though a RPM? That's a typical use for the "conf.d" idiom, besides letting user modifications not interfere with system updates.
If we consider that for this particular case it makes no sense (I'm myself not familiar enough with the matter), then fine. But we could also decide to restrict the liberty given in multipath.conf (which incidently people may already have used, making this path maybe harder to choose...)

alexbrett and others added 21 commits October 24, 2023 16:22
@alexbrett
XSI-1498: Log any exceptions parsing existing bootloader config
0e4b38d 
Previously any exceptions which occurred were silently suppressed, instead log
such occurrences to enable easier debugging from the installer log.

Signed-off-by: Alex Brett <alex.brett@cloud.com>
@alexbrett
CA-385350 Fix handling of NTP configuration over upgrade
09f0562 
Previously we were not setting `ntp-config-method` when parsing the NTP
configuration from an old version, resulting in any NTP servers that were read
being ignored.

Set the appropriate `ntp-config-method`, and also filter out any 'default' NTP
servers from the `ntp-servers` list so we only load customer specified servers.
Note this is done using a fixed list of default domains, as this has changed in
the past and may change in future, so cannot be read from the current release
config.

Signed-off-by: Alex Brett <alex.brett@cloud.com>
@freddy77
CA-384621: Do not move partition to preserve data
07c1a7d 
During installation/upgrade we remove all partitions and then create
new partitions as required. This is done also for partitions that
needs to be preserved (like Dell utility partition).
By default partition utilities tries to align start of the
partitions for better performance but this causes partitions with
data to be recreated with different start/end corrupting them.
To avoid the corruption do the alignment from our code when
we create new partitions and avoid alignment done in the utilities
(adding "--set-alignment=1" parameter).
As a minor change create a single command to create and setup
a single partition instead of using multiple commands.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Use explicit integer division where necessary
7f88db7 
By default in Python 3 a division, even between integers, is done
using floating point. This causes the result to be float and potentially
reduce precision (float precision is 52 bits).
To avoid changes in behaviour between Python 2 and Python 3 and
possible precision losses use explicit integer divisions that will
produce same results.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@stormi @freddy77
Include mmcblk-devices in disk selection
d974caf 
Originally-by: Jöran Malek <alivedevil@users.noreply.github.com>
Signed-off-by: Samuel Verschelde <stormi@laposte.net>
Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
@freddy77
Use constant octal syntax compatible with Python3
c6238e3 
This change reduces the difference between Python2 and Python3
making easier to cherry pick or rebase commits from a branch to
another.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@GeraldEV
CP-37929: Remove legacy logrotate mechanism
6a7b6f1 
Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@citrix.com>
@freddy77
CP-44441: Restore SNMP service config
3cf696b 
Signed-off-by: Deli Zhang <deli.zhang@citrix.com>
@rosslagerwall
CP-47538: Deprecate legacy BIOS boot
417078c 
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
@rosslagerwall @freddy77
CP-45553: Add library for shrinking LVM devices
9bffd2e 
In preparation for supporting partition layout upgrades with data on the
local SR, add a library for shrinking LVM devices. Shrinking from the
start and the end is supported.

This is implemented from scratch since the LVM tools lack support for
moving the start of an LVM device. To avoid needing to test too many
different options, only a limited subset of LVM is supported (e.g. only
a single PV, segments must have a single stripe, etc.).

Co-authored-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
@rosslagerwall @freddy77
CP-45554: Implement support for upgrading from old MBR to new GPT layout
2d15318 
This adds support for upgrading an installation with an old MBR
partition layout (with optional utility partition) to the new GPT
partition layout. Unlike with previous partition layout upgrades, this
can be performed with VDIs in the local SR (only supported for LVM).

The basic algorithm is as follows:

1) Add a new convertTarget() step before the backup step of the
   upgrader.
   The convertTarget() step calculates how much extra space is needed
   for the new layout and then shrinks the LVM SR accordingly. This step
   may take some time as it needs to move up to 33.5 GiB of data.

2) In the backup step, it replaces the old backup partition with a
   larger, aligned one and then takes a backup.

3) In the prepareTarget step, it replaces the root partition with an
   aligned log partition, and creates the rest of the required
   partitions in the space that was freed from the beginning of the
   local SR.

This upgrade support is activated by the presence of a
/var/preserve/safe2mbrupgrade file in the root filesystem of the
existing installation.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
@freddy77
Update probeDisk function documentation
2eeb155
@freddy77
Remove unused variable
e76eb11
@freddy77
Remove unused parameter from probeDisk
89d2193
@freddy77
Reuse constants.logsfs_label_prefix
07c8fb4
@freddy77
Return an object from probeDisk, not a tuple of tuples
f765104 
Most of the callers use a minimal set of the result and all these
results make adding new result more complicated.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Rename inspectTargetDisk function
d277ab8 
The name of this function is misleading.
Is not really inspecting the partition but preparing the partitions
based on answers given.
It should not be used to discovery the current layout (which is done
for instance in restore.py calling this function).
This causes some possible corruption (for instance if there is
a preserved partition but the preserve flag was not specified).

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Allows to restore really old layout used by NetScaler SDX machines
e9d686a 
This change allows to restore SDX machines updated to XenServer 8.
SDX still uses MBR and 3 partition schema (plus one possible
utility partition). The possibility to restore this schema
was removed from installer time ago but we need to add back
to support restore of SDX products.
After restoring backup data to logs (which used to be the root
partition) all additional partitions (EFI boot, swap, logs) not
necessary are removed, backup recreated and partition schema
is moved from GPT to MBR.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@rosslagerwall
CP-45747 Allow 4KN-capable SR type to be used
6387fe6 
The 'lvm' and 'ext' SR types will not work with 4K native disks - or
indeed any where the logical block size is not 512 bytes - but if a
suitable alternative SR type is available then this can be used instead.

* The feature flag file "large-block-capable-sr-type" may now be used to
  specify the name of an SR type that can be used for local storage on
  disks where the logical block size can be something other than 512
  bytes.
* The installer detects logical block size at points in the process
  this is necessary.
* For interactive installs, there are now separate screens for selecting
  which disks to use for local SR storage and determining what SR type
  to use, so that the options for the latter can depend on the former.
* Tweak the disk-probing logic so that it is easy for a new SR type to
  name volume groups such that the installer can detect them.

For non-interactive installs:
* Sanity checking to prevent 'lvm' or 'ext' being used on conjunction
  with disks where they won't work.
* Allow the use of an available 4KN-capable SR type even if the disk
  block size does not make this necessary.
* If the answer file doesn't specify an SR type, try to default to one
  that will work, given the block size.

Signed-off-by: Robin Newton <robin.newton@cloud.com>
@rosslagerwall
CP-45747 Minor wording tweaks for local storage screens
81047f8 
- Fixed grammatical error in message shown when a large block SR type is
  needed but isn't available
- Bring the message on the disk selection screen into line with house
  style.

Signed-off-by: Robin Newton <robin.newton@cloud.com>
@rosslagerwall
CP-47742 Allow non-4kn-capable SR type use
05be3fc 
In the situation where there is no 4kn capable SR type available (which
is to say, when there is no large-block-capable-sr-type feature file),
but one is needed, remove the block on lvn or ext being used. This is
for consistency with previous releases.

Signed-off-by: Robin Newton <robin.newton@cloud.com>
ydirson and others added 28 commits October 28, 2024 09:41
@ydirson @freddy77
restore_file: make a single lookup of 're' in dict
103440d 
Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
@ydirson @freddy77
restore_file: when no re is given for a dir, preserve whole dir in on…
1095053 
…e go

If the specified dir exists but is empty (as can happen for
/etc/stunnel/certs), the current code would not restore it, as in fact
it restores not the dir, but its immediate children matching the pattern.

When no pattern is given, avoid complications by using the fact that
restore_file perfectly works with directories too.

Note this makes it prominent that the 're' handling is only ever applied
to immediate children of the dir; we may simplify further by moving the
pattern-matching code down to restore_file, but then do we want to keep
the current behavior?

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
@freddy77
Change buildRestoreList to return list of files
db1bf25 
No reason to store the list in the upgrade object.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Dynamically compute layout string based on partition availability
179e42f 
Allows to exclude some partitions from the layout.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Use partition numbers to sort partitionTargetDisk output
983c451 
partitionTargetDisk returns a tuple with boot mode and all
partition numbers. Sort the returned partitions based on expected
order.
No change in functionality.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Remove support for very old Xapi DB name
c36d778 
That name was used in 2012, probably version 5.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Add a comment for ID_LINUX partition GUID
934d166 
The name is currently misleading.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Simplify results from partitionTargetDisk
020b868 
Instead of coding lot of logic in the "return" line use a
"part_nums" list.
This will make easier to change the partition layout avoiding
long and hard to understand lines.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Start adding support for "target-platform" configuration
01f5af0 
Parse "target-platform" from answer file and store it.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Allows to avoid some partitions during installation
e20f8ca 
If log partition won't be specified simple directory will be used.
If swap partition won't be specified a file will be used.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Allows automatic conversion of some partitions to GPT
3ec7596 
Newer Gdisks returns different GUID for Linux, handle them.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
CP-51857: Support clean install for SDX 8900 platform
670ca91 
These machine have limited disk space so reuse the old root+backup
as new root.
Change some "constants" variable and use them.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
CP-51858: Support upgrades for SDX 8900 platform
6ca7ba2 
These machine have limited disk space so reuse the old root+backup
as new root.
As the layout have no backup partition store the required files
for the upgrade in a directory under /tmp (tmpfs during installation,
the size is less than 20 mb).
For this upgrade we don't change the storage as other SDX platforms.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
Simplify partition order computation upgrading partition schema
0a48ce6 
It was not clear the offset.
This will allow to easily avoid some partitions if needed.
Also, is more similar to writeDom0DiskPartitions in backend.py.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@freddy77
CA-403412: Do not backup GPT layout on temporary directory
e4ca591 
If the backup is done into the temporary directory we don't
be able to use it on failure so don't do it.
This also fixes a problem of failure doing such backup; this
occurred in an installation with backup GPT correct but invalid
primary GPT.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@chunjiez
CA-400058: fix preserve-first-partition installation
86347fc 
  if preserve-first-partition is yes in answer file, then
  installer fails with an IndexError because it tries to
  access a partition which does not exist

Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com>
@liulinC
CP-50298: Support restoring dir to specified mode and user/group
bcea46d 
attributes

Signed-off-by: Deli Zhang <deli.zhang@citrix.com>
(cherry picked from commit 0ccfe46)
@liulinC
CA-407867: Add missed "attr" parameter to restore_file()
ff102e1 
Signed-off-by: Deli Zhang <deli.zhang@cloud.com>
@chunjiez
revert "CA-400058: fix preserve-first-partition installation"
457b7ed 
This reverts commit 86347fc

Signed-off-by: Chunjie Zhu <chunjie.zhu@cloud.com>
@liulinC @freddy77
CP-47621: Enable ability to set hostname from DHCP
78dfd0c 
According to https://www.freedesktop.org/software/systemd/man/latest/hostname.html
Systemd set hostname with following sequence
- kernel parameter, systemd.hostname
- static hostname in /etc/hostname
- transient hostname like DHCP
- localhost at systemd compile time

Set static hostname `localhost.localdomain` in /etc/hostname
prevents obtain hostname from DHCP, and all XenRT installed host
got localhost.localdomain as the hostname

This commit set empty static hostname to permit DHCP

Signed-off-by: Lin Liu <lin.liu@citrix.com>
(cherry picked from commit e39f268)
@ydirson @freddy77
Makefile: give more flexibility to select base multipath.conf
3c6b1ff 
Useful to take that file from a source tree rather than a RPM
deployed in /.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
(cherry picked from commit 96db5f4)
@ydirson @freddy77
doc/answerfile: clarify and fix description of --network_device
074bd0a 
Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
(cherry picked from commit d1fcdcf)
@ydirson @freddy77
__mkinitrd: pass -f to dracut
bdd4872 
Before 5abd998 this code was calling
grubby's new-kernel-pkg, which unconditionally passes -f to mkinitrd.
Not using -f will cause install failure when a driver disk's rpm's
scripts have generated an initrd already (as is the case with the
intel igb driver)

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
(cherry picked from commit 0c0e139)
@ydirson @freddy77
Avoid attempting to re-read inventory file from unmounted partition
b4eb53d 
e9d686a triggers an unmount of backup_fs
before we're done with it, causing a restoration failure in 10.10.25:

 INFO     [2025-03-19 14:33:50] INSTALL FAILED.
 INFO     [2025-03-19 14:33:50] A fatal exception occurred:
 INFO     [2025-03-19 14:33:50] Traceback (most recent call last):
   File "/opt/xensource/installer/install.py", line 274, in go
     restore.restoreFromBackup(backup, progress)
   File "/opt/xensource/installer/restore.py", line 214, in restoreFromBackup
     branding = util.readKeyValueFile(os.path.join(backup_fs.mount_point, constants.INVENTORY_FILE))
   File "/opt/xensource/installer/util.py", line 322, in readKeyValueFile
     f = open(filename, "r")
 IOError: [Errno 2] No such file or directory: '/tmp/restore-backup-hrJwAR/etc/xensource-inventory'


Fortunately that data we want was already loaded, so this is a minimal fix.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
@ydirson @freddy77
doc/answerfile: reduce admin-interface confusion
d24b832 
Make the optional attributes description immediately follow the
mandatory ones, instead of getting inserted between ipv4 and ipv6
elements, where it just creates confusion.

Signed-off-by: Yann Dirson <yann.dirson@vates.tech>
(cherry picked from commit 2140091)
@GeraldEV
XSI-1863: Use a disk size of zero if a disk size cannot be determined
6e8cbf6 
getDiskDeviceSize may fail to find the correct disk size, which could
lead to the Python default of None being returned - in turn causing
larger issues later in the installer where a numeric type is expected.

Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
@freddy77
XSI-1887: Fix upgrade
1bf7cc9 
Due to missing initialisation during a manual upgrade the installer
was failing due to missing key in "answers" dictionary.
Provides the missing default to avoid the missing key.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
@benjamreis @ydirson
Keep user multipath configuration upon upgrade
6c5cafe 
All files matchin `custom.*\.conf` will be kept
upon upgrade.

See: xapi-project/sm#600

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>
@ydirson
Copy link
Contributor

ydirson commented May 5, 2025

@alexbrett the question would be, would it be valid for eg. an OEM to install files there though a RPM? That's a typical use for the "conf.d" idiom, besides letting user modifications not interfere with system updates. If we consider that for this particular case it makes no sense (I'm myself not familiar enough with the matter), then fine. But we could also decide to restrict the liberty given in multipath.conf (which incidently people may already have used, making this path maybe harder to choose...)

Can we make this go forward with this?

@ydirson ydirson force-pushed the restore-multipath-conf branch from bb6f238 to 6c5cafe Compare May 12, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarkSymsCtx MarkSymsCtx Awaiting requested review from MarkSymsCtx

2 more reviewers

@stormi stormi stormi left review comments

@ydirson ydirson ydirson left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.