[ART-3883] multiple rhcos containers #618
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
sosiouxme
force-pushed
the
20220701-multi-rhcos
branch
from
f2f2f58
to
9898993
Compare
|
Thanks so much for working on this! I am not familiar with the codebase, but at a superficial level the changes LGTM. One other note; the current plan(ish) calls not just for potentially having Even further, I think things may be cleaner if we introduce a separate |
sosiouxme
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
sosiouxme
force-pushed
the
20220701-multi-rhcos
branch
from
9e1a9ef
to
b26f733
Compare
|
|
||
|
|
||
| def rhcos_content_tag(runtime) -> str: |
There was a problem hiding this comment.
turns out nothing references this; we may need to resurrect it somehow when RHCOS is building for rhel8 and rhel9.
sosiouxme
force-pushed
the
20220701-multi-rhcos
branch
from
b26f733
to
b644672
Compare
| @classmethod | ||
| def find_payload_entries(clazz, assembly_inspector: AssemblyInspector, arch: str, dest_repo: str) -> (Dict[str, PayloadEntry], List[AssemblyIssue]): |
There was a problem hiding this comment.
i broke this method up because it was impossible to unit test. i only ended up unit testing the new stuff but more tests are welcome :)
| for arch in runtime.arches: | ||
| if arch not in mosc_by_arch: | ||
| if arch not in rhcos_by_tag[list(rhcos_tag_names)[0]]: |
There was a problem hiding this comment.
list(rhcos_tag_names)[0] is nondeterministic. It could be any RHCOS tag name defined in group.yml.
Should we check all tags (or the primary tag)?
There was a problem hiding this comment.
currently they should all be from the same RHCOS builds so it shouldn't matter which we get. but admittedly this does look weird, and will be clearer if i just use the primary tag.
| else: | ||
| exit_with_error(f'Did not find machine-os-content image for active group architecture: {arch}') | ||
| exit_with_error(f'Did not find RHCOS container image for active group architecture: {arch}') |
There was a problem hiding this comment.
Would like to include the tag name in the error message.
There was a problem hiding this comment.
makes this easier too :)
|
|
||
| def get_container_names(runtime): | ||
| return {tag.name for tag in get_container_configs(runtime)} | ||
| """ |
| for tag in get_container_configs(runtime): | ||
| if tag.primary: | ||
| return tag | ||
| raise Exception("Need to provide a group.yml rhcos.payload_tags entry with primary=true") |
There was a problem hiding this comment.
Could be less generic, like ValueError:
| raise Exception("Need to provide a group.yml rhcos.payload_tags entry with primary=true") | |
| raise ValueError("Need to provide a group.yml rhcos.payload_tags entry with primary=true") |
There was a problem hiding this comment.
if i care at all what type of exception is raised, i'll generally define one specifically for it, so i can test for it. i don't think i care here...
| @@ -265,30 +265,30 @@ def _detect_rhcos_status(runtime, kubeconfig) -> list: | |||
| for arch in runtime.arches: | |||
| for private in (False, True): | |||
| name = f"{version}-{arch}{'-priv' if private else ''}" | |||
| tagged_mosc_id = _tagged_mosc_id(kubeconfig, version, arch, private) | |||
| tagged_rhcos_id = _tagged_rhcos_id(kubeconfig, rhcos.get_primary_container_name(runtime), version, arch, private) | |||
There was a problem hiding this comment.
Do we need to check other tags? Are they guaranteed to be updated at the same time?
There was a problem hiding this comment.
the same RHCOS build supplies all the containers in the payload, so we should only need to check one.
i'll need to make sure that continues to be the case when we re-introduce gating RHCOS inclusion on CI tests.
this will get more complicated when we have multiple RHCOS builds to include, but we'll cross that bridge later.
openshift-merge-robot
added
the
needs-rebase
| for arch in runtime.arches: | ||
| if arch not in mosc_by_arch: | ||
| if arch not in rhcos_by_tag[list(rhcos_tag_names)[0]]: |
There was a problem hiding this comment.
currently they should all be from the same RHCOS builds so it shouldn't matter which we get. but admittedly this does look weird, and will be clearer if i just use the primary tag.
| else: | ||
| exit_with_error(f'Did not find machine-os-content image for active group architecture: {arch}') | ||
| exit_with_error(f'Did not find RHCOS container image for active group architecture: {arch}') |
There was a problem hiding this comment.
makes this easier too :)
| @@ -265,30 +265,30 @@ def _detect_rhcos_status(runtime, kubeconfig) -> list: | |||
| for arch in runtime.arches: | |||
| for private in (False, True): | |||
| name = f"{version}-{arch}{'-priv' if private else ''}" | |||
| tagged_mosc_id = _tagged_mosc_id(kubeconfig, version, arch, private) | |||
| tagged_rhcos_id = _tagged_rhcos_id(kubeconfig, rhcos.get_primary_container_name(runtime), version, arch, private) | |||
There was a problem hiding this comment.
the same RHCOS build supplies all the containers in the payload, so we should only need to check one.
i'll need to make sure that continues to be the case when we re-introduce gating RHCOS inclusion on CI tests.
this will get more complicated when we have multiple RHCOS builds to include, but we'll cross that bridge later.
|
|
||
| def get_container_names(runtime): | ||
| return {tag.name for tag in get_container_configs(runtime)} | ||
| """ |
| for tag in get_container_configs(runtime): | ||
| if tag.primary: | ||
| return tag | ||
| raise Exception("Need to provide a group.yml rhcos.payload_tags entry with primary=true") |
There was a problem hiding this comment.
if i care at all what type of exception is raised, i'll generally define one specifically for it, so i can test for it. i don't think i care here...
sosiouxme
force-pushed
the
20220701-multi-rhcos
branch
from
b644672
to
4ac7f06
Compare
openshift-merge-robot
removed
the
needs-rebase
Build #10 |
https://issues.redhat.com/browse/ART-3883
We will need to configurably inject multiple containers from an RHCOS build into the payload. Eventually
machine-os-contentmay not be the reference container and may not be present at all. https://github.com/openshift/ocp-build-data/pull/1773/files shows how we might expect config to look for the first additional container. openshift-eng/ocp-build-data-validator#78 enablesrhel-coreos-8in assembly definitions.If the changes seem a bit much to review, it will likely be easier to review one commit at a time, as each represents a discrete change.