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.

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

Rework method of updating atomic-updated data volumes #57422

Merged
merged 1 commit into from Jan 18, 2018

Conversation

@joelsmith
Contributor

joelsmith commented Dec 19, 2017

What this PR does / why we need it:

This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality.

  • Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories.
  • Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory.
  • Fix data dir timestamp format year
  • Create ..data symlink even when a data volume has no data so consumers can have simplified update watch logic.

Which issue(s) this PR fixes:
Fixes #57421

Fixes #60814 for master / 1.10

Release note:

Fix a bug affecting nested data volumes such as secret, configmap, etc.
@joelsmith

This comment has been minimized.

Contributor

joelsmith commented Dec 19, 2017

/kind bug

@joelsmith

This comment has been minimized.

Contributor

joelsmith commented Dec 19, 2017

/retest

1 similar comment
@joelsmith

This comment has been minimized.

Contributor

joelsmith commented Dec 20, 2017

/retest

@liggitt

This comment has been minimized.

@joelsmith

This comment has been minimized.

Contributor

joelsmith commented Jan 2, 2018

@saad-ali @jsafrane would you be able to help review this?

@pmorie

This comment has been minimized.

Member

pmorie commented Jan 5, 2018

I'm happy to review this but need to allocate some time to give this my undivided attention (probably will not be until Tuesday Jan 9).

@sjenning

My comments are cleanup suggestions that might ease backporting. Looks good. I'll wait to lgtm until you respond and make any changes with which you agree.

// empty oldTsDir indicates that it didn't exist
oldTsDir = ""
} else if err != nil && !os.IsNotExist(err) {
glog.Errorf("%s: error reading link for data directory: %v", w.logContext, err)

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

This block was confusing to me. I think it would look better like this:

	if err != nil {
		if !os.IsNotExist(err) {
			glog.Errorf("%s: error reading link for data directory: %v", w.logContext, err)
			return err
		}
		// although Readlink() returns "" on err, don't be fragile by relying on it (since it's not specified in docs)
		// empty oldTsDir indicates that it didn't exist
		oldTsDir = ""
	}
@@ -270,9 +276,9 @@ func validatePath(targetPath string) error {
}
// shouldWritePayload returns whether the payload should be written to disk.
func (w *AtomicWriter) shouldWritePayload(payload map[string]FileProjection) (bool, error) {
func shouldWritePayload(payload map[string]FileProjection, oldTsDir string) (bool, error) {

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

nice cleanup removing from the struct 👍

Can we just use dataDirName instead of oldTsDir and avoid adding a param to the func?

This comment has been minimized.

@joelsmith

joelsmith Jan 17, 2018

Contributor

On this one, yes, we could keep the struct, then do something like:

 shouldWrite, err := shouldWriteFile(path.Join(w.targetDir, dataDirName, userVisiblePath), fileProjection.Data)

and then shouldWriteFile() would follow the ..data symlink.

@@ -348,7 +350,7 @@ func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection) (sets.St
// newTimestampDir creates a new timestamp directory
func (w *AtomicWriter) newTimestampDir() (string, error) {
tsDir, err := ioutil.TempDir(w.targetDir, fmt.Sprintf("..%s.", time.Now().Format("1981_02_01_15_04_05")))
tsDir, err := ioutil.TempDir(w.targetDir, time.Now().UTC().Format("..2006_01_02_15_04_05."))

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

nice cleanup 👍

if err != nil {
return err
}
l := strings.Index(userVisiblePath, string(os.PathSeparator))

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

That 'l' looks so much like a '1'. Different variable name?

return err
ps := string(os.PathSeparator)
var lasterr error
for p := range paths {

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

Makes sense that we don't need the path ordering anymore. Just calling it out to make sure.

This comment has been minimized.

@joelsmith

joelsmith Jan 17, 2018

Contributor

Right, the ordered removal is no longer necessary. Thanks for making sure.

}
if err := os.Remove(path.Join(w.targetDir, p)); err != nil {
glog.Errorf("%s: error pruning old user-visible path %s: %v", w.logContext, p, err)
lasterr = err

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

Removal failures are not immediately fatal now. We could lose errors. Just pointing it out.

This comment has been minimized.

@joelsmith

joelsmith Jan 17, 2018

Contributor

I did this on purpose, but I don't know that it's better. My thinking was "If we have a problem removing 1 file, isn't it better to clean up as much of the rest of it as possible?" but I'm not sure what the answer is. The other problem with the new approach is that any errors but the last one will not be seen by the caller (but will be in the error log). I'm happy with either behavior (old/new).

tsDir, err := w.newTimestampDir()
if err != nil {
glog.V(4).Infof("%s: error creating new ts data directory: %v", w.logContext, err)
return err
}
tsDirName := filepath.Base(tsDir)

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

better than _, tsDirName := filepath.Split(tsDir) 👍

// determines which paths should be removed (if any) after the payload is
// written to the target directory.
func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection) (sets.String, error) {
func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir string) (sets.String, error) {

This comment has been minimized.

@sjenning

sjenning Jan 17, 2018

Contributor

Can we just use dataDirName instead of oldTsDir and avoid adding a param to the func?

This comment has been minimized.

@joelsmith

joelsmith Jan 17, 2018

Contributor

I don't think so because we need an absolute path for the root passed to filepath.Walk(). Also, because filepath.Walk() doesn't follow symlinks, we'd have to construct an absolute path out of w.targetDir and dataDirName, and then we'd need to follow that symlink to get the path. My thought is that it's better to follow the symlink once (to create oldTsDir) and pass that to anything that needs it.

@sjenning

This comment has been minimized.

Contributor

sjenning commented Jan 17, 2018

/lgtm

Rework method of updating atomic-updated data volumes
This change affects the way that secret, configmap, downwardAPI and projected
volumes (which all use the same underlying code) implement their data update
functionality.

* Instead of creating a subdirectory hierarchy that itself
  will contain symlinks to each actual data file, create only
  symlinks to items in the root of the volume, whether they
  be files or directories.
* Rather than comparing the user-visible data directory
  to see if an update is needed, compare with the current
  version of the data directory.
* Fix data dir timestamp format year
* Create ..data symlink even when a data volume has no data so
  consumers can have simplified update watch logic.
@sjenning

This comment has been minimized.

Contributor

sjenning commented Jan 17, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 17, 2018

@jsafrane

This comment has been minimized.

Member

jsafrane commented Jan 18, 2018

I tested it with various volumes (downward, secrets, configmap), they get updated with the API object changes correctly and any other files created either by user or kubelet are ignored and not removed.

I noticed only one difference in handling directories: instead of a directory with symlink to a file there is a symlink to directory with a file:

Old: dir/subdir/file -> ..data/dir/subdir/file and file has the right content
Now: dir -> ..data/dir and ..data/dir has the right subdir/file.

I don't think this can cause any issues. Reading dir/subdir/file leads to the right content in both cases.

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelsmith, jsafrane, sjenning

Associated issue: #57421

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

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

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 18, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 18, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 3c99777 into kubernetes:master Jan 18, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation joelsmith authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@joelsmith joelsmith deleted the joelsmith:nested_data_vol branch Jan 18, 2018

k8s-merge-robot added a commit that referenced this pull request Jan 19, 2018

Merge pull request #58460 from joelsmith/automated-cherry-pick-of-#57422
-upstream-release-1.8

Automatic merge from submit-queue.

[1.8] Automated cherry pick of #57422: Rework method of updating atomic-updated data volumes

Cherry pick of #57422 on release-1.8.

#57422: Rework method of updating atomic-updated data volumes

openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 19, 2018

Merge pull request #18165 from joelsmith/pick-57422
Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165).

UPSTREAM: 57422: Rework method of updating atomic-updated data volumes

**What this PR does / why we need it**:

This is a backport of kubernetes/kubernetes#57422

This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality.

* Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories.
* Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory.
* Fix data dir timestamp format year
* Create `..data` symlink even when a data volume has no data so consumers can have simplified update watch logic.

**Which issue(s) this PR fixes**
This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1516569 and https://bugzilla.redhat.com/show_bug.cgi?id=1430322

**Special notes for your reviewer**:

**Release note**:
```release-note
Allow volumes to be mounted beneath secret volumes [1430322] [1516569]
```

k8s-merge-robot added a commit that referenced this pull request Jan 19, 2018

Merge pull request #58463 from joelsmith/automated-cherry-pick-of-#57422
-upstream-release-1.7

Automatic merge from submit-queue.

[1.7] Automated cherry pick of #57422: Rework method of updating atomic-updated data volumes

Cherry pick of #57422 on release-1.7.

#57422: Rework method of updating atomic-updated data volumes

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 24, 2018

Merge pull request kubernetes#18165 from joelsmith/pick-57422
Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165).

UPSTREAM: 57422: Rework method of updating atomic-updated data volumes

**What this PR does / why we need it**:

This is a backport of kubernetes#57422

This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality.

* Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories.
* Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory.
* Fix data dir timestamp format year
* Create `..data` symlink even when a data volume has no data so consumers can have simplified update watch logic.

**Which issue(s) this PR fixes**
This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1516569 and https://bugzilla.redhat.com/show_bug.cgi?id=1430322

**Special notes for your reviewer**:

**Release note**:
```release-note
Allow volumes to be mounted beneath secret volumes [1430322] [1516569]
```

Origin-commit: 91d864d470581d67e38bbe3fe3b967aeea3feded

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 25, 2018

Merge pull request kubernetes#18165 from joelsmith/pick-57422
Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165).

UPSTREAM: 57422: Rework method of updating atomic-updated data volumes

**What this PR does / why we need it**:

This is a backport of kubernetes#57422

This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality.

* Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories.
* Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory.
* Fix data dir timestamp format year
* Create `..data` symlink even when a data volume has no data so consumers can have simplified update watch logic.

**Which issue(s) this PR fixes**
This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1516569 and https://bugzilla.redhat.com/show_bug.cgi?id=1430322

**Special notes for your reviewer**:

**Release note**:
```release-note
Allow volumes to be mounted beneath secret volumes [1430322] [1516569]
```

Origin-commit: 91d864d470581d67e38bbe3fe3b967aeea3feded

k8s-merge-robot added a commit that referenced this pull request Jan 27, 2018

Merge pull request #58458 from joelsmith/automated-cherry-pick-of-#57422
-upstream-release-1.9

Automatic merge from submit-queue.

[1.9] Automated cherry pick of #57422: Rework method of updating atomic-updated data volumes

Cherry pick of #57422 on release-1.9.

#57422: Rework method of updating atomic-updated data volumes
@blakebarnett

This comment has been minimized.

Collaborator

blakebarnett commented Feb 15, 2018

We are seeing some differences in volumeMounts after upgrading to 1.8.8 and think this might be the cause:

1.8.8:

kubectl exec flipper-8496978cdf-l94lv -- ls -l /etc/confd/conf.d/cernan.toml
-rw-r--r--    1 root     root           215 Feb 15 00:12 /etc/confd/conf.d/cernan.toml

1.8.6:

kubectl exec flipper-67b85b69d-wnnh6 -- ls -l /etc/confd/conf.d/cernan.toml
lrwxrwxrwx    1 root     root            28 Feb 15 00:01 /etc/confd/conf.d/cernan.toml -> ../..data/conf.d/cernan.toml

This seems to work fine for most things, but for example confd tries to list files in this directory and can't see them:

[pid   332] futex(0xc420030810, FUTEX_WAIT, 0, NULL2018-02-15T01:24:17Z flipper-8496978cdf-l94lv ./confd-0.15.0-linux-amd64[329]: DEBUG Loading template resources from confdir /etc/confd
 <unfinished ...>
[pid   334] <... write resumed> )       = 136
[pid   330] <... pselect6 resumed> )    = 0 (Timeout)
[pid   334] stat("/etc/confd",  <unfinished ...>
[pid   330] clock_gettime(CLOCK_MONOTONIC,  <unfinished ...>
[pid   334] <... stat resumed> {st_mode=S_IFDIR|0777, st_size=4096, ...}) = 0
[pid   330] <... clock_gettime resumed> {tv_sec=8644, tv_nsec=900518357}) = 0
[pid   334] lstat("/etc/confd/conf.d",  <unfinished ...>
[pid   330] epoll_wait(4,  <unfinished ...>
[pid   334] <... lstat resumed> {st_mode=S_IFLNK|0777, st_size=13, ...}) = 0
[pid   330] <... epoll_wait resumed> [], 128, 0) = 0
[pid   334] clock_gettime(CLOCK_REALTIME,  <unfinished ...>
[pid   330] pselect6(0, NULL, NULL, NULL, {tv_sec=0, tv_nsec=20000}, NULL <unfinished ...>
[pid   334] <... clock_gettime resumed> {tv_sec=1518657858, tv_nsec=84617856}) = 0
[pid   334] clock_gettime(CLOCK_MONOTONIC, {tv_sec=8644, tv_nsec=900886308}) = 0
[pid   330] <... pselect6 resumed> )    = 0 (Timeout)
[pid   334] clock_gettime(CLOCK_REALTIME,  <unfinished ...>
[pid   330] clock_gettime(CLOCK_MONOTONIC,  <unfinished ...>
[pid   334] <... clock_gettime resumed> {tv_sec=1518657858, tv_nsec=84816499}) = 0
[pid   330] <... clock_gettime resumed> {tv_sec=8644, tv_nsec=901001962}) = 0
[pid   334] clock_gettime(CLOCK_MONOTONIC,  <unfinished ...>
[pid   330] pselect6(0, NULL, NULL, NULL, {tv_sec=0, tv_nsec=20000}, NULL <unfinished ...>
[pid   334] <... clock_gettime resumed> {tv_sec=8644, tv_nsec=901075021}) = 0
[pid   334] openat(AT_FDCWD, "/proc/sys/kernel/hostname", O_RDONLY|O_CLOEXEC) = 3
[pid   334] epoll_ctl(4, EPOLL_CTL_ADD, 3, {EPOLLIN|EPOLLOUT|EPOLLRDHUP|EPOLLET, {u32=919723888, u64=139767745470320}}) = 0
[pid   330] <... pselect6 resumed> )    = 0 (Timeout)
[pid   334] fcntl(3, F_GETFL <unfinished ...>
[pid   330] clock_gettime(CLOCK_MONOTONIC,  <unfinished ...>
[pid   334] <... fcntl resumed> )       = 0x8000 (flags O_RDONLY|O_LARGEFILE)
[pid   330] <... clock_gettime resumed> {tv_sec=8644, tv_nsec=901370126}) = 0
[pid   334] fcntl(3, F_SETFL, O_RDONLY|O_NONBLOCK|O_LARGEFILE <unfinished ...>
[pid   330] pselect6(0, NULL, NULL, NULL, {tv_sec=0, tv_nsec=20000}, NULL <unfinished ...>
[pid   334] <... fcntl resumed> )       = 0
[pid   334] read(3,  <unfinished ...>
[pid   330] <... pselect6 resumed> )    = 0 (Timeout)
[pid   334] <... read resumed> "flipper-8496978cdf-l94lv\n", 512) = 25
[pid   330] clock_gettime(CLOCK_MONOTONIC,  <unfinished ...>
[pid   334] epoll_ctl(4, EPOLL_CTL_DEL, 3, 0xc42004265c <unfinished ...>
[pid   330] <... clock_gettime resumed> {tv_sec=8644, tv_nsec=901649274}) = 0
[pid   334] <... epoll_ctl resumed> )   = 0
[pid   330] pselect6(0, NULL, NULL, NULL, {tv_sec=0, tv_nsec=20000}, NULL <unfinished ...>
[pid   334] close(3)                    = 0
[pid   334] getpid()                    = 329
[pid   334] write(2, "2018-02-15T01:24:18Z flipper-849"..., 106 <unfinished ...>
[pid   330] <... pselect6 resumed> )    = 0 (Timeout)
[pid   330] clock_gettime(CLOCK_MONOTONIC, {tv_sec=8644, tv_nsec=902026848}) = 0
2018-02-15T01:24:18Z flipper-8496978cdf-l94lv ./confd-0.15.0-linux-amd64[329]: WARNING Found no templates
@blakebarnett

This comment has been minimized.

Collaborator

blakebarnett commented Feb 26, 2018

The change in behavior is strange, but I assume #58720 will break what this was setup for anyway, so I'll just audit our usage and make sure nobody's doing this anywhere anymore.

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Feb 27, 2018

Merge pull request kubernetes#18165 from joelsmith/pick-57422
Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165).

UPSTREAM: 57422: Rework method of updating atomic-updated data volumes

**What this PR does / why we need it**:

This is a backport of kubernetes#57422

This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality.

* Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories.
* Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory.
* Fix data dir timestamp format year
* Create `..data` symlink even when a data volume has no data so consumers can have simplified update watch logic.

**Which issue(s) this PR fixes**
This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1516569 and https://bugzilla.redhat.com/show_bug.cgi?id=1430322

**Special notes for your reviewer**:

**Release note**:
```release-note
Allow volumes to be mounted beneath secret volumes [1430322] [1516569]
```

Origin-commit: 91d864d470581d67e38bbe3fe3b967aeea3feded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment