add support for kubevirt container image pushing #860

dustymabe · 2023-04-19T19:34:50Z

No description provided.

dustymabe · 2023-04-19T19:35:45Z

jobs/release.Jenkinsfile

@@ -207,6 +207,7 @@ lock(resource: "release-${params.STREAM}", extra: locks) {
        // OCP ART doesn't actually care what the tag name is (it's just to stop GC), we
        // hardcode it.
        def push_containers = ['oscontainer': ['ostree', 'base-oscontainer', ''],
+                               'kubevirt': ['kubevirt', 'kubevirt', ''],


I think this one is tricky because I think upstream we don't want a tag suffix, but downstream we do.

@jlebon does that match your understanding?

Indeed. We can't not add a suffix for this downstream. It'll conflict with the base-oscontainer. Upstream, we could push as quay.io/fedora/fedora-coreos-kubevirt:stable-kubevirt, but that looks awkward. Probably time to make this configurable. One idea is to make it templated, like we do for other keys. E.g.:

registry_repos: kubevirt: quay.io/example/monorepo:${STREAM}

or

registry_repos: kubevirt: quay.io/example/monorepo:${STREAM}-suffix

To make it backwards-compatible, we could have the third element in the tuple here represent the default tag template when none is given, so e.g.

'extensions': ['extensions-container', 'extensions-container', '${STREAM}-extensions']

I liked this idea when I first read it, but I'm starting to think it's not complete enough.

we also have the registry_repos.add_build_tag piece that adds --tag=${params.VERSION}${tag_suffix} so we'd have to split out tag_suffix from quay.io/example/monorepo:${STREAM}-suffix

Maybe instead we should just add more flexibility in the config spec:

registry_repos: kubevirt: registry: quay.io/example/monorepo tags: - ${STREAM}-suffix - ${VERSION}-suffix

IIUC we don't need to preserve backwards compat here because we use the latest pipeline code and latest pipecfg (i.e. there are only one copy of them for all branches and we can update them at the same time).

Yup, making the templating bits a list works for me too! The add_build_tag knob is definitely a bit of a hack, so it'd be good to get rid of it. The part about backcompat was more to provide a path to make this move more seamless, not that we can't change everything if we wanted to.

ok pushed a new upload that should take care of this.

gursewak1997 · 2023-04-19T19:58:05Z

Not sure about the tag suffix requirements, but the rest lgtm

config.yaml

jlebon · 2023-04-20T20:11:10Z

jobs/release.Jenkinsfile

@@ -207,6 +207,7 @@ lock(resource: "release-${params.STREAM}", extra: locks) {
        // OCP ART doesn't actually care what the tag name is (it's just to stop GC), we
        // hardcode it.
        def push_containers = ['oscontainer': ['ostree', 'base-oscontainer', ''],
+                               'kubevirt': ['kubevirt', 'kubevirt', ''],


Indeed. We can't not add a suffix for this downstream. It'll conflict with the base-oscontainer. Upstream, we could push as quay.io/fedora/fedora-coreos-kubevirt:stable-kubevirt, but that looks awkward. Probably time to make this configurable. One idea is to make it templated, like we do for other keys. E.g.:

registry_repos: kubevirt: quay.io/example/monorepo:${STREAM}

or

registry_repos: kubevirt: quay.io/example/monorepo:${STREAM}-suffix

To make it backwards-compatible, we could have the third element in the tuple here represent the default tag template when none is given, so e.g.

'extensions': ['extensions-container', 'extensions-container', '${STREAM}-extensions']

If not specified the code will default to all built arches. We need this because we'll be adding kubevirt soon and it will only be built for x86_64. See also coreos/coreos-assembler#3436

For Fedora CoreOS we'll ship to quay.io/fedora/fedora-coreos-kubevirt. See coreos/fedora-coreos-tracker#1126 (comment)

This is going to be handled by the container pushing code in the release job so we don't need it here. The buildextend-kubevirt --upload pieces were also removed in coreos/coreos-assembler@a4cf183 so it doesn't work anyway.

dustymabe · 2023-05-01T20:27:53Z

ok I pushed up a new upload here. This should handle all outstanding concerns. I tested this in the downstream devel pipeline.

dustymabe · 2023-05-01T20:28:17Z

Let's hold this until this week's releases are out.

jlebon

Can we prefix the last commit title with e.g. config:?
Also, there's a "wentb" typo in the last commit message.

config.yaml

docs/config.yaml

jlebon · 2023-05-02T14:10:30Z

docs/config.yaml

+	registry: quay.io/fedora/fedora-coreos
+    tags: ["${STREAM}"]


Should add REQUIRED markers on these and docstrings for consistency.

added REQUIRED docstrings

jlebon · 2023-05-02T14:11:17Z

docs/config.yaml

-  oscontainer: quay.io/fedora/fedora-coreos
+  oscontainer:
+	registry: quay.io/fedora/fedora-coreos
+    tags: ["${STREAM}"]


I know it seems obvious since we're using it in the examples, but for completeness, can we explicitly document the supported variables?

I did in the first instance; didn't for the others.

We could even drop the REQUIRED comments in the following ones, but cool as is too!

jlebon · 2023-05-02T14:14:54Z

utils.groovy

+            // this is a boolean option, not a registry repo
+            continue
+        }
+        if (registry_repos."${repo}".tags) {


Minor/optional: could also do

registry_repos[repos].tags

which looks cleaner.

jlebon · 2023-05-02T14:21:16Z

config.yaml

-  oscontainer: quay.io/fedora/fedora-coreos
-  kubevirt: quay.io/fedora/fedora-coreos-kubevirt
+  oscontainer:
+	registry: quay.io/fedora/fedora-coreos


Hmm, I guess technically this key should be called repo rather than registry?

jlebon · 2023-05-02T14:23:29Z

jobs/release.Jenkinsfile

+                            if (pipecfg.hotfix) {
+                                // this is a hotfix build; include the hotfix name
+                                // in the tag suffix so we don't clobber official
+                                // tags
+                                tag += "-hotfix-${pipecfg.hotfix.name}"
+                            }


Seems like it'd be cleaner to have this be part of get_registry_repos now that it does tag stuff?

And then here we'd have:

def tag_args = registry_repos[configname].tags.collect{"--tag=$it"}

jlebon · 2023-05-02T14:28:07Z

jobs/release.Jenkinsfile

-                            // in the tag suffix so we don't clobber official
-                            // tags
-                            tag_suffix += "-hotfix-${pipecfg.hotfix.name}"
+                        def repo = registry_repos[configname]['registry']


Hmm, interesting that here we went with the opposite notation. :) Regardless of which one we go with, let's have it match get_registry_repos?

dustymabe · 2023-05-02T18:23:55Z

ok I think I addressed all comments. I need to run this through another round of testing; will report back on that in an hour.

jlebon

Some minor comments, but LGTM as is too!

jlebon · 2023-05-02T18:52:34Z

docs/config.yaml

-  oscontainer: quay.io/fedora/fedora-coreos
+  oscontainer:
+	registry: quay.io/fedora/fedora-coreos
+    tags: ["${STREAM}"]


We could even drop the REQUIRED comments in the following ones, but cool as is too!

jlebon · 2023-05-02T18:53:29Z

docs/config.yaml

@@ -113,17 +115,38 @@ s3:
 # OPTIONAL: container registry-related keys
 registry_repos:
  # OPTIONAL: repo to which to push oscontainer


Minor: maybe this now can be:

Suggested change

# OPTIONAL: repo to which to push oscontainer

# OPTIONAL: repo and tags to which to push oscontainer

jlebon · 2023-05-02T18:54:08Z

docs/config.yaml

@@ -113,17 +115,38 @@ s3:
 # OPTIONAL: container registry-related keys
 registry_repos:
  # OPTIONAL: repo to which to push oscontainer
-  oscontainer: quay.io/fedora/fedora-coreos
+  oscontainer:
+    # REQUIRED: repo to which to push oscontainer


Minor: and here, just

Suggested change

# REQUIRED: repo to which to push oscontainer

# REQUIRED: repo name

jlebon · 2023-05-02T18:57:20Z

For the commit title, how about something like:

config: add container image tags to push to and drop add_build_tags

?

dustymabe · 2023-05-02T19:20:02Z

config: add container image tags to push to and drop add_build_tags

Sorry I forgot about this in the last upload. New upload just landed with fixes. I'll start testing this now.

This commit updates the config to be able to specify which tags to push to inside the registry_repos definition. For example, we went from: oscontainer: quay.io/fedora/fedora-coreos to: oscontainer: repo: quay.io/fedora/fedora-coreos tags: ["${STREAM}"] This allows us to do things like embed any needed "tag_suffix" in the config rather than in the code. i.e. downstream now our extensions container entry will look like: extensions: repo: quay.io/openshift-release-dev/ocp-v4.0-art-dev tags: ["${STREAM}-extensions", "${VERSION}-extensions"] It also means we can now get rid of the `add_build_tag` hack that we had in the past.

dustymabe · 2023-05-02T19:39:02Z

ok this seems to pass in some basic testing.

gursewak1997 · 2023-05-02T19:50:55Z

ok this seems to pass in some basic testing.

How you are testing this? 👀

dustymabe · 2023-05-02T19:54:21Z

ok this seems to pass in some basic testing.

How you are testing this? eyes

Downstream devel pipeline. Look at the most recent runs of the release job.

dustymabe commented Apr 19, 2023

View reviewed changes

dustymabe requested a review from gursewak1997 April 19, 2023 19:36

jlebon reviewed Apr 20, 2023

View reviewed changes

dustymabe added 4 commits May 1, 2023 14:21

jobs/release: drop --arch args for cosa push-container-manifest

516c654

If not specified the code will default to all built arches. We need this because we'll be adding kubevirt soon and it will only be built for x86_64. See also coreos/coreos-assembler#3436

add registry_repo config for kubevirt (containerdisk)

cc341c5

For Fedora CoreOS we'll ship to quay.io/fedora/fedora-coreos-kubevirt. See coreos/fedora-coreos-tracker#1126 (comment)

libcloud: drop code to upload Kubevirt

1ed32eb

This is going to be handled by the container pushing code in the release job so we don't need it here. The buildextend-kubevirt --upload pieces were also removed in coreos/coreos-assembler@a4cf183 so it doesn't work anyway.

config: start building kubevirt artifact (again)

b1a85e0

dustymabe force-pushed the dusty-kubevirt branch from 013efcb to dbd395e Compare May 1, 2023 20:26

jlebon reviewed May 2, 2023

View reviewed changes

dustymabe force-pushed the dusty-kubevirt branch from dbd395e to 05ad79a Compare May 2, 2023 18:23

jlebon approved these changes May 2, 2023

View reviewed changes

dustymabe force-pushed the dusty-kubevirt branch from 05ad79a to 25acfdf Compare May 2, 2023 19:20

dustymabe force-pushed the dusty-kubevirt branch from 25acfdf to ae8918b Compare May 2, 2023 19:38

dustymabe marked this pull request as ready for review May 2, 2023 19:39

gursewak1997 approved these changes May 2, 2023

View reviewed changes

dustymabe merged commit 1471a83 into coreos:main May 3, 2023
1 check passed

dustymabe mentioned this pull request May 3, 2023

Platform Request: kubevirt coreos/fedora-coreos-tracker#1126

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for kubevirt container image pushing #860

add support for kubevirt container image pushing #860

dustymabe commented Apr 19, 2023

dustymabe Apr 19, 2023

jlebon Apr 20, 2023

dustymabe Apr 20, 2023

jlebon Apr 20, 2023

dustymabe May 1, 2023

gursewak1997 commented Apr 19, 2023

jlebon Apr 20, 2023

dustymabe commented May 1, 2023

dustymabe commented May 1, 2023

jlebon left a comment

jlebon May 2, 2023

dustymabe May 2, 2023

jlebon May 2, 2023

dustymabe May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

dustymabe commented May 2, 2023

jlebon left a comment

jlebon May 2, 2023

jlebon May 2, 2023

jlebon May 2, 2023

jlebon commented May 2, 2023

dustymabe commented May 2, 2023

dustymabe commented May 2, 2023

gursewak1997 commented May 2, 2023

dustymabe commented May 2, 2023

	# OPTIONAL: repo to which to push oscontainer
	# OPTIONAL: repo and tags to which to push oscontainer

	# REQUIRED: repo to which to push oscontainer
	# REQUIRED: repo name

add support for kubevirt container image pushing #860

add support for kubevirt container image pushing #860

Conversation

dustymabe commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursewak1997 commented Apr 19, 2023

Choose a reason for hiding this comment

dustymabe commented May 1, 2023

dustymabe commented May 1, 2023

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented May 2, 2023

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented May 2, 2023

dustymabe commented May 2, 2023

dustymabe commented May 2, 2023

gursewak1997 commented May 2, 2023

dustymabe commented May 2, 2023