cosa2stream: Rework to better optimize openshift/installer flow #2052

cgwalters · 2021-02-22T19:00:26Z

When I was writing this originally I was focused on the goal
of converting cosa metadata to stream format, hence the name etc.

However upon trying to use this for openshift/installer I realized
something important: after we do the switch, the installer git
will be a "source of truth" for the pinned bootimages.

The expected user story here will be e.g. "update x86_64 to <version>".

So now we support this flow:

$ plume cosa2stream --distro rhcos --output data/data/rhcos.json x86_64=48.83.202102192019-0 s390x=47.83.202102090311-0

which hardcodes the URL we use now, and makes the arch+version mapping
explicit. We also paper over the fact that currently for RHCOS
there are separate cosa builds per architecture; the non-x86_64 ones
are named e.g rhcos-4.8-s390x instead of just rhcos-4.8.

jlebon · 2021-02-23T16:22:35Z

src/cmd-generate-release-meta

 parser = ArgumentParser()
 parser.add_argument("--workdir", help="cosa workdir")
 parser.add_argument("--build-id", help="build id")
+parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'])


Suggested change

parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'])

parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'], default='fcos')

Looks like it doesn't default to the first choice:

In [8]: parser = argparse.ArgumentParser() In [9]: parser.add_argument('--foo', choices=['bar', 'baz']) Out[9]: _StoreAction(option_strings=['--foo'], dest='foo', nargs=None, const=None, default=None, type=None, choices=['bar', 'baz'], help=None, metavar=None) In [10]: parser.parse_args([]) Out[10]: Namespace(foo=None)

vs

In [14]: parser = argparse.ArgumentParser() In [15]: parser.add_argument('--foo', choices=['bar', 'baz'], default='bar') Out[15]: _StoreAction(option_strings=['--foo'], dest='foo', nargs=None, const=None, default='bar', type=None, choices=['bar', 'baz'], help=None, metavar=None) In [16]: parser.parse_args([]) Out[16]: Namespace(foo='bar')

Well...we could default, but it's kind of intentional not to. We don't need to special case fcos here right now.

The idea here is that while some parts of cosa default to fcos, it's cleaner if it's more "mechanism" here. So after this PR lands we could change the pipeline to pass --distro fcos explicitly?

Ahh right OK. Just wanted to make sure this doesn't break FCOS as soon as it merges. But yeah, this doesn't.

Passing --distro fcos SGTM.

One thing I wanted to comment that's related - while we could hardcode/default FCOS in cosa and have rhcos be overrides, I think as a general rule it's good to split mechanism from configuration/policy.

Because even if all we cared about was FCOS, we'd still have the problem of e.g. CI on pull requests may need to behave differently, as well as supporting multiple streams with different possible tweaks between them, etc.

jlebon · 2021-02-23T16:26:27Z

mantle/cmd/plume/cosa2stream.go

 	cmdCosaBuildToStream.Flags().StringVar(&streamName, "name", "", "Stream name")
-	cmdCosaBuildToStream.MarkFlagRequired("name")
+	cmdCosaBuildToStream.Flags().StringVar(&distro, "distro", "", "Distribution (fcos, rhcos)")
+	cmdCosaBuildToStream.Flags().StringVar(&output, "output", "", "Use this file as baseline and target for output (default: no baseline, print to stdout)")


Bikeshed: wonder if e.g. target is a better name for this? Since it's actually both an input and an output.

OK, renamed.

mantle/cmd/plume/cosa2stream.go

jlebon · 2021-02-23T20:17:54Z

src/cmd-generate-release-meta

 parser = ArgumentParser()
 parser.add_argument("--workdir", help="cosa workdir")
 parser.add_argument("--build-id", help="build id")
+parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'])


Ahh right OK. Just wanted to make sure this doesn't break FCOS as soon as it merges. But yeah, this doesn't.

Passing --distro fcos SGTM.

jlebon · 2021-02-23T20:21:47Z

src/cmd-generate-release-meta

+parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'])
 parser.add_argument("--stream-name", help="Override the stream ID (default is derived from coreos-assembler)")
-parser.add_argument("--stream-baseurl", help="Prefix URL for stream content", default=FCOS_STREAM_ENDPOINT)
+parser.add_argument("--stream-baseurl", help="Override prefix URL for stream content")


But OTOH dropping the default= on this one would break FCOS. Hmm, let's just have it key off of --distro if unspecified and then we definitely do follow up this PR with one for the pipeline?

Ah yep, good catch! I actually had a "dispatch on distro" code but then removed it, this change was an artifact of that. For now to keep things even clearer and minimize distro-dispatching I just reverted it.

jlebon

/lgtm

When I was writing this originally I was focused on the goal of converting cosa metadata to stream format, hence the name etc. However upon trying to use this for openshift/installer I realized something important: *after* we do the switch, the installer git will be a "source of truth" for the pinned bootimages. The expected user story here will be e.g. "update x86_64 to <version>". So now we support this flow: ``` $ plume cosa2stream --distro rhcos --output data/data/rhcos.json x86_64=48.83.202102192019-0 s390x=47.83.202102090311-0 ``` which hardcodes the URL we use now, and makes the arch+version mapping explicit. We also paper over the fact that currently for RHCOS there are separate cosa builds per architecture; the non-x86_64 ones are named e.g `rhcos-4.8-s390x` instead of just `rhcos-4.8`.

jlebon · 2021-02-23T21:31:58Z

/lgtm

We should add a call to cosa generate-release-meta to CI here.

openshift-ci-robot · 2021-02-23T21:32:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from jlebon and travier February 22, 2021 19:00

openshift-ci-robot added the approved label Feb 22, 2021

cgwalters force-pushed the rhcos-endpoint-schema branch from a2b7fac to 0001c53 Compare February 22, 2021 19:10

cgwalters mentioned this pull request Feb 22, 2021

Add and use stream metadata for RHCOS, add openshift-install coreos print-stream-json openshift/installer#4582

Merged

jlebon reviewed Feb 23, 2021

View reviewed changes

cgwalters force-pushed the rhcos-endpoint-schema branch from 0001c53 to ab7da26 Compare February 23, 2021 20:07

jlebon reviewed Feb 23, 2021

View reviewed changes

cgwalters force-pushed the rhcos-endpoint-schema branch from ab7da26 to e6d3613 Compare February 23, 2021 20:53

jlebon approved these changes Feb 23, 2021

View reviewed changes

openshift-ci-robot assigned jlebon Feb 23, 2021

openshift-ci-robot added the lgtm label Feb 23, 2021

cgwalters force-pushed the rhcos-endpoint-schema branch from e6d3613 to 40f7458 Compare February 23, 2021 21:26

openshift-ci-robot removed the lgtm label Feb 23, 2021

openshift-ci-robot added the lgtm label Feb 23, 2021

openshift-merge-robot merged commit 44e519c into coreos:master Feb 23, 2021

	parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'])
	parser.add_argument("--distro", help="Distro selects stream defaults such as baseurl and format", choices=['fcos', 'rhcos'], default='fcos')

cosa2stream: Rework to better optimize openshift/installer flow #2052

cosa2stream: Rework to better optimize openshift/installer flow #2052

Uh oh!

Conversation

cgwalters commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

jlebon commented Feb 23, 2021

Uh oh!

openshift-ci-robot commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cgwalters commented Feb 22, 2021 •

edited

Loading