Improve conversion to support multiple packages #9971

smarterclayton · 2015-06-17T20:27:32Z

OpenShift uses multiple API packages (types are split) which
Kube will also eventually have as we introduce more plugins.
These changes make the generators able to handle importing different
API object packages into a single generator function.

Other cleanups that make Kube only assumptions less hardcoded.

k8s-bot · 2015-06-17T21:02:31Z

GCE e2e build/test passed for commit 45b2899de912d95e48e9b783efc6f34c18b5915c.

saad-ali · 2015-06-17T21:59:27Z

@bgrant0607 for v1.0 triage

smarterclayton · 2015-06-17T22:03:03Z

This is not critical for us because we're carrying it as a patch - can be post 1.0

----- Original Message -----

@bgrant0607 for v1.0 triage

Reply to this email directly or view it on GitHub:
#9971 (comment)

bgrant0607 · 2015-06-19T04:43:38Z

Ref #1744

bgrant0607 · 2015-06-19T04:46:58Z

cc @wojtek-t

wojtek-t · 2015-06-19T07:34:41Z

I'm no the GOTO Amsterdam conference right now - I will try to look into it today evening.

wojtek-t · 2015-06-19T08:07:04Z

I took a very quick look now - generaly I like the direction of this change, but I will take a deeper look in the evening.

wojtek-t · 2015-06-19T08:08:01Z

pkg/runtime/conversion_generator.go

 	OverwritePackage(pkg, overwrite string)
+	AssumePrivateConversions()


What is the purpose of it? I don't see it used anywhere and I'm not sure if I understand why it is really needed.

Right now conversions for Kube objects are private methods. OpenShift embeds some of the Kube objects (pod, type meta, object meta) and so when the OpenShift private convertor functions they have to go back to scheme.Convert in order to get access to the method. Ideally we'd either make the core conversions public or visible through a fast path named accessor, but this was the compromise I made for now.

On Jun 19, 2015, at 4:08 AM, Wojciech Tyczynski notifications@github.com wrote:

In pkg/runtime/conversion_generator.go:

OverwritePackage(pkg, overwrite string)

AssumePrivateConversions()
What is the purpose of it? I don't see it used anywhere and I'm not sure if I understand why it is really needed.

—
Reply to this email directly or view it on GitHub.

OK I see - thanks for explanation.

wojtek-t · 2015-06-22T07:49:42Z

I took a deeper look (sorry for late response). I had just few minor comments.

smarterclayton · 2015-06-26T18:25:41Z

Comments addressed or answered.

k8s-bot · 2015-06-26T18:43:31Z

GCE e2e build/test passed for commit ab60585.

wojtek-t · 2015-06-29T07:37:56Z

LGTM - @smarterclayton do you want this to be merged before 1.0?
If so we need 2 leads to LGTM it according to our current policy.

smarterclayton · 2015-06-29T13:50:09Z

It's not critical for 1.0, although I believe it's low risk since it's a
prebuild thing.

On Jun 29, 2015, at 3:38 AM, Wojciech Tyczynski notifications@github.com
wrote:

LGTM - @smarterclayton https://github.com/smarterclayton do you want this
to be merged before 1.0?
If so we need 2 leads to LGTM it according to our current policy.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

wojtek-t · 2015-06-29T13:52:13Z

I agree it's relatively low risk.

@davidopp as the first lead to LGTM

davidopp · 2015-06-29T21:38:14Z

I don't really know this code at all, but why do you say it's low risk? The changes to conversion_generator.go and deep_copy_generator.go look substantial.

smarterclayton · 2015-06-30T00:58:00Z

The observed output on a large sample size of our objects is identical
(except for the desired outcome). So if there are bugs, they don't yield a
different output on our API. But it's not zero risk.

On Jun 29, 2015, at 5:38 PM, David Oppenheimer notifications@github.com
wrote:

I don't really know this code at all, but why do you say it's low risk? The
changes to conversion_generator.go and deep_copy_generator.go look
substantial.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

thockin · 2015-06-30T02:14:28Z

Unless this is directly tied to a v1 issue (it's not) it's not going in before 1.0. This looks good overall, but it can queue up for after 1.0

wojtek-t · 2015-06-30T08:09:08Z

@smarterclayton - in that case I think I will merge it jut after 1.0

thockin · 2015-06-30T18:01:33Z

low risk by itself is not sufficient criteria at this point :)

