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 build strategy labels to task run #465

Merged
merged 5 commits into from
Nov 5, 2020
Merged

Add build strategy labels to task run #465

merged 5 commits into from
Nov 5, 2020

Conversation

HeavyWombat
Copy link
Contributor

When monitoring a system that uses https://github.com/shipwright-io/build, it would be helpful to make that the build work load can be identified very easy and correctly. At the moment, there is no direct way to differentiate between a buildrun that uses Kaniko or Buildpacks from a purely Kubernetes point of view by just looking at the actual task run pod.

Suggestion: Add more labels to the task run and therefore the task run pod to help to identify the respective work load.

---
apiVersion: v1
kind: Pod
metadata:
 annotations:
   pipeline.tekton.dev/release: v0.17.1
 creationTimestamp: "2020-11-03T15:23:07Z"
 labels:
   app.kubernetes.io/managed-by: tekton-pipelines
   build.build.dev/generation: "1"
   build.build.dev/name: test-kaniko-0
   buildrun.build.dev/generation: "1"
   buildrun.build.dev/name: test-kaniko-0
   clusterbuildstrategy.build.dev/name: kaniko
   clusterbuildstrategy.build.dev/generation: "1"

In order to keep the symmetry with build, and buildrun, the new labels would come as the same type of pair with a name and the generation.

To achieve this in code while not introducing additional code duplication, I decided to introduce an interface to encapsulate the common elements of both BuildStrategy and ClusterBuildStrategy. In theory, this could later help to reduce some code duplication that are in the code to date.

@zhangtbj
Copy link
Contributor

zhangtbj commented Nov 4, 2020

When monitoring a system that uses https://github.com/shipwright-io/build, it would be helpful to make that the build work load can be identified very easy and correctly.

BTW, Matthias, I still have a question, can you give an example, once we add these labels to the taskrun and taskrun pod, how can you use it in a monitoring system better? Or it is just easily for checking a taskrun/pod easily by manual?

@HeavyWombat
Copy link
Contributor Author

When monitoring a system that uses https://github.com/shipwright-io/build, it would be helpful to make that the build work load can be identified very easy and correctly.

BTW, Matthias, I still have a question, can you give an example, once we add these labels to the taskrun and taskrun pod, how can you use it in a monitoring system better? Or it is just easily for checking a taskrun/pod easily by manual?

Consider the use case that you have multiple cluster build strategies for Kaniko builds to address different sizing. Now you want to see whether this one Kaniko cluster build strategy sizing is correct. You need to see all builds done with exactly that cluster build strategy in order to evaluate the results. At the end, you would use something like this:

  • Search all container CPU/Memory quota usage metrics
  • Filter result set based on existence of label buildrun.build.dev/name (scope)
  • Filter result set based on existence of substring step-build in container name (scope)
  • Plot graph for each unique occurrence of a label clusterbuildstrategy.build.dev/name (segmentation)

@HeavyWombat HeavyWombat requested a review from zhangtbj November 4, 2020 10:02
In theory the label names should *not* change during runtime.

Make the label name constants.
The `buildstrategy_types.go` contains code that is common and should be externalised.

Move common code that applies to both kinds of strategies into a separate file.
There are two build strategies: The namespaced build strategy and the cluster
build strategy. They have some things in common, but are two types. This can
result in duplication in code to support both strategies in a function.

Introduce strategy interface that encapsulates the common things.
Change function `GenerateTaskRun` to use the build strategy interface rather
than the build steps in order to be more flexible for future changes.
Add section to add build strategy labels to the task run to include:
- name of the build strategy
- generation of the build strategy

Based on the name of the build strategy label, it can further be used
to differentiate between namespace based or cluster based build strategies.
Copy link
Contributor

@zhangtbj zhangtbj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@qu1queee
Copy link
Contributor

qu1queee commented Nov 5, 2020

let me take a look on this today

Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

I took a look on this, lgtm! This simplifies dealing with strategies scopes and helps to implement a single interface

@qu1queee
Copy link
Contributor

qu1queee commented Nov 5, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
@qu1queee
Copy link
Contributor

qu1queee commented Nov 5, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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

@openshift-merge-robot openshift-merge-robot merged commit 5d67ecb into shipwright-io:master Nov 5, 2020
@HeavyWombat HeavyWombat deleted the add/buildstrategy-label branch December 8, 2020 10:17
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants