BackendConfig support for timeouts and connection draining by bpineau · Pull Request #513 · kubernetes/ingress-gce

bpineau · 2018-10-17T17:47:45Z

As suggested in #28, BackendConfig is a natural way to expose
those settings.

As suggested in #28, BackendConfig is a natural way to expose those settings.

bowei · 2018-10-17T19:17:13Z

/assign @rramkumar1 @bowei

cc: @agau4779

rramkumar1

Thanks for this PR, looks great!

rramkumar1 · 2018-10-17T19:50:23Z

pkg/apis/backendconfig/v1beta1/types.go

 }
+
+// ConnectionDrainingConfig contains configuration for connection draining.
+// For now only a timeout, later possibly ForceSendFields or NullFields.


This part of the comment doesn't really make sense. ForceSendFields and NullFields are semantics of the GCE compute API's, not these K8s APIs. Just say something like "For now only a timeout, but in the future more settings".

rramkumar1 · 2018-10-17T19:53:18Z

pkg/apis/backendconfig/v1beta1/types.go

+	Iap                *IAPConfig                `json:"iap,omitempty"`
+	Cdn                *CDNConfig                `json:"cdn,omitempty"`
+	SecurityPolicy     *SecurityPolicyConfig     `json:"securityPolicy,omitempty"`
+	TimeoutSec         int64                     `json:"timeoutSec,omitempty"`


Can we make TimeoutSec a pointer to a struct?

Reason is that making it a pointer allows us to understand three different types of state (rather than 2). These three states are:

User does not care to use this feature, pointer is nil (set to GCE default which is 30 sec's I think)

User left this feature empty, empty struct but non-nil pointer (they want timeout to be 0)

User set some sort of value for this feature (i.e 100)

If we use a struct pointer, we can encapsulate all three states while using just an int, we can only capture states 2 & 3.

It may be possible to make it a pointer to int64, if the API machinery allows for it? (note: have to try it myself)

Excellent idea! I just did that, and confirm *int64 works fine with apimachinery.
Adapted DrainingTimeoutSec the same way, so we don't confuse a value changed back to 0 with no value.

MrHohn · 2018-10-17T20:35:38Z

pkg/backends/features/features.go

 	versionToFeatures = map[meta.Version][]string{
 		meta.VersionAlpha: []string{},
-		meta.VersionBeta:  []string{FeatureSecurityPolicy, FeatureNEG, FeatureHTTP2},
+		meta.VersionBeta:  []string{FeatureSecurityPolicy, FeatureNEG, FeatureHTTP2, FeatureTimeout, FeatureDraining},


Timeout and Draining should not be in beta feature (they are available as GA API: https://godoc.org/google.golang.org/api/compute/v1).

idealhack · 2018-10-18T15:35:04Z

/ok-to-test

rramkumar1 · 2018-10-18T21:58:20Z

pkg/backends/features/draining.go

+// there are defaults for missing sub-types. Will be more useful once we support more than just
+// timeouts.
+func setDrainingDefaults(beConfig *backendconfigv1beta1.BackendConfig) {
+	if beConfig.Spec.ConnectionDraining == nil {


This should be beConfig.Spec.ConnectionDraining.DrainingTimeoutSec == nil

You already check if ConnectionDraining is nil in line 33

rramkumar1 · 2018-10-18T21:59:31Z

pkg/backends/features/features_test.go

 	}
 }
+
+func intPtr(x int64) *int64 {


Do you really need a func for this?

Is it not possible to do &(some number) in all places?

rramkumar1 · 2018-10-18T22:01:00Z

pkg/backends/features/draining.go

+	}
+}
+
+// setDrainingDefaults initializes any nil pointers in Drainning configuration which ensures that


rramkumar1 · 2018-10-18T22:02:02Z

pkg/backends/features/draining.go

+}
+
+// applyDrainingSettings applies the ConnectionDraining settings specified in the
+// BackendConfig to the passed in compute.BackendService. A GCE API call still needs


composite.BackendService

rramkumar1 · 2018-10-18T22:02:14Z

pkg/backends/features/timeout.go

+
+// applyTimeoutSettings applies the Timeout settings specified in the BackendConfig
+// to the passed in compute.BackendService. A GCE API call still needs to be
+// made to actually persist the changes.


composite.BackendService

rramkumar1 · 2018-10-18T22:06:52Z

pkg/backends/features/features.go

 	// FeatureNEG defines the feature name of NEG.
 	FeatureNEG = "NEG"
+	// FeatureTimeout define the feature name for timeouts.
+	FeatureTimeout = "Timeout"


You shouldn't need to modify anything in this file since Timeout and Connection draining are both GA.

I'll add a comment about this later so it is more clear.

rramkumar1 · 2018-10-18T22:08:55Z

pkg/backends/features/timeout_test.go

+			},
+			updateExpected: false,
+		},
+		{


I think you can remove this test case because BackendService will never have timeout missing.

rramkumar1 · 2018-10-18T22:09:50Z

pkg/backends/features/draining_test.go

+		updateExpected bool
+	}{
+		{
+			desc: "connection draining setting are missing from spec, no update needed",


You also need a test case for when the DrainingTimeoutSeconds fields in the ConnectionDrainingConfig is missing from the spec.

Right, done

rramkumar1 · 2018-10-19T16:16:32Z

/lgtm

/assign @MrHohn

k8s-ci-robot · 2018-10-19T16:16:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bpineau, rramkumar1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rramkumar1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rramkumar1 · 2018-10-19T16:38:55Z

@bpineau Can you please add e2e tests for this in a separate PR?

See https://github.com/kubernetes/ingress-gce/tree/master/pkg/fuzz/features & https://github.com/kubernetes/ingress-gce/tree/master/cmd/e2e-test

bpineau · 2018-10-22T09:49:38Z

@rramkumar1 ack, will do. For now, getting my feet wet with the e2e test framework, as it fails here.

MrHohn · 2018-10-22T17:12:03Z

Retroactive LGTM, thanks for the works!

adrianlop · 2018-11-01T11:36:50Z

hi guys, the documentation is not updated (https://cloud.google.com/kubernetes-engine/docs/concepts/backendconfig), is this GA in GKE already or we have to wait?

thank you.

rramkumar1 · 2018-11-01T16:09:03Z

@adrianlop This will be rolling out on GKE in a week or so. Documentation will drop at the same time.

matti · 2018-11-21T11:25:45Z

@rramkumar1 also pinging here - what is the updated ETA now?

rramkumar1 · 2018-11-29T23:00:39Z

@matti

FYI: This is now launched on GKE for cluster versions at or above 1.11.3-gke.18!

Docs: https://cloud.google.com/kubernetes-engine/docs/how-to/configure-backend-service

BackendConfig support for timeouts and connection draining

10671c4

As suggested in #28, BackendConfig is a natural way to expose those settings.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2018

k8s-ci-robot requested review from MrHohn and thockin October 17, 2018 17:47

k8s-ci-robot assigned bowei and rramkumar1 Oct 17, 2018

rramkumar1 reviewed Oct 17, 2018

View reviewed changes

MrHohn reviewed Oct 17, 2018

View reviewed changes

Review feedback

6a94127

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2018

rramkumar1 reviewed Oct 18, 2018

View reviewed changes

Review feedback (2)

57f44a8

rramkumar1 mentioned this pull request Oct 19, 2018

Support session affinity via Ingress annotation #516

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2018

k8s-ci-robot assigned MrHohn Oct 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2018

k8s-ci-robot merged commit 9b8b7e9 into kubernetes:master Oct 19, 2018

matti mentioned this pull request Nov 21, 2018

[GLBC] Expose GCE backend parameters in Ingress object API #28

Closed

Conversation

bpineau commented Oct 17, 2018

Uh oh!

bowei commented Oct 17, 2018

Uh oh!

rramkumar1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idealhack commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rramkumar1 commented Oct 19, 2018

Uh oh!

k8s-ci-robot commented Oct 19, 2018

Uh oh!

rramkumar1 commented Oct 19, 2018

Uh oh!

bpineau commented Oct 22, 2018

Uh oh!

MrHohn commented Oct 22, 2018

Uh oh!

adrianlop commented Nov 1, 2018

Uh oh!

rramkumar1 commented Nov 1, 2018

Uh oh!

matti commented Nov 21, 2018

Uh oh!

rramkumar1 commented Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants