Refactoring CreateJob and UpdateStatus methos in Gitjob Reonciler

[Tommy12789]

Refers to #2343

Changed the Update method into the updateStatus method of the gitjob reconciler into the controller to the Patch method.

./internal/cmd/controller/gitops/reconciler/gitjob_test.go

[aruiz14]

I'm not convinced of this change. Update is immediately happening after Get, so it's weird that it finds a conflict (meaning another client wrote to the same object in the meantime).

In fact, I'm not sure this even correct, and there could be a race condition:

1. createJob receives a GitRepo, and does something based on that information
2. At the end, it updates the ObservedGeneration field in the status. However, the value being used is the one that was just read from the API, which may differ from the one used as an input.

The Generation field is used to represent a revision of the object Spec has changed. My understanding is that it ObservedGeneration should be set to the last revision being processed, so that if ObservedGeneration != Generation, it means that the a reconciliation is needed.

[manno]

Ah, good find.
So, I think this is a left-over from when we had a separate gitjob resource: https://github.com/rancher/fleet/blame/684c5cb2db1ced993421120e3c1f50a7ae51d5b4/internal/cmd/controller/gitjob/gitjob.go#L151

When the createJob func returns, there is another function which updates the same status.
I think we can remove this.

That being said:

• retryOnConflict only makes sense, when we use the outer value to update the freshly retrieved resource. It should be gitRepoFromCluster.Status.ObservedGeneration = gitRepo.Generation. Hm, reading it again, maybe that was intentional to update the status of the freshly created gitjob.
• gitRepoFromCluster is a bit verbose, t is fine, the variable is not that important

Regarding this whole file:

• we might want to rename this reconciler to GitOpsReconciler and fix the comment which mentions a CronJobReconciler
• newJob, computeJobSpec and generateInitContainer do a good job at hiding what they actually create: containers for "fleet gitclone" and "fleet apply".
• actually this has a lot of overlap with our gitrepo reconciler in the other container, both update the same status. However both trigger on different events.

[manno]

After our discussion, I'd suggest the following changes:

• createJob can just return the error from err = r.Create(ctx, job), updating the observed generation in the status is already handled by the other controller.
• updateStatus in line 224 should use retry.RetryOnConflict. This is similar to https://github.com/rancher/fleet/blob/main/internal/cmd/controller/reconciler/gitrepo_controller.go#L216-L237. However it shouldn't need to update the observedGeneration. It should only update the fields it just calculated, not the whole status, like
  • the conditions
  • kstatus.SetError / SetActice
  • Status.Commit

[manno]

I don't think we need this variable anymore 🤷
And the next line too

[Tommy12789]

Of course, I removed them