Extend Kettle build fields to be used for determining flakes #18197

MushuEE · 2020-07-07T19:21:34Z

Based on @spiffxp 's proposal for enabling flake detection on POD-Util jobs I am going through the process of adding new build fields. This will require a redeploy of kettle.

Also some boy-scouting of code

…uild This is based on spiffxp's proposal off issue kubernetes#14643

k8s-ci-robot · 2020-07-07T19:21:42Z

Hi @MushuEE. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

…e/add_new_kettle_fields

krzyzacy · 2020-07-07T20:11:23Z

/ok-to-test

krzyzacy · 2020-07-07T20:12:01Z

/assign @spiffxp

spiffxp

A few suggestions/questions

spiffxp · 2020-07-07T21:10:25Z

kettle/make_json.py

+        build['repo_commit'] = started.get('repo-commit', started.get('repo-version'))
+        build['repos'] = started.get('repos')


I recognize there is a dearth of comments here already, so you're just following the pattern. But I think it might be useful for future maintainers to comment where these are expected to come from, eg: a link back to the testgrid metadata struct or something

+1 I will try to make some back reference.

spiffxp · 2020-07-07T21:13:24Z

kettle/make_json.py


-    metadata = get_metadata()
+    metadata = get_metadata(build)


not sure I understand why this refactor?

So I didn't like doing this but I want to access the metadata repos key. I have access to all key-value pairs outside of this internal method but I would have to iterate over them because I no longer have a direct map to the repos value. The quickest solution I saw was to edit the map object while handling the metadata.

spiffxp · 2020-07-07T21:15:01Z

kettle/make_json.py

        if metadata:
+            if not build.get('repos'):
+                build['repos'] = metadata.get('repos')


updating build in a function named get_metadata feels surprising, maybe we could do this after the get_metadata call below?

Yeah ^ above comment kinda covers it. Just feels a little ugly to iterate over the list of pairs. I could return both the list AND the repos from get_metadata and assign that second value to build['repos']. Also in your doc you mention that Repos needs to be a nullable string. Do I need to join these repos with commas?

I majorly refactored. I decided to stay on the safe side and just remove all keys for None attrs so that there isn't anything crazy that might happen with nullable fields. I think this cleans things up and creates a structure for the build object so it is easier to map to the TestGrid Schema

This removes all None attrs to behaive like orginal where nullable values don't exist

spiffxp

/lgtm
/approve
this is much cleaner

k8s-ci-robot · 2020-07-10T17:50:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MushuEE, 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:

~~kettle/OWNERS~~ [spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spiffxp · 2020-07-10T17:52:23Z

I'm going to caution that right now, restarting kettle takes for. ev. er. So while part of me wants to say we should redeploy to verify this works, another part of me wants to suggest holding off until you've take a look at what startup involves. Your call.

spiffxp · 2020-07-10T17:53:39Z

You can see #17069 for a depiction of what restarting looked like the last time we had to

alvaroaleman · 2020-07-13T13:06:16Z

@MushuEE could you in the future please try to avoid merging a lot of tiny fixup commits that are not useful in the master branch history by either:

Squashing them as described e.G. here https://github.com/wprig/wprig/wiki/How-to-squash-commits
Telling Tide to squash them on merge by commenting with /label tide/merge-method-squash

MushuEE · 2020-07-13T20:50:44Z

@alvaroaleman, yes thank you. I will save these options in my notes.

Do some boyscouting and add new repos and repo_commit fields to the b…

dcc207d

…uild This is based on spiffxp's proposal off issue kubernetes#14643

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2020

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/jobs area/kettle area/testgrid sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 7, 2020

k8s-ci-robot requested review from chases2 and krzyzacy July 7, 2020 19:21

MushuEE added 3 commits July 7, 2020 12:24

Merge branch 'master' of github.com:kubernetes/test-infra into featur…

12c59b9

…e/add_new_kettle_fields

Pulled in change I didn't want to again

f126912

White Space

f907413

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2020

k8s-ci-robot assigned spiffxp Jul 7, 2020

Pylint not happy with returns

0f7e5c1

spiffxp reviewed Jul 7, 2020

View reviewed changes

Build object to represent Build and methods to generate it

b65cb49

This removes all None attrs to behaive like orginal where nullable values don't exist

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2020

Spelling

b13c387

chases2 removed their request for review July 8, 2020 18:26

k8s-ci-robot added the area/metrics label Jul 8, 2020

MushuEE force-pushed the feature/add_new_kettle_fields branch from ce52daa to b13c387 Compare July 8, 2020 20:34

spiffxp approved these changes Jul 10, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2020

k8s-ci-robot merged commit 1b90b9c into kubernetes:master Jul 10, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Kettle build fields to be used for determining flakes #18197

Extend Kettle build fields to be used for determining flakes #18197

MushuEE commented Jul 7, 2020

k8s-ci-robot commented Jul 7, 2020

krzyzacy commented Jul 7, 2020

krzyzacy commented Jul 7, 2020

spiffxp left a comment

spiffxp Jul 7, 2020

MushuEE Jul 7, 2020

spiffxp Jul 7, 2020

MushuEE Jul 7, 2020

spiffxp Jul 7, 2020

MushuEE Jul 7, 2020

MushuEE Jul 8, 2020

spiffxp left a comment

k8s-ci-robot commented Jul 10, 2020

spiffxp commented Jul 10, 2020

spiffxp commented Jul 10, 2020

alvaroaleman commented Jul 13, 2020

MushuEE commented Jul 13, 2020

		build['repo_commit'] = started.get('repo-commit', started.get('repo-version'))
		build['repos'] = started.get('repos')

Extend Kettle build fields to be used for determining flakes #18197

Extend Kettle build fields to be used for determining flakes #18197

Conversation

MushuEE commented Jul 7, 2020

k8s-ci-robot commented Jul 7, 2020

krzyzacy commented Jul 7, 2020

krzyzacy commented Jul 7, 2020

spiffxp left a comment

Choose a reason for hiding this comment

spiffxp Jul 7, 2020

Choose a reason for hiding this comment

MushuEE Jul 7, 2020

Choose a reason for hiding this comment

spiffxp Jul 7, 2020

Choose a reason for hiding this comment

MushuEE Jul 7, 2020

Choose a reason for hiding this comment

spiffxp Jul 7, 2020

Choose a reason for hiding this comment

MushuEE Jul 7, 2020

Choose a reason for hiding this comment

MushuEE Jul 8, 2020

Choose a reason for hiding this comment

spiffxp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 10, 2020

spiffxp commented Jul 10, 2020

spiffxp commented Jul 10, 2020

alvaroaleman commented Jul 13, 2020

MushuEE commented Jul 13, 2020