Handle Headless Services when no ports are present #157

sebgoa · 2016-09-16T12:17:58Z

Addresses #146

kadel · 2016-09-19T11:52:04Z

When I tried that I got error:

time="2016-09-19T13:47:10+02:00" level=warning msg="[redis] No ports defined, we will create a Headless service." 
error validating "STDIN": error validating data: found invalid field portalIP for v1.ServiceSpec; if you choose to ignore these errors, turn validation off with --validate=false

When I removed portalIP from generated service everything worked fine.

sebgoa · 2016-09-19T11:59:46Z

what compose file did you give it ?

kadel · 2016-09-19T12:03:17Z

version: "2"

services:
    web:
      image: tuna/docker-counter23
      ports:
        - "5000:5000"
      links:
        - redis
      networks:
        - default

    redis:
      image: redis:3.0
      networks: 
        - default

kadel · 2016-09-27T13:05:32Z

I've done some digging around to figure out where PortalIP is coming from, because PortalIP is old deprecated name for ClusterIP.

Because of OpenShift we are using Kubernetes from openshift/kubernetes.
Problem is that they added this for backward compatibility :-(
Result of this is that Services with PortalIP can be deployed to OpenShift but it fails on Kubernetes :-(

We need to figure the way how to use upstream Kubernetes for Kubernetes conversion. But I don't have idea how to do it :-( OpenShift is not doing import rewrite so it includes k8s.io/kubernetes but expect this to be their fork of k8s from openshift/kubernetes :-(

cdrage · 2016-09-30T13:21:03Z

pkg/transformer/kubernetes/k8sutils.go

+func CreateHeadlessService(name string, service kobject.ServiceConfig, objects []runtime.Object) *api.Service {
+	svc := InitSvc(name, service)
+
+    servicePorts := []api.ServicePort{}


i believe a tab is missing here (needs to be ran through gofmt)

fixing in next push, thanks

kadel · 2016-10-17T12:39:42Z

I just tested this with newer openshift version and PortalIP is gone.
OpenShift and K8S upgrade is part of #200. As soon as #200 get merged we can merge this.

surajssd · 2016-10-18T17:57:08Z

And #200 is merged!

sebgoa · 2016-10-18T17:59:11Z

and we are stuck on my CLA, even though I can probably override this...

sebgoa · 2016-10-18T17:59:34Z

I will rebase tomorrow anyway

ngtuna · 2016-11-02T23:12:47Z

@sebgoa could you check the failed test-cases ? That would be great to introduce handless svc support at kubecon talk.

sebgoa · 2016-11-29T16:57:54Z

@kadel @surajssd you want to have a look at this ? I fixed the headless stuff, it is rebased...

sebgoa · 2016-11-29T19:45:26Z

ok tests fixed

cdrage · 2016-11-29T21:21:46Z

pkg/transformer/kubernetes/k8sutils.go

@@ -230,7 +230,7 @@ func convertToVersion(obj runtime.Object, groupVersion unversioned.GroupVersion)

 func (k *Kubernetes) PortsExist(name string, service kobject.ServiceConfig) bool {
 	if len(service.Port) == 0 {
-		logrus.Warningf("[%s] Service cannot be created because of missing port.", name)
+		logrus.Warningf("[%s] No ports defined, we will create a Headless service.", name)


Logging output could be better. If I was the user, I wouldn't understand what "headless" means. I don't think it should be capitalized either. http://kubernetes.io/docs/user-guide/services/#headless-services

Perhaps: No ports are defined. A headless service will be created. Load-balancing will not be enabled and cluster IP will be set to 'None'.

kadel

We should do this also for openshift.
Right now it displays warning that headless service will be created but no service is actually created.

Adding the same three lines to openshfit.go as in kubernetes.go should fix it.

+		} else {
+			svc := o.CreateHeadlessService(name, service, objects)
+			objects = append(objects, svc)

kadel · 2017-01-03T17:36:21Z

I've rebased this and added headless services for OpenShift.

ping @sebgoa @ngtuna

coveralls · 2017-01-03T17:42:16Z

Coverage decreased (-0.7%) to 45.9% when pulling 425df63 on sebgoa:headless into b059c44 on kubernetes-incubator:master.

ngtuna · 2017-01-03T21:45:52Z

👍 @kadel Could you add an unit test for this ?

kadel · 2017-01-04T12:29:21Z

Of course

coveralls · 2017-01-05T10:19:19Z

Coverage decreased (-0.7%) to 45.9% when pulling ff8d656 on sebgoa:headless into 6e260ba on kubernetes-incubator:master.

cdrage · 2017-01-23T13:24:36Z

pkg/app/app_test.go

+import "testing"
+
+func TestCompareUpAndConvert(t *testing.T) {
+}


blank test, wut

removing :-) that mistake, you were faster than me :-)

cdrage · 2017-01-23T13:25:01Z

pkg/transformer/kubernetes/k8sutils.go

@@ -258,7 +258,7 @@ func convertToVersion(obj runtime.Object, groupVersion unversioned.GroupVersion)
 // PortsExist checks if service has ports defined
 func (k *Kubernetes) PortsExist(name string, service kobject.ServiceConfig) bool {
 	if len(service.Port) == 0 {
-		logrus.Warningf("[%s] Service cannot be created because of missing port.", name)
+		logrus.Warningf("[%s] No ports defined, we will create a Headless service.", name)


Still need to be fixed :( I think the log can be more explicit.

I know :-( But I don't have anything short, that might be more helpful :-(

Ideally it should be just one sentence.

Does user even cares about this?
Maybe we can get rid of this message completely.

@kadel I agree. Removing the log would be best, or adding it to Debug instead. If a port isn't passed, it's assumed to be headless anyways.

yes,
changed to debug

cdrage · 2017-01-23T13:25:32Z

pkg/transformer/kubernetes/k8sutils.go

@@ -282,6 +282,27 @@ func (k *Kubernetes) CreateService(name string, service kobject.ServiceConfig, o
 	return svc
 }

+// CreateHeadlessService creates a k8s headless service
+func (k *Kubernetes) CreateHeadlessService(name string, service kobject.ServiceConfig, objects []runtime.Object) *api.Service {


What is a 'Headless' service? I know what it means, but perhaps adding a comment to the kubernetes documentation may help.

Update, I've tried explain why we use Headless services in comment

cdrage · 2017-01-23T13:25:53Z

pkg/transformer/kubernetes/k8sutils_test.go

+		t.Error(err)
+	}
+
+}


cdrage · 2017-01-23T13:26:06Z

pkg/transformer/kubernetes/k8sutils.go

+	svc.ObjectMeta.Annotations = annotations
+
+	return svc
+}


woops, that was suppose to be a 👍

sebgoa force-pushed the headless branch from 6dd296b to 0e42800 Compare September 16, 2016 12:19

sebgoa mentioned this pull request Sep 16, 2016

Detecting exposed ports in images #146

Closed

kadel mentioned this pull request Sep 27, 2016

[EPIC] Use kubernetes/client-go #175

Closed

cdrage reviewed Sep 30, 2016

View reviewed changes

sebgoa force-pushed the headless branch 2 times, most recently from 1c2ea54 to 34676e4 Compare October 19, 2016 15:09

ngtuna added the review needed label Nov 2, 2016

kadel mentioned this pull request Nov 21, 2016

A better missing port warning message #291

Closed

sebgoa force-pushed the headless branch from 34676e4 to a3cf33e Compare November 29, 2016 16:56

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 29, 2016

sebgoa force-pushed the headless branch from a3cf33e to dec05a8 Compare November 29, 2016 19:37

ngtuna approved these changes Nov 30, 2016

View reviewed changes

cdrage requested changes Nov 30, 2016

View reviewed changes

kadel requested changes Dec 1, 2016

View reviewed changes

kadel force-pushed the headless branch from dec05a8 to 425df63 Compare January 3, 2017 17:34

kadel approved these changes Jan 3, 2017

View reviewed changes

kadel force-pushed the headless branch from 425df63 to ff8d656 Compare January 5, 2017 10:11

surajssd added the rebase needed label Jan 11, 2017

kadel force-pushed the headless branch from ff8d656 to c78b607 Compare January 23, 2017 10:58

cdrage approved these changes Jan 23, 2017

View reviewed changes

cdrage reviewed Jan 23, 2017

View reviewed changes

kadel force-pushed the headless branch 4 times, most recently from f3146a2 to 2af2427 Compare January 25, 2017 09:23

sebgoa and others added 4 commits January 25, 2017 11:28

Handle Headless Services when no ports are present

3193ac3

Handle Headless Services for OpenShift

5a62864

Test Headless Services

8f2dd5b

Change warning to debug for services without ports.

90febe7

kadel force-pushed the headless branch from 2af2427 to 90febe7 Compare January 25, 2017 12:20

kadel merged commit 5383914 into kubernetes:master Jan 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Headless Services when no ports are present #157

Handle Headless Services when no ports are present #157

sebgoa commented Sep 16, 2016

kadel commented Sep 19, 2016

sebgoa commented Sep 19, 2016

kadel commented Sep 19, 2016

kadel commented Sep 27, 2016

cdrage Sep 30, 2016

sebgoa Oct 19, 2016

kadel commented Oct 17, 2016 •

edited

Loading

surajssd commented Oct 18, 2016

sebgoa commented Oct 18, 2016

sebgoa commented Oct 18, 2016

ngtuna commented Nov 2, 2016

sebgoa commented Nov 29, 2016

sebgoa commented Nov 29, 2016

cdrage Nov 29, 2016

kadel left a comment •

edited

Loading

kadel commented Jan 3, 2017

coveralls commented Jan 3, 2017

ngtuna commented Jan 3, 2017

kadel commented Jan 4, 2017

coveralls commented Jan 5, 2017

cdrage Jan 23, 2017

kadel Jan 23, 2017

cdrage Jan 23, 2017

kadel Jan 23, 2017 •

edited

Loading

kadel Jan 24, 2017

cdrage Jan 24, 2017

kadel Jan 24, 2017

cdrage Jan 23, 2017

kadel Jan 23, 2017

cdrage Jan 23, 2017

cdrage Jan 23, 2017 •

edited

Loading

kadel Jan 23, 2017

cdrage Jan 23, 2017

kadel Jan 23, 2017

Handle Headless Services when no ports are present #157

Handle Headless Services when no ports are present #157

Conversation

sebgoa commented Sep 16, 2016

kadel commented Sep 19, 2016

sebgoa commented Sep 19, 2016

kadel commented Sep 19, 2016

kadel commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Oct 17, 2016 • edited Loading

surajssd commented Oct 18, 2016

sebgoa commented Oct 18, 2016

sebgoa commented Oct 18, 2016

ngtuna commented Nov 2, 2016

sebgoa commented Nov 29, 2016

sebgoa commented Nov 29, 2016

Choose a reason for hiding this comment

kadel left a comment • edited Loading

Choose a reason for hiding this comment

kadel commented Jan 3, 2017

coveralls commented Jan 3, 2017

ngtuna commented Jan 3, 2017

kadel commented Jan 4, 2017

coveralls commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Jan 23, 2017 • edited Loading

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

cdrage Jan 23, 2017 • edited Loading

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

kadel commented Oct 17, 2016 •

edited

Loading

kadel left a comment •

edited

Loading

kadel Jan 23, 2017 •

edited

Loading

cdrage Jan 23, 2017 •

edited

Loading