atlassian-archive/ctrl

Tiny Kubernetes controller framework

ctrl's Issues

Add structured logging panic handlers to all launched go routines

Directly I can see leader election, startstoptlsserver, and canceloninterrupt directly launching go routines. If they panic we end up with golangs default multi-line panic printer. Each of these cases (and any other I missed) should allow calling code to set a panic handling function to be called in the event of a panic in their launched go routine.

Code would accept a no arg func() and defer it.

A default implementation of that such a panic handler might look like this:

func StructuredRecover() {
	if r := recover(); r != nil {
		structuredPanic(os.Stderr, r, time.Now().Format(time.RFC3339), debug.Stack())
		panic(r)
	}
}

func structuredPanic(out io.Writer, i interface{}, time string, stack []byte) {
	bytes, err := json.Marshal(struct {
		Level   string `json:"level"`
		Time    string `json:"time"`
		Message string `json:"msg"`
		Stack   string `json:"stack"`
	}{
		Level:   "fatal",
		Time:    time,
		Message: fmt.Sprintf("%v", i),
		Stack:   string(stack),
	})
	if err != nil {
		fmt.Fprintf(out, "error while serializing cmd exit panic: %+v\n", err) // nolint: errcheck
		fmt.Fprintf(out, "original panic: %+v\n", i)                           // nolint: errcheck
		return
	}
	fmt.Fprintf(out, "%s\n", bytes) // nolint: errcheck
}

@ash2k thoughts?

Support ready for work func for servers

Fix object name fields in logs

Handlers are not using the right name functions.

Ctrl owns too much of the application

@wryun and I had a chat and we figured that Ctrl "owns" too much. Right now, if we want to spin up any additional components alongside the controller, and they require flags, we always need to do one of:

1. Pulling the Logger out, pull the flags out of the constructor after the App has been NewFromFlags' and having the init code alongside the App, which feels a bit wrong.
2. Have all the init live inside the ControllerConstructor (which feels like the "owner" is wrong")

Maybe it would be better to have the ownership of flags and loggers be separate to Ctrl. That way, users can decide how the application is started up.

The current method is nice however, since we basically don't need to worry about writing any sort of init code, but it means things are a bit opaque unless you understand what Ctrl is doing.

Improve controller constructor flag processing for testing

Currently, the controller constructor sets up flags by doing something similar to the following:

1. AddFlags - which the user can use to mutate the flagset and read to variables
2. ParseFlags - golang parses the flags
3. Run - which calls the run method for the controllers, which can access the flags.

AddFlags needs to reference some block of memory which the Run() method can also access, which is the ControllerConstructor's struct.

However, this prevents us from storing anything other than the basic flag values into ControllerConstructor because any code that does "post-processing of the flags" can't run until the Run() method (after the flags have been parsed), and at that point we already have relied on ControllerConstructor to pass the basic flag values.

This is important because your integration testing flow might rely on a config file being read as part of flag, and then reading the config file for informer config. This following flow would be impossible with a config file:

1. Create a ControllerConstructor type which has the configuration values to allow your informers to talk to an integration cluster with various values changed
2. Use Smith to spin up your controller talking to your integration cluster

Instead if there was some sort of construction like:

1. AddFlags()
2. FlagsParsed()
3. Run()

Then this would be possible.

Ability to provide custom comparison function to see if interested in update

Often a controller is not interested in an update (e.g. just status change), and if it's expensive to resync we really don't want to be processing it.

Either Process should give us the old object and the new object, or we should have the option of giving a custom comparison function to ctrl so we only see interesting updates.

LookupHandler needs to print the logs after performing the lookup

Otherwise it logs the "original object" instead of the mapped one, which ends up spamming the logs even if it maps to nothing.