On Tue, Jun 30, 2015 at 1:09 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton - in that case I
think I will merge it jut after 1.0

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

smarterclayton · 2015-06-30T18:04:33Z

That's fine - this is just upstream refactors we carry.

On Tue, Jun 30, 2015 at 2:01 PM, Tim Hockin notifications@github.com
wrote:

low risk by itself is not sufficient criteria at this point :)

On Tue, Jun 30, 2015 at 1:09 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton - in that case I
think I will merge it jut after 1.0

—
Reply to this email directly or view it on GitHub
<
#9971 (comment)

.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

kubernetes/kubernetes#9971

k8s-bot · 2015-07-14T21:25:47Z

GCE e2e build/test passed for commit 4b5bc8c385f2a12afccb9dcaab26e5542eb758a1.

pmorie · 2015-07-22T21:44:34Z

@smarterclayton @thockin I'm not sure how this one is the in the 'checks passed' state -- I'm working on a change to make OpenShift run the upstream tests and found this as a breakage. The tests for pkg/runtime need to change since they call NewConversionGenerator. I tested in a local copy of this branch and the unit tests fail:

$ hack/test-go.sh pkg/runtime
Running tests for APIVersion: v1beta3 with etcdPrefix: registry
+++ [0722 17:42:48] Running unit tests without code coverage
# github.com/GoogleCloudPlatform/kubernetes/pkg/runtime_test
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/conversion_generation_test.go:37: not enough arguments in call to runtime.NewConversionGenerator
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/conversion_generation_test.go:50: not enough arguments in call to g.RegisterConversionFunctions
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/deep_copy_generation_test.go:36: not enough arguments in call to runtime.NewDeepCopyGenerator
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/deep_copy_generation_test.go:50: too many arguments in call to g.WriteImports
FAIL    github.com/GoogleCloudPlatform/kubernetes/pkg/runtime [build failed]
!!! Error in hack/test-go.sh:159
  'go test "${goflags[@]:+${goflags[@]}}" ${KUBE_RACE} ${KUBE_TIMEOUT} "${@+${@/#/${KUBE_GO_PACKAGE}/}}" "${testargs[@]:+${testargs[@]}}"' exited with status 2
Call stack:
  1: hack/test-go.sh:159 runTests(...)
  2: hack/test-go.sh:221 main(...)
Exiting with status 1

smarterclayton · 2015-07-23T01:01:38Z

Probably because of another change since this last built? Something had to have changed.

k8s-bot · 2015-07-23T21:38:25Z

GCE e2e build/test passed for commit 4b5bc8c385f2a12afccb9dcaab26e5542eb758a1.

wojtek-t · 2015-07-28T11:37:58Z

LGTM

@smarterclayton - can you pleas rebase?

pmorie · 2015-07-29T15:29:54Z

@smarterclayton possibly -- but we need to verify whether the unit tests will actually pass at this point

OpenShift uses multiple API packages (types are split) which Kube will also eventually have as we introduce more plugins. These changes make the generators able to handle importing different API object packages into a single generator function.

smarterclayton · 2015-07-29T21:09:39Z

Rebased

On Wed, Jul 29, 2015 at 11:30 AM, Paul Morie notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton possibly -- but we
need to verify whether the unit tests will actually pass at this point

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

k8s-bot · 2015-07-29T21:26:25Z

GCE e2e build/test passed for commit 1d41f5a.

wojtek-t · 2015-07-30T07:22:54Z

LGTM

…xible Improve conversion to support multiple packages

mikedanese · 2015-07-30T17:52:27Z

This broke unit tests. For some reason shippable decided not to run on this. 👎 I see that @pmorie commented on this.

smarterclayton · 2015-07-30T19:24:55Z

Fixed in #12033

On Thu, Jul 30, 2015 at 1:52 PM, Mike Danese notifications@github.com
wrote:

This broke unit tests. For some reason shippable decided not to run on
this. [image: 👎]

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

liggitt · 2015-08-10T15:49:03Z

pkg/runtime/conversion_generator.go

 	if err := g.generateConversionsBetween(inKey, outKey); err != nil {
 		return err
 	}
 	inValue := inType.Elem()
 	outValue := outType.Elem()
+	g.addImportByPath(inKey.PkgPath())
+	g.addImportByPath(outKey.PkgPath())


