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

Add informer for Service definitions #62

Merged
merged 2 commits into from Dec 3, 2018

Conversation

Projects
None yet
3 participants
@alexellis
Member

alexellis commented Dec 2, 2018

Add informer for Service definitions

Signed-off-by: Alex Ellis (VMware) alexellis2@gmail.com

Description

  • adds informer for Service definitions to fix #61 and #60 -
    since it seemed that deleting Service definitions was not tracked
    or synchronized and the code was depending on the Deployment
    from being deleted or updated.

Motivation and Context

  • I have raised an issue to propose this change (required)

Fixes #61 and #60

How Has This Been Tested?

Tested on K8s 1.10 on single node Linux cluster by deleting
Service and seeing it re-created in the logs. Happy path of
deploying from store also works unaffected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Impact to existing users

Improved durability when resources owned by controller are deleted manually or accidentally by automation.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Add informer for Service definitions
- adds informer for Service definitions to fix #61 and #60 -
since it seemed that deleting Service definitions was not tracked
or synchronized and the code was depending on the Deployment
from being deleted or updated.

Tested on K8s 1.10 on single node Linux cluster by deleting
Service and seeing it re-created in the logs. Happy path of
deploying from store also works unaffected.

Signed-off-by: Alex Ellis (VMware) <alexellis2@gmail.com>
@stefanprodan

LGTM

serviceInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: controller.handleObject,
UpdateFunc: func(old, new interface{}) {
newDepl := new.(*corev1.Service)

This comment has been minimized.

@LucasRoesler

LucasRoesler Dec 3, 2018

Member

looks like some copy-pasta here in the variable name. Should be newSvc and oldSvc. Same for the comment, it also references Deployments

This comment has been minimized.

@alexellis

alexellis Dec 3, 2018

Member

Fair point.

@stefanprodan

This comment has been minimized.

Member

stefanprodan commented Dec 3, 2018

There is no need for handling updates anyway, since the code only deals with creating a service if it doesn’t exists

Update comments as per PR review
- renames temp. variables and comments which were copied from
Deployment block.

Signed-off-by: Alex Ellis (VMware) <alexellis2@gmail.com>
@alexellis

This comment has been minimized.

Member

alexellis commented Dec 3, 2018

@stefanprodan do you mean we can drop AddFunc and UpdateFunc just leaving the handler for DeleteFunc?

@alexellis

This comment has been minimized.

Member

alexellis commented Dec 3, 2018

Or should we leave the subscription and in the future remediate any changes made to Services that are under our control (by ownership reference)?

@stefanprodan

This comment has been minimized.

Member

stefanprodan commented Dec 3, 2018

We need to implement a 3 way merge like kubectl does. Even if the openfaas operator is the owner of the service it shouldn’t override other operators that could add annotations, labels, etc (for example Istio or Flagger)

@LucasRoesler

This comment has been minimized.

Member

LucasRoesler commented Dec 3, 2018

Is there a concrete change being suggested, should UpdateFunc be nil or implement 3-way merge before we merge this PR? Seems like setting it to nil would be safest for now.

@alexellis

This comment has been minimized.

Member

alexellis commented Dec 3, 2018

Given 2x approvals I'll merge, happy to adapt in new PR if needed.

@alexellis alexellis merged commit 04827e0 into master Dec 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment