merge Ingress NEG annotation and Expose NEG annotation #350
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
k8s-ci-robot
added
needs-ok-to-test
agau4779
force-pushed
the
merge-neg-annotations
branch
from
7dc2bd4
to
d672649
Compare
|
/assign freehan |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
freehan
reviewed
Jun 20, 2018
pkg/annotations/service.go
Outdated
| // ExposeNEGAnnotationKey key, and maps ServicePort to attributes of the NEG that should be | ||
| // NegAnnotation is the format of the annotation associated with the | ||
| // NEGAnnotationKey key. | ||
| // Ingress indicates whether or not the Service backs an Ingress. If yes, then |
There was a problem hiding this comment.
Move this to the "ingress" key
"ingress" indicates whether to enable NEG feature for Ingress referencing the service. Each NEG correspond to a service port. NEGs will be created and managed under the following conditions:
1. Service is referenced by ingress
2. "ingress" is set to "true". Default to "false"
When the above conditions are satisfied, Ingress will create load balancer and target corresponding NEGs as backends. Service Nodeport is no longer required.
pkg/annotations/service.go
Outdated
| type ExposeNegAnnotation map[int32]NegAttributes | ||
| type NegAnnotation struct { | ||
| Ingress bool `json:"ingress,omitempty"` | ||
| ExposedPorts map[int32]NegAttributes `json:"exposed_ports,omitempty"` |
There was a problem hiding this comment.
add comment:
ExposedPorts specifies the service ports to be exposed as stand-alone NEG. The exposed NEGs will be created and managed by NEG controller.
agau4779
force-pushed
the
merge-neg-annotations
branch
from
d672649
to
bae2424
Compare
freehan
reviewed
Jun 21, 2018
pkg/annotations/service.go
Outdated
| // ExposeNEGAnnotationKey key, and maps ServicePort to attributes of the NEG that should be | ||
| // NegAnnotation is the format of the annotation associated with the | ||
| // NEGAnnotationKey key. | ||
| // WARNING: The feature will NOT be effective in the following circumstances: |
pkg/annotations/service.go
Outdated
| // WARNING: The feature will NOT be effective in the following circumstances: | ||
| // 1. Service is not referenced in any ingress. | ||
| // 2. Adding this annotation on ingress. | ||
| // ExposedPorts maps ServicePort to attributes of the NEG that should be |
There was a problem hiding this comment.
Move to the top of ExposedPorts Attribute
agau4779
force-pushed
the
merge-neg-annotations
branch
4 times, most recently
from
74d28a0
to
9435da4
Compare
agau4779
force-pushed
the
merge-neg-annotations
branch
from
9435da4
to
0db5cca
Compare
freehan
reviewed
Jun 22, 2018
pkg/neg/controller.go
Outdated
| service, err = svcClient.Update(service) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return c.serviceLister.Update(service) |
There was a problem hiding this comment.
no need to Update serviceLister
pkg/neg/controller.go
Outdated
| service, err = svcClient.Update(service) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return c.serviceLister.Update(service) |
There was a problem hiding this comment.
no need to Update serviceLister
pkg/neg/controller.go
Outdated
|
|
||
| // Remove NEG Status Annotation when no NEG is needed | ||
| if len(portMap) == 0 { | ||
| if _, ok := service.Annotations[annotations.NEGStatusKey]; ok { | ||
| // TODO: use PATCH to remove annotation | ||
| delete(service.Annotations, annotations.NEGStatusKey) | ||
| glog.V(2).Infof("Removing expose NEG annotation from service: %s/%s", namespace, name) | ||
| service, err = svcClient.Update(service) |
There was a problem hiding this comment.
no need to track service, just return err
agau4779
force-pushed
the
merge-neg-annotations
branch
from
0db5cca
to
1e78753
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
pushed a commit
to kubernetes/kubernetes
that referenced
this pull request
Jun 23, 2018
Automatic merge from submit-queue (batch tested with PRs 65338, 64535). 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] e2e test for expose neg on gce ingress **What this PR does / why we need it**: - Adds e2e test for the expose NEG annotation (which allows for standalone NEGs) **Special notes for your reviewer**: Note, kubernetes/ingress-gce#350 must be merged first before this is merged. `[Unreleased]` tag is on this PR because it depends on code from kubernetes/ingress-gce#350 and kubernetes/ingress-gce#284 being in an Ingress release. Will update this test and test-infra once this is released in the next Ingress. **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.
Follow-up to #284 .
{"ingress": true,"exposed_ports":{"3000":{},"4000":{}}}