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

kep/VolumeSubpathEnvExpansion #71351

Merged
merged 1 commit into from Feb 20, 2019

Conversation

@kevtaylor
Copy link
Contributor

kevtaylor commented Nov 22, 2018

What type of PR is this?
/kind api-change

What this PR does / why we need it:
Implements API change after discussion here: #64604

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/community#2871

Special notes for your reviewer:
kubernetes/enhancements#559
Extends original alpha implementation: #49388

Does this PR introduce a user-facing change?:

Extends the VolumeSubpathEnvExpansion alpha feature to support environment variable expansion
Implements subPathExpr field for expanding environment variables into a subPath
The fields subPathExpr and subPath are mutually exclusive
Note: This is a breaking change from the previous version of this alpha feature
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 22, 2018

Hi @kevtaylor. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Nov 22, 2018

/assign @msau42
/cc @thockin

@s-ito-ts

This comment has been minimized.

Copy link
Contributor

s-ito-ts commented Nov 29, 2018

I confirmed that [Feature:VolumeSubpathEnvExpansion] test passed.
I think this PR work correctly.

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Dec 10, 2018

/ok-to-test

Show resolved Hide resolved pkg/apis/core/types.go
@@ -2306,6 +2306,22 @@ func ValidateVolumeMounts(mounts []core.VolumeMount, voldevices map[string]strin
}
}

if len(mnt.SubPathExpr) > 0 {
if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeSubpath) {

This comment has been minimized.

@msau42

msau42 Dec 10, 2018

Member

We recently found there are issues with this validation in downgrade scenarios. After downgrading or disabling the feature, you won't be able to delete your pods because it will fail this validation.

My suggested approach is to not check for feature gates here. Instead, in Pod PrepareForUpdate, we should:

  • allow subpath to continue being set if the old object already used it
  • disallow changing subpath

And in PrepareForCreate:

  • drop the field if the feature gate is not set

cc @liggitt

This comment has been minimized.

@kevtaylor

kevtaylor Dec 10, 2018

Author Contributor

Presumably this applies to subPath itself? as this follows the same pattern as that

This comment has been minimized.

@msau42

msau42 Dec 10, 2018

Member

Yes, I believe @liggitt was handling the actual subpath validation changes, so wanted to make sure you guys agree upon the proper approach.

This comment has been minimized.

@liggitt

liggitt Dec 18, 2018

Member

#70490 is the direction we need to go: tolerate existing data on update, drop in PrepareForCreate/PrepareForUpdate rather than failing in validation

This comment has been minimized.

@liggitt

liggitt Dec 19, 2018

Member

#70490 is merged, please remove the feature gate checks from validation, and add them into podutil.DropDisabledFields, following the existing subpath example

This comment has been minimized.

@kevtaylor

kevtaylor Dec 20, 2018

Author Contributor

@liggitt merged in latest utils and changed validation to match subpath

//
// Allow subpath environment variable substitution
// Only applicable if the VolumeSubpath feature is also enabled
VolumeSubpathEnvExpansion utilfeature.Feature = "VolumeSubpathEnvExpansion"

This comment has been minimized.

@msau42

msau42 Dec 10, 2018

Member

do we need to update the staging file, or is just this one sufficient? https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go

This comment has been minimized.

@kevtaylor

kevtaylor Dec 10, 2018

Author Contributor

I did the staging one because it said so in the api guide but it wasnt totally clear how the 2 are related.

This comment has been minimized.

@msau42

msau42 Dec 17, 2018

Member

I don't think the staging one is necessary. Can you try removing it?

This comment has been minimized.

@kevtaylor

kevtaylor Dec 17, 2018

Author Contributor

reverted - seems to behave itself regarding feature gates

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Dec 10, 2018

/test pull-kubernetes-e2e-kops-aws

@kevtaylor kevtaylor force-pushed the HotelsDotCom:kep/VolumeSubpathEnvExpansion branch from 6174e0c to 2f8ed2e Feb 16, 2019

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 16, 2019

/test pull-kubernetes-e2e-gce-alpha-features

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 16, 2019

@liggitt the verify of the BUILD file failed. Looks like it has somehow got a duplicate entry in it but cant see why. Can you spot the issue?
I think actually I havent put it in alphabetic sequence. Will update new one shortly

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 16, 2019

It's an ordering problem. https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/71351/pull-kubernetes-verify/122614 has details

running hack/update-bazel.sh will fix up the file for you

@kevtaylor kevtaylor force-pushed the HotelsDotCom:kep/VolumeSubpathEnvExpansion branch from 2f8ed2e to 767a271 Feb 17, 2019

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 17, 2019

/test pull-kubernetes-integration

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 17, 2019

/test pull-kubernetes-e2e-gce-alpha-features

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 17, 2019

@msau42 All passing. New parsing format applied which is better than using regex Thanks to @liggitt
Please lgtm

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 19, 2019

/retest

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 19, 2019

@liggitt The local-e2e-containerized is totally broken everywhere and the kops one is no longer in the suite

@msau42
Copy link
Member

msau42 left a comment

lgtm, just one naming nit

Show resolved Hide resolved pkg/kubelet/container/helpers_test.go Outdated

@kevtaylor kevtaylor force-pushed the HotelsDotCom:kep/VolumeSubpathEnvExpansion branch from 767a271 to a64b854 Feb 20, 2019

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Feb 20, 2019

/lgtm
Thanks for working through all our comments!

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 20, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2019

@kevtaylor: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 7f11dd7 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-local-e2e-containerized a64b854 link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 20, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 20, 2019

/skip

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 20, 2019

/skip pull-kubernetes-local-e2e-containerized

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Feb 20, 2019

I'm migrating statuses, ref: kubernetes/test-infra#11338 (comment)

Will spot check this once done

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Feb 20, 2019

spot check looks good

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 20, 2019

@spiffxp Not sure if I read this correctly but looks like the bot got unblocked but the tide is still in pending state?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 20, 2019

looks like the bot got unblocked but the tide is still in pending state?

nope, all green. you're in the merge queue

@kevtaylor

This comment has been minimized.

Copy link
Contributor Author

kevtaylor commented Feb 20, 2019

@liggitt okay. Thanks

@k8s-ci-robot k8s-ci-robot merged commit 5bfea15 into kubernetes:master Feb 20, 2019

17 checks passed

cla/linuxfoundation kevtaylor authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.