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.

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

use v1beta2 as the storage version of PingSource #4750

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

eclipselu
Copy link
Contributor

Fixes #4539

Proposed Changes

  • 🧹 flip storage version to v1beta2 in PingSource CRD
  • 🎁 add post-install job to migrate existing PingSource objects to v1beta2

Release Note

Action Required: You need to run the storage migration tool after the upgrade to migrate from v1beta1 to v1 `pingsources.sources.knative.dev` resources.

Docs

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 15, 2021
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 15, 2021
- "sinkbindings.sources.knative.dev"
- "apiserversources.sources.knative.dev"
- "containersources.sources.knative.dev"
- "pingsources.sources.knative.dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is confusing, but likely the whole /post-install needs to be recreated. @lionelvillard

Copy link
Member

Choose a reason for hiding this comment

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

@zhongduo why?

/lgtm

Copy link
Contributor Author

@eclipselu eclipselu Jan 15, 2021

Choose a reason for hiding this comment

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

I'm not sure I follow your question. this is equivalent to deleting existing post-install folder and create new one.

Also I have one question:

  1. User upgrade from 0.19 -> 0.20, the post-install job will be run:
k get job -n knative-eventing
NAME                        COMPLETIONS   DURATION   AGE
storage-version-migration   1/1           3s         6m52s
  1. If the job still exists and user upgrade from 0.20 -> 0.21, since the job with the same name is already COMPLETED, IIUC it won't run again. should I choose a different Job name? Or is there a way to auto-delete the job after it's completion, like https://kubernetes.io/docs/concepts/workloads/controllers/job/#clean-up-finished-jobs-automatically?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eclipselu I realized it is equivalent too. My original concern is that I didn't see a remove of the directory and not sure if there is other logic in the directory. e.g. maybe tomorrow when they are cleaning up, they will remove the directory without checking. But as long as this file is the only job now, I agree we can reuse it.

Copy link
Member

@pierDipi pierDipi Jan 15, 2021

Choose a reason for hiding this comment

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

@eclipselu this is what ttlSecondsAfterFinished does and we already use it:


However, it's an alpha feature which means it is not available in all K8s clusters, so I think we should use generated values and ttlSecondsAfterFinished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eclipselu this is what ttlSecondsAfterFinished does and we already use it:

However, it's an alpha feature which means it is not available in all K8s clusters, so I think we should use generated values and ttlSecondsAfterFinished.

Oh, sorry I did not realize that we already have this. It seems ttlSecondsAfterFinished is not enabled on my cluster, will investigate on generated values, thanks for the pointer!

Copy link
Contributor Author

@eclipselu eclipselu Jan 15, 2021

Choose a reason for hiding this comment

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

It seems we cannot use generateName with kubectl apply -f, kubernetes/kubernetes#44501
I also tried myself using:

apiVersion: batch/v1
kind: Job
metadata:
  generateName: storage-version-migration-
...

and it did not work:

ko apply -f $GOPATH/src/knative.dev/eventing/config/post-install
2021/01/15 13:43:14 Using base gcr.io/distroless/static:nonroot for knative.dev/eventing/vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate
2021/01/15 13:43:15 Building knative.dev/eventing/vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate for linux/amd64
2021/01/15 13:43:20 Publishing us.gcr.io/danglu-knative-dev-290519/migrate-86cf205285e60d2a8b66a01f2a57a58b:latest
2021/01/15 13:43:20 existing blob: sha256:72164b581b02b1eb297b403bcc8fc1bfa245cb52e103a3a525a0835a58ff58e2
2021/01/15 13:43:20 existing blob: sha256:5749e56bea7178768b00aac0bf087558fccd5b8e0c807610618be7568459f359
2021/01/15 13:43:20 existing blob: sha256:bb06e95bf79a0e27cf4855dff6e1091225dfd7bfd3e3f24c781139388f4a7fc4
2021/01/15 13:43:20 existing blob: sha256:5338a849ff04b47d70707f299ce9baf5746eb484e14abb8714d69737f899517e
2021/01/15 13:43:21 us.gcr.io/danglu-knative-dev-290519/migrate-86cf205285e60d2a8b66a01f2a57a58b:latest: digest: sha256:150e69e03ac3457dd468348acdc57790e68639c0e8fadcbdb648737fbe803232 size: 751
2021/01/15 13:43:21 Published us.gcr.io/danglu-knative-dev-290519/migrate-86cf205285e60d2a8b66a01f2a57a58b@sha256:150e69e03ac3457dd468348acdc57790e68639c0e8fadcbdb648737fbe803232
clusterrole.rbac.authorization.k8s.io/knative-eventing-post-install-job-role unchanged
serviceaccount/knative-eventing-post-install-job unchanged
clusterrolebinding.rbac.authorization.k8s.io/knative-eventing-post-install-job-role-binding unchanged
error: from storage-version-migration-: cannot use generate name with apply

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved now? @pierDipi

Copy link
Member

Choose a reason for hiding this comment

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

It seems we cannot use generateName with kubectl apply -f

Ok, then we should manually change the name until ttlSecondsAfterFinished becomes a beta feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a prefix to the Job name.

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #4750 (d029e47) into master (0f2f567) will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4750      +/-   ##
==========================================
+ Coverage   81.11%   81.25%   +0.14%     
==========================================
  Files         291      291              
  Lines        8239     8281      +42     
==========================================
+ Hits         6683     6729      +46     
+ Misses       1156     1143      -13     
- Partials      400      409       +9     
Impacted Files Coverage Δ
pkg/apis/messaging/v1/channel_template_types.go
pkg/duck/channel.go 54.54% <0.00%> (ø)
pkg/reconciler/mtbroker/broker.go 80.46% <0.00%> (+2.02%) ⬆️
pkg/reconciler/sequence/sequence.go 71.00% <0.00%> (+3.22%) ⬆️
pkg/reconciler/parallel/parallel.go 58.82% <0.00%> (+3.77%) ⬆️
pkg/reconciler/channel/channel.go 71.60% <0.00%> (+4.46%) ⬆️
pkg/reconciler/mtbroker/resources/channel.go 100.00% <0.00%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f2f567...d029e47. Read the comment docs.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021
- flip storage version to v1beta2 in PingSource CRD
- add post-install job to migrate existing PingSource objects to v1beta2
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2021
@eclipselu
Copy link
Contributor Author

/test pull-knative-eventing-integration-tests

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021
@zhongduo
Copy link
Contributor

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eclipselu, zhongduo

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021
@eclipselu
Copy link
Contributor Author

/test pull-knative-eventing-conformance-tests

@knative-prow-robot knative-prow-robot merged commit d1bac1e into knative:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PingSource: make v1beta2 the storage version and migrate existing objects to v1beta2
4 participants