Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Removing/Updating old publisher, ra, and pubsub resources #1380

Merged
merged 12 commits into from
Jul 7, 2020

Conversation

nachocano
Copy link
Member

@nachocano nachocano commented Jul 6, 2020

Fixes #1217

Proposed Changes

  • 🧽 Removing old PS and Topics k8s objects and re-create them. Detecting whether it was old or not based on changes to spec.topic between the desired and the one stored. The only way this can happen is when upgrading to 0.16, as spec.topic is immutable. Deleting the wrapper objects, will take care of deleting their owned objects (RA, publisher's, scaled objects).
  • 🧽 Updating scheduler target pubsub topic.
  • 🧽 Deleting and re-creating the storage notification if appropriate. Cannot find a way to update the existing notification

I first tried deleting the PubSub resources instead of our wrappers K8s objects, but that has the problem that we cannot update them to use the newer topic names, as spec.topic is immutable.

Release Note

Given that we have renamed many resources in https://github.com/google/knative-gcp/pull/1207, when upgrading to 0.16 we will delete those old resources and re-create them with the newer names. You may experience some delay in getting your resources back to the ready state.

Docs

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano

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

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Jul 6, 2020
@nachocano
Copy link
Member Author

This won't actually work as the topic.spec.topic is immutable...

@nachocano nachocano changed the title [WIP] Removing old publisher, ra, and pubsub resources Removing old publisher, ra, and pubsub resources Jul 6, 2020
@nachocano
Copy link
Member Author

/assign @Harwayne
so that you can start to take a look. I haven't fully tested it, will create a new cluster with 0.15, create some resources there, and then apply these changes.

Integration are still failing due to monitoring, but that should have been fixed. Need to wait for 1 hour or so until we get a new nightly

pkg/reconciler/events/auditlogs/auditlogs.go Outdated Show resolved Hide resolved
pkg/reconciler/events/auditlogs/auditlogs.go Outdated Show resolved Hide resolved
pkg/reconciler/messaging/channel/channel.go Outdated Show resolved Hide resolved
pkg/reconciler/messaging/channel/channel.go Show resolved Hide resolved
pkg/reconciler/messaging/channel/channel.go Show resolved Hide resolved
@chizhg
Copy link
Member

chizhg commented Jul 6, 2020

/retest

2 similar comments
@chizhg
Copy link
Member

chizhg commented Jul 6, 2020

/retest

@chizhg
Copy link
Member

chizhg commented Jul 6, 2020

/retest

@nachocano
Copy link
Member Author

Synced with @grac3gao, it seems that the previous run picked a cluster with no workload identity enabled, so all tests failed...
re-running

@nachocano
Copy link
Member Author

Installed 0.15, then created a few sources and a channel... Sent events, all worked...
Then did a ko apply from my branch... Things were removed properly and newer stuff was created. Examples:

CAL

topic: cloudauditlogssource-d56c6c3d-4ab4-4f38-8ded-9b207f51f1c5 --> cre-src_default_test-auditlogs_d56c6c3d-4ab4-4f38-8ded-9b207f51f1c5
sub: cre-pull-0b062770-0ed5-4e14-bd81-e0948661ef96 --> cre-src_default_test-auditlogs_11819cd7-6b73-4bb8-bb72-e5e838563a45
ksvc: cre-test-auditlogs-publish --> removed
ra: cre-pull-0b062770-0ed5-4e14-bd81-e0948661ef96-67c8ff6d57-9798q --> cre-src-test-auditlogs-11819cd7-6b73-4bb8-bb72-e5e838563a45llns


scheduler

topic: scheduler-0867c821-cf1c-4c2e-9909-5087f0d9e87e --> cre-src_default_test-scheduler_0867c821-cf1c-4c2e-9909-5087f0d9e87e
sub: cre-pull-ea0d5c66-9572-442f-a63e-33c96dc09fc5 --> cre-src_default_test-scheduler_34ad9c55-2498-4120-892e-8be2183d3c39
ksvc: cre-test-scheduler-publish --> removed
ra: cre-pull-ea0d5c66-9572-442f-a63e-33c96dc09fc5-67fcf95c79-h5mx2 --> cre-src-test-scheduler-34ad9c55-2498-4120-892e-8be2183d3c387vfx


storage

topic: storage-111f1ef4-b734-4335-be58-fdf083e19e8d --> cre-src_default_test-storage_111f1ef4-b734-4335-be58-fdf083e19e8d
sub: cre-pull-03d4ce13-3d09-4ba5-90a3-c4d84d0ec7a2 --> cre-src_default_test-storage_1be82382-4225-4dc6-bede-0b0acd5a2742
ksvc: cre-test-storage-publish -> removed
ra: cre-pull-03d4ce13-3d09-4ba5-90a3-c4d84d0ec7a2-b966cdffb-64vvk --> cre-src-test-storage-1be82382-4225-4dc6-bede-0b0acd5a2742-kqdfh

@nachocano
Copy link
Member Author

Few problems I faced.

The controller seems to be trying to export metrics to stackdriver, even custom metrics is disabled. Not sure if this is a problem from the upgrade itself or if it's in master.
Log from the controller
2020/07/06 22:04:03 Failed to export to Stackdriver: [rpc error: code = PermissionDenied desc = Permission monitoring.metricDescriptors.create denied (or the resource may not exist).; rpc error: code = PermissionDenied desc = Permission monitoring.timeSeries.create denied (or the resource may not exist).]

Failing to read the auth defaults from the new configMap:
{"level":"error","ts":"2020-07-06T23:10:10.202Z","logger":"controller.github.com-google-knative-gcp-pkg-reconciler-intevents-pullsubscription-static.Reconciler","caller":"v1beta1/pubsub_defaults.go:33","msg":"Failed to get the GCPAuthDefaults","commit":"167157b","knative.dev/traceid":"cc189e25-c378-405b-b0cb-e9d33b1ac2fa","knative.dev/key":"default/test-scheduler","stacktrace":"github.com/google/knative-gcp/pkg/apis/duck/v1beta1.(*PubSubSpec).SetPubSubDefaults\n\tgithub.com/google/knative-gcp/pkg/apis/duck/v1beta1/pubsub_defaults.go:33\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1beta1.(*PullSubscriptionSpec).SetDefaults\n\tgithub.com/google/knative-gcp/pkg/apis/intevents/v1beta1/pullsubscription_defaults.go:51\ngithub.com/google/knative-gcp/pkg/apis/intevents/v1beta1.(*PullSubscription).SetDefaults\n\tgithub.com/google/knative-gcp/pkg/apis/intevents/v1beta1/pullsubscription_defaults.go:36\nknative.dev/pkg/reconciler.PreProcessReconcile\n\tknative.dev/pkg@v0.0.0-20200626022628-f1ee372577e1/reconciler/reconcile_common.go:42\ngithub.com/google/knative-gcp/pkg/client/injection/reconciler/intevents/

Failing to delete the old SD sink:

{"level":"error","ts":"2020-07-06T23:10:11.548Z","logger":"controller.cloud-run-events-cloudauditlogssource-controller","caller":"auditlogs/auditlogs.go:223","msg":"Failed to delete StackDriver sink","commit":"167157b","events.cloud.google.com/controller":"cloud-run-events-cloudauditlogssource-controller","auditlogsource":{"metadata":{"name":"test-auditlogs","namespace":"default","selfLink":"/apis/events.cloud.google.com/v1beta1/namespaces/default/cloudauditlogssources/test-auditlogs","uid":"d56c6c3d-4ab4-4f38-8ded-9b207f51f1c5","resourceVersion":"27216","generation":1,"creationTimestamp":"2020-07-06T21:25:26Z","annotations":{"cluster-name":"kn-upgrade","kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"events.cloud.google.com/v1beta1\",\"kind\":\"CloudAuditLogsSource\",\"metadata\":{\"annotations\":{},\"name\":\"test-auditlogs\",\"namespace\":\"default\"},

The Channel didn't work until I delete the subscription and created it again.
PubSub worked as is.
I was having a problem with Storage because I had it created with the old event type.

Will keep on investigating

@nachocano
Copy link
Member Author

Still getting errors trying to delete the SD sink. Given that is not a topic/subscription, but rather some SD internal stuff, I think is fine to leave it as is. As the code is structured, given that the sink should be in the status of the resource, it will keep on using that old sink name and things will keep on working.
For newer resources, the sink will have a more readable name.

@nachocano nachocano changed the title Removing old publisher, ra, and pubsub resources Removing/Updating old publisher, ra, and pubsub resources Jul 7, 2020
@nachocano
Copy link
Member Author

/test pull-google-knative-gcp-wi-tests

@nachocano
Copy link
Member Author

nachocano commented Jul 7, 2020 via email

@nachocano
Copy link
Member Author

/retest

@nachocano
Copy link
Member Author

nachocano commented Jul 7, 2020 via email

@nachocano
Copy link
Member Author

nachocano commented Jul 7, 2020 via email

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests 1/3

Automatically retrying due to test flakiness...
/test pull-google-knative-gcp-wi-tests

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/events/scheduler/scheduler.go 78.6% 75.3% -3.2
pkg/reconciler/events/storage/storage.go 72.2% 68.4% -3.8
pkg/reconciler/intevents/reconciler.go 78.8% 68.8% -10.0
pkg/reconciler/messaging/channel/channel.go 78.7% 67.2% -11.5

@nachocano
Copy link
Member Author

All sources working. Channel still does not mark the subscription ready when upgrading from 0.15.
I'd suggest we get this in asap, and I keep on investigating. Need to fix the auto-update deps as well, because it's not working

@capri-xiyue
Copy link
Contributor

/lgtm
/hold

@nachocano
Copy link
Member Author

/hold cancel

Cancelling the hold as I was this in before the CI breaks again and I need to do a bunch of re-tests.
The upgrade of Channel is still not working. Will try to fix it during the day (probably after fixing update deps). And if someone has cycles and can take a look at it, it'd be great.
The other bits were working in my tests.

@knative-prow-robot knative-prow-robot merged commit 80454f1 into google:master Jul 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove old GCP resources, receive adapters, and publishers
8 participants