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

Prow job labeled with prow version #21054

Merged
merged 2 commits into from
Mar 1, 2021

Conversation

chaodaiG
Copy link
Contributor

As proposed in kubernetes/enhancements#2540, k8s prow will be bumped more frequently than once per work day, adding this label to the job so that it could help associate failures with specific prow versions

As proposed in kubernetes/enhancements#2540, k8s prow will be bumped more frequently than once per work day, adding this label to the job so that it could help associate failures with specific prow versions
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 26, 2021
@chaodaiG
Copy link
Contributor Author

/cc @spiffxp

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/prow Issues or PRs related to prow area/prow/plank Issues or PRs related to prow's plank component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 26, 2021
@chaodaiG
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2021
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2021
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
If you want to change the label name at all. Otherwise we'll save that for future improvements

@@ -41,6 +41,9 @@ const (
// job names can be arbitrarily long, this is added as
// an annotation instead of a label.
ProwJobAnnotation = "prow.k8s.io/job"
// ProwVersionLabel is added in resources created by prow and
// carries the version of prow that decorated this job.
ProwVersionLabel = "prow.k8s.io/version"
Copy link
Member

Choose a reason for hiding this comment

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

We deploy all prow components as a bunch of micro services in lockstep today. It may be worth considering the versions of the components themselves though.

In terms of components that would be relevant to why pods landed on a cluster and how they ended up in their current form. Ideally we could have:

  • prow.k8s.io/hook-version is responsible for triggering a job in response to GitHub events
  • prow.k8s.io/plank-version is responsible for making a pod out of a prowjob
  • prow is using a specific config when creating this pod (sha of the repo from which the config was deployed? hash of configmap contents? revision of the configmap resource?), or maybe specific component and job configs

Copy link
Member

Choose a reason for hiding this comment

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

A better approach to this would be leverage the managedFields. We would need to figure out how to set a correct fieldmanager (xref kubernetes-sigs/controller-runtime#1215), then something like kubect-blame can be used to figure out what component in what version set which fields. Requires kube 1.18 though.

Copy link
Member

Choose a reason for hiding this comment

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

Although hm, they are intended as user identifier in server side apply, which means if we put a version inside that, we would prevent ourselves from starting to use server side apply, if we ever wanted.

Copy link
Member

@spiffxp spiffxp Feb 27, 2021

Choose a reason for hiding this comment

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

The nice thing about labels in my experience is they automatically get attached to metrics within google cloud monitoring, and I'm pretty sure kubernetes/kube-state-metrics does the same.

The more I think about it,prow.k8s.io/plank-version makes sense to start with, as that is the thing creating the pod. Some time later I could see prow.k8s.io/hook-version getting attached to ProwJobs created by hook, and propagating through (since it's entirely possible for ProwJobs to be created by something other than hook)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point and done

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2021
@chaodaiG
Copy link
Contributor Author

chaodaiG commented Mar 1, 2021

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2021
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, fejta, spiffxp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e465510 into kubernetes:master Mar 1, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 1, 2021
chaodaiG added a commit to chaodaiG/enhancements that referenced this pull request Mar 1, 2021
chaodaiG added a commit to chaodaiG/enhancements that referenced this pull request Mar 1, 2021
@chaodaiG chaodaiG deleted the prow-version-annotation branch March 1, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/prow/plank Issues or PRs related to prow's plank component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants