[WIP] Auto reload the local operator for code changes

[baijum]

When running the operator using operator-sdk up local command,
restart the operator whenever there is a change in Go source code
in the file-system.

Fixes #1484

• Flag to disable auto-reload
• Tests
• Flag to set ignore directories (default: .git,build)

[joelanford]

@baijum This looks like a good start. Just a couple of other ideas to think about:

1. Can you make this work for the helm and ansible operator types as well?
2. It would be interesting to see this implemented using the functional options pattern entirely behind the exsting API of projutil.ExecCmd, where the function could be updated to something along the lines of:

func ExecCmd(cmd *exec.Cmd, opts ...func(*ExecOptions) error) error {
    execOptions := ExecOptions{}
    for _, opt := range opts {
        if err := opt(&execOptions); err != nil {
            return err
        }
    }
    ...
}

Then could export a new RestartOnChange function that configures ExecCmd to watch a directory and restart the passed cmd on change:

func RestartOnChange(rootDir string, ignorePaths []string) func(*ExecOption) error {
    return func(opt *ExecOption) error {
        opt.RestartOnChange = true
        opt.WatchRootDir = rootDir
        opt.WatchIgnorePaths = ignorePaths
        return nil
    }
}

With this pattern, existing usages would be unchanged:

projutil.ExecCmd(myOneShotCmd)

But to restart a command based on a watch, it could look like:

projutil.ExecCmd(
    myRestartableCmd, 
    projutil.RestartOnChange(".", []string{".git", "deploy", "build"})
)

I think doing something along these lines would make it easy to extend this to other commands (like ansible and helm), and it would make it easy to extend the ExecCmd function in other ways in the future.

Thoughts?

[baijum]

@joelanford Thanks for the idea. I will study that pattern.
I agree that helm and ansible should be covered.

[baijum]

@joelanford Looks like ansible and helm operators are not using ExecCmd to start the operator.

operator-sdk/pkg/ansible/operator/operator.go

Line 83 in bd4dc8c

done <- mgr.Start(c)

operator-sdk/pkg/helm/run.go

Line 111 in bd4dc8c

if err = mgr.Start(signals.SetupSignalHandler()); err != nil {

[joelanford]

@baijum Oh, right. In that case, we probably need an abstraction that handles each case. In fact, the entire ansible.Run and helm.Run commands would need to be wrapped so that the watches.yaml file could be reloaded.

It looks like the projutil.ExecCmd, ansible.Run, and helm.Run commands all have the same signature, but it also looks like we'd have to have two different ways to stop them.

We might have to have something like the following with a couple of struct implementations (one for ExecCmd and another for the Run functions, perhaps):

type Runnable interface {
   Run() error
   Stop() error
}

[joelanford]

@baijum Before you get too far with this, I want to make sure this is something we're likely to accept.

/cc @hasbro17 @lilic @shawn-hurley @estroz @AlexNPavel @theishshah

Thoughts on including this functionality in operator-sdk? Would we be better off documenting how to do this with inotify or fswatch?

[openshift-ci-robot]

@baijum: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lilic]

@joelanford I think this is very useful and right now I think users just assume this is already working and run into problems when changes are not applied, thinking the problem is in their code. So this is very much needed 👍

[openshift-ci-robot]

@baijum: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/images	ecbc8fd	link	/test images
ci/prow/e2e-aws-ansible	ecbc8fd	link	/test e2e-aws-ansible
ci/prow/e2e-aws-helm	ecbc8fd	link	/test e2e-aws-helm
ci/prow/e2e-aws-go	ecbc8fd	link	/test e2e-aws-go
ci/prow/e2e-aws-subcommand	ecbc8fd	link	/test e2e-aws-subcommand

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[estroz]

@baijum thanks again for opening this PR. Do you plan on working on this going forward? If not, one of the SDK main contributors can potentially pick it up.

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci-robot]

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.