Skip to content

Commit

Permalink
Configuration writes should be atomic
Browse files Browse the repository at this point in the history
This change introduces a method `atomicFileWrite(...)` which writes to a temporary file and then renames it to avoid partial reads, e.g.

```
Mar 29 04:13:05 labkubedualhost-node-1 crio[33593]: time="2024-03-29 04:13:05.977398607+09:00" level=info msg="CNI monitoring event CREATE        \"/etc/cni/net.d/00-multus.conf\""
Mar 29 04:13:05 labkubedualhost-node-1 crio[33593]: time="2024-03-29 04:13:05.977767189+09:00" level=error msg="Error loading CNI config file /etc/cni/net.d/00-multus.conf: error parsing configuration: unexpected end of JSON input"
```
  • Loading branch information
dougbtv committed Mar 28, 2024
1 parent 0fd3fa7 commit 5d0ff07
Showing 1 changed file with 29 additions and 2 deletions.
31 changes: 29 additions & 2 deletions pkg/server/config/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"os"
"path/filepath"
"sync"
Expand Down Expand Up @@ -64,6 +65,32 @@ func NewManager(config MultusConf) (*Manager, error) {
return newManager(config, defaultPluginName)
}

// atomicFileWrite writes our configs atomically to avoid partial reads
func atomicFileWrite(filename string, data []byte, perm os.FileMode) error {

Check warning on line 69 in pkg/server/config/manager.go

View workflow job for this annotation

GitHub Actions / test (1.20.x, ubuntu-latest)

parameter 'perm' seems to be unused, consider removing or renaming it as _

Check warning on line 69 in pkg/server/config/manager.go

View workflow job for this annotation

GitHub Actions / test (1.21.x, ubuntu-latest)

parameter 'perm' seems to be unused, consider removing or renaming it as _
// Create a temporary file in the same directory as the destination file
dir := filepath.Dir(filename)
tmpFile, err := ioutil.TempFile(dir, ".multus.tmp")
if err != nil {
return err
}
tmpFileName := tmpFile.Name()

// Always remove that temp file...
defer os.Remove(tmpFileName)

// Write data to the temporary file
if _, err := tmpFile.Write(data); err != nil {
tmpFile.Close()
return err
}
if err := tmpFile.Close(); err != nil {
return err
}

// Atomically rename the temporary file to the final filename
return os.Rename(tmpFileName, filename)
}

// overrideCNIVersion overrides cniVersion in cniConfigFile, it should be used only in kind case
func overrideCNIVersion(cniConfigFile string, multusCNIVersion string) error {
path, err := filepath.Abs(cniConfigFile)
Expand All @@ -87,7 +114,7 @@ func overrideCNIVersion(cniConfigFile string, multusCNIVersion string) error {
return fmt.Errorf("couldn't update cluster network config: %v", err)
}

err = os.WriteFile(path, configBytes, 0644)
err = atomicFileWrite(path, configBytes, 0644)
if err != nil {
return fmt.Errorf("couldn't update cluster network config: %v", err)
}
Expand Down Expand Up @@ -268,7 +295,7 @@ func (m *Manager) PersistMultusConfig(config string) (string, error) {
} else {
logging.Debugf("Writing Multus CNI configuration @ %s", m.multusConfigFilePath)
}
return m.multusConfigFilePath, os.WriteFile(m.multusConfigFilePath, []byte(config), UserRWPermission)
return m.multusConfigFilePath, atomicFileWrite(m.multusConfigFilePath, []byte(config), UserRWPermission)
}

func (m *Manager) shouldRegenerateConfig(event fsnotify.Event) bool {
Expand Down

0 comments on commit 5d0ff07

Please sign in to comment.