images/builder: Allow GCB builds from arbitrary build directories #14747
Conversation
k8s-ci-robot
added
sig/release
k8s-ci-robot
added
priority/important-soon
k8s-ci-robot
added
area/images
sig/testing
k8s-ci-robot
added
the
lgtm
|
/hold This will break a number of existing jobs — I think all the test-infra uploads, which both rely on the I'm not sure it's better to change this behaviour, given doing so potentially creates the need for submodules or substantial refactoring in monorepos (as you already experienced, as a result of my faulty recollection of how this code actually works — sorry about that). |
k8s-ci-robot
added
the
do-not-merge/hold
justaugustus
force-pushed
the
img-build
branch
from
3747cb8 to
70c4e93
Compare
k8s-ci-robot
added
area/config
justaugustus
force-pushed
the
img-build
branch
from
70c4e93 to
8cb6740
Compare
|
@Katharine -- thanks for the chat earlier today to decide on approach! I've tweaked the builder so it now supports:
I've tested this on a few of the |
There was a problem hiding this comment.
This still breaks the existing jobs - if nothing else, the bazel handling must happen before anything else. I'm also not sure that variants.yaml is being correctly colocated with cloudbuild.yaml, since you have changed the handling of cloudbuild.yaml but not touched variants.yaml.
I would prefer it if it looked like this (and see inline comments for more context - this review is somewhat poorly structured, sorry!):
(I will use "config dir" to refer to what is provided as the positional argument)
- If
BAZEL_WORKSPACE_DIRECTORYis set, chdir there and use that as the cwd - If a
--build-diris provided, use that (relative to cwd) as the directory we should upload to GCB. Otherwise set the build dir to the config dir - Look for
cloudbuild.yamlandvariants.yamlin the config dir (resolved relative to cwd). - chdir to the build dir (which may be the config dir)
- perform the existing logic - maybe upload something to GCS, then invoke gcloud
Let me know what you think?
images/builder/main.go
Outdated
| } | ||
| } | ||
| } else { | ||
| o.cloudbuildPath = "cloudbuild.yaml" |
There was a problem hiding this comment.
I'm not sure that this cloudbuildPath option is what we want. There are essentially two concepts we have:
- Where are our config files located?
- What are we sending to GCB?
Notably, there are two config files - cloudbuild.yaml and variants.yaml - which should be together, but not necessarily in the root. I think that this should be the path to that directory, and the paths to the relevant files can be assembled by appending to it later.
The other concept - what we send to GCB - either matches the config dir (by default), or is "the root", which we are really interpreting as "the current working directory" (which is a slightly awkward concept).
I still wonder whether it would make sense to instead of having --build-at-root, instead have --upload-dir, which would be equivalent to the current build-at-root with --upload-dir=. Then you would invoke the builder as image-builder --upload-dir=. ./images/builder/ or something. I think in this case, both upload-dir and the position argument should be relative to the current working directory, rather than eachother (since there is not actually any requirement that the config be part of the build).
justaugustus
force-pushed
the
img-build
branch
from
8cb6740 to
5c74945
Compare
k8s-ci-robot
added
size/M
justaugustus
force-pushed
the
img-build
branch
2 times, most recently
from
d4c230e to
af5bf7d
Compare
justaugustus
changed the title
|
@Katharine -- I updated the builder based on your suggestions and tested it on a few k/release jobs, including one with a I've also re-updated the test-infra-trusted jobs to use the |
k8s-ci-robot
added
size/L
- Remove cdToRootDir
This function was not actually cd-ing into directories
- Add --build-dir flag to support builds that require the entire
repo/workspace to be uploaded to GCB
- Add a validation method to ensure config directory and cloudbuild.yaml
exist before submitting build
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
justaugustus
force-pushed
the
img-build
branch
from
21e9ca1 to
355419c
Compare
images/builder/main.go
Outdated
|
|
||
| func getVariants(o options) (variants, error) { | ||
| content, err := ioutil.ReadFile(path.Join(o.imageDirectory, "variants.yaml")) | ||
| content, err := ioutil.ReadFile("variants.yaml") |
There was a problem hiding this comment.
Is this right? We should be looking for this in the config dir, but at this point we will be in the build dir.
As we're about to cd into the build directory, we need a consistent way to reference the config files when the config directory is not the same as the build directory. Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Katharine
left a comment
Katharine
left a comment
There was a problem hiding this comment.
Looks good, I think. One question where I'm not sure where you did something, but I don't think it matters.
| } | ||
| o.imageDirectory = flag.Arg(0) | ||
|
|
||
| o.configDir = strings.TrimSuffix(flag.Arg(0), "/") |
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
@Katharine -- I don't think it's strictly necessary, but it's defensive against any weirdness with path joins, double /s, etc.
There was a problem hiding this comment.
filepath.Clean takes care of that and more: https://golang.org/pkg/path/filepath/#Clean
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, Katharine The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Thanks @Katharine! |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@justaugustus: Updated the
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This PR appears to have rendered the image builder unable to build itself, reverting |
Revert "Merge pull request #14747 from justaugustus/img-build"
7 participants
cdToRootDirThis function was not actually cd-ing into directories
--build-dirflag to support builds that require the entirerepo/workspace to be uploaded to GCB
cloudbuild.yamlexist before submitting build
Signed-off-by: Stephen Augustus saugustus@vmware.com
/assign @Katharine @dims @tpepper
cc: @alexeldeib @kubernetes/release-engineering
/sig release
/area release-eng
/milestone v1.17
/priority important-soon
After wiring k/release up to do image building/pushing via post-submit (#14711, #14735, kubernetes/release#895, kubernetes/release#896), I noticed that we had a few build failures:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/post-release-push-image-k8s-cloud-builder/1182793236758925320:https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/post-release-push-image-deb-builder/1182793236758925319:https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/post-release-push-image-rpm-builder/1182793236758925321:Basically what's happening is the builder is looking for assets in
/workspaceand can't find them.This was happening because the
os.Chdircommand was in a separate function scope, so it was ultimately not being respected. This was further obfuscated by the fact that the builder in its' current state specifies thecloudbuild.yamlwhile you're outside of the image directory.I tried testing this locally with commands close to what would be running in CI and all seems to be working...
Build the image builder from the root of k/test-infra:
Submitting an image push from the root of k/release: