fix: handle dangling symlink in config-manager updateSymlink - #1982
Conversation
|
For context: we hit this in production. After a ConfigMap update, the Retrying never recovers because the dangling symlink itself persists across restarts, so the pod needs manual intervention (delete the symlink or recreate the pod). |
|
Thanks @yoonhyunwoo for fixing this. LGTM. Can we also add a regression test covering the dangling-symlink case? |
|
/ok to test f2f059f |
|
@yoonhyunwoo Thanks for adding the tests. Can you please rebase the branch on the latest |
Signed-off-by: yoonhyunwoo <yjs88zerg@gmail.com>
f2f059f to
1a69452
Compare
Done! |
|
/ok to test 1a69452 |
| return false, nil | ||
| } | ||
| } else if !os.IsNotExist(err) && !errors.Is(err, syscall.EINVAL) { | ||
| return false, fmt.Errorf("error reading symlink '%s': %v", f.ConfigFileDst, err) |
There was a problem hiding this comment.
If you want to quote strings it's better to use %q instead of %s. In this case however, it's better to just unquote the string as the logs are emitted in JSON format.
| return false, fmt.Errorf("error reading symlink '%s': %v", f.ConfigFileDst, err) | |
| return false, fmt.Errorf("error reading symlink %s: %w", f.ConfigFileDst, err) |
| } | ||
| err = os.Remove(f.ConfigFileDst) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return false, fmt.Errorf("error removing existing config: %v", err) |
There was a problem hiding this comment.
| return false, fmt.Errorf("error removing existing config: %v", err) | |
| return false, fmt.Errorf("error removing existing config: %w", err) |
| return false, fmt.Errorf("error removing existing config: %v", err) | ||
| } | ||
| err = os.Remove(f.ConfigFileDst) | ||
| if err != nil && !os.IsNotExist(err) { |
There was a problem hiding this comment.
Isn't this redundant?
!os.IsNotExist(err)
We already check for an IsNotExist error earlier.
| config: "missing-config", | ||
| wantChanged: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
This test is not clear to me. It seems like it depends on the previous test. If this is indeed the case, then it's an antipattern. The unit test should be independent of any other unit tests and it should pass when run in isolation.
3 participants
Fixes #364
Follow-up to #365
dangling symlink is misjudged as "missing" and recreation fails.
os.Readlink checks the symlink itself, so it works correctly even when dangling.