#1729 Support multiple ostree installers
Opened a year ago by lsedlar. Modified a year ago

file modified
+29 -20
@@ -1137,27 +1137,36 @@ 

                  "additionalProperties": False,

              },

              "ostree_installer": _variant_arch_mapping(

-                 {

-                     "type": "object",

-                     "properties": {

-                         "repo": {"$ref": "#/definitions/repos"},

-                         "release": {"$ref": "#/definitions/optional_string"},

-                         "failable": {"$ref": "#/definitions/list_of_strings"},

-                         "installpkgs": {"$ref": "#/definitions/list_of_strings"},

-                         "add_template": {"$ref": "#/definitions/list_of_strings"},

-                         "add_arch_template": {"$ref": "#/definitions/list_of_strings"},

-                         "add_template_var": {"$ref": "#/definitions/list_of_strings"},

-                         "add_arch_template_var": {

-                             "$ref": "#/definitions/list_of_strings"

+                 _one_or_list(

+                     {

+                         "type": "object",

+                         "properties": {

+                             "repo": {"$ref": "#/definitions/repos"},

+                             "release": {"$ref": "#/definitions/optional_string"},

+                             "failable": {"$ref": "#/definitions/list_of_strings"},

+                             "installpkgs": {"$ref": "#/definitions/list_of_strings"},

+                             "add_template": {"$ref": "#/definitions/list_of_strings"},

+                             "add_arch_template": {

+                                 "$ref": "#/definitions/list_of_strings"

+                             },

+                             "add_template_var": {

+                                 "$ref": "#/definitions/list_of_strings"

+                             },

+                             "add_arch_template_var": {

+                                 "$ref": "#/definitions/list_of_strings"

+                             },

+                             "rootfs_size": {"type": "string"},

+                             "template_repo": {"type": "string"},

+                             "template_branch": {"type": "string"},

+                             "extra_runroot_pkgs": {

+                                 "$ref": "#/definitions/list_of_strings"

+                             },

+                             "skip_branding": {"type": "boolean"},

+                             "subvariant": {"type": "string"},

                          },

-                         "rootfs_size": {"type": "string"},

-                         "template_repo": {"type": "string"},

-                         "template_branch": {"type": "string"},

-                         "extra_runroot_pkgs": {"$ref": "#/definitions/list_of_strings"},

-                         "skip_branding": {"type": "boolean"},

-                     },

-                     "additionalProperties": False,

-                 }

+                         "additionalProperties": False,

+                     }

+                 )

              ),

              "ostree_use_koji_plugin": {"type": "boolean", "default": False},

              "ostree_container_use_koji_plugin": {"type": "boolean", "default": False},

@@ -130,7 +130,9 @@ 

  

          filename = compose.get_image_name(arch, variant, disc_type=disc_type)

          self._copy_image(compose, variant, arch, filename, output_dir)

-         self._add_to_manifest(compose, variant, arch, filename)

+         self._add_to_manifest(

+             compose, variant, arch, filename, config.get("subvariant", variant.uid)

+         )

          self.pool.log_info("[DONE ] %s" % (msg))

  

      def _clone_templates(self, compose, url, branch="master"):
@@ -161,7 +163,7 @@ 

          except OSError:

              shutil.copy2(boot_iso, iso_path)

  

-     def _add_to_manifest(self, compose, variant, arch, filename):

+     def _add_to_manifest(self, compose, variant, arch, filename, subvariant):

          full_iso_path = compose.paths.compose.iso_path(arch, variant, filename)

          iso_path = compose.paths.compose.iso_path(

              arch, variant, filename, relative=True
@@ -178,7 +180,7 @@ 

          img.disc_number = 1

          img.disc_count = 1

          img.bootable = True

-         img.subvariant = variant.uid

+         img.subvariant = subvariant

          img.implant_md5 = implant_md5

          setattr(img, "can_fail", self.can_fail)

          setattr(img, "deliverable", "ostree-installer")

This PR updates the config schema to support multiple configurations for ostree installer, and allows to tell them apart with subvariant field. The code itself already supports running multiple tasks.

could we possibly just have the ostree_installer phase config use the same layout as most of the other phases? i.e. a dict whose keys are variants and values are lists of dicts which specify arches and subvariants, like the image_build and live_media phases. this "list-of-2-tuples" approach is weirdly different and I at least find it hard to get my head around.

edit: to be clear the problematic design already exists, but this PR seems like an opportunity to change it. of course it's probably more disruptive.

Hm, that is an interesting idea. I like it. The list of arches would then either be inside the dict, or implicitly derived from variant architectures.

It would require ability to insert current architecture into the lorax template strings, but that doesn't seem complicated (or maybe it's possible with --add-arch-template-var, though I don't know how exactly that works).

It could look like this maybe?
https://pagure.io/fork/lsedlar/pungi-fedora/c/32e9a9416ff8f6c69e243e37f9a321c737245dad?branch=tentative-config-change

Is this what you have in mind?

I like that a lot. It seems much more sane and a lot less repetitive.
It could be done in a backwards compatible manner too (except for the arch interpolation, that could potentially conflict if someone is using the same syntax, which I very much doubt someone actually does).

And then when with all images merged under one variant:
https://pagure.io/fork/lsedlar/pungi-fedora/c/c85ae16999df735aa633ef247485594da3f599eb?branch=tentative-config-change (variant definition not included)

There may be a problem with having all the images in one variant though. The ISO is not the only output, there's also the boot configuration:
https://kojipkgs.fedoraproject.org/compose/branched/latest-Fedora-40/compose/Kinoite/x86_64/os/
https://kojipkgs.fedoraproject.org/compose/branched/latest-Fedora-40/compose/Onyx/x86_64/os/
https://kojipkgs.fedoraproject.org/compose/branched/latest-Fedora-40/compose/Silverblue/x86_64/os/
If the images are in one variant, they are going to overwrite the files from other subvariants, and an arbitrary one will win. This should probably be somehow solved (some namespacing based on subvariant? not copying these files at all if subvariant is different from main variant? something else?)

The list of arches would then either be inside the dict, or implicitly derived from variant architectures.

Well, yeah, that's how it is for other phases. In both image_build and live_media phases, the lowest-level dicts describing the images themselves have arches keys. Indeed, even in the ostree phase, this is the design. AFAICS, ostree_installer is the only phase that does it this way, with tuples.

Is this what you have in mind?

Yup, pretty much. Is the 'templating' of the arch values like that possible? Maybe those being arch-specific was the reason it was designed this way?

If the images are in one variant, they are going to overwrite the files from other subvariants, and an arbitrary one will win. This should probably be somehow solved (some namespacing based on subvariant? not copying these files at all if subvariant is different from main variant? something else?)

Yeah, I see the issue. I guess the question is do we really want/need to ship that stuff for the Atomic Desktop variants. I believe it's there to support various somewhat 'unusual' install scenarios, like PXE installs and direct-kernel-boot installs. If supporting those is in scope, they might not want this design. I guess all we can do is ask them - I'll post back on the original ticket asking for input.

I kinda like your idea of "only copy the files if variant and subvariant match", though it would have to be clearly documented somewhere as it's kinda magic behaviour; that would allow us to still potentially have ostree installer variants where we do publish all that stuff, if we want to have them in a different context.

Also CCing @kevin , as he may be able to think of some other reason why we would or wouldn't need the boot configuration stuff.

I would defer to ostree experts, I am not really sure myself...

I don't think the question really relates to ostree payloads. IIUC, anyway. I think the question boils down to "what possibilities do we lose if we don't ship the os/ directory?", because that's waht we'd actually lose.