BackendService Naming #239
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
agau4779
commented
Apr 25, 2018
agau4779
commented
- Use NEG naming schema for NEG Backends associated with NEG-enabled ServicePorts. Rely on garbage collection to remove Backends with the incorrect naming schema.
- Instead of md5 hash, use a sha256 hash of concatenated values for NEG name suffix
- Use BackendService name as the key in BackendPool.snapshotter
k8s-ci-robot
added
needs-ok-to-test
|
/assign bowei nicksardo |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
nicksardo
reviewed
Apr 25, 2018
pkg/backends/backends.go
Outdated
| beGa, err = b.cloud.GetGlobalBackendService(nodePortBsName) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Move the API call and error handling out of the conditions and only choose between the names.
nicksardo
reviewed
Apr 25, 2018
pkg/backends/backends.go
Outdated
|
|
||
| if p.NEGEnabled { | ||
| beName = b.namer.NEG(p.SvcName.Namespace, p.SvcName.Name, p.SvcTargetPort) | ||
| } |
There was a problem hiding this comment.
This seems to be calculated a few times in ensureBackendService. Move to top and use it everywhere?
nicksardo
reviewed
Apr 25, 2018
pkg/backends/backends.go
Outdated
| beGa, err := b.cloud.GetGlobalBackendService(b.namer.Backend(port)) | ||
| if err != nil { | ||
| return nil, err | ||
| func (b *Backends) Get(port ServicePort, isAlpha bool) (*BackendService, error) { |
There was a problem hiding this comment.
Maybe this function just takes the backend name?
|
/assign freehan |
nicksardo
reviewed
Apr 25, 2018
pkg/utils/namer.go
Outdated
| func (n *Namer) ParseNEGName(name string) *NameComponents { | ||
| l := strings.Split(name, "-") | ||
| return &NameComponents{ | ||
| ClusterName: l[1], |
There was a problem hiding this comment.
ParseNegName should gracefully handle non-neg named things, including things that aren't valid names like "ABC". Please add a unit test case for NameBelongsToCluster with unexpected names like that.
agau4779
force-pushed
the
backendsvc-naming
branch
3 times, most recently
from
d84ee6e to
7da4b49
Compare
nicksardo
reviewed
May 4, 2018
pkg/backends/backends_test.go
Outdated
| // Ensuring these ports again without first Garbage Collecting goes over | ||
| // the set quota. Expect an error here, until GC is called. | ||
| err := pool.Ensure(tc.newPorts, nil) | ||
| if tc.expectSyncErr && err == nil { |
There was a problem hiding this comment.
Doesn't capture the case of getting an error and not expecting an error.
if tc.expectSyncErr != (err != nil) {
nicksardo
reviewed
May 4, 2018
pkg/backends/backends_test.go
Outdated
| // the set quota. Expect an error here, until GC is called. | ||
| err := pool.Ensure(tc.newPorts, nil) | ||
| if tc.expectSyncErr && err == nil { | ||
| t.Errorf("Expect initial sync to go over quota, but received no error") |
There was a problem hiding this comment.
Nit: use standardized error messages
t.Errorf("Ensure(%v, nil) = %v, want err? %v", tc.newPorts, err, tc.expectSyncErr)
nicksardo
reviewed
May 4, 2018
pkg/backends/backends_test.go
Outdated
| } | ||
|
|
||
| if err := pool.Ensure(tc.oldPorts, nil); err != nil { | ||
| t.Errorf("Expected backend pool to add node ports, err: %v", err) |
There was a problem hiding this comment.
nit: start using standard pattern
t.Errorf("Ensure(%v, nil) = %v, want nil", tc.oldPorts, err)
3d851d7 to
d2ac975
Compare
nicksardo
reviewed
May 4, 2018
| @@ -189,8 +189,7 @@ func (h *HealthChecks) getHealthCheckLink(port int64, alpha bool) (string, error | |||
| } | |||
|
|
|||
| // Delete deletes the health check by port. | |||
| func (h *HealthChecks) Delete(port int64) error { | |||
| name := h.namer.Backend(port) | |||
| func (h *HealthChecks) Delete(name string) error { | |||
There was a problem hiding this comment.
Doesn't there need to be more work for the healthchecker to not use nodeport?
agau4779
force-pushed
the
backendsvc-naming
branch
from
c7cc11c to
9e97151
Compare
k8s-github-robot
pushed a commit
to kubernetes/kubernetes
that referenced
this pull request
May 14, 2018
Automatic merge from submit-queue (batch tested with PRs 55511, 63372, 63400, 63100, 63769). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. [GCE] check for new backend naming scheme **What this PR does / why we need it**: Checks for both the old Backend naming scheme (Nodeport in the name) and the new naming scheme (same scheme used to name NEGs). We need to check for both, in order for both the tests against Ingress head (once kubernetes/ingress-gce#239 gets merged) and tests against prior Ingress versions to pass. See kubernetes/ingress-gce#239 . **Release note**: ```release-note NONE ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.