jetstack / navigator

Managed Database-as-a-Service (DBaaS) on Kubernetes
Apache License 2.0
271 stars 31 forks source link

Refactor genericpilot into modular subpackages #183

Closed munnerz closed 6 years ago

munnerz commented 6 years ago

What this PR does / why we need it:

This refactors genericpilot into modular subpackages. I've done this so that we can more easily deviate from the 'GenericPilot' implementation, as I'm thinking it could become to restrictive.

This also tidies up the genericpilot package in general and separates out responsibilities more clearly.

I'm going to be adding a leaderelection package soon and attempt to integrate it with the Elasticsearch pilot. There may be further changes to genericpilot (i.e. shifting of responsibilities), but that can be in follow up PRs.

The API surface for GenericPilot is mostly the same, so the cassandra pilot should not have issues rebasing on this.

Release note:

NONE
munnerz commented 6 years ago

/test e2e

munnerz commented 6 years ago

/test e2e

munnerz commented 6 years ago

This is now working a lot more reliably than before.

There's currently a bug upon SIGTERM being sent that results in immediate subprocess exit (calling process.Wait multiple times). I'm going to refactor the processmanager package significantly in a follow up PR to have a signature closer to:

type ProcessManager interface {
    // Start will start the underlying subprocess
    Start() error
    // Stop will stop the underlying subprocess and wait for it to exit
    Stop() error
    // Wait will return a channel that closes upon the subprocess exiting.
    // If the subprocess has not been started yet, the returned chan will
    // not close until the subprocess has been started and then stopped.
    Wait() <-chan struct{}
}

We can look at adding more things in future if needs be, but I'd rather keep the process manager interface as simple as possible for now.

Edit: I've just implemented this change as part of this PR

/test e2e

munnerz commented 6 years ago

New output of errcheck:

➜  navigator git:(refactor-genericpilot) ✗ errcheck ./pkg/pilot/...
pkg/pilot/elasticsearch/v5/write_config.go:63:16:   defer in.Close()
pkg/pilot/genericpilot/healthz.go:10:11:    }).Listen()
pkg/pilot/genericpilot/healthz.go:16:11:    }).Listen()
pkg/pilot/genericpilot/probe/listen.go:20:12:   w.Write([]byte(err.Error()))

I'm going to be refactoring/rejigging healthz endpoints (which covers the last 3), and the top one is as part of a defer (plus outside of the 'scope' of this PR imo)

munnerz commented 6 years ago

/retest

munnerz commented 6 years ago

/approve

jetstack-ci-bot commented 6 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files: - ~~[OWNERS](https://github.com/jetstack/navigator/blob/master/OWNERS)~~ [munnerz] You can indicate your approval by writing `/approve` in a comment You can cancel your approval by writing `/approve cancel` in a comment
jetstack-ci-bot commented 6 years ago

/test all [submit-queue is verifying that this PR is safe to merge]

munnerz commented 6 years ago

I restarted the build infra workers

/retest

jetstack-ci-bot commented 6 years ago

Automatic merge from submit-queue.