is this a typo? wasn't inKey/PkgPath() and outKey.PkgPath() already added above?

Needs to be out value, otherwise values from a different package don't get added to the imports.

I have 140 commits in this directory and I get a lot of cleanup reviews and want to be able to approve changes to hack/.golint_failures. 0e69316 delete unused cache b9c7007 enable token review when openapi is generated d5bbc35 make deps-approvers the approvers of sample-cli-plugin/Godeps 4186abf bzl: fix update-bazel.sh 7b47229 remove deprecated /proxy paths b973840 gke-certificates-controller: rm -rf 4961065 cluster: remove unused functions 1e2b644 cluster: move logging library to hack/ bef68f7 cluster: build gci mounter like other go binaries fe7ba9e kubeadm: use kubelet bootstrap instead of reimplementing 3c39173 fixit: break sig-cluster-lifecycle tests into subpackage 64f77eb enable race detection on integration tests cdcfa35 promote tls-bootstrap to beta ff4a814 migrate set generation to go genrule 3600d49 delete benchmark integration tests that don't work at all 21617a6 don't use build tags to mark integration tests 59fc948 bump rules_go and go version for bazel builds ba5c285 bazel: implement git build stamping ad42b42 move kubeadm api group testing to kubeadm package c8ce55f Revert "Merge pull request kubernetes#41132 from kubernetes/revert-40893-kubelet-auth" cbe5bd9 bump gazel to v14 86d9493 remove second CA used for kubelet auth in favor of webhook auth 04a7880 update repo local config to allow redirects from gopkg.in 44b7246 autogenerated 96c146c promote certificates.k8s.io to beta 087016d update gazel to v8 837eee4 pin gazel to v3 e225625 add a configuration for kubelet to register as a node with taints 584689f implement kubectl procelain csr commands 93f737e fix verify-bazel.sh on mac and windows 5dc7554 bazel: implement set-gen as a bazel genrule 61bd6aa remove docs/user-guide from bindata search path 224e32b make godep licenses/copyright check case insensitive 1cd2968 godep: vendor go-bindata d380cb1 fix realpath issue on mac ea632fa Revert "disable bazel build" 27116c6 rename build/ to build-tools/ ee15c80 disable bazel build 999c967 ignore BUILD in the flags-underscore.py validation b250a88 don't check BUILD file when verifying godeps a2eec91 add bazel presubmits to verify BUILD files are up to date c17a8a7 kubectl: apply prune should fallback to basic delete when a resource has no reaper 25e4dcc kubeadm: fix conversion macros and add kubeadm to round trip testing 6d17a87 kubectl: add two more test of kubectl apply --prune 62960aa add a test for kubectl apply --prune 6339d91 add a test to test-cmd.sh for apply -f with label selector b421bf4 build kube-discovery and kubeadm with release 0c76cf5 fix hack/verify-codegen.sh 9f379df add an option to controller-manager to auto approve all CSRs 95e2e29 move kube-dns to the cluster/addons/ directory f3de21b move integration tests into individual pacakges af0177e cleanup hack/verify-govet.sh to throttle process creation 2c93ea5 Merge pull request kubernetes#27289 from mikedanese/split-verify ee34c76 split verify out of unit/integration suite d046275 now that go test runs iteration loops, use that instead of custom executor 1ef1906 Merge pull request kubernetes#26197 from wonderfly/update_default_master_image fbf6bbc Merge pull request kubernetes#25596 from derekparker/inotify 3e1c0b5 run kube-addon-manager in a pod c5cc0c3 Merge pull request kubernetes#24277 from ihmccreery/upgrade-timeout 132c427 add linux fastbuild option to ./build/release.sh 2857baa use defaults in test-dockerized for etcd prefix and api versions 695211e Merge pull request kubernetes#21105 from caesarxuchao/watchCacheForIntegration 2172e0d Merge pull request kubernetes#21108 from mml/slow-flake 1478cf3 Merge pull request kubernetes#21090 from ihmccreery/feature-reboot b3172a4 kubelet: add a pidfile b1743a6 this is a manual reversion of kubernetes#20702 5b27055 Merge pull request kubernetes#19378 from ihmccreery/remove-update-jobs b743827 Merge pull request kubernetes#19659 from ihmccreery/timeout-reboot a6589f7 hack: ignore cluster/env.sh in boilerplate check f71657d retrofit the scheduler with the leader election client. bf763bb Merge pull request kubernetes#19498 from pwittrock/nodelabels 22cfa5e build: move some of hack/lib/ into a new cluster/lib/ b174fc9 Merge pull request kubernetes#18994 from bprashanth/flannel_suite a09d85b expose master count configuration in a cli option on apiserver c2753d7 bump ci go version to 1.5.2 0655e65 fall back to old behavior when deciding mem availablity during build 1d9d11c run kube-proxy in a static pod 91de3a1 cleanup some nits in hack/get-build.sh cd79c6c fix unbound variable error in hace/get-build.sh 5e64590 renable enable var to correct name and only use it when needed 9bdb860 add apigroup installer and tests e6d3b47 add componentconfig api group to autogen stuff 88008de Merge pull request kubernetes#16459 from mikedanese/enable-exp d28d134 Merge pull request kubernetes#16533 from ihmccreery/upgrade-test-fixes 3343522 enable deployment and daemonset in gce upgrade tests 7cbf249 Merge pull request kubernetes#15836 from wojtek-t/codecgen_from_godeps 92404e7 add upgrade test between 1.0 and 1.1 for gce 95b8394 Merge pull request kubernetes#15861 from mikedanese/upgrade-num-minion ece5779 increase NUM_MINIONS for jenkins gce upgrade test b8b35af actually promote daemonset simple test out of flaky and skip all daemonset tests in gke d379a36 copy directory not contents of directory 402e68e add slow test for terminated pod garbage collection c0943f1 add intermediate e2e runs to gce upgrade 10d56ff promote simple daemonset test out of flaky b635fc5 Merge pull request kubernetes#15228 from mesosphere/sttts-conformance-tags 392f33e Merge pull request kubernetes#14054 from mikedanese/register-master fa60bbe add flag to kubelet to ignore the cidr passed down by the apiserver on the master 53e14c7 diff all of pkg/ when verifying swagerspec instead of just pkg/api/ 05ef8ed Merge pull request kubernetes#15104 from mikedanese/ds-e2e fe820fc break up daemonset test into two tests 833be48 enable all experimental flags with one controller 905e971 be explicit about minion group size in upgrade test ae7d3d5 add gce-upgrade to jenkins/e2e.sh 376faea add pod garbage collection b0457be Merge pull request kubernetes#13058 from mvdan/go1.5 a48f218 Merge pull request kubernetes#13754 from tummychow/labels-deps 1fec199 Merge pull request kubernetes#13824 from kubernetes/revert-13547-hpa-kubeup fa40ced move contrib/for-tests to test/images f061875 updating all references in .sh scripts 8326697 rewrite all links to prs to k8s links fb02b33 fix build 8e48431 Revert "demote to flaky tests from parallel e2e" b56edd1 Merge pull request kubernetes#11727 from ZJU-SEL/build-nonstatic-hyperkube cf4cb1a Merge pull request kubernetes#10474 from kargakis/scale-multiple-controllers e376a09 demote to flaky service tests from parallel e2e 7c47d6b Merge pull request kubernetes#12009 from smarterclayton/fix_cmd_config 0269e2b Merge pull request kubernetes#11941 from GoogleCloudPlatform/enact_version_md 94a387d Revert "Improve conversion to support multiple packages" 1a613c4 Merge pull request kubernetes#9971 from smarterclayton/make_conversion_more_flexible 0ae48c4 Merge pull request kubernetes#11927 from wojtek-t/remove_shell_services 59a1dd4 Merge pull request kubernetes#11789 from mbforbes/nodesNetwork 6294070 Merge pull request kubernetes#11803 from wojtek-t/move_back_from_flaky daa6d4d Merge pull request kubernetes#11285 from liggitt/ca 9f16fd9 Merge pull request kubernetes#11860 from ingvagabund/delimiter-for-X-option-eparis c0acfbc Merge pull request kubernetes#11421 from nikhiljindal/exposeServcPort ae1c8e5 Merge pull request kubernetes#11737 from thockin/cleanup-remove-v1beta3 01ee1b8 Merge pull request kubernetes#10840 from jbeda/master d4d99de make mungedoc exit 1 if manual changes are needed and wire up erro message. 337772a fix all tests 055115a fake realpath, and standardize treatment of trailing / of dirs in gendoc b4514ee fix run-gendocs to point to new repo location c053b9a add documentation and script on how to get recent and "nightly" builds 719870f add publishing of latest-green.txt to jenkins e2e tests on success 1e130e0 remove --machines from code and docs dbb47fe remove e2e run before cluster upgrade de55e17 e2e test cluster stability during upgrade c9fcf45 fix bad cmd-test for patch. 9f91532 fix error where we can't use patch and add cmd-test for patch and file update

googlebot added the cla: yes label Jun 17, 2015

saad-ali assigned bgrant0607 Jun 17, 2015

saad-ali added this to the v1.0-post milestone Jun 18, 2015

bgrant0607 assigned wojtek-t and unassigned bgrant0607 Jun 19, 2015

bgrant0607 mentioned this pull request Jun 19, 2015

Supporting multiple API prefixes #10009

Closed

wojtek-t reviewed Jun 19, 2015
View reviewed changes

smarterclayton force-pushed the make_conversion_more_flexible branch from 45b2899 to ab60585 Compare June 26, 2015 18:25

bparees added a commit to bparees/origin that referenced this pull request Jul 7, 2015

UPSTREAM: add imports for map conversion types

665915b

kubernetes/kubernetes#9971

bparees added a commit to bparees/origin that referenced this pull request Jul 7, 2015

UPSTREAM: add imports for map conversion types (PR openshift#9971)

f427217

kubernetes/kubernetes#9971

smarterclayton force-pushed the make_conversion_more_flexible branch from ab60585 to 4b5bc8c Compare July 14, 2015 21:01

pmorie mentioned this pull request Jul 22, 2015

Run upstream unit tests from hack/test-go.sh openshift/origin#3849

Merged

bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015

smarterclayton added 3 commits July 29, 2015 17:05

Update deep copies

487fe2d

Update generated conversions

1d41f5a

smarterclayton force-pushed the make_conversion_more_flexible branch from 4b5bc8c to 1d41f5a Compare July 29, 2015 21:07

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2015

mikedanese added a commit that referenced this pull request Jul 30, 2015

Merge pull request #9971 from smarterclayton/make_conversion_more_fle…

1a613c4

…xible Improve conversion to support multiple packages

mikedanese merged commit 1a613c4 into kubernetes:master Jul 30, 2015

mikedanese mentioned this pull request Jul 30, 2015

Revert "Improve conversion to support multiple packages" #12027

Merged

liggitt reviewed Aug 10, 2015
View reviewed changes

mikedanese mentioned this pull request Feb 4, 2020

add myself to hack/OWNERS #87826

Merged

		OverwritePackage(pkg, overwrite string)
		AssumePrivateConversions()

Improve conversion to support multiple packages #9971

Improve conversion to support multiple packages #9971

Conversation

smarterclayton commented Jun 17, 2015

k8s-bot commented Jun 17, 2015

saad-ali commented Jun 17, 2015

smarterclayton commented Jun 17, 2015

bgrant0607 commented Jun 19, 2015

bgrant0607 commented Jun 19, 2015

wojtek-t commented Jun 19, 2015

wojtek-t commented Jun 19, 2015

wojtek-t Jun 19, 2015

Choose a reason for hiding this comment

smarterclayton Jun 19, 2015

Choose a reason for hiding this comment

wojtek-t Jun 22, 2015

Choose a reason for hiding this comment

wojtek-t commented Jun 22, 2015

smarterclayton commented Jun 26, 2015

k8s-bot commented Jun 26, 2015

wojtek-t commented Jun 29, 2015

smarterclayton commented Jun 29, 2015

wojtek-t commented Jun 29, 2015

davidopp commented Jun 29, 2015

smarterclayton commented Jun 30, 2015

thockin commented Jun 30, 2015

wojtek-t commented Jun 30, 2015

thockin commented Jun 30, 2015

smarterclayton commented Jun 30, 2015

k8s-bot commented Jul 14, 2015

pmorie commented Jul 22, 2015

smarterclayton commented Jul 23, 2015

k8s-bot commented Jul 23, 2015

wojtek-t commented Jul 28, 2015

pmorie commented Jul 29, 2015

smarterclayton commented Jul 29, 2015

k8s-bot commented Jul 29, 2015

wojtek-t commented Jul 30, 2015

mikedanese commented Jul 30, 2015

smarterclayton commented Jul 30, 2015

liggitt Aug 10, 2015

Choose a reason for hiding this comment

smarterclayton Aug 10, 2015

Choose a reason for hiding this comment