Have prow inform child jobs about their parent #3966
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @BenTheElder. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. I understand the commands that are listed here. |
|
I'm not sure if this is the right solution but I think this is at least somewhat reasonable. Jenkins jobs may also need to be informed as well (?) /cc @fejta @ixdy @pipejakob @spxtr |
|
After this is worked out we should be able to update jobs to use Please commence yak shaving but do not merge this yet. |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
| Spec ProwJobSpec `json:"spec,omitempty"` | ||
| Status ProwJobStatus `json:"status,omitempty"` | ||
| ParentStatus ProwJobStatus `json:"parent_status,omitempty"` | ||
| ParentJob string `json:"parent_job,omitempty"` |
There was a problem hiding this comment.
API-wise it would be more correct to use OwnerReferences in the object metadata.
There was a problem hiding this comment.
https://github.com/kubernetes/community/blob/548eb7292d45a4af78e0141d33094130dfe6080a/contributors/design-proposals/garbage-collection.md
https://github.com/kubernetes/community/blob/31d62c70214d66ca2c21716c02b83d9087fdf3ff/contributors/design-proposals/controller-ref.md
It doesn't make sense to fully support owner references (adoption, orphaning) but we could use them for denoting relationships between prowjobs.
There was a problem hiding this comment.
I think we don't want to support any form of ownership, and I specifically intend to inject those environment variables mimicking the variables supplied to the current job, but for the parent. I think we may also need to support this for Jenkins jobs
I don't currently have other use cases besides a build job being followed by test jobs, but I think you could pass anything you wanted through the output buckets.
There was a problem hiding this comment.
So the flow is the build job pushes the artifacts in a bucket and plank injects the location of the bucket in the test jobs?
There was a problem hiding this comment.
That's an option, but I wanted to avoid restricting it to build jobs so the flow is:
Job writes to logs bucket alongside started.json, finished.json, etc with data for the child job to consume (say child_data.json or something), prow injects the parent job name / buildID into the child when it starts and the child goes to logs/$PARENT_JOB_NAME/$PARENT_BUILD_ID/ and uses child_data.json to do whatever (in this get the build location).
|
Will you clarify how and why this is necessary? The output of the build job should be a function of the inputs to that job, which we can also send to the second job. Is this to avoid having the second job check out the repos and merge in the PRs to determine the hash? |
|
It's not strictly necessary but I wanted jobs to be able to put out a file
along with finished.json, started.json and their logs meant to be consumed
by child jobs with whatever data they wanted to pass (like the build bucket
gs://...). I first mentioned it here:
kubernetes/kubernetes#49884 (comment)
I think @ixdy originally had the idea for plumbing it that way.
…On Tue, Aug 8, 2017 at 11:12 AM, Erick Fejta ***@***.***> wrote:
Will you clarify how and why this is necessary?
The output of the build job should be a function of the inputs to that
job, which we can also send to the second job. Is this to avoid having the
second job check out the repos and merge in the PRs to determine the hash?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3966 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq-AAyWMRHUADTHtQgbCwggQ8Kl8Pks5sWKUjgaJpZM4OwFpX>
.
|
|
It sounds good, my only question/concern would be to ensure that it is really easier to do it this way than to just recalculate it in the other jobs :) |
Each time you create a merge, the commit hash (and thus the "kubernetes version") will be different, since the merge was created at a different time. If we use the kubernetes version in the GCS path (as we do with most of the jobs), we need to have some way to pass that between jobs. The bazel/kubeadm PR jobs are getting around this right now by using the full |
|
I guess a different approach would be to do something like this and explicitly set date/author/email on the merge commit in bootstrap so that the version is always the same for a given |
|
e.g. what GitHub does: $ git fetch upstream pull/3966/merge; git log --pretty=fuller FETCH_HEAD
From https://github.com/kubernetes/test-infra
* branch refs/pull/3966/merge -> FETCH_HEAD
commit 6835fd65cef523aefa9e4d8f2d3379fa07ff5b50
Merge: fa6daadb 7e65fedd
Author: Benjamin Elder <[elided]>
AuthorDate: Tue Aug 8 12:08:22 2017
Commit: GitHub <noreply@github.com>
CommitDate: Tue Aug 8 12:08:22 2017
Merge 7e65fedd01fc3547bc5ee7039e68d55eab07d4c3 into fa6daadbd23f4bf46333c918e52805b3840d0d7e
...I have no idea how it sets that date - maybe lazily, whenever someone requests a mergeability check? |
|
My biggest concern is that this is following the same pattern that is currently causing trouble for the bazel/kubeadm split: it's setting more variables but still expecting both halves of the pipe (parent and child job) to be able to construct the same consistent path using these variables to actually find the data. This is what caused problems every time we tried to change the path for the bazel artifacts, we're just moving the problem up a layer. You could argue that this path to the job's output is likely more stable than the bazel ci-artifacts path has been, but it still has the same inflexibility if we ever do want to change it. Instead, if the general idea is for the child job to be able to find the parent job's output file, I would rather it be told explicitly where that file is (via a full URL) rather than requiring any knowledge about how the parent job might have used its name and build number to construct the |
Or make plank pass the name of the parent as an ENV in the child job?
+1 |
|
I would like to have the child job output the full path somewhere that
bootstrap knows about (eg
kubernetes-jenkins/logs/jobName/buildID/build_path.env or something along
those lines). That way prow doesn't need to know anything about the job
output path.
…On Tue, Aug 8, 2017 at 1:19 PM, Jakob ***@***.***> wrote:
My biggest concern is that this is following the same pattern that is
currently causing trouble for the bazel/kubeadm split: it's setting more
variables but still expecting both halves of the pipe (parent and child
job) to be able to construct the same consistent path using these variables
to actually find the data. This is what caused problems every time we tried
to change the path for the bazel artifacts, we're just moving the problem
up a layer. You could argue that this path to the job's output is likely
more stable than the bazel ci-artifacts path has been, but it still has the
same inflexibility if we ever do want to change it.
Instead, if the general idea is for the child job to be able to find the
parent job's output file, I would rather it be told explicitly where that
file is (via a full URL) rather than requiring any knowledge about how the
parent job might have used its name and build number to construct the
gs:// path.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3966 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq2QLefDv0HjMdUEYVMfktSUKhbhXks5sWMLJgaJpZM4OwFpX>
.
|
That's what this does (?) If the child job env has the parent job name and buildID in ENV it can fetch a file from the parent's logs bucket. I definitely do not want child jobs magically constructing URLs but bootstrap already constructs the logs dir URL and we can just generate the parent's bucket and get whatever we need passed in from there. There are other ways we could do this but I think this way has lower coupling in prow at least. |
Sorry, incomplete thought. I wanted to suggest doing this by default w/o the api change. |
|
Ah, yes I thought about that but we still need the buildID so I thought we
might as well pass the name too to avoid hardcoding. Then potentially you
could run tests against bazel and docker builds at some point more easily
perhaps.
On Aug 8, 2017 14:32, "Michail Kargakis" <notifications@github.com> wrote:
That's what this does (?)
Sorry, uncompleted thought. I wanted to suggest doing this by default w/o
the api change.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3966 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq6MuVETz2n7_Y98559kbrN2przBuks5sWNPggaJpZM4OwFpX>
.
|
BenTheElder
changed the title
|
@pipejakob further discussion with @ixdy brings up the idea of specifically using a file in |
|
I'll reiterate my main objection to this approach, and I apologize if there's a disconnect in our wordings or mental models about what problem this is solving. Here are the goals as I see them:
In order to solve these goals, the workflow proposed as I understand it is:
I'm curious if (and why) there's any opposition to just telling the child job exactly where this path is directly. It doesn't need to know the parent job's name or build id, all it actually cares about is this path.
I think this is just repeating the exact pattern and problem we had before: the bazel job (the parent) was outputting artifacts into a specific path, the child job (kubeadm e2e) needed to use the same path, and rather than being told the exact path to use, both halves tried to keep in sync by independently constructing the path based on shared attributes (the build SemVer, or the PULL_REFS, etc.). This solution is functionally the same, just using different attributes. Once you're playing the game of If the contract is "I'll just tell you exactly where to find the file," then our path structure is infinitely flexible and requires no coordination between parent/child jobs to update, if we find it necessary to change gcs buckets, add more parameters to the path structure, etc. |
|
I agree with the caveat though that I don't think that path structure should be baked into prow at all and I'm not sure of a cleaner solution that avoids this. With the log paths we're using a structure that doesn't depend on pull refs etc so providing the parent job name / id was sort of a middle ground for simple paths but without prow knowing about them. |
|
What if we do:
So you'd have something like: |
|
Oh, one quick but important clarification: I didn't mean to prescribe that I think prow should be populating this environment variable, especially if/since it has no context about these paths. If step 1 was that prow set these variables and then step 2 was that bootstrap.py or some other layer in the stack used them to figure out the exact path to be used and put that in another environment variable, then that's fine. As a job owner, I just don't want it to be my responsibility to construct that path myself (either in the job runner, the scenario, or kubetest, etc.). Our current solution with the bazel and kubeadm e2e jobs fell apart because the bazel job was owned by EngProd and I was owning the kubeadm e2es, and we both had to keep our implementations of this path-construction logic in sync in different places. If all of the test-infra glue between the jobs handles these paths, and the kubeadm e2e is ultimately told "here is the path to use," then I don't actually care which parts of that glue made that magic happen. When we've been saying "the parent job does this" and "the child job does that," those were likely very overloaded in our minds to mean different things, and we should start being a lot more specific about the exact responsibilities and proposed changes to the different layers of the test stack. |
This sounds great to me. Slight caveat that I'd expect them to be paths to a specific file and not just a directory. |
|
Closed in favor of #4047 |
Right now
run_after_successjobs know nothing about their parent job, this PR allows child jobs to know the name and build ID of their parent job.Why do we need this?