Use canonical meta types in prowjobs #6199

0xmichalis · 2018-01-09T17:20:52Z

This PR vendors k8s/apimachinery and updates the prowjob API to use the canonical meta types. I will follow-up with vendoring k8s/api and k8s/client-go so we can drop the core k8s APIs that we maintain in test-infra and start generating prowjob informers.

Part of #1986

0xmichalis · 2018-01-10T12:12:35Z

My latest try with dep seems more reasonable. What I did:

dep ensure
dep ensure -update k8s.io/apimachinery
dep prune
./hack/update-bazel.sh

There are still some dependencies that are updated but seem unrelated to the apimachinery repo which I am investigating.

0xmichalis · 2018-01-10T12:52:54Z

Gopkg.lock changes seem correct.

0xmichalis · 2018-01-10T15:39:57Z

Not sure how to debug the bazel failure.

stevekuznetsov · 2018-01-10T15:43:36Z

@Kargakis I am going to try to get dependencies working this morning

0xmichalis · 2018-01-12T10:26:58Z

Even more reasonable diff now. I just skipped dep ensure -update k8s.io/apimachinery, dep ensure seems to update the dependencies already. If we have green tests, then this should be ready to merge.

cjwagner

/lgtm

k8s-ci-robot · 2018-01-12T19:32:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, kargakis

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [cjwagner]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

cjwagner · 2018-01-12T19:35:43Z

/hold
Oh, the kube.ObjectMeta type can be removed now.

0xmichalis · 2018-01-13T01:37:56Z

Oh, the kube.ObjectMeta type can be removed now.

Still used by pods. I am planning on moving pods to use the canonical types in a follow-up and then we can remove our types.

fejta · 2018-01-14T23:20:46Z

Try hack/update-deps.sh? You may need to add some drop dep lines (I haven't yet automated this part of the script just yet) to this script

0xmichalis · 2018-01-15T15:21:43Z

The issue seems to be that some proto rules in the generated BUILD files inside apimachinery don't include vendor/ as a prefix. For example, here is an error from pull-test-infra-bazel:

W0115 15:13:28.474] ERROR: /workspace/k8s.io/test-infra/vendor/k8s.io/apimachinery/pkg/api/resource/BUILD:4:1: no such package 'k8s.io/apimachinery/pkg/util/intstr': BUILD file not found on package path and referenced by '//vendor/k8s.io/apimachinery/pkg/api/resource:resource_proto'

And here is the generated rule:

proto_library(
    name = "resource_proto",
    srcs = ["generated.proto"],
    visibility = ["//visibility:public"],
    deps = ["//k8s.io/apimachinery/pkg/util/intstr:intstr_proto"],
)

If //k8s.io/apimachinery/pkg/util/intstr:intstr_proto changes to //vendor/k8s.io/apimachinery/pkg/util/intstr:intstr_proto, everyone is happy. There are also a couple of other issues but I would expect those to go away by dropping dep lines in the script @fejta mentioned.

May it be an issue with how we use bazel? @ixdy @BenTheElder

0xmichalis · 2018-01-16T16:36:44Z

I did some digging today and came up with a couple of different issues:

gazelle does not know how to handle vendored proto rules: Gazelle doesn't know how to handle vendored proto deps bazel-contrib/bazel-gazelle#33
keep directives are broken: keep directives are getting rewritten incorrectly bazel-contrib/bazel-gazelle#92

0xmichalis · 2018-01-16T17:13:52Z

Any idea why/how we generate BUILD-e files?

ixdy · 2018-01-16T19:56:41Z

At least somewhat related to the vendor/ issue is bazel-contrib/bazel-gazelle#63, but I'm not sure if that's the same issue that's happening here.

We might be running into issues with k/k (and thus apimachinery) using gazelle's "legacy" proto mode, while test-infra now uses the "normal" mode. I will investigate that theory.

What are BUILD-e files?

ixdy · 2018-01-16T19:57:53Z

@BenTheElder what are these mysterious BUILD-e files?

0xmichalis · 2018-01-18T10:10:17Z

Fixed the remaining issues since kazel is updated. Hoping for green tests now 🙏 🙏 🙏

0xmichalis · 2018-01-18T10:13:09Z

prow/kube/prowjob.go

-	Metadata   ObjectMeta    `json:"metadata,omitempty"`
-	Spec       ProwJobSpec   `json:"spec,omitempty"`
-	Status     ProwJobStatus `json:"status,omitempty"`
+	metav1.TypeMeta   `json:",inline"`


Hrm, I just realized this is not supported in Go yet: golang/go#6213

k8s is using a forked version of the json package.

Seems that k8s is using https://github.com/json-iterator/go for JSON marshalling/unmarshalling.

@deads2k @sttts what JSON library am I supposed to use outside of k8s in order to inline typemeta?

Or this is something I shouldn't care about since it's supposedly handled by client-go?

@deads2k @sttts what JSON library am I supposed to use outside of k8s in order to inline typemeta?

jsoniter doesn't handle some cases of embedding. As I recall, it didn't work for openshift downstream. In theory, I think client-go will sort it for you. If not, I'd ask @thockin

0xmichalis · 2018-01-19T13:11:40Z

I didn't have a lot of courage to deal with bazel this morning so I ended up creating a prowjob controller that uses client-go and informers in a separate repo to play around: https://github.com/kargakis/prowjob-controller

I changed ProwJob to use v1.PodSpec vs our own type and metav1.Time vs time.Time
GIven the correct directory structure, generation works like a breeze (thanks @sttts and the rest of the folks for making it happen!)
the controller seems to work as expected but I still need to verify watching of individual events

