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

Include a job to help operators migrate their storage version #7499

Merged
merged 1 commit into from Apr 10, 2020
Merged

Include a job to help operators migrate their storage version #7499

merged 1 commit into from Apr 10, 2020

Conversation

dprotaso
Copy link
Member

@dprotaso dprotaso commented Apr 5, 2020 •
edited

Fixes #6726
Depended on knative/pkg#1197 to merge

The sigs.k8s.io/kube-storage-version-migrator seems to migrate all K8s resources/CRDs and it does it at a specific intervals. One thing it doesn't do is drop older storage versions from the CRD's status block. That's still a manual process - but this is necessary for us to drop older versions in our spec. Additionally, the tool performs full PUTs which I don't believe is necessary (issue here: kubernetes-sigs/kube-storage-version-migrator#65)

Thus I felt it would simpler to write a custom job to perform the storage migration specific to serving's resources (service, revision, configuration, route)

Proposed Changes

  • Include a K8s Job to help migrate Service, Configuration, Revision, Route resources between storage versions

Release Note

Include a K8s Job to help migrate Service, Configuration, Revision, Route resources between storage versions

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 5, 2020
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 5, 2020
@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Apr 5, 2020
@dprotaso dprotaso mentioned this pull request Apr 5, 2020
Merged
@dprotaso
Copy link
Member Author

dprotaso commented Apr 5, 2020

/assign @mattmoor

mattmoor
mattmoor reviewed Apr 5, 2020
View reviewed changes
hack/generate-yamls.sh
ko resolve ${KO_YAML_FLAGS} -f config/post-install/ | "${LABEL_YAML_CMD[@]}" > "${SERVING_DEFAULT_DOMAIN_YAML}"
ko resolve ${KO_YAML_FLAGS} -f config/post-install/default-domain.yaml | "${LABEL_YAML_CMD[@]}" > "${SERVING_DEFAULT_DOMAIN_YAML}"

ko resolve ${KO_YAML_FLAGS} -f config/post-install/storage-version-migration.yaml | "${LABEL_YAML_CMD[@]}" > "${SERVING_STORAGE_VERSION_MIGRATE_YAML}"
Copy link
Member

Choose a reason for hiding this comment

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

We should add docs for this new artifact.

Copy link
Member Author

Choose a reason for hiding this comment

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

in the repo or website?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did it for the website - knative/docs#2369

config/post-install/storage-version-migration.yaml Show resolved Hide resolved
@vagababov
Copy link
Contributor

Should this in general be in some other repo?

@dprotaso
Copy link
Member Author

dprotaso commented Apr 6, 2020

Should this in general be in some other repo?

The utility lives in knative.dev/pkg - the k8s job specific to serving is what's being PR'd

vaikas
vaikas reviewed Apr 6, 2020
View reviewed changes
vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate/config.go Outdated
"k8s.io/client-go/tools/clientcmd"
)

// TODO - make a package that's reusable here and by sharedmain
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I used sharedmain config set up in my PR: Can you use that?
https://github.com/knative/eventing/pull/2910/files#diff-28e3377805ade46afcad16b416392053R37

Copy link
Member Author

Choose a reason for hiding this comment

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

technically yeah - but that would pull in that package and all it's dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

that's already part of serving? Or am I confused about something?

vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate/main.go Outdated
logger := setupLogger()
defer logger.Sync()

config := configOrDie()
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/knative/eventing/pull/2910/files#diff-28e3377805ade46afcad16b416392053R37

@dprotaso
Copy link
Member Author

dprotaso commented Apr 7, 2020

rebased

@dprotaso dprotaso marked this pull request as ready for review April 7, 2020 13:33
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2020
@vagababov
Copy link
Contributor

I think you need more rebases :)

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@dprotaso
Copy link
Member Author

dprotaso commented Apr 7, 2020

lol i knew I should have held the other one :)

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 7, 2020
mattmoor
mattmoor reviewed Apr 10, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, mattmoor

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 Apr 10, 2020
@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests pull-knative-serving-integration-tests
pull-knative-serving-integration-tests		 2/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/scale.TestScaleToN/scale-10
test/e2e.TestConflictingRouteService
test/e2e.TestProbeWhitelist
test/e2e.TestServiceToServiceCall
test/e2e.TestCallToPublicService
test/e2e.TestSubrouteLocalSTS
test/e2e.TestServiceToServiceCallViaActivator/a-disabled
test/e2e.TestServiceToServiceCallViaActivator/b-disabled

@dprotaso
Copy link
Member Author

dprotaso commented Apr 10, 2020 via email

@knative-prow-robot knative-prow-robot merged commit f38e976 into knative:master Apr 10, 2020
@dprotaso dprotaso deleted the storage-version-job branch April 10, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor left review comments

@vaikas vaikas vaikas left review comments

@steuhs steuhs Awaiting requested review from steuhs

@vagababov vagababov Awaiting requested review from vagababov

Assignees

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. 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.

Change the storage version to v1 types
7 participants
@dprotaso @vagababov @knative-prow-robot @knative-test-reporter-robot @mattmoor @vaikas @googlebot