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

use v1beta2 as the storage version of PingSource #4750

Merged
merged 1 commit into from
Jan 20, 2021
Merged

use v1beta2 as the storage version of PingSource #4750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/core/resources/pingsource.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ spec:
- <<: *version
name: v1beta1
served: true
storage: true
storage: false
schema:
openAPIV3Schema:
type: object
Expand Down Expand Up @@ -323,7 +323,7 @@ spec:
- <<: *version
name: v1beta2
served: true
storage: false
storage: true
schema:
openAPIV3Schema:
type: object
Expand Down
4 changes: 1 addition & 3 deletions config/post-install/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ rules:
- apiGroups:
- "sources.knative.dev"
resources:
- "sinkbindings"
- "apiserversources"
- "containersources"
- "pingsources"
verbs:
- "get"
- "list"
Expand Down
11 changes: 7 additions & 4 deletions config/post-install/storage-version-migrator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@
apiVersion: batch/v1
kind: Job
metadata:
name: storage-version-migration
# Adding a prefix to avoid naming conflict with previous version's post-install jobs,
# we cannot use `generateName` here as it's not supported by `kubectl apply -f`
#
# If `ttlSecondsAfterFinished` feature gate becomes generally available in the future,
# we can rely on that and keep using the same Job name.
name: v0.21-storage-version-migration
namespace: knative-eventing
labels:
app: "storage-version-migration"
Expand All @@ -37,6 +42,4 @@ spec:
- name: migrate
image: ko://knative.dev/eventing/vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate
args:
- "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 •
edited
Loading

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 •
edited
Loading

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:

eventing/config/post-install/storage-version-migrator.yaml

Line 24 in 0f2f567

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.

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:

eventing/config/post-install/storage-version-migrator.yaml

Line 24 in 0f2f567

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!

Copy link
Contributor Author

@eclipselu eclipselu Jan 15, 2021 •
edited
Loading

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.