Rework method of updating atomic-updated data volumes #57422
Conversation
k8s-ci-robot
added
release-note
|
/kind bug |
joelsmith
force-pushed
the
nested_data_vol
branch
from
ec6f5db
to
d2e86df
Compare
|
/retest |
joelsmith
force-pushed
the
nested_data_vol
branch
4 times, most recently
from
cfddeb1
to
082d520
Compare
|
/retest |
joelsmith
force-pushed
the
nested_data_vol
branch
2 times, most recently
from
090419c
to
ee2ae94
Compare
|
@kubernetes/sig-storage-pr-reviews @kubernetes/sig-node-pr-reviews |
k8s-ci-robot
added
sig/storage
|
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). |
pkg/volume/util/atomic_writer.go
Outdated
| // 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) |
There was a problem hiding this comment.
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) { | |||
There was a problem hiding this comment.
nice cleanup removing from the struct 👍
Can we just use dataDirName instead of oldTsDir and avoid adding a param to the func?
There was a problem hiding this comment.
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.")) | |||
pkg/volume/util/atomic_writer.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| l := strings.Index(userVisiblePath, string(os.PathSeparator)) |
There was a problem hiding this comment.
That 'l' looks so much like a '1'. Different variable name?
| return err | ||
| ps := string(os.PathSeparator) | ||
| var lasterr error | ||
| for p := range paths { |
There was a problem hiding this comment.
Makes sense that we don't need the path ordering anymore. Just calling it out to make sure.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Removal failures are not immediately fatal now. We could lose errors. Just pointing it out.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Can we just use dataDirName instead of oldTsDir and avoid adding a param to the func?
There was a problem hiding this comment.
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.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
removed
the
lgtm
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.
joelsmith
force-pushed
the
nested_data_vol
branch
from
56490da
to
b4e0923
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
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: I don't think this can cause any issues. Reading /approve |
|
[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 |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
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] ```
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
|
We are seeing some differences in volumeMounts after upgrading to 1.8.8 and think this might be the cause: 1.8.8: 1.8.6: This seems to work fine for most things, but for example |
|
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. |
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
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.
..datasymlink 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: