Skip to content
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

Changes to sidecar to fix some issues #30

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 29 additions & 41 deletions cmd/mixtool/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
package main

import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"os"
"path/filepath"

"github.com/urfave/cli"
)
Expand Down Expand Up @@ -99,58 +98,47 @@ type ruleProvisioner struct {
// to existing, does not provision them. It returns whether Prometheus should
// be reloaded and if an error has occurred.
func (p *ruleProvisioner) provision(r io.Reader) (bool, error) {
b := bytes.NewBuffer(nil)
tr := io.TeeReader(r, b)

f, err := os.Open(p.ruleFile)
if err != nil && !os.IsNotExist(err) {
return false, fmt.Errorf("open rule file: %w", err)
}
if os.IsNotExist(err) {
f, err = os.Create(p.ruleFile)
if err != nil {
return false, fmt.Errorf("create rule file: %w", err)
}
fileData, err := ioutil.ReadFile(p.ruleFile)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than reading both rule files completely into memory, i think it could be better to:

  1. Write the new rules to a temp file
  2. Get a file pointer to the old file
  3. Compare the readers for both files using the existing helper fun
  4. If equal, return early
  5. If not equal, rename the temp file

if err != nil {
return false, fmt.Errorf("unable to read %w", err)
allenmqcymp marked this conversation as resolved.
Show resolved Hide resolved
}

equal, err := readersEqual(tr, f)
newData, err := ioutil.ReadAll(r)
if err != nil {
return false, fmt.Errorf("compare existing rules with provisioned intention: %w", err)
return false, fmt.Errorf("unable to read %w", err)
allenmqcymp marked this conversation as resolved.
Show resolved Hide resolved
}
if equal {

res := bytes.Compare(fileData, newData)
if res == 0 {
return false, nil
}

if err := f.Truncate(0); err != nil {
return false, fmt.Errorf("truncate file: %w", err)
// create a temp file
tempfile, err := ioutil.TempFile(filepath.Dir(p.ruleFile), "temp-mixtool")
if err != nil {
return false, fmt.Errorf("unable to create temp file %w", err)
}

if _, err := io.Copy(f, b); err != nil {
return false, fmt.Errorf("provision rule to file: %w", err)
n, err := tempfile.Write(newData)
if err != nil {
return false, fmt.Errorf("error when writing %w", err)
}

return true, nil
}
if n != len(newData) {
return false, fmt.Errorf("writing error, wrote %v bytes, expected %v", n, len(newData))
allenmqcymp marked this conversation as resolved.
Show resolved Hide resolved
}

func readersEqual(r1, r2 io.Reader) (bool, error) {
buf1 := bufio.NewReader(r1)
buf2 := bufio.NewReader(r2)
for {
b1, err1 := buf1.ReadByte()
b2, err2 := buf2.ReadByte()
if err1 != nil && !errors.Is(err1, io.EOF) {
return false, err1
}
if err2 != nil && !errors.Is(err2, io.EOF) {
return false, err2
}
if errors.Is(err1, io.EOF) || errors.Is(err2, io.EOF) {
return err1 == err2, nil
}
if b1 != b2 {
return false, nil
}
tempfile.Sync()

// delete the original file and rename tempfile to the original file
allenmqcymp marked this conversation as resolved.
Show resolved Hide resolved
if err = os.Remove(p.ruleFile); err != nil {
return false, fmt.Errorf("failed to remove old rule file %w", err)
}

if err = os.Rename(tempfile.Name(), p.ruleFile); err != nil {
return false, fmt.Errorf("cannot rename %w", err)
}
return true, nil
}

type prometheusReloader struct {
Expand Down