clusterup add .skip_pv marker #16631

mjudeikis · 2017-10-01T19:39:43Z

Added .skip_pv marker for oc cluster up command.
When the marker is detected in the PV folder - no PV's will be created leaving this configuration for the user.

as per https://trello.com/c/HbpYqEkm/1250-1-cluster-up-option-to-set-number-of-pvs-to-create-clusterup added pv count.

@csrwng @jim-minter

mjudeikis · 2017-10-02T08:20:03Z

/retest

mjudeikis · 2017-10-02T15:57:05Z

@smarterclayton new flag to oc cluster up --pv-count specifying how many PV we need to create. Arou happy with flag as such?

jim-minter

LGTM - two minor nits

jim-minter · 2017-10-02T17:58:57Z

pkg/oc/bootstrap/docker/openshift/pvsetup.go

@@ -81,7 +80,8 @@ for i in $(seq -f "%%04g" 1 %[1]d); do
 done
 `

-func (h *Helper) SetupPersistentStorage(authorizationClient authorizationtypedclient.ClusterRoleBindingsGetter, kclient kclientset.Interface, securityClient securityclient.Interface, dir string) error {
+// SetupPersistentStorage setup persistent storage


SetsUpPersistenStorage?

no, the second word :)

jim-minter · 2017-10-02T17:59:20Z

pkg/oc/bootstrap/docker/up.go

@@ -269,6 +270,7 @@ func (config *CommonStartConfig) Bind(flags *pflag.FlagSet) {
 	flags.StringVar(&config.HTTPProxy, "http-proxy", "", "HTTP proxy to use for master and builds")
 	flags.StringVar(&config.HTTPSProxy, "https-proxy", "", "HTTPS proxy to use for master and builds")
 	flags.StringArrayVar(&config.NoProxy, "no-proxy", config.NoProxy, "List of hosts or subnets for which a proxy should not be used")
+	flags.IntVar(&config.PVCount, "pv-count", 100, "Number of PV to be created. Default 100.")


Suggest "Number of persistent volumes to be created"

smarterclayton · 2017-10-02T18:16:07Z

Not really. Why aren't we creating enough PVs for this never to be a problem?

csrwng · 2017-10-02T18:37:01Z

@smarterclayton the request came from the minishift team to have the option to set the count to 0 if desired.

csrwng · 2017-10-02T19:09:04Z

@mangirdaz, I talked to @smarterclayton and came up with an alternative for adding a flag. Basically if there's directories inside the pv directory you specify, skip creating the pv's.

mjudeikis · 2017-10-03T13:26:00Z

@csrwng added 2 options:

If the folder contains any folders new pvs will not be created. So each second run OR rerun on existing config will not recreate PV's
If folder containers marker file PV creation will be skipped too. allows to setup behavior for 1 run too

csrwng · 2017-10-03T13:47:36Z

@mangirdaz thinking about it some more, I think we should just go with option 2
The job to setup the persistent volumes is re-entrant and it's meant to continue if you shut down your cluster and start it again later. So we wouldn't be able to tell if the existing folders were created previously by the job and it just needs to continue or it shouldn't do anything.

mjudeikis · 2017-10-05T07:36:56Z

/test extended_conformance_gce

mjudeikis · 2017-10-10T10:13:16Z

@csrwng So these will hang on now until master branch will be unlocked?

csrwng · 2017-10-10T13:55:02Z

@mjudeikis yes

mjudeikis · 2017-10-14T17:10:46Z

@csrwng @jim-minter Can you please review the code when you have a spare minute? :)

jorgemoralespou · 2017-10-23T21:21:41Z

@csrwng is the option implemented in this PR, skipping the PV creation on marker file existence satisfactory to the use case that triggered this issue, from the minishift team? Haven't seen any conversation.

csrwng · 2017-10-24T01:57:10Z

@jorgemoralespou sorry no, but I will have the conversation with the minishift team. It is imho the best we can do without adding new flags to cluster up. Because minishift passes the directory to use for pv's to cluster up, the contents of that directory are under their control.

LalatenduMohanty · 2017-10-24T19:09:31Z

@csrwng I think it would solve the original issue of permissions for us. Now we can create the pv (dirs) ourself and then fix the permission if required. cc @hferentschik

gbraad · 2017-10-25T06:03:35Z

@LalatenduMohanty the code to be modified/related to this is: Ref: https://github.com/minishift/minishift/blob/40c37de0cc16651d821cccf826082fff97e6cf9e/pkg/minishift/clusterup/clusterup.go#L217

mjudeikis · 2017-10-26T08:26:59Z

@csrwng what about if we start doing something like hidden flags? the ones which do not appear in -h? not nice? we used to do something similar with few other RH products.

hferentschik · 2017-10-26T09:23:49Z

Personally, I am more in favour of flags and have no issue with them being hidden. I think this keeps things together. A marker file is a bit of a backdoor hack imo. That said, it would work as well of course.

the code to be modified/related to this is: Ref: https://github.com/minishift/minishift/blob/40c37de0cc16651d821cccf826082fff97e6cf9e/pkg/minishift/clusterup/clusterup.go#L217

@LalatenduMohanty, not quite. If we are going down the marker file route, we need to do things before we call cluster up. The code you are pointing out is something we do after cluster up and in fact, the PV creation job has completed. I guess the code you point out might change in a way that we would create PVs at this point ourselves. So it's related ;-)

gbraad · 2017-10-26T09:36:34Z

I agree, this is also in my opinion a workaround. it can work, but not very charming.

@hferentschik as mentioned, it is related, as some of the backstory was missing. it has been nearly 6 months since the request and the offered implementation. Yes, the implementation would happen before clusterup is handled, so right after startHost in start.go (similar to how the proxy is set).

csrwng · 2017-11-14T16:00:14Z

pkg/oc/bootstrap/docker/openshift/pvsetup.go

+	pvSetupJobName     = "persistent-volume-setup"
+	pvInstallerSA      = "pvinstaller"
+	pvSetupNamespace   = "default"
+	pvIgnoreMarkerFile = ".dont_create_pv"


s/.dont_create_pv/.skip_pv/
(just suggestion)
skip_pv seems more concise

csrwng · 2017-11-14T16:00:49Z

@mjudeikis a minor suggestion and can you also update the title/description of the PR? Other than that, it should be ready to merge.

mjudeikis · 2017-11-14T21:44:30Z

Adding this test to standard test was breaking other tests.
Added new test as persistentvolume with an idea that when we do "host provisioner" story to add it, it will go to same set of tests.

csrwng · 2017-11-15T01:04:20Z

/retest

mjudeikis · 2017-11-15T08:21:28Z

/retest

mjudeikis · 2017-11-17T07:39:08Z

@csrwng done

csrwng · 2017-11-17T16:36:03Z

/lgtm

csrwng · 2017-11-17T16:36:29Z

/assign @bparees

bparees · 2017-11-17T16:45:58Z

/approve

openshift-merge-robot · 2017-11-17T16:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, csrwng, mjudeikis

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/oc/bootstrap/OWNERS~~ [bparees,csrwng]
~~test/extended/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-11-18T07:48:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

mjudeikis · 2017-11-18T10:54:29Z

/restest

openshift-merge-robot · 2017-11-18T14:49:31Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-11-18T14:50:05Z

Automatic merge from submit-queue (batch tested with PRs 17289, 16631).

openshift-ci-robot · 2017-11-18T17:16:46Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`7f448c0`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/extended_conformance_install	`7f448c0`	link	`/test extended_conformance_install`
ci/openshift-jenkins/extended_conformance_gce	`7f448c0`	link	`/test extended_conformance_gce`

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.

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 1, 2017

openshift-merge-robot assigned csrwng and coreydaley Oct 1, 2017

jim-minter reviewed Oct 2, 2017

View reviewed changes

mjudeikis force-pushed the oc-cluster-up-pv-number branch from 239c58e to 79148fb Compare October 3, 2017 13:23

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2017

mjudeikis force-pushed the oc-cluster-up-pv-number branch from 79148fb to af102e5 Compare October 4, 2017 18:16

This was referenced Oct 24, 2017

[cluster up] Allow to change the permissions of created persistent volumes #14076

Closed

Pods unable to write to persistent volumes minishift/minishift#856

Closed

csrwng reviewed Nov 14, 2017

View reviewed changes

mjudeikis force-pushed the oc-cluster-up-pv-number branch from af102e5 to 5973127 Compare November 14, 2017 16:18

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 14, 2017

mjudeikis force-pushed the oc-cluster-up-pv-number branch from 5973127 to 922a15d Compare November 14, 2017 16:41

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017

mjudeikis changed the title ~~oc cluster up - add flag for pv count~~ clusterup add .skip_pv flag Nov 14, 2017

mjudeikis changed the title ~~clusterup add .skip_pv flag~~ clusterup add .skip_pv marker Nov 14, 2017

mjudeikis force-pushed the oc-cluster-up-pv-number branch from 922a15d to 04f0fdb Compare November 14, 2017 18:13

add skip_pv marker to skip PV creation

7f448c0

mjudeikis force-pushed the oc-cluster-up-pv-number branch from 04f0fdb to 7f448c0 Compare November 14, 2017 21:42

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2017

openshift-ci-robot assigned bparees Nov 17, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017

openshift-merge-robot merged commit 294f07f into openshift:master Nov 18, 2017

praveenkumar mentioned this pull request Dec 29, 2017

skip_pv marker file doesn't work as expected for oc cluster up #17948

Closed

clusterup add .skip_pv marker #16631

clusterup add .skip_pv marker #16631

Conversation

mjudeikis commented Oct 1, 2017 • edited Loading

mjudeikis commented Oct 2, 2017

mjudeikis commented Oct 2, 2017

jim-minter left a comment

Choose a reason for hiding this comment

jim-minter Oct 2, 2017

Choose a reason for hiding this comment

mjudeikis Oct 3, 2017

Choose a reason for hiding this comment

csrwng Oct 3, 2017

Choose a reason for hiding this comment

jim-minter Oct 2, 2017

Choose a reason for hiding this comment

smarterclayton commented Oct 2, 2017

csrwng commented Oct 2, 2017

csrwng commented Oct 2, 2017

mjudeikis commented Oct 3, 2017

csrwng commented Oct 3, 2017

mjudeikis commented Oct 5, 2017

mjudeikis commented Oct 10, 2017

csrwng commented Oct 10, 2017

mjudeikis commented Oct 14, 2017

jorgemoralespou commented Oct 23, 2017

csrwng commented Oct 24, 2017

LalatenduMohanty commented Oct 24, 2017

gbraad commented Oct 25, 2017

mjudeikis commented Oct 26, 2017

hferentschik commented Oct 26, 2017

gbraad commented Oct 26, 2017

csrwng Nov 14, 2017

Choose a reason for hiding this comment

csrwng commented Nov 14, 2017

mjudeikis commented Nov 14, 2017

csrwng commented Nov 15, 2017

mjudeikis commented Nov 15, 2017

mjudeikis commented Nov 17, 2017

csrwng commented Nov 17, 2017

csrwng commented Nov 17, 2017

bparees commented Nov 17, 2017

openshift-merge-robot commented Nov 17, 2017

openshift-bot commented Nov 18, 2017

mjudeikis commented Nov 18, 2017

openshift-merge-robot commented Nov 18, 2017

openshift-merge-robot commented Nov 18, 2017

openshift-ci-robot commented Nov 18, 2017

mjudeikis commented Oct 1, 2017 •

edited

Loading