Implement multi-port Services #6182

thockin · 2015-03-30T19:35:31Z

There are still a few things to do here, but it builds and passes unit tests.

While this is up for review I will do manual testing, finish last nits, and get an e2e run. As before, please focus on API, conversions, defaults - as these are most likely to go wrong, IMO.

bgrant0607 · 2015-03-30T19:50:11Z

api/swagger-spec/v1beta3.json

Please run hack/update-swagger-spec.sh

done for next

bgrant0607 · 2015-03-30T20:34:36Z

pkg/api/v1beta1/types.go

@@ -750,6 +752,35 @@ type Service struct {

 	// Optional: Supports "ClientIP" and "None".  Used to maintain session affinity.
 	SessionAffinity AffinityType `json:"sessionAffinity,omitempty" description:"enable client IP based session affinity; must be ClientIP or None; defaults to None"`
+
+	// Optional: Ports to expose on the service.  If this field is


FWIW, the long source comment isn't discoverable by users. I'd prefer to have any relevant details in the description.

grammar rules? Are these full sentences?

bgrant0607 · 2015-03-30T21:02:27Z

Thanks a lot for plowing through this.

API LGTM, and a read through all the code LGTM, also.

Is there a snowball's chance we could quickly agree on a resolution to #4811?

Do you plan to punt multiple ports in other cloudproviders to later? I'd be fine with filing issues. In the meantime, errors for unsupported use cases would be nice if not too hard.

bgrant0607 · 2015-03-30T21:04:16Z

pkg/api/types.go

 	// Required: Supports "ClientIP" and "None".  Used to maintain session affinity.
 	SessionAffinity AffinityType `json:"sessionAffinity,omitempty"`
 }

+type ServicePort struct {
+	// Optional if only one ServicePort is defined on this service: The


Technically, isn't the first port allowed to be nameless, regardless how many ports there are?

most of the system won't differentiate whether it is the first port or not,
but validation enforces it.

On Mon, Mar 30, 2015 at 2:04 PM, Brian Grant notifications@github.com
wrote:

In pkg/api/types.go
#6182 (comment)
:

// Required: Supports "ClientIP" and "None". Used to maintain session affinity. SessionAffinity AffinityType `json:"sessionAffinity,omitempty"`

}

