Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bind delete #59

Merged
merged 4 commits into from
May 24, 2018
Merged

Fix bind delete #59

merged 4 commits into from
May 24, 2018

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented May 23, 2018

Fixes Issue #

Proposed Changes

  • If there is no route when deleting, keep on deleting the binding. For example if a route has been deleted before binding.
  • Do not error a bind delete if pubsub subscription has been already deleted
  • Do not error a bind delete if deployment has already been deleted, if deployment creation fails, try to delete the pubsub subscription so it doesn't get orphaned

@google-prow-robot google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2018
mattmoor
mattmoor approved these changes May 24, 2018
View reviewed changes
pkg/controller/bind/controller.go Outdated
accessor, err := meta.Accessor(bind)
if err != nil {
log.Printf("Failed to get metadata: %s", err)
panic("Failed to get metadata")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.Fatalf instead of the two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/sources/gcp_pubsub.go Outdated
return err
glog.Warningf("Failed to delete subscription %q : %s : %q", subscriptionName, err)
// If it's already been deleted, don't error out
if !strings.Contains(err.Error(), "Resource not found") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing better we can do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how to get the fields of the error because it's just an error. I tried to hunt a little to figure out what the underlying error actually is so I could cast it, it's some kind of rpc error but couldn't really figure out where it's coming from. I'll dig some more tomorrow.

pkg/controller/bind/controller.go Outdated
functionDNS := route.Status.Domain
if route != nil {
functionDNS = route.Status.Domain
glog.Infof("Found route DNS as '%q'", functionDNS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to quote %q :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh. I knew that.

vaikas
vaikas commented May 24, 2018
View reviewed changes
Copy link
Contributor Author

@vaikas vaikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

pkg/controller/bind/controller.go Outdated
accessor, err := meta.Accessor(bind)
if err != nil {
log.Printf("Failed to get metadata: %s", err)
panic("Failed to get metadata")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/controller/bind/controller.go Outdated
functionDNS := route.Status.Domain
if route != nil {
functionDNS = route.Status.Domain
glog.Infof("Found route DNS as '%q'", functionDNS)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh. I knew that.

pkg/sources/gcp_pubsub.go Outdated
return err
glog.Warningf("Failed to delete subscription %q : %s : %q", subscriptionName, err)
// If it's already been deleted, don't error out
if !strings.Contains(err.Error(), "Resource not found") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how to get the fields of the error because it's just an error. I tried to hunt a little to figure out what the underlying error actually is so I could cast it, it's some kind of rpc error but couldn't really figure out where it's coming from. I'll dig some more tomorrow.

@mattmoor
Copy link
Member

/lgtm

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2018
@mattmoor
Copy link
Member

/approve
/lgtm

@mchmarny
Copy link
Member

/lgtm

@mchmarny
Copy link
Member

/approve

1 similar comment
@vaikas
Copy link
Contributor Author

vaikas commented May 24, 2018

/approve

@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, mchmarny, vaikas-google

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-prow-robot google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2018
@google-prow-robot google-prow-robot merged commit 44bdaa7 into knative:master May 24, 2018
@vaikas vaikas deleted the fix_bind_delete branch May 24, 2018 16:50
dprotaso added a commit to dprotaso/eventing that referenced this pull request Sep 12, 2018
@dprotaso
bump knative/pkg to 8fc80de
7d1b2b6 
Includes the following changes:
8fc80de Few changes to the configmap package (knative#59)
0122abd The test logger will now log the correct caller (knative#63)
e7a4b0d Dont call flag.parse in pkg/test (knative#62)
b213523 Update test-infra dependency (knative#61)
382a2bf Make kube_checks generic so that it can be used in serving (knative#58)
760afb6 cleanup (knative#57)
eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54)
6eff182 Remove docker repo from e2e flags (knative#53)
4be5c07 Vendor the test-infra scripts (knative#52)
8c687df Update WaitForEndpointState to return response (knative#51)
8f6a3be Update knative/pkg/test (knative#50)
3ca4270 Add a logkey for the reconcile key. (knative#49)
62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43)
f4a77d7 Add a common test clients file (knative#46)
450739d Add common test logging module (knative#45
knative-prow-robot pushed a commit that referenced this pull request Sep 17, 2018
@dprotaso @knative-prow-robot
Bump knative/pkg to 8fc80de (#428)
d2586db 
* bump knative/pkg to 8fc80de

Includes the following changes:
8fc80de Few changes to the configmap package (#59)
0122abd The test logger will now log the correct caller (#63)
e7a4b0d Dont call flag.parse in pkg/test (#62)
b213523 Update test-infra dependency (#61)
382a2bf Make kube_checks generic so that it can be used in serving (#58)
760afb6 cleanup (#57)
eedc0a9 Make verify-codegen.sh compatible with OS X (#54)
6eff182 Remove docker repo from e2e flags (#53)
4be5c07 Vendor the test-infra scripts (#52)
8c687df Update WaitForEndpointState to return response (#51)
8f6a3be Update knative/pkg/test (#50)
3ca4270 Add a logkey for the reconcile key. (#49)
62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (#43)
f4a77d7 Add a common test clients file (#46)
450739d Add common test logging module (#45

* Use updated methods that indicate that the configmap.Watcher is using an informer
matzew pushed a commit to matzew/eventing that referenced this pull request Apr 25, 2019
@openshift-merge-robot
Merge pull request knative#59 from matzew/SRVKE-70_patch_finalizers
bc11c3e 
Adding finalizers
matzew pushed a commit to matzew/eventing that referenced this pull request Dec 23, 2022
@openshift-cherrypick-robot @skonto
use v1beta1 (knative#59)
e57f7ed 
Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor approved these changes

@mchmarny mchmarny mchmarny approved these changes

Assignees

@mchmarny mchmarny

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vaikas @mattmoor @mchmarny @google-prow-robot