Initialize cloud provider in node #11620

jsafrane · 2016-10-27T15:07:49Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1379600

Be careful with review, I don't know these parts of OpenShift very well. I tested AWS and GCE and it fixes the bug there.

jsafrane · 2016-10-27T15:09:08Z

[test]

ncdc · 2016-10-27T15:10:56Z

pkg/cmd/server/kubernetes/node_config.go

+			return nil, err
+		}
+		if cloud != nil {
+			glog.V(2).Infof("Successfully initialized cloud provider: %q from the config file: %q\n", server.CloudProvider, server.CloudConfigFile)


No need for \n

fixed, however master prints with '\n': https://github.com/openshift/origin/blob/master/pkg/cmd/server/kubernetes/master_config.go#L213

ncdc · 2016-10-27T15:12:31Z

cc @liggitt @deads2k @pmorie @derekwaynecarr @smarterclayton @mfojtik

jsafrane · 2016-10-27T15:17:15Z

Be careful with review, I don't know these parts of OpenShift very well.

I mean, it does the right thing, I am not sure it does it on the right place.

smarterclayton · 2016-10-27T15:25:44Z

pkg/cmd/server/kubernetes/node_config.go

@@ -214,6 +216,18 @@ func BuildKubernetesNodeConfig(options configapi.NodeConfig, enableProxy, enable
 		return nil, err
 	}

+	// Initialize cloud provider
+	if len(server.CloudProvider) > 0 && server.CloudProvider != v1alpha1.AutoDetectCloudProvider {


I'd like to have at least one test that validates this is setup or executed so it doesn't regress.

I added some simple unit tests for server.CloudProvider parsing, however there is not a single unit test that would test whole BuildKubernetesNodeConfig. This function does many decisions that are not tested anywhere and building a cloud provider is just one of its duties.

IMO, there should be e2e tests that would catch GCE or AWS volume plugin throwing errors when mounting a volume.

openshift-bot · 2016-10-31T14:05:26Z

Evaluated for origin test up to 8c698c5

ncdc · 2016-10-31T14:10:21Z

pkg/cmd/server/kubernetes/node_config_test.go

+	server := kubeletoptions.NewKubeletServer()
+	server.CloudProvider = providerName
+
+	cloud, err := buildCloudProvider(server)


@smarterclayton are you looking for a test that ensures that BuildKubernetesNodeConfig ends up with the cloud provider filled in correctly?

smarterclayton · 2016-10-31T14:13:27Z

Yes.

On Mon, Oct 31, 2016 at 10:10 AM, Andy Goldstein [email protected]
wrote:

@ncdc commented on this pull request.

In pkg/cmd/server/kubernetes/node_config_test.go
#11620 (review)
:

@@ -157,3 +160,49 @@ func TestProxyConfig(t *testing.T) {
}

}
+
+func TestBuildCloudProviderFake(t *testing.T) {
providerName := "fake"

cloudprovider.RegisterCloudProvider(providerName, func(config io.Reader) (cloudprovider.Interface, error) {
return &fake.FakeCloud{}, nil
})
+

server := kubeletoptions.NewKubeletServer()

server.CloudProvider = providerName
+

cloud, err := buildCloudProvider(server)
@smarterclayton https://github.com/smarterclayton are you looking for a
test that ensures that BuildKubernetesNodeConfig ends up with the cloud
provider filled in correctly?

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

ncdc · 2016-10-31T14:15:10Z

@jsafrane do you think you could take a stab at writing a new test that calls BuildKubernetesNodeConfig?

jsafrane · 2016-10-31T14:24:17Z

@ncdc no, I really do not feel comfortable testing this code. I've barely seen it and it looks pretty complex to me.

ncdc · 2016-10-31T14:24:50Z

I'll write one

On Mon, Oct 31, 2016 at 10:24 AM, Jan Šafránek [email protected]
wrote:

@ncdc https://github.com/ncdc no, I really do not feel comfortable
testing this code. I've barely seen it and it looks pretty complex to me.

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

jsafrane · 2016-10-31T14:35:33Z

Thanks! I'd like to point out that this PR blocks testing of OpenShift on GCE and AWS, our QE literally cannot do anything there and there is plenty of new stuff to test in 1.4.

ncdc · 2016-10-31T14:38:15Z

Let's merge this as-is and I'll do a follow-up with the test (assuming it's not impossible).

LGTM. Let's get 1 more +1 and then we'll tag - @liggitt @derekwaynecarr @mfojtik @pweil- @deads2k anyone? 😄

smarterclayton · 2016-10-31T14:49:47Z

[merge]

On Mon, Oct 31, 2016 at 10:38 AM, Andy Goldstein [email protected]
wrote:

Let's merge this as-is and I'll do a follow-up with the test (assuming
it's not impossible).

LGTM. Let's get 1 more +1 and then we'll tag - @liggitt
https://github.com/liggitt @derekwaynecarr
https://github.com/derekwaynecarr @mfojtik https://github.com/mfojtik
@pweil- https://github.com/pweil- @deads2k https://github.com/deads2k
anyone? 😄

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

openshift-bot · 2016-10-31T14:53:23Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10851/) (Image: devenv-rhel7_5279)

openshift-bot · 2016-10-31T14:53:24Z

Evaluated for origin merge up to 8c698c5

openshift-bot · 2016-10-31T15:28:39Z

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

openshift-bot · 2016-10-31T15:28:41Z

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

jsafrane added this to the 1.4.0 milestone Oct 27, 2016

jsafrane force-pushed the initialize-cloud branch from 1742e3d to 55f3024 Compare October 27, 2016 15:08

jsafrane changed the title ~~Initialize cloud config in node~~ Initialize cloud provider in node Oct 27, 2016

ncdc reviewed Oct 27, 2016

View reviewed changes

jsafrane force-pushed the initialize-cloud branch from 55f3024 to df73137 Compare October 27, 2016 15:14

smarterclayton reviewed Oct 27, 2016

View reviewed changes

Initialize cloud provider in node

8c698c5

jsafrane force-pushed the initialize-cloud branch from df73137 to 8c698c5 Compare October 31, 2016 14:04

ncdc reviewed Oct 31, 2016

View reviewed changes

openshift-bot merged commit 11d3928 into openshift:master Oct 31, 2016

ncdc mentioned this pull request Oct 31, 2016

Add minimal BuildKubernetesNodeConfig unit test #11666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize cloud provider in node #11620

Initialize cloud provider in node #11620

jsafrane commented Oct 27, 2016

jsafrane commented Oct 27, 2016

ncdc Oct 27, 2016

jsafrane Oct 27, 2016

ncdc commented Oct 27, 2016

jsafrane commented Oct 27, 2016

smarterclayton Oct 27, 2016

jsafrane Oct 31, 2016

openshift-bot commented Oct 31, 2016

ncdc Oct 31, 2016

smarterclayton commented Oct 31, 2016

@ncdc commented on this pull request.

ncdc commented Oct 31, 2016

jsafrane commented Oct 31, 2016

ncdc commented Oct 31, 2016

jsafrane commented Oct 31, 2016

ncdc commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

openshift-bot commented Oct 31, 2016 •

edited

Loading

openshift-bot commented Oct 31, 2016

openshift-bot commented Oct 31, 2016

openshift-bot commented Oct 31, 2016

Initialize cloud provider in node #11620

Initialize cloud provider in node #11620

Conversation

jsafrane commented Oct 27, 2016

jsafrane commented Oct 27, 2016

ncdc Oct 27, 2016

Choose a reason for hiding this comment

jsafrane Oct 27, 2016

Choose a reason for hiding this comment

ncdc commented Oct 27, 2016

jsafrane commented Oct 27, 2016

smarterclayton Oct 27, 2016

Choose a reason for hiding this comment

jsafrane Oct 31, 2016

Choose a reason for hiding this comment

openshift-bot commented Oct 31, 2016

ncdc Oct 31, 2016

Choose a reason for hiding this comment

smarterclayton commented Oct 31, 2016

@ncdc commented on this pull request.

ncdc commented Oct 31, 2016

jsafrane commented Oct 31, 2016

ncdc commented Oct 31, 2016

jsafrane commented Oct 31, 2016

ncdc commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

openshift-bot commented Oct 31, 2016 • edited Loading

openshift-bot commented Oct 31, 2016

openshift-bot commented Oct 31, 2016

openshift-bot commented Oct 31, 2016

openshift-bot commented Oct 31, 2016 •

edited

Loading