Skip to content
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.

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

Add basic support for hotfix builds #740

Merged
merged 3 commits into from
Nov 9, 2022
Merged

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 3, 2022

ART needs the ability to perform hotfix builds, i.e. builds outside the
regular stream for a specific purpose. These builds must not pollute
production stream references.

Support this via the new hotfix section of the pipecfg. Require a
name subkey which is then used to modify the S3 subpath we upload to
and the oscontainer tags we push to. Add new hotfix parameters to the
build, build-arch, and release jobs which will be used to pass the
repo containing the hotfix pipecfg.

Don't support hotfix cloud uploads since it's not yet needed and
requires plumbing of the hotfix prefix.

Requires: #739

@jlebon
Copy link
Member Author

jlebon commented Nov 3, 2022

Still need to test all this!

utils.groovy Outdated Show resolved Hide resolved
utils.groovy Show resolved Hide resolved
jobs/build-arch.Jenkinsfile Show resolved Hide resolved
jobs/build-arch.Jenkinsfile Outdated Show resolved Hide resolved
jobs/release.Jenkinsfile Outdated Show resolved Hide resolved
@jlebon jlebon force-pushed the hotfix-builds branch 11 times, most recently from 196c44e to 4037de9 Compare November 5, 2022 22:49
@jlebon jlebon marked this pull request as ready for review November 5, 2022 22:49
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

I'm not the biggest fan of the first commit I'm not sure the benefit is worth us losing the default list that shows up in the cases where a human is typing parameters in manually.

2nd commit LGTM

description: 'Space-separated list of additional target architectures',
defaultValue: pipecfg.additional_arches.join(" "),
description: 'Override default additional target architectures (space-separated)',
defaultValue: "",
Copy link
Member

Choose a reason for hiding this comment

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

we've actually been using this (overriding the default) today. Right now the pipeline is configured to build for ppc64le too but we tell ourselves when we go to make official builds to remove the ppc64le from the list.

Now there will be no list to remove an item from, but rather an empty field. It's not a big deal but I kind of prefer the other UX.

@jlebon
Copy link
Member Author

jlebon commented Nov 8, 2022

I'm not the biggest fan of the first commit I'm not sure the benefit is worth us losing the default list that shows up in the cases where a human is typing parameters in manually.

IMO we should avoid humans typing parameters when possible. Ideally, all the information needed is in git in the pipecfg. Having parameters is still valuable of course when on-the-fly overriding is necessary. But then we should make that semantic really clear, which is what the patch aims to achieve. The relationship was already confusing before between the parameter and the pipecfg key, but with hotfix pipecfgs it becomes even more so.

For the ppc64le situation right now in FCOS, my suggestion is to either support an additional_arches override or a skip_arches per-stream knob to avoid building it in the prod stream. Then we can git revert it when we're actually ready to build ppc64le.

@dustymabe
Copy link
Member

I'm not the biggest fan of the first commit I'm not sure the benefit is worth us losing the default list that shows up in the cases where a human is typing parameters in manually.

IMO we should avoid humans typing parameters when possible. Ideally, all the information needed is in git in the pipecfg. Having parameters is still valuable of course when on-the-fly overriding is necessary. But then we should make that semantic really clear, which is what the patch aims to achieve. The relationship was already confusing before between the parameter and the pipecfg key, but with hotfix pipecfgs it becomes even more so.

I agree. Typing in things isn't ideal, but it is really nice for when you need it to have the default values there in front of you to edit. How about we compromise and still show the values (not sourced from the pipecfg) and then validate them after the hotfix pipecfg is processed? The following is just draft code but conveys the idea I think:

