Support storageclass storage updates to v1 #46116
Conversation
ncdc
added
release-note-action-required
k8s-ci-robot
added
sig/storage
k8s-github-robot
added
the
size/M
|
cc @kubernetes/sig-cluster-lifecycle-pr-reviews |
k8s-ci-robot
added
the
sig/cluster-lifecycle
|
@fejta: GitHub didn't allow me to request PR reviews from the following users: enj. Note that only people with write access to kubernetes/kubernetes can review this PR and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
hack/test-update-storage-objects.sh
Outdated
|
|
||
| # Verify that the server is able to read the object. | ||
| kube::log::status "Verifying we can retrieve ${resource}/${namespace}/${name} via kubectl" | ||
| ${KUBECTL} get --namespace=${namespace} ${resource}/${name} | ||
| if [ "${namespace}" == "cluster-scoped" ]; then |
There was a problem hiding this comment.
why "cluster-scoped" and not just ""?
There was a problem hiding this comment.
@liggitt fixed in latest revision
hack/test-update-storage-objects.sh
Outdated
| ) | ||
|
|
||
| KUBE_OLD_API_VERSION="v1,extensions/v1beta1" |
There was a problem hiding this comment.
did this even work before setting runtime config?
There was a problem hiding this comment.
I know this test passed previously, but I have a feeling it was coincidental given that the previous value of this env var is in the wrong format.
|
@caesarxuchao would like your eyes on the API_VERSION envvar usage |
|
The PR lgtm. The original code regarding the runtime-config was wrong. I think the test passed because our validation was too relax: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/default_storage_factory_builder.go#L100-L102. It should report an error IMO. @nikhiljindal @lavalamp do you know if the linked code was intentional? |
ncdc
force-pushed
the
storageclass-etcd-upgrade
branch
from
62961ce
to
538eb3c
Compare
|
Bump, anyone else want to review? |
|
LGTM |
|
@smarterclayton FYI we need this so admins can update storageclasses in etcd to v1 prior to upgrading to kube 1.8. |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this (slow namespace deletion) |
|
/approve Maybe wait to get rest of lgtm? |
k8s-github-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, ncdc, smarterclayton
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@k8s-bot pull-kubernetes-e2e-kops-aws test this |
|
Automatic merge from submit-queue |
What this PR does / why we need it: enable cluster administrators to update storageclasses stored in etcd from storage.k8s.io/v1beta1 to storage.k8s.io/v1.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: I had a hard time getting the test to work with how it was handling KUBE_API_VERSIONS and RUNTIME_CONFIG. I would appreciate some extra review attention there. Also, I had to hack in a
cluster-scoped"namespace" to get the verification portions of the test script to work. I'm definitely open to ideas for how to improve that if needed.Release note:
cc @kubernetes/sig-storage-pr-reviews @kubernetes/sig-api-machinery-pr-reviews @jsafrane @deads2k @saad-ali @enj