-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BackendConfig support for timeouts and connection draining #513
BackendConfig support for timeouts and connection draining #513
Conversation
As suggested in #28, BackendConfig is a natural way to expose those settings.
/assign @rramkumar1 @bowei cc: @agau4779 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, looks great!
@@ -107,3 +109,10 @@ type SecurityPolicyConfig struct { | |||
// Name of the security policy that should be associated. | |||
Name string `json:"name"` | |||
} | |||
|
|||
// ConnectionDrainingConfig contains configuration for connection draining. | |||
// For now only a timeout, later possibly ForceSendFields or NullFields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
Iap *IAPConfig `json:"iap,omitempty"` | ||
Cdn *CDNConfig `json:"cdn,omitempty"` | ||
SecurityPolicy *SecurityPolicyConfig `json:"securityPolicy,omitempty"` | ||
TimeoutSec int64 `json:"timeoutSec,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be possible to make it a pointer to int64, if the API machinery allows for it? (note: have to try it myself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/backends/features/features.go
Outdated
) | ||
|
||
var ( | ||
// versionToFeatures stores the mapping from the required API | ||
// version to feature names. | ||
versionToFeatures = map[meta.Version][]string{ | ||
meta.VersionAlpha: []string{}, | ||
meta.VersionBeta: []string{FeatureSecurityPolicy, FeatureNEG, FeatureHTTP2}, | ||
meta.VersionBeta: []string{FeatureSecurityPolicy, FeatureNEG, FeatureHTTP2, FeatureTimeout, FeatureDraining}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout and Draining should not be in beta feature (they are available as GA API: https://godoc.org/google.golang.org/api/compute/v1).
/ok-to-test |
pkg/backends/features/draining.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be beConfig.Spec.ConnectionDraining.DrainingTimeoutSec == nil
You already check if ConnectionDraining is nil in line 33
@@ -293,3 +343,7 @@ func TestIsLowerVersion(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
func intPtr(x int64) *int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need a func for this?
Is it not possible to do &(some number) in all places?
pkg/backends/features/draining.go
Outdated
} | ||
} | ||
|
||
// setDrainingDefaults initializes any nil pointers in Drainning configuration which ensures that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
pkg/backends/features/draining.go
Outdated
} | ||
|
||
// applyDrainingSettings applies the ConnectionDraining settings specified in the | ||
// BackendConfig to the passed in compute.BackendService. A GCE API call still needs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
composite.BackendService
pkg/backends/features/timeout.go
Outdated
|
||
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
composite.BackendService
pkg/backends/features/features.go
Outdated
@@ -33,6 +33,10 @@ const ( | |||
FeatureSecurityPolicy = "SecurityPolicy" | |||
// FeatureNEG defines the feature name of NEG. | |||
FeatureNEG = "NEG" | |||
// FeatureTimeout define the feature name for timeouts. | |||
FeatureTimeout = "Timeout" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}, | ||
updateExpected: false, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this test case because BackendService will never have timeout missing.
updateExpected bool | ||
}{ | ||
{ | ||
desc: "connection draining setting are missing from spec, no update needed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need a test case for when the DrainingTimeoutSeconds fields in the ConnectionDrainingConfig is missing from the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, done
/lgtm /assign @MrHohn |
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@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 |
@rramkumar1 ack, will do. For now, getting my feet wet with the e2e test framework, as it fails here. |
Retroactive LGTM, thanks for the works! |
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. |
@adrianlop This will be rolling out on GKE in a week or so. Documentation will drop at the same time. |
@rramkumar1 also pinging here - what is the updated ETA now? |
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 |
As suggested in #28, BackendConfig is a natural way to expose
those settings.