Switch to internalclientset #11916

soltysh · 2016-11-15T14:34:21Z

This is switching us to use internalclientset instead of plain client.

@ncdc we need this land before the rebase
@bparees for the build parts
@Kargakis for the deployment parts

soltysh · 2016-11-15T14:35:05Z

[test]

0xmichalis · 2016-11-15T14:37:06Z

pkg/deploy/api/test/ok.go

@@ -22,7 +22,9 @@ const (
 func OkDeploymentConfig(version int64) *deployapi.DeploymentConfig {
 	return &deployapi.DeploymentConfig{
 		ObjectMeta: kapi.ObjectMeta{
-			Name: "config",
+			Name:      "config",
+			Namespace: kapi.NamespaceDefault,


Why do you need this change? It will probably break tests.

fake.NewSimpleClientset requires to have namespace set, it panicked without it. The unit tests are green already ;)

0xmichalis · 2016-11-15T14:39:06Z

pkg/deploy/registry/deploylog/rest.go

@@ -221,7 +225,7 @@ func (r *REST) returnApplicationPodName(target *kapi.ReplicationController) (str
 	selector := labels.Set(target.Spec.Selector).AsSelector()
 	sortBy := func(pods []*kapi.Pod) sort.Interface { return controller.ByLogging(pods) }

-	pod, _, err := kcmdutil.GetFirstPod(r.pn, target.Namespace, selector, r.timeout, sortBy)
+	pod, _, err := kcmdutil.GetFirstPod(r.oldPn, target.Namespace, selector, r.timeout, sortBy)


Can you open a follow-up upstream to switch this to use a clientset?

Or issue is just fine

They already are: https://github.com/kubernetes/kubernetes/blob/2bd8bb26995a82282afd63c57173df00c781015d/pkg/kubectl/cmd/util/factory.go#L901

cool, so we just need the rebase

That's why this PR is a prereq 😜

ncdc · 2016-11-15T15:06:27Z

cc @deads2k @liggitt @sttts

bparees · 2016-11-15T17:52:37Z

pkg/build/controller/image_change_controller_test.go

@@ -305,7 +305,8 @@ func mockBuildConfig(baseImage, triggerImage, repoName, repoTag string) *buildap
 	dockerfile := "FROM foo"
 	return &buildapi.BuildConfig{
 		ObjectMeta: kapi.ObjectMeta{
-			Name: "testBuildCfg",
+			Name:      "testBuildCfg",
+			Namespace: kapi.NamespaceDefault,


is something requiring we set this namespace now?

Yes, the fake clientset is now namespace aware, without it you'll get nasty panic complaining that you're trying to use namespaced resource without namespace.

bparees · 2016-11-15T17:54:20Z

pkg/build/generator/generator_test.go

@@ -741,7 +741,7 @@ func TestGenerateBuildFromConfig(t *testing.T) {
 	bc := &buildapi.BuildConfig{
 		ObjectMeta: kapi.ObjectMeta{
 			Name:      "test-build-config",
-			Namespace: "test-namespace",
+			Namespace: kapi.NamespaceDefault,


why?

The new testing mock used by generated fake clients requires that namespaced resources come in with namespaces. It doesn't understand defaulting.

and we don't consider that an upstream bug? ok.....

also that still doesn't explain why this object which had a valid namespace, is being changed to use a different namespace?

(I don't think this particular test actually cares what the namespace value is, but it seems odd to change it)

Differences between tests caused errors, I've unified all to use kapi.NamespaceDefault. Even if test doesn't care about namespace the fake clientset will complain, see my previous comment.

bparees · 2016-11-15T17:56:57Z

I have concerns/questions about all the default namespacing you're introducing in the tests, otherwise lgtm.

sttts · 2016-11-16T08:38:10Z

pkg/deploy/registry/deploylog/rest_test.go

@@ -144,15 +146,49 @@ func mockREST(version, desired int64, status api.DeploymentStatus) *REST {
 				Phase: kapi.PodRunning,
 			},
 		}
-		fakePn.PrependReactor("get", "pods", func(action ktestclient.Action) (handled bool, ret runtime.Object, err error) {
+		oldPn.PrependReactor("get", "pods", func(action ktestclient.Action) (handled bool, ret runtime.Object, err error) {


What's the reason for this pretty big change here?

rolling and recreate invoke commands from k8s.io/kubernetes/pkg/kubectl package, which is using clientset only in 1.5, which we'll get after the rebase. Example line:
kubectl.ScalerFor(kapi.Kind("ReplicationController"), oldClient) for that reason we need old k8s client in only certain places, which will be removed after the rebase. See my discussion with Michalis in this PR.

soltysh · 2016-11-18T18:25:45Z

@ncdc I think this is ready, eventual leftovers we'll handle afterwards/during the rebase itself.

I need lots of eyes on your area:
@knobunc for networking stuff
@bparees for build stuff
@deads2k @liggitt for auth stuff
@smarterclayton overall

smarterclayton · 2016-11-18T18:39:28Z

Are there any changes here that aren't just simple refactoring?

ncdc · 2016-11-18T20:12:51Z

@soltysh needs rebase. Last failure flaked on #11775 and this:

--- FAIL: TestPullThroughInsecure (4.14s)
    dockerregistry_pullthrough_test.go:175: External registry got GET /v2/
    dockerregistry_pullthrough_test.go:175: External registry got GET /v2/test/testrepo/manifests/testtag
    dockerregistry_pullthrough_test.go:175: External registry got HEAD /v2/test/testrepo/manifests/testtag
    dockerregistry_pullthrough_test.go:175: External registry got GET /v2/test/testrepo/manifests/sha256:958608f8ecc1dc62c93b6c610f3a834dae4220c9642e6e8b4e0f2b3ad7cbd238
    dockerregistry_pullthrough_test.go:246: unexpected status 0: unversioned.Status{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ListMeta:unversioned.ListMeta{SelfLink:"", ResourceVersion:""}, Status:"Failure", Message:"Internal error occurred: invalid character '\\n' in string literal", Reason:"InternalError", Details:(*unversioned.StatusDetails)(0xc825c3b360), Code:500}

soltysh · 2016-11-18T20:44:17Z

Are there any changes here that aren't just simple refactoring?

Most is just simple refactoring, but there are a few places where bigger changes were required.

soltysh · 2016-11-18T22:01:45Z

Rebased and fixed the test Andy pointed out.

liggitt · 2016-11-18T22:11:25Z

pkg/client/testclient/testclient.go

+	for _, o := range objs {
+		if itemMeta, err := meta.Accessor(o); err == nil {
+			// we can't set namespace for everything, some resources are non-namespaced
+			if _, ok := o.(*projectapi.Project); ok {


why special case this? We have a registry of root scope vs namespace scope

This all used to be handled by the RESTMapper. Why are we having to special case it now?

liggitt · 2016-11-18T22:12:06Z

pkg/cmd/cli/describe/projectstatus_test.go

-				"on fedora:23",
-				"-> repo-base:latest",
+				"on istag/fedora:23",
+				"-> istag/repo-base:latest",


why did this output change?

I was actually wondering why it wasn't like that before, looking at other test cases.

liggitt · 2016-11-18T22:12:17Z

pkg/cmd/cli/describe/projectstatus_test.go

@@ -313,9 +312,9 @@ func TestProjectStatus(t *testing.T) {
 			ErrFn: func(err error) bool { return err == nil },
 			Contains: []string{
 				"In project example on server https://example.com:8443\n",
-				"svc/galera[default] (headless):3306",
+				"svc/galera (headless):3306",


same question

Here, situation is a bit more complicated. Fake clientset forces to always use namesapces, previously it was not required. I've added them and previously we've had parts of the petset created in default namespace and now it's all in one. Thus the change.

smarterclayton · 2016-11-18T23:25:02Z

I asked because the mechanical transform need to be separate from the
important changes somehow so we can focus on what needs reviewing.

On Nov 18, 2016, at 5:31 PM, Maciej Szulik notifications@github.com wrote:

@soltysh commented on this pull request.

In pkg/cmd/cli/describe/projectstatus_test.go
#11916:

@@ -313,9 +312,9 @@ func TestProjectStatus(t *testing.T) {
ErrFn: func(err error) bool { return err == nil },
Contains: []string{
"In project example on server https://example.com:8443\n",
          "svc/galera[default](headless):3306",
          "svc/galera (headless):3306",

Here, situation is a bit more complicated. Fake clientset forces to always
use namesapces, previously it was not required. I've added them and
previously we've had parts of the petset created in default namespace and
now it's all in one. Thus the change.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11916, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7btI0LTWa71bpzWB42Fk9a04eJCks5q_ic0gaJpZM4Kyl-W
.

soltysh · 2016-11-18T23:36:57Z

I asked because the mechanical transform need to be separate from the important changes somehow so we can focus on what needs reviewing.

OK, I'll update that PR into what needs review and the rest.

soltysh · 2016-11-18T23:38:37Z

Looks like one another error:

--- FAIL: TestDeployScale (4.23s)
    deploy_scale_test.go:48: Couldn't create DeploymentConfig: the namespace of the provided object does not match the namespace sent on the request &api.DeploymentConfig{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"config", GenerateName:"", Namespace:"default", SelfLink:"/oapi/v1/namespaces/default/deploymentconfig/config", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil), ClusterName:""}, Spec:api.DeploymentConfigSpec{Strategy:api.DeploymentStrategy{Type:"Recreate", CustomParams:(*api.CustomDeploymentStrategyParams)(nil), RecreateParams:(*api.RecreateDeploymentStrategyParams)(0xc82ffda3c0), RollingParams:(*api.RollingDeploymentStrategyParams)(nil), Resources:api.ResourceRequirements{Limits:api.ResourceList{"cpu":resource.Quantity{i:resource.int64Amount{value:10, scale:0}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"10", Format:"DecimalSI"}, "memory":resource.Quantity{i:resource.int64Amount{value:10, scale:9}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"10G", Format:"DecimalSI"}}, Requests:api.ResourceList(nil)}, Labels:map[string]string(nil), Annotations:map[string]string(nil)}, MinReadySeconds:0, Triggers:[]api.DeploymentTriggerPolicy{}, Replicas:1, RevisionHistoryLimit:(*int32)(nil), Test:false, Paused:false, Selector:map[string]string{"a":"b"}, Template:(*api.PodTemplateSpec)(0xc82d4cfa40)}, Status:api.DeploymentConfigStatus{LatestVersion:0, ObservedGeneration:0, Replicas:0, UpdatedReplicas:0, AvailableReplicas:0, UnavailableReplicas:0, Details:(*api.DeploymentDetails)(nil), Conditions:[]api.DeploymentCondition(nil)}}
    etcd.go:99: dumping etcd to "/tmp/openshift/test-integration//etcd-dump-runtime.call32.json"
FAIL

but I think I'll leave that for tomorrow to handle.

soltysh · 2016-11-19T19:49:39Z

Fixed the test and re-organized into 2 commits boring and interesting, per Clayton request.

@smarterclayton, @liggitt @ncdc ptal

soltysh · 2016-11-22T20:54:37Z

Rebased and fixed the testclient.go @liggitt ptal

liggitt · 2016-11-22T21:40:52Z

@soltysh ReadObjectsFromPath change LGTM, thanks

soltysh · 2016-11-22T21:50:15Z

Thx, @smarterclayton anything more or we're good to merge this as is?

soltysh · 2016-11-23T10:04:14Z

Rebased.

soltysh · 2016-11-23T11:49:00Z

Flake #10988

[test]

soltysh · 2016-11-23T13:29:38Z

Flake #10951

[test]

soltysh · 2016-11-23T16:17:34Z

Flake #10988

[test]

soltysh · 2016-11-23T21:23:47Z

Flake #10663

[test]

soltysh · 2016-11-24T07:47:51Z

Rebased.

soltysh · 2016-11-24T10:10:37Z

Flake #11016

[test]

openshift-bot · 2016-11-24T10:13:18Z

Evaluated for origin test up to 97e6f1d

openshift-bot · 2016-11-24T11:40:02Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11687/) (Base Commit: 571845b)

soltysh · 2016-11-24T12:05:03Z

@smarterclayton anything more or we're good to go as is?

smarterclayton · 2016-11-24T21:26:51Z

Lgtm [merge]

soltysh · 2016-11-25T10:20:44Z

Flake #8502.

[merge]

soltysh · 2016-11-25T14:25:39Z

Flake #10951

[merge]

openshift-bot · 2016-11-25T14:29:36Z

Evaluated for origin merge up to 97e6f1d

openshift-bot · 2016-11-25T14:29:44Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11705/) (Base Commit: e115e25) (Image: devenv-rhel7_5418)

soltysh mentioned this pull request Nov 15, 2016

1.5 k8s rebase prereqs #11748

Closed

2 tasks

0xmichalis reviewed Nov 15, 2016

View reviewed changes

bparees reviewed Nov 15, 2016

View reviewed changes

sttts reviewed Nov 16, 2016

View reviewed changes

soltysh force-pushed the remove_unversionedclient branch 3 times, most recently from c304cb2 to ddd2cf5 Compare November 18, 2016 18:23

soltysh changed the title ~~[WIP] Switch to internalclientset~~ Switch to internalclientset Nov 18, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2016

soltysh force-pushed the remove_unversionedclient branch from ddd2cf5 to 93ce1d9 Compare November 18, 2016 22:00

liggitt reviewed Nov 18, 2016

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2016

soltysh force-pushed the remove_unversionedclient branch from 93ce1d9 to 7daf1c6 Compare November 19, 2016 19:48

soltysh force-pushed the remove_unversionedclient branch from 7daf1c6 to c7ad80c Compare November 19, 2016 20:57

soltysh force-pushed the remove_unversionedclient branch 2 times, most recently from b0cbac2 to 199f3c4 Compare November 22, 2016 20:54

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2016

soltysh force-pushed the remove_unversionedclient branch from 199f3c4 to bacdba6 Compare November 23, 2016 10:03

soltysh removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2016

soltysh added 2 commits November 24, 2016 08:44

Switch to internalclientset - boring changes

97e6f1d

Switch to internalclientset - interesting changes

7cf64f4

soltysh force-pushed the remove_unversionedclient branch from bacdba6 to 7cf64f4 Compare November 24, 2016 07:47

soltysh removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2016

openshift-bot merged commit 6f7e2fd into openshift:master Nov 25, 2016

soltysh deleted the remove_unversionedclient branch November 28, 2016 09:02

Switch to internalclientset #11916

Switch to internalclientset #11916

Conversation

soltysh commented Nov 15, 2016

soltysh commented Nov 15, 2016

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

ncdc commented Nov 15, 2016

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

bparees commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Nov 18, 2016

smarterclayton commented Nov 18, 2016

ncdc commented Nov 18, 2016

soltysh commented Nov 18, 2016

soltysh commented Nov 18, 2016

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

smarterclayton commented Nov 18, 2016

@soltysh commented on this pull request.

soltysh commented Nov 18, 2016

soltysh commented Nov 18, 2016

soltysh commented Nov 19, 2016

soltysh commented Nov 22, 2016

liggitt commented Nov 22, 2016

soltysh commented Nov 22, 2016

soltysh commented Nov 23, 2016

soltysh commented Nov 23, 2016

soltysh commented Nov 23, 2016

soltysh commented Nov 23, 2016

soltysh commented Nov 23, 2016

soltysh commented Nov 24, 2016

soltysh commented Nov 24, 2016

openshift-bot commented Nov 24, 2016

openshift-bot commented Nov 24, 2016

soltysh commented Nov 24, 2016

smarterclayton commented Nov 24, 2016

soltysh commented Nov 25, 2016

soltysh commented Nov 25, 2016

openshift-bot commented Nov 25, 2016

openshift-bot commented Nov 25, 2016 • edited

openshift-bot commented Nov 25, 2016 •

edited