-
Notifications
You must be signed in to change notification settings - Fork 600
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
Conversation
- "sinkbindings.sources.knative.dev" | ||
- "apiserversources.sources.knative.dev" | ||
- "containersources.sources.knative.dev" | ||
- "pingsources.sources.knative.dev" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhongduo why?
/lgtm
There was a problem hiding this comment.
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:
- 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
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
ttlSecondsAfterFinished: 600 |
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
.
There was a problem hiding this comment.
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:
ttlSecondsAfterFinished: 600 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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
- flip storage version to v1beta2 in PingSource CRD - add post-install job to migrate existing PingSource objects to v1beta2
/test pull-knative-eventing-integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[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 |
/test pull-knative-eventing-conformance-tests |
Fixes #4539
Proposed Changes
Release Note
Docs