0xmichalis · 2018-01-19T13:36:49Z

Also, the binary size of the controller, including k8s/api, k8s/apimachinery, and k8s/client-go is 34MB. Go seems to compile only what's actually used.

0xmichalis · 2018-01-20T01:43:04Z

@ixdy I am updating gazelle to pick up bazel-contrib/bazel-gazelle#78, ptal

0xmichalis · 2018-01-20T02:04:21Z

Finally green tests! 🎉

0xmichalis · 2018-01-20T02:10:50Z

prow/kube/prowjob.go

-	ObjectMeta ObjectMeta    `json:"metadata,omitempty"`
-	Spec       ProwJobSpec   `json:"spec,omitempty"`
-	Status     ProwJobStatus `json:"status,omitempty"`
+	APIVersion        string `json:"apiVersion,omitempty"`


I am leaving type meta as is for now. I will change to metav1.TypeMeta once we switch to use client-go (otherwise, it won't work).

0xmichalis · 2018-01-20T02:11:27Z

@spxtr fyi

spxtr · 2018-01-20T03:36:38Z

I endorse this PR.

Go seems to compile only what's actually used.

Unfortunately, because the reflect package exists, go cannot compile only what's actually used.

BenTheElder · 2018-01-22T19:31:16Z

@Kargakis this is pretty awesome, sadly it now needs another rebase after #6356. 😞

Do not switch type meta yet since struct inlining does not work with Go's encoding/json.

0xmichalis · 2018-01-22T23:20:06Z

Rebased

0xmichalis · 2018-01-23T21:09:04Z

Green light on updating gazelle at #6388 (comment)

@fejta hopefully this won't break your update-deps.sh change.

/hold cancel

fejta · 2018-01-23T21:10:30Z

Happy to deal with the fallout :)

fejta · 2018-01-23T21:10:37Z

thanks for getting this in!

BenTheElder · 2018-01-23T21:43:28Z

whoo! thanks @Kargakis! 🎉

fejta · 2018-01-23T21:47:06Z

I guess "happy" would be more accurate here 😉

0xmichalis requested a review from cjwagner as a code owner January 9, 2018 17:20

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 9, 2018

cjwagner approved these changes Jan 12, 2018

View reviewed changes

k8s-ci-robot assigned cjwagner Jan 12, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2018

0xmichalis mentioned this pull request Jan 16, 2018

Gazelle doesn't know how to handle vendored proto deps bazel-contrib/bazel-gazelle#33

Open

0xmichalis changed the title ~~[WIP] Use canonical meta types in prowjobs~~ Use canonical meta types in prowjobs Jan 18, 2018

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 Jan 18, 2018

0xmichalis commented Jan 18, 2018

View reviewed changes

0xmichalis mentioned this pull request Jan 18, 2018

Rename Metadata to ObjectMeta #6339

Merged

0xmichalis mentioned this pull request Jan 19, 2018

Use client-go and core API types #1986

Closed

0xmichalis commented Jan 20, 2018

View reviewed changes

0xmichalis requested a review from ixdy January 22, 2018 10:03

0xmichalis added 4 commits January 22, 2018 23:49

Use canonical meta types in prowjobs

9964b62

Do not switch type meta yet since struct inlining does not work with Go's encoding/json.

Use updated version of gazelle

e4a155c

Fix unit tests

8df1991

Vendor k8s/apimachinery

34ac93c

This was referenced Jan 22, 2018

Use selector tooling from its canonical location #6375

Closed

Fix hack/update-deps.sh to ignore go_binary rules in vendor #6388

Merged

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

k8s-ci-robot merged commit 600e402 into kubernetes:master Jan 23, 2018

0xmichalis deleted the import-canonical-k8s-api branch January 23, 2018 21:26

Use canonical meta types in prowjobs #6199

Use canonical meta types in prowjobs #6199

Conversation

0xmichalis commented Jan 9, 2018 • edited Loading

0xmichalis commented Jan 10, 2018

0xmichalis commented Jan 10, 2018

0xmichalis commented Jan 10, 2018

stevekuznetsov commented Jan 10, 2018

0xmichalis commented Jan 12, 2018

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 12, 2018

cjwagner commented Jan 12, 2018

0xmichalis commented Jan 13, 2018

fejta commented Jan 14, 2018

0xmichalis commented Jan 15, 2018

0xmichalis commented Jan 16, 2018

0xmichalis commented Jan 16, 2018

ixdy commented Jan 16, 2018

ixdy commented Jan 16, 2018

0xmichalis commented Jan 18, 2018

0xmichalis Jan 18, 2018

Choose a reason for hiding this comment

0xmichalis Jan 18, 2018

Choose a reason for hiding this comment

0xmichalis Jan 18, 2018

Choose a reason for hiding this comment

0xmichalis Jan 18, 2018

Choose a reason for hiding this comment

deads2k Jan 18, 2018

Choose a reason for hiding this comment

0xmichalis commented Jan 19, 2018

0xmichalis commented Jan 19, 2018

0xmichalis commented Jan 20, 2018

0xmichalis commented Jan 20, 2018

0xmichalis Jan 20, 2018

Choose a reason for hiding this comment

0xmichalis commented Jan 20, 2018

spxtr commented Jan 20, 2018

BenTheElder commented Jan 22, 2018

0xmichalis commented Jan 22, 2018

0xmichalis commented Jan 23, 2018

fejta commented Jan 23, 2018

fejta commented Jan 23, 2018

BenTheElder commented Jan 23, 2018

fejta commented Jan 23, 2018

0xmichalis commented Jan 9, 2018 •

edited

Loading