Add flag to disable dynamic provisioning #8426

markturansky · 2016-04-08T15:08:54Z

Refactor "VolumeConfig" to "NodeVolumeConfig"
Add MasterVolumeConfig w/ boolean to turn off DP
Only start provisioning controller when the flag is true.
Set true by default

OpenShift Online would like to disable the default dynamic provisioning inherent in Kube. We're implementing our own provisioning controller that will run in a pod on the cluster.

@liggitt @deads2k @ironcladlou @abhgupta

@smarterclayton approved this new boolean config item.

liggitt · 2016-04-08T15:14:06Z

shouldn't that already be expressable with controllers: *,-DynamicProvisioningController or something like that? (would still need changes to honor that internally, and would probably want to assign names to all controllers)

liggitt · 2016-04-08T15:14:43Z

pkg/cmd/server/api/types.go

+// MasterVolumeConfig contains options for configuring volumes on the node.
+type MasterVolumeConfig struct {
+	// EnableDynamicProvisioning is a boolean that toggles the dynamic provisioning feature on/off
+	EnableDynamicProvisioning bool


I don't really want a one-off flag for a particular controller, I'd rather work to make the controllers config param effective for disabling a subset of controllers

We don't have time for the broader refactor. It's too risky a change for
1.2. DynamicProvisioning as a feature flag is ok.

On Fri, Apr 8, 2016 at 11:14 AM, Jordan Liggitt [email protected]
wrote:

In pkg/cmd/server/api/types.go
#8426 (comment):

// LocalQuota contains options for controlling local volume quota on the node. LocalQuota LocalQuota

}

+// MasterVolumeConfig contains options for configuring volumes on the node.
+type MasterVolumeConfig struct {

// EnableDynamicProvisioning is a boolean that toggles the dynamic provisioning feature on/off

EnableDynamicProvisioning bool

I don't really want a one-off flag for a particular controller, I'd rather
work to make the controllers config param effective for disabling a
subset of controllers

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8426/files/e58a75d0de05e59a0b4fe05a06e20adc90aec313#r59040426

As a general rule, controllers enablement is about sharding controllers
(admin operational performance concern). Disabling dynamic provisioning is
an operational admin feature.

On Fri, Apr 8, 2016 at 11:20 AM, Clayton Coleman [email protected]
wrote:

We don't have time for the broader refactor. It's too risky a change for
1.2. DynamicProvisioning as a feature flag is ok.

On Fri, Apr 8, 2016 at 11:14 AM, Jordan Liggitt [email protected]
wrote:

In pkg/cmd/server/api/types.go
#8426 (comment):

// LocalQuota contains options for controlling local volume quota on the node.
LocalQuota LocalQuota
}

+// MasterVolumeConfig contains options for configuring volumes on the node.
+type MasterVolumeConfig struct {

// EnableDynamicProvisioning is a boolean that toggles the dynamic provisioning feature on/off

EnableDynamicProvisioning bool

I don't really want a one-off flag for a particular controller, I'd
rather work to make the controllers config param effective for disabling
a subset of controllers

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8426/files/e58a75d0de05e59a0b4fe05a06e20adc90aec313#r59040426

@jsafrane @swagiaal The new consolidated controller coming in Kube 1.3 will need a constructor arg to enable/disable only one piece/subset of your overall controller. We can then use this proposed flag for that arg. A similar flag can be added to kube-controller-manager as well.

in the external version, this has to be a *bool so we can tell the difference between unspecified (which needs to default to true for backwards compatibility with current configs) and false

deads2k · 2016-04-08T15:25:11Z

@smarterclayton approved this new boolean config item.

Approved for 1.2 or 1.2.x?

markturansky · 2016-04-08T15:30:01Z

@deads2k 1.2 so we can get this into OSE. OSO Ops would then update and configure this new flag.

markturansky · 2016-04-08T16:35:29Z

@liggitt boolean pointer added to external types along with conversions funcs and fuzzer.

liggitt · 2016-04-08T17:33:09Z

pkg/cmd/server/api/serialization_test.go

@@ -68,6 +69,11 @@ func fuzzInternalObject(t *testing.T, forVersion unversioned.GroupVersion, item
 				// The final value of OAuthConfig.MasterCA should never be nil
 				obj.OAuthConfig.MasterCA = &s
 			}
+			if time.Now().Second()%2 == 0 {


if all the iterations happen in a second you'll only exercise one path
use c.RandBool()

actually, the FuzzNoCustom above should be doing that already

You're right. I tested without this and the serialization test works. Removed.

markturansky · 2016-04-08T18:21:54Z

@liggitt as discussed in IRC, i inverted the opt-in to opt-out by changing to "Disable..." and removing the pointer from the bool. Since I got to remove code to make this change, it's the right way to go and it preserves current behavior.

liggitt · 2016-04-08T18:22:58Z

pkg/cmd/server/api/v1/types.go

@@ -71,13 +71,19 @@ type NodeConfig struct {
 	IPTablesSyncPeriod string `json:"iptablesSyncPeriod"`

 	// VolumeConfig contains options for configuring volumes on the node.
-	VolumeConfig VolumeConfig `json:"volumeConfig"`
+	VolumeConfig NodeVolumeConfig


don't remove json tags

markturansky · 2016-04-08T18:26:45Z

@liggitt all json tags added.

liggitt · 2016-04-08T18:27:32Z

pkg/cmd/server/api/v1/types.go

 	// LocalQuota contains options for controlling local volume quota on the node.
-	LocalQuota LocalQuota `json:"localQuota"`
+	LocalQuota LocalQuota


liggitt · 2016-04-08T18:31:47Z

pkg/cmd/server/api/v1/swagger_doc.go

@@ -767,6 +767,6 @@ var map_VolumeConfig = map[string]string{
 	"localQuota": "LocalQuota contains options for controlling local volume quota on the node.",
 }

-func (VolumeConfig) SwaggerDoc() map[string]string {
+func (NodeVolumeConfig) SwaggerDoc() map[string]string {


hack/update-generated-swagger-descriptions.sh should create one like this for MasterVolumeConfig

markturansky · 2016-04-08T20:58:03Z

@liggitt I added code for types_test.go but I am getting diff errors in the test.

I am not familiar with these configs, so I am probably missing something obvious. Can you help me figure out how to get the test correct? I feel it's close, but something is missing....

liggitt · 2016-04-08T21:18:55Z

I think you just put it in the wrong place... needs to be at the end of the master config, alphabetically

markturansky · 2016-04-09T03:00:05Z

@liggitt and that was it :)

liggitt · 2016-04-10T04:11:40Z

[test]
looks reasonable to me, go ahead and squash. @deads2k for a second, @smarterclayton for approval

markturansky · 2016-04-10T06:30:40Z

@liggitt squashed. thanks!

deads2k · 2016-04-11T11:47:17Z

lgtm

smarterclayton · 2016-04-11T13:56:33Z

Explain to me why we are using Disabled* instead of DynamicallyProvisionVolumes *bool and defaulting nil to true? I do not like negative variable names.

markturansky · 2016-04-11T15:30:45Z

@smarterclayton @liggitt I reverted back to *bool. PTAL. I'll squash that 2nd commit after you've had a chance to review it.

smarterclayton · 2016-04-11T16:54:30Z

API docs need be updated.

On Mon, Apr 11, 2016 at 12:30 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2886/)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8426 (comment)

markturansky · 2016-04-11T18:32:35Z

Unrelated failure? Both builds worked but the "conformance build" had a failure:

I0411 13:05:01.390585   19303 plugins.go:46] Registered cloud provider "aws"
I0411 13:05:01.391485   19303 plugins.go:46] Registered cloud provider "gce"
!!! Error in test/extended/conformance.sh:65
    'return $out' exited with status 1
Call stack:
    1: test/extended/conformance.sh:65 main(...)
Exiting with status 1

smarterclayton · 2016-04-11T18:49:53Z

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/85/testReport/ is #8441 and #8412

smarterclayton · 2016-04-11T18:50:09Z

Approved but needs re-review

liggitt · 2016-04-11T19:12:20Z

pkg/cmd/server/api/v1/conversions.go

@@ -307,6 +307,23 @@ func addConversionFuncs(scheme *runtime.Scheme) {
 			out.Location = in.Location
 			return nil
 		},
+		func(in *MasterVolumeConfig, out *internal.MasterVolumeConfig, s conversion.Scope) error {


simplify to

out.DynamicProvisioningEnabled = (in.DynamicProvisioningEnabled == nil) || (*in.DynamicProvisioningEnabled)

changed. this is more succinct.

smarterclayton · 2016-04-11T22:32:21Z

[test]

On Mon, Apr 11, 2016 at 6:05 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2903/)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8426 (comment)

markturansky · 2016-04-12T12:43:38Z

squashed

markturansky · 2016-04-12T14:24:47Z

Unrelated flake:

Cannot find a valid baseurl for repo: extras/7/x86_64
Could not retrieve mirrorlist http://mirrorlist.centos.org/?release=7&arch=x86_64&repo=extras&infra=stock error was
12: Timeout on http://mirrorlist.centos.org/?release=7&arch=x86_64&repo=extras&infra=stock: (28, 'Operation too slow. Less than 1000 bytes/sec transferred the last 30 seconds')

markturansky · 2016-04-12T14:25:04Z

[test]

smarterclayton · 2016-04-12T15:37:11Z

Tests passed except for the deployment flake

markturansky · 2016-04-13T16:29:53Z

@smarterclayton @liggitt Anything else I can do to get this PR merged?

markturansky · 2016-04-15T17:14:31Z

@abhgupta @smarterclayton @liggitt do we want to cherry pick this into 3.2 so Online can use it for the Dev Preview?

liggitt · 2016-04-15T17:23:02Z

LGTM [test]

openshift-bot · 2016-04-15T17:25:21Z

Evaluated for origin test up to e056714

openshift-bot · 2016-04-15T18:25:22Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3012/)

markturansky · 2016-04-15T18:43:14Z

Flake from conformance test. Two other Jenkins jobs both passed.

Stderr from the command:

info: password for stats user admin has been set to 6TKNeTuco5
I0415 13:55:50.696519   19448 plugins.go:62] Registered admission plugin "ServiceAccount"
I0415 13:55:50.712572   19448 plugins.go:46] Registered cloud provider "aws"
I0415 13:55:50.713638   19448 plugins.go:46] Registered cloud provider "gce"
I0415 13:55:52.815063   19549 plugins.go:62] Registered admission plugin "ServiceAccount"
I0415 13:55:52.827799   19549 plugins.go:46] Registered cloud provider "aws"
I0415 13:55:52.828855   19549 plugins.go:46] Registered cloud provider "gce"
Error: No such image or container: 889acc371ac6
Error: No such image or container: b58194f866d5
!!! Error in test/extended/conformance.sh:65
    'return $out' exited with status 1
Call stack:
    1: test/extended/conformance.sh:65 main(...)
Exiting with status 1

liggitt · 2016-04-15T18:46:22Z

[merge]
please open an issue in the openshift-ansible repo with info about the option so it can be set for ansible-driven installs

openshift-bot · 2016-04-15T18:50:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5599/) (Image: devenv-rhel7_3978)

openshift-bot · 2016-04-15T18:50:20Z

Evaluated for origin merge up to e056714

markturansky · 2016-04-15T19:24:04Z

@liggitt issue created openshift/openshift-ansible#1759

liggitt reviewed Apr 8, 2016
View reviewed changes

markturansky force-pushed the provisioning_flag branch 3 times, most recently from 019d74a to b5a4964 Compare April 8, 2016 16:32

liggitt reviewed Apr 8, 2016
View reviewed changes

markturansky force-pushed the provisioning_flag branch from bc96c95 to d55997e Compare April 8, 2016 18:28

liggitt reviewed Apr 8, 2016
View reviewed changes

markturansky force-pushed the provisioning_flag branch from cbaabb9 to b2d75d7 Compare April 10, 2016 06:10

smarterclayton added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2016

markturansky force-pushed the provisioning_flag branch from de85f98 to 03e7959 Compare April 11, 2016 16:11

smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2016

liggitt reviewed Apr 11, 2016
View reviewed changes

Added provisioning flag and refactored VolumeConfig struct names

e056714

markturansky force-pushed the provisioning_flag branch from f51240a to e056714 Compare April 12, 2016 12:43

markturansky mentioned this pull request Apr 15, 2016

Allow opt-out of Dynamic Provisioning openshift/openshift-ansible#1759

Closed

openshift-bot merged commit 906d584 into openshift:master Apr 15, 2016

Add flag to disable dynamic provisioning #8426

Add flag to disable dynamic provisioning #8426

Conversation

markturansky commented Apr 8, 2016

liggitt commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Apr 8, 2016

markturansky commented Apr 8, 2016

markturansky commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Apr 8, 2016

Choose a reason for hiding this comment

markturansky commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Apr 8, 2016

liggitt commented Apr 8, 2016

markturansky commented Apr 9, 2016

liggitt commented Apr 10, 2016

markturansky commented Apr 10, 2016

deads2k commented Apr 11, 2016

smarterclayton commented Apr 11, 2016

markturansky commented Apr 11, 2016

smarterclayton commented Apr 11, 2016

markturansky commented Apr 11, 2016

smarterclayton commented Apr 11, 2016

smarterclayton commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Apr 11, 2016

markturansky commented Apr 12, 2016

markturansky commented Apr 12, 2016

markturansky commented Apr 12, 2016

smarterclayton commented Apr 12, 2016

markturansky commented Apr 13, 2016

markturansky commented Apr 15, 2016

liggitt commented Apr 15, 2016

openshift-bot commented Apr 15, 2016

openshift-bot commented Apr 15, 2016

markturansky commented Apr 15, 2016

liggitt commented Apr 15, 2016

openshift-bot commented Apr 15, 2016

openshift-bot commented Apr 15, 2016

markturansky commented Apr 15, 2016