+type ServicePort struct {

// Optional if only one ServicePort is defined on this service: The

Technically, isn't the first port allowed to be nameless, regardless how
many ports there are?

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6182/files#r27431745
.

thockin · 2015-03-30T21:57:30Z

I'll @ the people who own the other providers.

On Mon, Mar 30, 2015 at 2:02 PM, Brian Grant notifications@github.com
wrote:

Thanks a lot for plowing through this.

API LGTM, and a read through all the code LGTM, also.

Is there a snowball's chance we could quickly agree on a resolution to
#4811 #4811?

Do you plan to punt multiple ports in other cloudproviders to later? I'd
be fine with filing issues. In the meantime, errors for unsupported use
cases would be nice if not too hard.

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

thockin · 2015-03-30T21:58:12Z

pkg/cloudprovider/openstack/openstack.go

@@ -515,7 +515,7 @@ func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP ne

 		_, err = members.Create(lb.network, members.CreateOpts{
 			PoolID:       pool.ID,
-			ProtocolPort: port,
+			ProtocolPort: ports[0], //FIXME: need to handle multi-port


@anguslees Could use advice on how to implement multi-port for openstack

There is no multiport feature in OpenStack LBaaS - so we'll need to create a new VIP with a new Pool (and new Pool Members) for each port. Basically just put a loop around this function body.

Urgh, no it isn't that easy because then you won't have a 1:1 with k8s' LoadBalancer name and the OpenStack VIPs, which will break the other TCPLoadBalancer methods. Assign me a followon bug and I'll have to come up with something involving regexes and non-atomic operations :(

It looks to me like the GCE change above for this implements a port range - is this the intended multiport semantics, or just an easy implementation? (neither interpretation exists in OpenStack, just wondering which one I should implement)

GCE only supports a range. It's not ideal, but it works. If I were implementing I would do both. e.g. "80,443,8000-9000" :)

I'll make this return an error for now. Thanks.

bgrant0607 · 2015-03-31T00:17:13Z

api/swagger-spec/v1beta1.json

@@ -103,18 +135,50 @@
    "description": "API at /api/v1beta1 version v1beta1",
    "operations": [
     {
-      "type": "v1beta1.EndpointsList",
+      "type": "*json.watchEvent",


Looks like fallout from @smarterclayton's watch changes.

watchEvent looks like a backdoor unversioned API type (from watch/json/types.go) that's not supposed to be public

I'm not sure the * syntax works -- it may well break swagger validation, which currently passes

We've lost documentation of the watch payload type

I 'm not sure if/what you're asking me to do here :)

On Mon, Mar 30, 2015 at 5:17 PM, Brian Grant notifications@github.com
wrote:

In api/swagger-spec/v1beta1.json
#6182 (comment)
:

@@ -103,18 +135,50 @@
"description": "API at /api/v1beta1 version v1beta1",
"operations": [
{

"type": "v1beta1.EndpointsList",

"type": "*json.watchEvent",

Looks like fallout from @smarterclayton
https://github.com/smarterclayton's watch changes.

watchEvent looks like a backdoor unversioned API type (from
watch/json/types.go) that's not supposed to be public

I'm not sure the * syntax works -- it may well break swagger
validation, which currently passes

We've lost documentation of the watch payload type

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6182/files#r27444231
.

On Mar 30, 2015, at 8:17 PM, Brian Grant notifications@github.com wrote:

In api/swagger-spec/v1beta1.json:

@@ -103,18 +135,50 @@
"description": "API at /api/v1beta1 version v1beta1",
"operations": [
{

"type": "v1beta1.EndpointsList",

"type": "*json.watchEvent",
Looks like fallout from @smarterclayton's watch changes.

watchEvent looks like a backdoor unversioned API type (from watch/json/types.go) that's not supposed to be public
It's versioned, but we could not run conversion at the time it was created because of limitations in the underlying conversion method. It is the real object that you get when you watch. Probably can just create a type alias.

I'm not sure the * syntax works -- it may well break swagger validation, which currently passes
Type aliasing should clear that up.
We've lost documentation of the watch payload type
Technically... there are multiple watch payload types. Status and the local Kind. I'd err on the side of describing both.
—
Reply to this email directly or view it on GitHub.

I'll handle it.

On Mar 30, 2015, at 8:25 PM, Tim Hockin notifications@github.com wrote:

In api/swagger-spec/v1beta1.json:

@@ -103,18 +135,50 @@
"description": "API at /api/v1beta1 version v1beta1",
"operations": [
{

"type": "v1beta1.EndpointsList",

"type": "*json.watchEvent",
I 'm not sure if/what you're asking me to do here :)
…
—
Reply to this email directly or view it on GitHub.

Spawned #6232 for this.

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

@@ -103,18 +135,50 @@
"description": "API at /api/v1beta1 version v1beta1",
"operations": [
{

"type": "v1beta1.EndpointsList",

"type": "*json.watchEvent",

Looks like fallout from @smarterclayton's watch changes.

watchEvent looks like a backdoor unversioned API type (from
watch/json/types.go) that's not supposed to be public

I'm not sure the * syntax works -- it may well break swagger validation,
which currently passes

We've lost documentation of the watch payload type

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6182/files#r27444231

abonas · 2015-03-31T17:07:04Z

@thockin this is breaking backwards comp of service entity, right? (just want to make sure we're on the same page)

bgrant0607 · 2015-03-31T18:44:32Z

@abonas Mostly this just breaks v1beta3 backwards compatibility.

bgrant0607 · 2015-03-31T19:24:39Z

LGTM

thockin · 2015-03-31T19:27:17Z

will need to rebase before merge, e2e is running now

thockin · 2015-03-31T19:54:56Z

e2e passed

bgrant0607 · 2015-03-31T19:55:16Z

Fire in the hole!

Implement multi-port Services

thockin · 2015-03-31T19:57:18Z

probably should have squashed :)

hurf · 2015-04-01T06:45:45Z

pkg/api/validation/validation_test.go

+			name: "remove portal IP",
+			tweakSvc: func(oldSvc, newSvc *api.Service) {
+				oldSvc.Spec.PortalIP = "1.2.3.4"
+				newSvc.Spec.PortalIP = ""


Hi Tim, I have a issue #5907 related to this part. newSvc.Spec.PortalIP = "" usually happens when portalIP is not given in the template. Since when creating a service not giving a portalIP means allow kubernetes automatically allocate one, I think we should keep this semantic while updating. That is use the system allocated one. For removing portalIP, if allowed later, we can use something like portalIP: None explicitly in the template. What do you think?

googlebot added the cla: yes label Mar 30, 2015

bgrant0607 self-assigned this Mar 30, 2015

bgrant0607 reviewed Mar 30, 2015
View reviewed changes

thockin mentioned this pull request Mar 30, 2015

Support AWS load balancer #5225

Closed

bgrant0607 reviewed Mar 30, 2015
View reviewed changes

thockin force-pushed the plural_services_20 branch from a09a5e5 to f00dabc Compare March 30, 2015 21:13

thockin reviewed Mar 30, 2015
View reviewed changes

thockin mentioned this pull request Mar 31, 2015

multi-port support in openstack load-balancers #6198

Closed

bgrant0607 reviewed Mar 31, 2015
View reviewed changes

thockin mentioned this pull request Mar 31, 2015

SRV for multiple ports skynetservices/skydns#144

Closed

WIP: Implement multi-port Services

186818d

thockin force-pushed the plural_services_20 branch 3 times, most recently from c0f90ff to d5be5dc Compare March 31, 2015 06:32

Fixes from feedback and testing

eeff1b7

thockin force-pushed the plural_services_20 branch from d5be5dc to eeff1b7 Compare March 31, 2015 06:52

bgrant0607 mentioned this pull request Mar 31, 2015

Enabling v1beta3 api version by default in master #6098

Merged

fixes from testing

b420871

bgrant0607 changed the title ~~WIP: Implement multi-port Services~~ Implement multi-port Services Mar 31, 2015

fix tests broken by stronger validation

e19f493

smarterclayton mentioned this pull request Mar 31, 2015

Improve the output of the swagger API for watch events #6232

Merged

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2015

bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2015

bgrant0607 added a commit that referenced this pull request Mar 31, 2015

Merge pull request #6182 from thockin/plural_services_20

3354cff

Implement multi-port Services

bgrant0607 merged commit 3354cff into kubernetes:master Mar 31, 2015

hurf reviewed Apr 1, 2015
View reviewed changes

abonas mentioned this pull request Apr 1, 2015

Kubernetes provider - service - port should be changed to ports ManageIQ/manageiq#2486

Closed

phemmer mentioned this pull request Apr 1, 2015

Multi-Port Servers in Services v2 #1802

Closed

zeari mentioned this pull request Apr 6, 2015

FIX #2486 Kubernetes provider - service - port was changed to ports ManageIQ/manageiq#2523

Merged

bgrant0607 mentioned this pull request Apr 10, 2015

Service reorg ideas #2585

Closed

smarterclayton mentioned this pull request Apr 16, 2015

WIP - Implement v1beta3 conversions #1600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi-port Services #6182

Implement multi-port Services #6182

thockin commented Mar 30, 2015

bgrant0607 Mar 30, 2015

thockin Mar 30, 2015

bgrant0607 Mar 30, 2015

thockin Mar 30, 2015

bgrant0607 commented Mar 30, 2015

bgrant0607 Mar 30, 2015

thockin Mar 30, 2015

thockin commented Mar 30, 2015

thockin Mar 30, 2015

anguslees Mar 31, 2015

thockin Mar 31, 2015

thockin Mar 31, 2015

bgrant0607 Mar 31, 2015

thockin Mar 31, 2015

smarterclayton Mar 31, 2015

smarterclayton Mar 31, 2015

smarterclayton Mar 31, 2015

abonas commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

thockin commented Mar 31, 2015

thockin commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

thockin commented Mar 31, 2015

hurf Apr 1, 2015

Implement multi-port Services #6182

Implement multi-port Services #6182

Conversation

thockin commented Mar 30, 2015

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

bgrant0607 commented Mar 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 30, 2015

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

Choose a reason for hiding this comment

abonas commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

thockin commented Mar 31, 2015

thockin commented Mar 31, 2015

bgrant0607 commented Mar 31, 2015

thockin commented Mar 31, 2015

Choose a reason for hiding this comment