diff --git a/jobs/build.Jenkinsfile b/jobs/build.Jenkinsfile
index 6108cac..26e48b9 100644
--- a/jobs/build.Jenkinsfile
+++ b/jobs/build.Jenkinsfile
@@ -24,7 +24,7 @@ properties([
              trim: true),
       string(name: 'ADDITIONAL_ARCHES',
              description: 'Override default additional target architectures (space-separated)',
-             defaultValue: "",
+             defaultValue: pipeutils.get_supported_additional_arches().join(' '),
              trim: true),
       booleanParam(name: 'FORCE',
                    defaultValue: false,
@@ -66,6 +66,13 @@ if (params.PIPECFG_HOTFIX_REPO || params.PIPECFG_HOTFIX_REF) {
 def cosa_img = params.COREOS_ASSEMBLER_IMAGE
 cosa_img = cosa_img ?: pipeutils.get_cosa_img(pipecfg, params.STREAM)
 def additional_arches = params.ADDITIONAL_ARCHES.split()
+
+for (arch in additional_arches) {
+    if (!pipecfg.additional_arches.contains(arch)) {
+        error('requested architecture disallowed by pipecfg')
+    }
+}
+
 additional_arches = additional_arches ?: pipecfg.additional_arches
 
 def stream_info = pipecfg.streams[params.STREAM]

For the ppc64le situation right now in FCOS, my suggestion is to either support an additional_arches override or a skip_arches per-stream knob to avoid building it in the prod stream. Then we can git revert it when we're actually ready to build ppc64le.

I think we should do that too. The ppc64le situation has gone on long enough that it warrants a knob if we can afford to implement that.

@dustymabe
Copy link
Member

I think with the suggestions above we get the best of both worlds (reached through collaboration and compromise).

@jlebon
Copy link
Member Author

jlebon commented Nov 8, 2022

How about we compromise and still show the values (not sourced from the pipecfg) and then validate them after the hotfix pipecfg is processed?

Hmm, in the case of a hotfix build for x86_64 only (or with stream-level knobs, any stream build where the arch set doesn't match the Jenkins set), that would require the operator to both edit additional_arches in pipecfg and trim down the list from the default set in the parameter. The UX there isn't ideal. I'm also still not a fan of the "off by one run" semantic that can cause confusion here. It's not like we're turning arches on and off very often but it's also such a subtle issue that it's bound to trip someone up down the line.

Two suggestions:

  1. Instead of modifying the default value, we put the list of supported arches in the description. E.g. tweaking your patch slightly:

    diff --git a/jobs/build.Jenkinsfile b/jobs/build.Jenkinsfile
    index 6108cac..26e48b9 100644
    --- a/jobs/build.Jenkinsfile
    +++ b/jobs/build.Jenkinsfile
    @@ -24,7 +24,7 @@ properties([
                  trim: true),
           string(name: 'ADDITIONAL_ARCHES',
    -             description: 'Override default additional target architectures (space-separated)',
    +             description: "Override default additional target architectures (space-separated). Supported arches: ${pipeutils.get_supported_additional_arches().join(' ')}",
                  defaultValue: "",
                  trim: true),
           booleanParam(name: 'FORCE',
                        defaultValue: false,

    So then if someone wants to manually insert arches, they can copy/paste or retype from that list and modify as needed.

  2. It sounds like what you're getting at is the ability to skip arches defined in the pipecfg via parameters. We could instead add a SKIP_ARCHES that would be a better fit for this.

WDYT?

@dustymabe
Copy link
Member

Two suggestions:

  1. Instead of modifying the default value, we put the list of supported arches in the description. E.g. tweaking your patch slightly:

Yeah. Let's go with 1. 👍

@jlebon
Copy link
Member Author

jlebon commented Nov 9, 2022

Updated!

@jlebon
Copy link
Member Author

jlebon commented Nov 9, 2022

For the ppc64le situation right now in FCOS, my suggestion is to either support an additional_arches override or a skip_arches per-stream knob to avoid building it in the prod stream. Then we can git revert it when we're actually ready to build ppc64le.

I think we should do that too. The ppc64le situation has gone on long enough that it warrants a knob if we can afford to implement that.

Follow-up for that in #747.

jobs/build.Jenkinsfile Outdated Show resolved Hide resolved
We need to make clearer the distinction between the set of architectures
that a Jenkins instance supports, and the set of architectures that a
pipecfg wants to build for. This is usually the same, but not always.
For example, a development stream may be temporarily unhealthy on some
arches, or a hotfix pipecfg may want to limit the set of arches to build
for.

Let's have the semantics of `additional_arches` be similar to that of
`cosa_img`: it's a config knob that can be overridden by a parameter.
Accordingly, set the job parameter default to be empty (matching
`COREOS_ASSEMBLER_IMAGE`).

Notice how this cleans up the `build-cosa` job, which no longer needs to
read in the pipecfg. This makes sense since it's not strictly part of
the build pipeline.

This also fixes a subtle issue with the way the arch-related params were
wired. Their defaults were derived from the pipecfg, but those default
values only actually affect the *next* run of the job. To be correct, we
should've been using the definition from the pipecfg throughout the job,
not the parameter.
ART needs the ability to perform hotfix builds, i.e. builds outside the
regular stream for a specific purpose. These builds must not pollute
production stream references.

Support this via the new `hotfix` section of the pipecfg. Require a
`name` subkey which is then used to modify the S3 subpath we upload to
and the oscontainer tags we push to. Add new hotfix parameters to the
`build`, `build-arch`, and `release` jobs which will be used to pass the
repo containing the hotfix pipecfg.

Don't support hotfix cloud uploads since it's not yet needed and
requires plumbing of the hotfix prefix.
… description

This way, an operator can tell what values are valid and easily
copy/paste the list and edit it.
@jlebon jlebon force-pushed the hotfix-builds branch 2 times, most recently from 2eb5e82 to cf8c63e Compare November 9, 2022 20:27
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe dustymabe merged commit 6b50c03 into coreos:main Nov 9, 2022
@jlebon jlebon deleted the hotfix-builds branch April 24, 2023 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants