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

Machine endpoints #2

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Machine endpoints #2

wants to merge 9 commits into from

Conversation

klump
Copy link

@klump klump commented Jan 27, 2022

Add missing endpoints to make tateru work.

Some points that might require more fixes/ discussion:

  • The boot-installer request is not blocking/ not idempotent
  • Handling of mutex unlock (defer is better than deadlocks in case of errors, but maybe we should use sync.Once?)
  • Should we expire installRequests after some time?
  • Should we implement an additional callback from the installer when it shuts down. Then we could clean up the installRequest.

- boot-installer
- installer-callback
- fetch-machine
Colors that that indicate certain states:
- cyan: pending (boot-installer called)
- blue: booting (request sent to manager)
- green: booted (installer-callback called)
@klump klump requested a review from bluecmd January 27, 2022 09:23
database.go Outdated Show resolved Hide resolved
database.go Show resolved Hide resolved
database.go Show resolved Hide resolved
database.go Outdated Show resolved Hide resolved
database.go Outdated
@@ -115,28 +187,220 @@ func (db *tateruDb) Poll() {
db.machinesMutex.Lock()
db.machines = machs
db.machinesMutex.Unlock()

// TODO: expire old install requests?
Copy link
Member

Choose a reason for hiding this comment

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

In tateru-vsphere I use github.com/hashicorp/golang-lru

Copy link
Author

Choose a reason for hiding this comment

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

Check out 846cb6b

database.go Outdated
time.Sleep(time.Second * 30)
}
}

func (db *tateruDb) HandleMachinesAPI(w http.ResponseWriter, r *http.Request) {
w.Header().Add("content-type", "application/json; charset=utf-8")

if r.Method != "GET" {
http.Error(w, "Unsupported method", http.StatusMethodNotAllowed)
db.machinesMutex.RLock()
Copy link
Member

Choose a reason for hiding this comment

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

Try to limit the area where the lock is held. You don't have to bend over backwards, here you only need it over the for-loop, and not during JSON construction and data writing.

Copy link
Author

Choose a reason for hiding this comment

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

Tried to improve mutex handling a bit in 846cb6b as well

database.go Outdated

db.machinesMutex.RLock()
b, err := json.MarshalIndent(db.machines, "", " ")
defer db.machinesMutex.RUnlock()
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

database.go Outdated
uuid := vars["uuid"]

db.machinesMutex.Lock()
defer db.machinesMutex.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

And especially here as you do calls to other machines while holding a Read/Write lock.

Instead of reusing machinesMutex to also guard installRequests, I think you should either rename the machinesMutex to be dbMutex or, preferably, create a new installRequestsMutex.

Also, if you haven't already - consider using https://pkg.go.dev/sync#Map to avoid another mutex altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants