Fill in more data for started.json in initupload #11304

krzyzacy · 2019-02-14T22:59:02Z

(trying out the draft PR feature?)

/assign @fejta
cc @cjwagner @stevekuznetsov

resolve most of #10544 beside the node name

stevekuznetsov · 2019-02-14T23:34:48Z

Love the direction here

cjwagner · 2019-02-15T01:12:50Z

Looks good so far, the organization is great.
"backfill" had me confused for a second though. This isn't backfilling data into existing started.json files, but rather adding more fields to files created going forward.

krzyzacy · 2019-02-15T01:23:57Z

I thought backfill means fill in something we are missing from before, but please bikeshed my English :-)

I'll add a few more test cases and handle #10359 in a separate PR :-)

fejta

/lgtm
/hold

fejta · 2019-02-15T18:05:27Z

prow/sidecar/run.go

-	} else if len(spec.ExtraRefs) > 0 {
-		finished.Revision = getRevisionFromRef(&spec.ExtraRefs[0])
-	}
+	finished.Revision = downwardapi.GetRevisionFromSpec(spec)


I would like us to drop Revision as it is not useful:

job-version in finished.json needs to be set to what the job configures at runtime and puts in metadata.json

repo-version should be in started.json, set from the refs

we should have some other field that does a more complex recording of the refs (either repos or else a new refs field).

Revision just duplicates repo-version in the wrong location.

I added that when we were thinking we should only keep a timestamp in started.json. Since I also changed places like testgrid and gubernator, I need to make some clean-up PRs to follow up.

we should have some other field that does a more complex recording of the refs (either repos or else a new refs field).

Agreed, although I'm not sure that repos is sufficient. The complete ref data also includes ref names and any pull request numbers and SHAs. Including the complete serialized list of Refs structs in a refs field may be the best option.

Does that mean testgrid need to know a prow struct? Note that we also need to handle other hand-crafted started.json :-)

Maybe we can dump the refs into metadata?

fejta · 2019-02-15T18:10:07Z

prow/initupload/run.go

+func specToStarted(spec *downwardapi.JobSpec) gcs.Started {
+	started := gcs.Started{
+		Timestamp:   time.Now().Unix(),
+		RepoVersion: downwardapi.GetRevisionFromSpec(spec),


Looks like this should be RepoCommit, rather than RepoVersion?

test-infra/jenkins/bootstrap.py

Line 503 in 9bb0955

meta['repo-commit'] = commit.strip()

Looks like I goofed here:

test-infra/testgrid/metadata/job.go

Line 31 in 60bca00

RepoVersion string `json:"repo-version,omitempty"`

And checking testgrid's internal updater it has been looking for repo-commit, job-version and/or version since 2017. So we should use repo-commit, not repo-version.

which somewhat makes sense, as repo-commit is a git commit, whereas job-version is a random string set by the job.

hold on hold on - this is not true at all - see

test-infra/jenkins/bootstrap.py

Line 322 in aa8d6d1

data['repo-version'] = version

, repo-commit is a field in metadata in finished.json instead of started.json

although this repo-version in started.json is 👎 (

test-infra/jenkins/bootstrap.py

Lines 596 to 621 in aa8d6d1

def find_version(call):

"""Determine and return the version of the build."""

# TODO(fejta): once job-version is functional switch this to

# git rev-parse [--short=N] HEAD^{commit}

version_file = 'version'

if os.path.isfile(version_file):

# e2e tests which download kubernetes use this path:

with open(version_file) as fp:

return fp.read().strip()

version_script = 'hack/lib/version.sh'

if os.path.isfile(version_script):

cmd = [

'bash', '-c', (

"""

set -o errexit

set -o nounset

export KUBE_ROOT=.

source %s

kube::version::get_version_vars

echo $KUBE_GIT_VERSION

""" % version_script)

]

return call(cmd, output=True).strip()

return 'unknown'

) - so - I guess I can just do a rev-parse like #11330?

Should link this thread to my "WTF are these fields even" issue 😆

k8s-ci-robot · 2019-02-15T18:11:48Z

LGTM label has been added.

Git tree hash: bfa79f35e4144f12fdb42c972d252e94ad837b40

krzyzacy · 2019-02-16T02:10:40Z

/hold

k8s-ci-robot · 2019-02-18T19:22:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzyzacy

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:

~~prow/initupload/OWNERS~~ [krzyzacy]
~~prow/pod-utils/OWNERS~~ [krzyzacy]
~~prow/sidecar/OWNERS~~ [krzyzacy]

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

krzyzacy · 2019-02-18T19:22:45Z

/hold cancel
Kept RepoVersion

fejta

/lgtm

k8s-ci-robot · 2019-02-19T19:10:29Z

LGTM label has been added.

Git tree hash: bfa79f35e4144f12fdb42c972d252e94ad837b40

krzyzacy · 2019-02-19T21:54:07Z

/retest

krzyzacy · 2019-02-19T21:56:07Z

https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/test-infra/11304/pull-test-infra-verify-file-perms/2 ?

krzyzacy · 2019-02-19T22:08:32Z

/retest

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2019

k8s-ci-robot assigned fejta Feb 14, 2019

k8s-ci-robot requested review from cjwagner and stevekuznetsov February 14, 2019 22:59

krzyzacy force-pushed the metadata-repo branch from e2121f6 to b26ff24 Compare February 14, 2019 23:09

krzyzacy force-pushed the metadata-repo branch from b26ff24 to 7fc2fd7 Compare February 15, 2019 01:30

krzyzacy marked this pull request as ready for review February 15, 2019 01:34

krzyzacy force-pushed the metadata-repo branch from 7fc2fd7 to d1c0e4c Compare February 15, 2019 01:36

backfill data in started.json

7f85356

krzyzacy force-pushed the metadata-repo branch from d1c0e4c to 7f85356 Compare February 15, 2019 01:43

krzyzacy changed the title ~~[WIP] backfill data in started.json~~ Fill in more data for started.json in initupload Feb 15, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 15, 2019

fejta reviewed Feb 15, 2019

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 15, 2019

k8s-ci-robot added area/testgrid and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 15, 2019

krzyzacy mentioned this pull request Feb 16, 2019

Add a record.FinalSHA field to hold the end state of a cloned ref #11330

Merged

krzyzacy force-pushed the metadata-repo branch from 08fbf9b to 7f85356 Compare February 18, 2019 19:21

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2019

fejta reviewed Feb 19, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot merged commit cb32034 into kubernetes:master Feb 19, 2019

krzyzacy mentioned this pull request Feb 19, 2019

Use Refs.String() Method to populate Repo data in started #11374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill in more data for started.json in initupload #11304

Fill in more data for started.json in initupload #11304

krzyzacy commented Feb 14, 2019 •

edited

stevekuznetsov commented Feb 14, 2019

cjwagner commented Feb 15, 2019

krzyzacy commented Feb 15, 2019

fejta left a comment

fejta Feb 15, 2019

krzyzacy Feb 15, 2019

cjwagner Feb 15, 2019

krzyzacy Feb 15, 2019

fejta Feb 15, 2019

krzyzacy Feb 15, 2019

krzyzacy Feb 16, 2019

stevekuznetsov Feb 16, 2019

k8s-ci-robot commented Feb 15, 2019

krzyzacy commented Feb 16, 2019

k8s-ci-robot commented Feb 18, 2019

krzyzacy commented Feb 18, 2019

fejta left a comment

k8s-ci-robot commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

	def find_version(call):
	"""Determine and return the version of the build."""
	# TODO(fejta): once job-version is functional switch this to
	# git rev-parse [--short=N] HEAD^{commit}
	version_file = 'version'
	if os.path.isfile(version_file):
	# e2e tests which download kubernetes use this path:
	with open(version_file) as fp:
	return fp.read().strip()

	version_script = 'hack/lib/version.sh'
	if os.path.isfile(version_script):
	cmd = [
	'bash', '-c', (
	"""
	set -o errexit
	set -o nounset
	export KUBE_ROOT=.
	source %s
	kube::version::get_version_vars
	echo $KUBE_GIT_VERSION
	""" % version_script)
	]
	return call(cmd, output=True).strip()

	return 'unknown'

Fill in more data for started.json in initupload #11304

Fill in more data for started.json in initupload #11304

Conversation

krzyzacy commented Feb 14, 2019 • edited

stevekuznetsov commented Feb 14, 2019

cjwagner commented Feb 15, 2019

krzyzacy commented Feb 15, 2019

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2019

krzyzacy commented Feb 16, 2019

k8s-ci-robot commented Feb 18, 2019

krzyzacy commented Feb 18, 2019

fejta left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

krzyzacy commented Feb 19, 2019

krzyzacy commented Feb 14, 2019 •

edited