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

Add RuntimeClassName feature flag #9072

Merged
merged 2 commits into from
Aug 27, 2020
Merged

Add RuntimeClassName feature flag #9072

merged 2 commits into from
Aug 27, 2020

Conversation

ianlewis
Copy link

Allow users to set runtimeClassName on services via the runtimeclassname
feature flag.

Issue #5306

Release Note

Added RuntimeClassName feature flag.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 15, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2020
@knative-prow-robot
Copy link
Contributor

Welcome @ianlewis! It looks like this is your first PR to knative/serving 🎉

@knative-prow-robot
Copy link
Contributor

Hi @ianlewis. Thanks for your PR.

I'm waiting for a knative 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.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/API API objects and controllers labels Aug 15, 2020
@ianlewis ianlewis mentioned this pull request Aug 15, 2020
Closed
julz
julz reviewed Aug 17, 2020
View reviewed changes
pkg/apis/serving/fieldmask.go
@@ -161,6 +161,9 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
if cfg.Features.PodSpecNodeSelector != config.Disabled {
out.NodeSelector = in.NodeSelector
}
if cfg.Features.PodSpecRuntimeClassName != config.Disabled {
Copy link
Member

Choose a reason for hiding this comment

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

worth adding a test for this, along same lines as the one for PodSpecTolerations?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@julz
Copy link
Member

julz commented Aug 17, 2020

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2020
@julz
Copy link
Member

julz commented Aug 17, 2020

@ianlewis out of interest, Im wondering whether using a PodSecurityPolicy to default RuntimeClass would fit your use case? Are there cases where you want the user to explicitly select runtimeClass for their ksvc, rather than having it implicit in a security policy (or an operator-set default)?

ianlewis
ianlewis commented Aug 19, 2020
View reviewed changes
Copy link
Author

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

@ianlewis out of interest, Im wondering whether using a PodSecurityPolicy to default RuntimeClass would fit your use case? Are there cases where you want the user to explicitly select runtimeClass for their ksvc, rather than having it implicit in a security policy (or an operator-set default)?

Thanks. That's a good question. I think adding the feature flag is the best option for a few reasons:

  1. I am trying to meet the needs of a broad set of customer use cases which may require sandboxed workloads to run alongside non-sandboxed workloads.
  2. PodSecurityPolicy is in beta and has had some growing pains IIRC. I don't feel comfortable right now asking customers to rely on it.
  3. Some customers will want to use PSP for validation rather than defaulting. i.e. set desired state explicitly themselves and validate using PSP, rather than setting the default for a namespace.
  4. PSP can be used as an alternative to, or in addition to this feature flag. i.e. they don't interfere with each other so it provides more flexibility and covers more use cases.

pkg/apis/serving/fieldmask.go
@@ -161,6 +161,9 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
if cfg.Features.PodSpecNodeSelector != config.Disabled {
out.NodeSelector = in.NodeSelector
}
if cfg.Features.PodSpecRuntimeClassName != config.Disabled {
Copy link
Author

Choose a reason for hiding this comment

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

Done

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-upgrade-tests 2020-08-19 01:01:10.785 +0000 UTC 1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-upgrade-tests

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests 2020-08-19 01:01:10.773 +0000 UTC 1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-autotls-tests 2020-08-19 01:01:10.801 +0000 UTC 1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-autotls-tests

@julz
Copy link
Member

julz commented Aug 19, 2020

/retest

julz
julz reviewed Aug 19, 2020
View reviewed changes
Copy link
Member

@julz julz left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2020
@julz
Copy link
Member

julz commented Aug 20, 2020

/assign @dprotaso @mattmoor

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2020
Ian Lewis added 2 commits August 23, 2020 20:33
Add RuntimeClassName feature flag
7e54b1c 
Allow users to set runtimeClassName on services via the runtimeclassname
feature flag.

Issue knative#5306
Add validation test for RuntimeClassName
f969c94
@knative-prow-robot knative-prow-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 24, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/serving/fieldmask.go 98.1% 98.1% 0.0

mattmoor
mattmoor reviewed Aug 27, 2020
View reviewed changes
pkg/apis/serving/k8s_validation_test.go
@@ -543,6 +550,8 @@ func TestPodSpecMultiContainerValidation(t *testing.T) {
}

func TestPodSpecFeatureValidation(t *testing.T) {
runtimeClassName := "test"
Copy link
Member

Choose a reason for hiding this comment

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

I assume there is no good way to validate the values passed (besides podspec dry run?), since it varies so much vendor to vendor...

Copy link
Author

Choose a reason for hiding this comment

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

I suppose the controller could validate that the RuntimeClass with that name exists and add to the knative service's status and emit an event if it doesn't. Currently the controller will fail to create pods and users will see that the service didn't create and see the detailed reason in events already so I'm not sure it would be a much better UX.

@mattmoor mattmoor mentioned this pull request Aug 27, 2020
Closed
@mattmoor
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ianlewis, 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 Aug 27, 2020
@knative-prow-robot knative-prow-robot merged commit 6657049 into knative:master Aug 27, 2020
@julz julz mentioned this pull request Apr 15, 2021
Merged
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

@julz julz julz left review comments

@grantr grantr Awaiting requested review from grantr

@mdemirhan mdemirhan Awaiting requested review from mdemirhan

Assignees

@dprotaso dprotaso

@julz julz

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ianlewis @knative-prow-robot @julz @knative-test-reporter-robot @knative-metrics-robot @mattmoor @dprotaso @googlebot