Change RunOnce duration plugin to act as a limit instead of override

pkg/quota/admission/runonceduration/admission.go

@@ -9,6 +9,7 @@ import (
 	"k8s.io/kubernetes/pkg/admission"
 	kapi "k8s.io/kubernetes/pkg/api"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
+	"k8s.io/kubernetes/pkg/util/integer"
 
 	oadmission "github.com/openshift/origin/pkg/cmd/server/admission"
 	configlatest "github.com/openshift/origin/pkg/cmd/server/api/latest"
@@ -49,7 +50,7 @@ func readConfig(reader io.Reader) (*api.RunOnceDurationConfig, error) {
 // NewRunOnceDuration creates a new RunOnceDuration admission plugin
 func NewRunOnceDuration(config *api.RunOnceDurationConfig) admission.Interface {
 	return &runOnceDuration{
-		Handler: admission.NewHandler(admission.Create, admission.Update),
+		Handler: admission.NewHandler(admission.Create),
 		config:  config,
 	}
 }
@@ -84,13 +85,13 @@ func (a *runOnceDuration) Admit(attributes admission.Attributes) error {
 		return nil
 	}
 
-	appliedProjectOverride, err := a.applyProjectAnnotationOverride(attributes.GetNamespace(), pod)
+	appliedProjectLimit, err := a.applyProjectAnnotationLimit(attributes.GetNamespace(), pod)
 	if err != nil {
 		return admission.NewForbidden(attributes, err)
 	}
 
-	if !appliedProjectOverride && a.config.ActiveDeadlineSecondsOverride != nil {
-		pod.Spec.ActiveDeadlineSeconds = a.config.ActiveDeadlineSecondsOverride
+	if !appliedProjectLimit && a.config.ActiveDeadlineSecondsLimit != nil {
+		pod.Spec.ActiveDeadlineSeconds = int64MinP(a.config.ActiveDeadlineSecondsLimit, pod.Spec.ActiveDeadlineSeconds)
 	}
 	return nil
 }
@@ -106,22 +107,34 @@ func (a *runOnceDuration) Validate() error {
 	return nil
 }
 
-func (a *runOnceDuration) applyProjectAnnotationOverride(namespace string, pod *kapi.Pod) (bool, error) {
+func (a *runOnceDuration) applyProjectAnnotationLimit(namespace string, pod *kapi.Pod) (bool, error) {
 	ns, err := a.cache.GetNamespace(namespace)
 	if err != nil {
 		return false, fmt.Errorf("error looking up pod namespace: %v", err)
 	}
 	if ns.Annotations == nil {
 		return false, nil
 	}
-	override, hasOverride := ns.Annotations[api.ActiveDeadlineSecondsOverrideAnnotation]
-	if !hasOverride {
+	limit, hasLimit := ns.Annotations[api.ActiveDeadlineSecondsLimitAnnotation]
+	if !hasLimit {
 		return false, nil
 	}
-	overrideInt64, err := strconv.ParseInt(override, 10, 64)
+	limitInt64, err := strconv.ParseInt(limit, 10, 64)
 	if err != nil {
-		return false, fmt.Errorf("cannot parse the ActiveDeadlineSeconds override (%s) for project %s: %v", override, ns.Name, err)
+		return false, fmt.Errorf("cannot parse the ActiveDeadlineSeconds limit (%s) for project %s: %v", limit, ns.Name, err)
 	}
-	pod.Spec.ActiveDeadlineSeconds = &overrideInt64
+	pod.Spec.ActiveDeadlineSeconds = int64MinP(&limitInt64, pod.Spec.ActiveDeadlineSeconds)
 	return true, nil
 }
+
+func int64MinP(a, b *int64) *int64 {
+	switch {
+	case a == nil:
+		return b
+	case b == nil:
+		return a
+	default:
+		c := integer.Int64Min(*a, *b)
+		return &c
+	}
+}

pkg/quota/admission/runonceduration/admission_test.go

@@ -28,7 +28,7 @@ func testCache(projectAnnotations map[string]string) *projectcache.ProjectCache
 
 func testConfig(n *int64) *api.RunOnceDurationConfig {
 	return &api.RunOnceDurationConfig{
-		ActiveDeadlineSecondsOverride: n,
+		ActiveDeadlineSecondsLimit: n,
 	}
 }
 
@@ -81,32 +81,41 @@ func TestRunOnceDurationAdmit(t *testing.T) {
 			expectedActiveDeadlineSeconds: nil,
 		},
 		{
-			name:   "expect configured duration to override existing duration",
+			name:   "expect configured duration to not limit lower existing duration",
 			config: testConfig(int64p(10)),
 			pod:    testRunOncePodWithDuration(5),
-			expectedActiveDeadlineSeconds: int64p(10),
+			expectedActiveDeadlineSeconds: int64p(5),
 		},
 		{
-			name:   "expect empty config to not override existing duration",
+			name:   "expect empty config to not limit existing duration",
 			config: testConfig(nil),
 			pod:    testRunOncePodWithDuration(5),
 			expectedActiveDeadlineSeconds: int64p(5),
 		},
 		{
-			name:   "expect project override to be used with nil global value",
+			name:   "expect project limit to be used with nil global value",
 			config: testConfig(nil),
-			pod:    testRunOncePodWithDuration(5),
+			pod:    testRunOncePodWithDuration(2000),
 			projectAnnotations: map[string]string{
-				api.ActiveDeadlineSecondsOverrideAnnotation: "1000",
+				api.ActiveDeadlineSecondsLimitAnnotation: "1000",
 			},
 			expectedActiveDeadlineSeconds: int64p(1000),
 		},
 		{
-			name:   "expect project override to have priority over global config value",
+			name:   "expect project limit to not limit a smaller set value",
+			config: testConfig(nil),
+			pod:    testRunOncePodWithDuration(10),
+			projectAnnotations: map[string]string{
+				api.ActiveDeadlineSecondsLimitAnnotation: "1000",
+			},
+			expectedActiveDeadlineSeconds: int64p(10),
+		},
+		{
+			name:   "expect project limit to have priority over global config value",
 			config: testConfig(int64p(10)),
-			pod:    testRunOncePodWithDuration(5),
+			pod:    testRunOncePodWithDuration(2000),
 			projectAnnotations: map[string]string{
-				api.ActiveDeadlineSecondsOverrideAnnotation: "1000",
+				api.ActiveDeadlineSecondsLimitAnnotation: "1000",
 			},
 			expectedActiveDeadlineSeconds: int64p(1000),
 		},
@@ -157,10 +166,53 @@ activeDeadlineSecondsOverride: 3600
 	if err != nil {
 		t.Fatalf("unexpected error reading config: %v", err)
 	}
-	if config.ActiveDeadlineSecondsOverride == nil {
-		t.Fatalf("nil value for ActiveDeadlineSecondsOverride")
+	if config.ActiveDeadlineSecondsLimit == nil {
+		t.Fatalf("nil value for ActiveDeadlineSecondsLimit")
 	}
-	if *config.ActiveDeadlineSecondsOverride != 3600 {
-		t.Errorf("unexpected value for ActiveDeadlineSecondsOverride: %d", config.ActiveDeadlineSecondsOverride)
+	if *config.ActiveDeadlineSecondsLimit != 3600 {
+		t.Errorf("unexpected value for ActiveDeadlineSecondsLimit: %d", config.ActiveDeadlineSecondsLimit)
+	}
+}
+
+func TestInt64MinP(t *testing.T) {
+	ten := int64(10)
+	twenty := int64(20)
+	tests := []struct {
+		a, b, expected *int64
+	}{
+		{
+			a:        &ten,
+			b:        nil,
+			expected: &ten,
+		},
+		{
+			a:        nil,
+			b:        &ten,
+			expected: &ten,
+		},
+		{
+			a:        &ten,
+			b:        &twenty,
+			expected: &ten,
+		},
+		{
+			a:        nil,
+			b:        nil,
+			expected: nil,
+		},
+	}
+
+	for _, test := range tests {
+		actual := int64MinP(test.a, test.b)
+		switch {
+		case actual == nil && test.expected != nil,
+			test.expected == nil && actual != nil:
+			t.Errorf("unexpected %v for %#v", actual, test)
+			continue
+		case actual == nil:
+			continue
+		case *actual != *test.expected:
+			t.Errorf("unexpected: %v for %#v", actual, test)
+		}
 	}
 }

pkg/quota/admission/runonceduration/api/types.go

@@ -5,18 +5,20 @@ import (
 )
 
 // RunOnceDurationConfig is the configuration for the RunOnceDuration plugin.
-// It specifies a default override value for ActiveDeadlineSeconds for a run-once pod.
+// It specifies a maximum value for ActiveDeadlineSeconds for a run-once pod.
 // The project that contains the pod may specify a different setting. That setting will
 // take precedence over the one configured for the plugin here.
 type RunOnceDurationConfig struct {
 	unversioned.TypeMeta
 
-	// ActiveDeadlineSecondsOverride is the value to set on containers of run-once pods
+	// ActiveDeadlineSecondsLimit is the maximum value to set on containers of run-once pods
 	// Only a positive value is valid. Absence of a value means that the plugin
 	// won't make any changes to the pod
-	ActiveDeadlineSecondsOverride *int64
+	ActiveDeadlineSecondsLimit *int64
 }
 
-// ActiveDeadlineSecondsOverrideAnnotation can be set on a project to override the number of
+// ActiveDeadlineSecondsLimitAnnotation can be set on a project to limit the number of
 // seconds that a run-once pod can be active in that project
-const ActiveDeadlineSecondsOverrideAnnotation = "openshift.io/active-deadline-seconds-override"
+// TODO: this label needs to change to reflect its function. It's a limit, not an override.
+// It is kept this way for compatibility. Only change it in a new version of the API.
+const ActiveDeadlineSecondsLimitAnnotation = "openshift.io/active-deadline-seconds-override"

pkg/quota/admission/runonceduration/api/v1/conversion.go

@@ -0,0 +1,25 @@
+package v1
+
+import (
+	"k8s.io/kubernetes/pkg/conversion"
+	"k8s.io/kubernetes/pkg/runtime"
+
+	internal "github.com/openshift/origin/pkg/quota/admission/runonceduration/api"
+)
+
+func addConversionFuncs(scheme *runtime.Scheme) {
+	err := scheme.AddConversionFuncs(
+		func(in *RunOnceDurationConfig, out *internal.RunOnceDurationConfig, s conversion.Scope) error {
+			out.ActiveDeadlineSecondsLimit = in.ActiveDeadlineSecondsOverride
+			return nil
+		},
+		func(in *internal.RunOnceDurationConfig, out *RunOnceDurationConfig, s conversion.Scope) error {
+			out.ActiveDeadlineSecondsOverride = in.ActiveDeadlineSecondsLimit
+			return nil
+		},
+	)
+	if err != nil {
+		// If one of the conversion functions is malformed, detect it immediately.
+		panic(err)
+	}
+}

pkg/quota/admission/runonceduration/api/v1/register.go

@@ -10,6 +10,7 @@ var SchemeGroupVersion = unversioned.GroupVersion{Group: "", Version: "v1"}
 
 func AddToScheme(scheme *runtime.Scheme) {
 	addKnownTypes(scheme)
+	addConversionFuncs(scheme)
 }
 
 // Adds the list of known types to api.Scheme.

pkg/quota/admission/runonceduration/api/v1/swagger_doc.go

@@ -6,8 +6,8 @@ package v1
 // ==== DO NOT EDIT THIS FILE MANUALLY ====
 
 var map_RunOnceDurationConfig = map[string]string{
-	"": "RunOnceDurationConfig is the configuration for the RunOnceDuration plugin. It specifies a default override value for ActiveDeadlineSeconds for a run-once pod. The project that contains the pod may specify a different setting. That setting will take precedence over the one configured for the plugin here.",
-	"activeDeadlineSecondsOverride": "ActiveDeadlineSecondsOverride is the value to set on containers of run-once pods Only a positive value is valid. Absence of a value means that the plugin won't make any changes to the pod",
+	"": "RunOnceDurationConfig is the configuration for the RunOnceDuration plugin. It specifies a maximum value for ActiveDeadlineSeconds for a run-once pod. The project that contains the pod may specify a different setting. That setting will take precedence over the one configured for the plugin here.",
+	"activeDeadlineSecondsOverride": "ActiveDeadlineSecondsOverride is the maximum value to set on containers of run-once pods Only a positive value is valid. Absence of a value means that the plugin won't make any changes to the pod",
 }
 
 func (RunOnceDurationConfig) SwaggerDoc() map[string]string {

pkg/quota/admission/runonceduration/api/v1/types.go

@@ -5,14 +5,16 @@ import (
 )
 
 // RunOnceDurationConfig is the configuration for the RunOnceDuration plugin.
-// It specifies a default override value for ActiveDeadlineSeconds for a run-once pod.
+// It specifies a maximum value for ActiveDeadlineSeconds for a run-once pod.
 // The project that contains the pod may specify a different setting. That setting will
 // take precedence over the one configured for the plugin here.
 type RunOnceDurationConfig struct {
 	unversioned.TypeMeta `json:",inline"`
 
-	// ActiveDeadlineSecondsOverride is the value to set on containers of run-once pods
+	// ActiveDeadlineSecondsOverride is the maximum value to set on containers of run-once pods
 	// Only a positive value is valid. Absence of a value means that the plugin
 	// won't make any changes to the pod
-	ActiveDeadlineSecondsOverride *int64 `json:"activeDeadlineSecondsOverride,omitempty",description:"value to override activeDeadlineSeconds in run-once pods"`
+	// TODO: change the external name of this field to reflect that it is a limit, not an override
+	// It is kept this way for compatibility. Only change it in a new version of the API.
+	ActiveDeadlineSecondsOverride *int64 `json:"activeDeadlineSecondsOverride,omitempty",description:"maximum value for activeDeadlineSeconds in run-once pods"`
 }

pkg/quota/admission/runonceduration/api/validation/validation.go

@@ -9,11 +9,11 @@ import (
 // ValidateRunOnceDurationConfig validates the RunOnceDuration plugin configuration
 func ValidateRunOnceDurationConfig(config *api.RunOnceDurationConfig) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if config == nil || config.ActiveDeadlineSecondsOverride == nil {
+	if config == nil || config.ActiveDeadlineSecondsLimit == nil {
 		return allErrs
 	}
-	if *config.ActiveDeadlineSecondsOverride <= 0 {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("activeDeadlineSecondsOverride"), config.ActiveDeadlineSecondsOverride, "must be greater than 0"))
+	if *config.ActiveDeadlineSecondsLimit <= 0 {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("activeDeadlineSecondsOverride"), config.ActiveDeadlineSecondsLimit, "must be greater than 0"))
 	}
 	return allErrs
 }

pkg/quota/admission/runonceduration/api/validation/validation_test.go

@@ -10,7 +10,7 @@ func TestRunOnceDurationConfigValidation(t *testing.T) {
 	// Check invalid duration returns an error
 	var invalidSecs int64 = -1
 	invalidConfig := &api.RunOnceDurationConfig{
-		ActiveDeadlineSecondsOverride: &invalidSecs,
+		ActiveDeadlineSecondsLimit: &invalidSecs,
 	}
 	errs := ValidateRunOnceDurationConfig(invalidConfig)
 	if len(errs) == 0 {
@@ -20,7 +20,7 @@ func TestRunOnceDurationConfigValidation(t *testing.T) {
 	// Check that valid duration returns no error
 	var validSecs int64 = 5
 	validConfig := &api.RunOnceDurationConfig{
-		ActiveDeadlineSecondsOverride: &validSecs,
+		ActiveDeadlineSecondsLimit: &validSecs,
 	}
 	errs = ValidateRunOnceDurationConfig(validConfig)
 	if len(errs) > 0 {

test/integration/runonce_duration_test.go

@@ -15,50 +15,61 @@ import (
 	testserver "github.com/openshift/origin/test/util/server"
 )
 
-func testRunOnceDurationPod() *kapi.Pod {
+func testRunOnceDurationPod(activeDeadlineSeconds int64) *kapi.Pod {
 	pod := &kapi.Pod{}
-	pod.Name = "testpod"
+	pod.GenerateName = "testpod"
 	pod.Spec.RestartPolicy = kapi.RestartPolicyNever
 	pod.Spec.Containers = []kapi.Container{
 		{
 			Name:  "container",
 			Image: "test/image",
 		},
 	}
+	if activeDeadlineSeconds > 0 {
+		pod.Spec.ActiveDeadlineSeconds = &activeDeadlineSeconds
+	}
 	return pod
 }
 
+func testPodDuration(t *testing.T, name string, kclient kclient.Interface, pod *kapi.Pod, expected int64) {
+	// Pod with no duration set
+	pod, err := kclient.Pods(testutil.Namespace()).Create(pod)
+	if err != nil {
+		t.Fatalf("%s: unexpected: %v", name, err)
+	}
+	if pod.Spec.ActiveDeadlineSeconds == nil {
+		t.Errorf("%s: unexpected nil value for pod.Spec.ActiveDeadlineSeconds", name)
+		return
+	}
+	if *pod.Spec.ActiveDeadlineSeconds != expected {
+		t.Errorf("%s: unexpected value for pod.Spec.ActiveDeadlineSeconds: %d. Expected: %d", name, *pod.Spec.ActiveDeadlineSeconds, expected)
+	}
+}
+
 func TestRunOnceDurationAdmissionPlugin(t *testing.T) {
 	var secs int64 = 3600
 	config := &pluginapi.RunOnceDurationConfig{
-		ActiveDeadlineSecondsOverride: &secs,
+		ActiveDeadlineSecondsLimit: &secs,
 	}
 	kclient := setupRunOnceDurationTest(t, config, nil)
-	pod, err := kclient.Pods(testutil.Namespace()).Create(testRunOnceDurationPod())
-	if err != nil {
-		t.Fatalf("Unexpected: %v", err)
-	}
-	if pod.Spec.ActiveDeadlineSeconds == nil || *pod.Spec.ActiveDeadlineSeconds != 3600 {
-		t.Errorf("Unexpected value for pod.ActiveDeadlineSeconds %v", pod.Spec.ActiveDeadlineSeconds)
-	}
+
+	testPodDuration(t, "global, no duration", kclient, testRunOnceDurationPod(0), 3600)
+	testPodDuration(t, "global, larger duration", kclient, testRunOnceDurationPod(7200), 3600)
+	testPodDuration(t, "global, smaller duration", kclient, testRunOnceDurationPod(100), 100)
 }
 
-func TestRunOnceDurationAdmissionPluginProjectOverride(t *testing.T) {
+func TestRunOnceDurationAdmissionPluginProjectLimit(t *testing.T) {
 	var secs int64 = 3600
 	config := &pluginapi.RunOnceDurationConfig{
-		ActiveDeadlineSecondsOverride: &secs,
+		ActiveDeadlineSecondsLimit: &secs,
 	}
 	nsAnnotations := map[string]string{
-		pluginapi.ActiveDeadlineSecondsOverrideAnnotation: "100",
+		pluginapi.ActiveDeadlineSecondsLimitAnnotation: "100",
 	}
 	kclient := setupRunOnceDurationTest(t, config, nsAnnotations)
-	pod, err := kclient.Pods(testutil.Namespace()).Create(testRunOnceDurationPod())
-	if err != nil {
-		t.Fatalf("Unexpected: %v", err)
-	}
-	if pod.Spec.ActiveDeadlineSeconds == nil || *pod.Spec.ActiveDeadlineSeconds != 100 {
-		t.Errorf("Unexpected value for pod.ActiveDeadlineSeconds %v", pod.Spec.ActiveDeadlineSeconds)
-	}
+	testPodDuration(t, "project, no duration", kclient, testRunOnceDurationPod(0), 100)
+	testPodDuration(t, "project, larger duration", kclient, testRunOnceDurationPod(7200), 100)
+	testPodDuration(t, "project, smaller duration", kclient, testRunOnceDurationPod(50), 50)
 }
 
 func setupRunOnceDurationTest(t *testing.T, pluginConfig *pluginapi.RunOnceDurationConfig, nsAnnotations map[string]string) kclient.Interface {