Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question/Bug] Requesting reconciliation on changes to the included GitRepository artifact #864

Open
ljakimczuk opened this issue May 17, 2023 · 3 comments

Comments

@ljakimczuk
Copy link

Hey! I'd like to ask for your help in understanding the Kustomize Controller behaviour regarding requesting reconciliation on GitRepository artifact changes. Sorry for the long post, I wanted to be possibly explicit in what I want, and what I have checked so far.

I am running the Kustomize Controller v0.35.1, and the background is as follow. It reconciles some Kustomization CR, say X, in long intervals, say 24h. This Kustomization CR references the GitRepository, say A, which includes another GitRepository, say B. Now, when I push a commit to the repository the A GitRepository points to, the Source Controller rebuilds the artifact, hence its revision and checksum both change, and the reconciliation of the X Kustomization CR is triggered. This is what I want, for I do not want to wait till the next schedule. When I do the same for the repository the B GitRepository points to, the Source Controller rebuilds artifacts for both repositorys, but the revision of the A GitRepository stays the same, only its checksum changes, which is normal for nothing has actually changed in the A repository. But in this scenario, the reconciliation of the X Kustomization CR is not triggered. Is this correct?

I also tried to look for an answer by myself, and if you could tell me if I understand the behaviour correctly, I would be thankful. So, my understanding is, the Kustomization CRs get reconciled in response to artifact changes thanks to the Kustomize Controller watching the GitRepository CRs related events, as configured here. The predicate is used as a watching option to subscribe to only certain changes, for which the transformation function is then used to build a list of reconciliation requests for Kustomization CRs. The predicate in the v0.35.1 version checks for revision only, but for the sake of discussion let's assume it also checks for checksums. When the transformation function kicks in, it gets the list of Kustomization CRs, which I suspect are now indexed in the cached client by the GitRepository CRs' names and namespaces, judging on the indexBy function, but this is the part I am the most unsure of, for I am not deep into the manager / client codebase. What led me to believe this is, I've seen only certain Kustomization CRs being reconciled on artifact changes, and not all of them, so I guess the Kustomize Controller must know about the relations between them somehow. And now, after getting the list, for each item on it the last attempted revision known to the Kustomization CR is compared to the revision from the GitRepository CR, and if both are the same, the reconciliation request for this Kustomization CR is not added to the list. In the light of this part of the transformation function, indexing Kustomization CRs by GitRepository CRs' names and namespaces makes sense, for the last attempted revision in Kustomization CR status is strictly related to the referenced repository, and hence, in my case, getting the X Kustomization in this function upon updates to artifact of repository B, would always result in reconciliation, for the both revisions would always be different, but this isn't something we want, I think.

If that was the case then the consequence would be, the reconciliation of Kustomization CRs would be requested only on changes to artifact revision of directly referenced GitRepository CRs, and changes to indirectly referenced CRs, via include feature, would not trigger any updates. Also, if that was the case, checksum would feel a better indicator of detecting the change, that would also enable picking up the included repository changes. But even if predicates accounted for this, like noted earlier, then it still wouldn't work for the checksum is not part of the Kustomization CR status, so couldn't be compared in the transformation function.

But I am not sure I get the code correctly. So, first of all, I would like to ask if the code I link above is responsible for the behaviour in question, or maybe something else is at play here as well, that I missed. Second of all, do I understand it correctly, at least to some general degree? If I am wrong in my understanding, can you hint me of what should I do in order to trigger reconciliation of Kustomization CR on changes to the included repository?

@stefanprodan
Copy link
Member

Yes this is a known limitation of Git includes, maybe we could add some build metadata +checksum to the revision, like we do for charts? cc @hiddeco

To properly solve this, we could introduce a source Bundle API e.g. fluxcd/source-controller#854 (comment)

@hiddeco
Copy link
Member

hiddeco commented May 23, 2023

Do not think we can because of indirect implications this has on the notification-controller and commit status updates :-).

In general, it would be better to just move forward with the Source bundle API, and deprecate the includes in favor of them. As this both serves more use-cases, and removes the awkwardness of one GitRepository being more of a first class citizen than another.

@ljakimczuk
Copy link
Author

I get your point, having the Source Bundle API to solve this sounds also better to me. It is a shame though, because I guess it will probably take some time to implement it, right? Could we maybe for the time being add a note to the docs about this limitation of included repositories? I mean, I am not sure how obvious it is for other people, I wouldn't probably realize it myself if I didn't increase the intervals, but even after noticing something is wrong, it wasn't immediately obvious how this thing works, hence the whole question.

Anyway, thank you for answering, I think I have found a workaround to this limitation, which I need to test, so I'm fine with closing this issue, should you not need it around for potential docs update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants