Provide a writable location for flexvolume plugins #315

gnufied · 2019-01-15T22:34:04Z

Fixes https://jira.coreos.com/browse/STOR-157

This parameter can not be configured via KubeletConfiguration because of kubernetes/kubernetes#72937

gnufied · 2019-01-15T22:35:25Z

corresponding change will also be needed to controller-manager fwiw.

ashcrow · 2019-01-16T14:24:40Z

/retest

cgwalters · 2019-01-16T14:29:41Z

Every config change needs to be (rather tediously) reflected in pkg/controller/template/test_data/ too. When I working on adding a new unit in #273
I ended up doing: for x in openstack none aws; do cp --reflink=auto pkg/controller/template/test_data/templates/{libvirt,$x}/master/units/rhcos-initial-pivot.service; done
although your case is a bit different.

It is tempting though to change the unit tests to only verify e.g. a few cases rather than duplicating all the data again 8 times.

templates/master/01-master-kubelet/_base/units/kubelet.yaml

gnufied · 2019-01-16T14:44:08Z

@cgwalters yep. I am going to push that stuff. I had that updated yesterday night, I forgot to push.

ashcrow · 2019-01-16T15:22:16Z

Unit tests look like they need updating. The end results no longer look like what was expected.

kikisdeliveryservice

Just a small typo fix. Thanks so much for adding this documentation!

pkg/controller/template/test_data/README.md

gnufied · 2019-01-18T12:01:36Z

/test e2e-aws

gnufied · 2019-01-18T12:01:47Z

@cgwalters @ashcrow can you PTAL

pkg/daemon/daemon.go

cgwalters · 2019-01-18T13:30:13Z

pkg/daemon/update.go

@@ -84,6 +84,11 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) error {
 		return fmt.Errorf("%s", msg)
 	}

+	// Create direcotry path for flexvolume plugin
+	if err = dn.fileSystemClient.MkdirAll(pathFlexVolumePlugin, DefaultDirectoryPermissions); err != nil {


I don't think we want this as Go code; it should just be an Ignition config like any others. Specifically an entry in directories; see the Ignition spec.

IOW you'd want this to be a file in templates like how we have the kubelet.yaml there.

However - looking at render.go, we'd need to extend it with e.g. directories.

okay, do you want me do that as part of this PR? I am kinda new to this code base, I am still trying to grok how Ignition spec works.

No worries @gnufied!

If the Ignition config comes through the install process Ignition will do exactly what is needed (creating the directories, dropping the file, etc..).

If it comes down through MCD then MCD itself has a subset of Ignition implementation to create files/dirs/etc.. I think this would make more sense within this area and/or here.

Is the goal here to just create the dir (that is always the same dir location) and never do anything else with it in the MCD?

For clarity, templates is fine. If you need to do some kind of operations then the areas I pointed out would be where to add logic.

Goal is just to create the directory. We don't need to do anything else in MCD.

I agree with @cgwalters that templates makes more sense.

Based on coreos/ignition#706 you could probably "cheat" and make a .dummy file there or so (assuming kubelet won't try to load a .dummy or whatever file as a plugin).

Yeah I think that would work. Kubelet seems to skip loading files that begin with . as plugins.

Related: #125

cgwalters · 2019-01-18T13:30:41Z

Before we proceed farther - does this change have a 4.0 freeze exception?

ashcrow · 2019-01-18T15:36:00Z

@cgwalters I did not see it upon quick scan of the list.

@gnufied can you confirm that this does or does not have the exception approval?

ashcrow · 2019-01-18T15:36:10Z

/hold

gnufied · 2019-01-18T16:52:46Z

@ashcrow @cgwalters this is a bug fix, not a new feature. Does it require 4.0 freeze exception? Adding @childsb @knobunc @tsmetana

childsb · 2019-01-18T16:55:09Z

@gnufied bugfixes shouldn't require an exception, but you should open a bugzilla to help support that its a bug.

gnufied · 2019-01-18T16:55:53Z

There is already a JIRA issue linked with the PR. How many issues must a man open to fix a bug? :D

ashcrow · 2019-01-18T18:14:09Z

OK, if this is indeed a bugfix it doesn't need an exception.

/hold cancel

pkg/daemon/daemon.go

ashcrow

A couple of requests added in comments ... but I'm following the logic now 😄

Fix tests and add a note about how to update testdata

ashcrow · 2019-01-18T21:55:25Z

Terraform/AWS flake

/test e2e-aws-op

ashcrow

This is much clearer 👍

cgwalters · 2019-01-18T22:02:50Z

/approve

kikisdeliveryservice · 2019-01-18T22:06:15Z

I'm going to pull this to verify. :)

cgwalters · 2019-01-18T22:52:32Z

/assign kikisdeliveryservice

Will let her do the final lgtm.

kikisdeliveryservice

Testing results:
Applied updated MCC to cluster and saw new configs for both master and worker were generated and contained:

ExecStart=/usr/bin/hyperkube \
              kubelet \
             ...
                --volume-plugin-dir=/etc/kubernetes/kubelet-plugins/volume/exec \

Manually verify dir created by sshing into master and worker:

[core@... ~]$ cd /etc/kubernetes/kubelet-plugins/volume/exec
[core@... exec]$ ls
[core@... exec]$

Finally check for the hidden dummy file:

[core@... exec]$ ls -a
.  ..  .dummy

kikisdeliveryservice · 2019-01-19T00:13:04Z

/lgtm

openshift-ci-robot · 2019-01-19T00:13:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, gnufied, kikisdeliveryservice

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:

~~OWNERS~~ [cgwalters]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kikisdeliveryservice · 2019-01-19T00:22:34Z

/test e2e-aws

openshift-ci-robot requested review from crawford and smarterclayton January 15, 2019 22:34

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 15, 2019

cgwalters reviewed Jan 16, 2019

View reviewed changes

templates/master/01-master-kubelet/_base/units/kubelet.yaml Show resolved Hide resolved

cgwalters mentioned this pull request Jan 17, 2019

*: include 127.0.0.1 and localhost in etcd peer SAN #322

Closed

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 17, 2019

kikisdeliveryservice reviewed Jan 18, 2019

View reviewed changes

pkg/controller/template/test_data/README.md Outdated Show resolved Hide resolved

pkg/controller/template/test_data/README.md Outdated Show resolved Hide resolved

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 Jan 18, 2019

gnufied force-pushed the change-kubelet-config branch from ea00c90 to 635cb79 Compare January 18, 2019 01:00

cgwalters reviewed Jan 18, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2019

ashcrow reviewed Jan 18, 2019

View reviewed changes

pkg/daemon/daemon.go Outdated Show resolved Hide resolved

ashcrow requested changes Jan 18, 2019

View reviewed changes

Provide a writable location for flexvolume plugins

c94174c

Fix tests and add a note about how to update testdata

gnufied force-pushed the change-kubelet-config branch from 635cb79 to c94174c Compare January 18, 2019 20:53

ashcrow reviewed Jan 18, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2019

openshift-ci-robot assigned kikisdeliveryservice Jan 18, 2019

kikisdeliveryservice reviewed Jan 18, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2019

openshift-merge-robot merged commit 5b93504 into openshift:master Jan 19, 2019

vrutkovs mentioned this pull request Apr 26, 2019

data/bootstrap: pass volume-plugin-dir to kubelet param to avoid flexvolume warnings openshift/installer#1681

Merged

Provide a writable location for flexvolume plugins #315

Provide a writable location for flexvolume plugins #315

Conversation

gnufied commented Jan 15, 2019

gnufied commented Jan 15, 2019

ashcrow commented Jan 16, 2019

cgwalters commented Jan 16, 2019

gnufied commented Jan 16, 2019

ashcrow commented Jan 16, 2019

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

gnufied commented Jan 18, 2019

gnufied commented Jan 18, 2019

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

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

cgwalters commented Jan 18, 2019

ashcrow commented Jan 18, 2019

ashcrow commented Jan 18, 2019

gnufied commented Jan 18, 2019

childsb commented Jan 18, 2019

gnufied commented Jan 18, 2019

ashcrow commented Jan 18, 2019

ashcrow left a comment

Choose a reason for hiding this comment

ashcrow commented Jan 18, 2019

ashcrow left a comment

Choose a reason for hiding this comment

cgwalters commented Jan 18, 2019

kikisdeliveryservice commented Jan 18, 2019

cgwalters commented Jan 18, 2019

kikisdeliveryservice left a comment • edited Loading

Choose a reason for hiding this comment

kikisdeliveryservice commented Jan 19, 2019

openshift-ci-robot commented Jan 19, 2019

kikisdeliveryservice commented Jan 19, 2019

kikisdeliveryservice left a comment •

edited

Loading