reconcileTFJobs is always triggered even with no update

[jian-he]

Found that reconcileTFJobs continuously gets triggered even with no update.

This patch fixes that

---

This change is 

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[TravisBuddy]

Travis tests have failed

Hey @jian-he,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./...

cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

travis_time:end:387b475f:start=1535144156722657402,finish=1535144362874978215,duration=206152320813

[coveralls]

Coverage decreased (-0.2%) to 58.112% when pulling 27480ec on jian-he:develop into a11a0ee on kubeflow:master.

[TravisBuddy]

Travis tests have failed

Hey @jian-he,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./...

cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./...

cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

travis_time:end:0fed45bd:start=1535144554726391036,finish=1535144724622344200,duration=169895953164

[gaocegege]

/ok-to-test

[ScorpioCPH]

LGTM.

[ScorpioCPH]

/lgtm
/approve

[gaocegege]

Please rebase and try again

[gaocegege]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, ScorpioCPH

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ScorpioCPH,gaocegege]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jlewi]

@jian-he can you please explain this change? I think running reconcile periodically is highly desirable as this ensures the job is correctly running. For example, by calling reconcile periodically we ensure that if any pods are interrupted we will detect this and take corrective action.

I don't think we want to depend on getting an event for all possible changes as we could easily miss a change.

I think one of the design principles for K8s controllers is that they are level set. This means we want to periodically check that everything is in the expected state and if not take corrective action.

I think this PR prevents that because on each call to reconcile we won't do anything because the resource version hasn't changed, so we won't actually check that everything is in the expected state.

/cc @gaocegege @ScorpioCPH

[johnugeorge]

I agree with @jlewi If we somehow miss taking complete action on an update event(may be an transient error or a return), this PR will skip further reconciliation.

[ScorpioCPH]

I think if any pods' states are changed, the reversion of TFJob will be updated, so we catch this immediately.

[johnugeorge]

old and current (https://github.com/kubeflow/tf-operator/blob/master/pkg/controller.v2/tfcontroller/tfjob.go#L66) are provided by the upstream. I feel, there is always a chance that controller may have missed updates or prior actions failed. In such cases, we won't take further actions since resource versions are same

[jian-he]

@jlewi @gaocegege @johnugeorge @ScorpioCPH
I commented the reason why this change is made. Could you please take a look ?
#800 (comment)