Added ClusterGitTrack #145
Conversation
|
Hi @sebastianroesch. Thanks for your PR. I'm waiting for a pusher member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
|
@sebastianroesch: The following tests failed, say
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. |
There was a problem hiding this comment.
So I think at the moment the behaviour options are:
- namespaced mode: Reconcile GitTracks and restrict resources to being in the same namespace, what's the behaviour for ClusterGitTracks here? I think it still reconciles them
- non-namespaced mode: Controller will reconcile GitTracks and ClusterGitTracks, ClusterGitTracks can own resources in both namespace and cluster scope, GitTracks can only own namespaced scope
I think what we want by the end of this PR is the following three modes:
- Cluster wide mode: Only reconcile ClusterGitTracks, which manage namespaced and non-namespaced resources
- Namespaced mode: Reconcile GitTracks which can only manage Namespaced resources
- ClusterScope mode: Reconcile ClusterGitTracks which can only manage Cluster scoped resources
You should use one ClusterScope controller in every cluster and a Namespaced controller in every namespace when you want to have the separation. Or you just use one Cluster wide controller which manages everything, but in this mode, what should the behaviour for GitTracks be? Does it manage them still? I guess it probably could without issue as long as they don't cross namespace boundaries or manage cluster scoped resources
This is a fairly complex change so I think it would benefit from having a proper design document written up, I'll see if I can get something written next week, I'm off on holiday for a few days now.
In the meantime post questions about what I've said here and we can answer them and copy the contents into the doc
Also, you have checked in the kustomize binary, can we remove that and add it to the gitignore maybe?
| A namespaced GitTrack, however, should never own cluster-scoped resources or | ||
| resources in other namespaces. | ||
|
|
||
| To maintain backward-compatibility, the following flag defaults to true. |
There was a problem hiding this comment.
Since we know this can destroy clusters, I'm very tempted to not maintain this backwards compatibility.
We are on an alpha version and I think, given the severity of this change, we will skip the 0.4.0 release and go straight to 0.5.0. People will expect breaking changes and I think there are better ways to handle this.
I discussed with @mthssdrbrg yesterday about having the GitTrackObject controller look for OwnerReferences pointing to the wrong namespace and "orphaning" the GitTracks. This could then be exposed as a metric which we can note in the release people should look for and make sure it goes to zero, what do you think to that approach instead? Easy migration vs backwards compatiblity
There was a problem hiding this comment.
I agree, it would be better to break the current behavior in this case.
I like the idea of exposing the metric and breaking the owner reference.
| // +kubebuilder:printcolumn:name="Resources Ignored",type="integer",JSONPath=".status.objectsIgnored" | ||
| // +kubebuilder:printcolumn:name="Children In Sync",type="integer",JSONPath=".status.objectsInSync" | ||
| // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" | ||
| type ClusterGitTrack struct { |
There was a problem hiding this comment.
I don't think this needs to be a separate API group. Adding a new type doesn't break backwards compatibility and the new API group really complicates this PR. I would prefer if we just moved this into v1alpha1, as it is the first version of this resource, it shouldn't be a problem.
There was a problem hiding this comment.
Fine with me, makes it a lot easier.
|
|
||
| // GetNamespacedName implementes the GitTrack interface | ||
| func (g *ClusterGitTrack) GetNamespacedName() string { | ||
| return fmt.Sprintf("%s/%s", g.Namespace, g.Name) |
There was a problem hiding this comment.
The namespaced name for a Cluster scope resource should just be g.Name
| return fmt.Sprintf("%s/%s", g.Namespace, g.Name) | |
| return g.Name |
| @@ -103,6 +104,11 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { | |||
| return err | |||
| } | |||
|
|
|||
| err = c.Watch(&source.Kind{Type: &farosv1alpha2.ClusterGitTrack{}}, &handler.EnqueueRequestForObject{}) | |||
There was a problem hiding this comment.
If the controller is running in namespaced mode, we don't want to be managing ClusterGitTracks right? Likewise, we will need another flag to suggest managing only ClusterGitTrack, any ideas for how we can solve this @sebastianroesch @mthssdrbrg ?
There was a problem hiding this comment.
So far, the different cases have only been checked in the function that ignores the resources if their namespace doesn't match, the controllers would still watch all resources.
According to my comment about the namespaced mode, we would have to watch all resources.
There was a problem hiding this comment.
I'd like to be able to switch off the watching of everything as in certain modes it shouldn't be necessary and, with fewer watches, would allow people to reduce their RBAC scope too.
According to my comment about the namespaced mode, we would have to watch all resources.
My thought here was that you will likely have multiple "namespaced" instances of the controller (one per namespace) and then a separate "cluster" instance running to manage the cluster scoped resources. So the individual namespaced controllers should be able to completely ignore ClusterGitTracks and ClusterGitTrackObjects in theory unless they are running both the Namespaced and ClusterScoped modes within the same controller instance
| if namespaced && farosflags.Namespace != "" && farosflags.Namespace != u.GetNamespace() { | ||
| r.log.V(1).Info("Object not in namespace", "object namespace", u.GetNamespace(), "managed namespace", farosflags.Namespace) | ||
| return true, fmt.Sprintf("namespace `%s` is not managed by this Faros", u.GetNamespace()), nil | ||
| if namespaced { |
There was a problem hiding this comment.
Do we need a check to make sure ClusterGitTracks don't manage namespaced objects when running in cluster only mode? (I realise this is yet to be defined)
There was a problem hiding this comment.
Agreed, if we decide that we want to add this mode as per your suggestion.
There was a problem hiding this comment.
I would like to have this mode so there can be definite separation between who is owning/who should be owning what
GitTrack for namespaced stuff
ClusterGitTrack for cluster scoped (in cluster scoped mode)
ClusterGitTrack for everything if not using GitTracks
| @@ -63,20 +65,34 @@ func (p OwnerInNamespacePredicate) Generic(e event.GenericEvent) bool { | |||
| // When it is restricted to a namespace this should only be the GitTracks | |||
| // in the namespace the controller is managing. | |||
| func (p OwnerInNamespacePredicate) ownerInNamespace(ownerRefs []metav1.OwnerReference) bool { | |||
There was a problem hiding this comment.
I don't know if this makes sense for ClusterGitTracks? This is used to check if a GTO or CGTO has an owner managed by this controller right? If the controller is running in cluster wide or cluster scope only mode, then all it needs check is that the OwnerRef points to a ClusterGitTrack, I'm not sure this predicate is required in this case?
There was a problem hiding this comment.
I wasn't entirely sure about what's going on here, but I think I had to make this change to receive the events of the GTOs owned by the ClusterGitTrack.
| waitForInstanceCreated(key) | ||
| }) | ||
|
|
||
| It("does not create ClusterGitTrackObject", func() { |
There was a problem hiding this comment.
I think the test here needs updating, you've updated the description but the test case does not make sense with the current description
|
I'll first answer your questions to the current behavior of this change:
If a Regarding the proposed modes: I am unsure if the modes you describe cover all cases. One other case I see is with the But thinking about it, I get the feeling that there's actually only two modes:
The namespaced mode you mentioned is the default and only behavior for In this case, we don't need the Accordingly, I agree that we would need a new flag that controls the mode of the |
|
@sebastianroesch: 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. |
|
@JoelSpeed any thoughts on my previous comment? |
|
@sebastianroesch As this is a fairly substantial change, I'd like to get some more detailed design done and make sure that we have an understanding of the desired behaviour before we go ahead on the work. |
|
@sebastianroesch Could you please take a look at and add comments to this doc that I've come up with https://paper.dropbox.com/doc/Introducing-ClusterGitTracks--AixMM37BMMhcugDznYFKsuD8AQ-XslbpUtYZeUTkigSSV4SS I think this outlines the new behaviour that we want and the different ways that I envisage people using Faros, let me know if you need any clarification on any of it |
|
@sebastianroesch @wimdec Do either of you have any further comments or questions on the proposal document or are we all agreed on the design to go forward with? |
|
@JoelSpeed I think it's a good proposal that we can go ahead with. I think it's rather complex compared to what I hoped for, but I understand that this might be necessary to support all those different use cases. |
|
@sebastianroesch I appreciate it's reasonably complicated but I know there are different people using the project in different ways and this change is going to affect them all I'd like to propose creating an issue for each of the bullet points at the bottom of the document and then people can claim the issues and can open a PR for each in turn and make this a more collaborative effort? I think the first couple of them can be cherry-picked from this PR's branch pretty much |
|
So you're aware, I have created a project for trying to resolve this issue https://github.com/pusher/faros/projects/1 |
|
@JoelSpeed I'd be happy to help out with some of the tasks over the next days. Anything that can be easily broken out? |
In order to resolve #143 we added a
v1alpha2.ClusterGitTrackresource which is cluster-scoped and can therefore own resources that are cluster-scoped or in any namespace.The
v1alpha1.GitTrackcan own resources in it's own namespace, or, to stay backward-compatible, can own resources in any namespace when theallow-cross-namespace-ownership=trueis set. This defaults totrueto remain backward-compatible, but it is recommended to set it tofalseand either use oneGitTrackper namespace or use the newv1alpha2.ClusterGitTrackinstead.For Faros to be able to access the Git credentials, this also adds the
secretNamespaceproperty to theGitTrackresources. It is required when using a cluster-scopedv1alpha2.ClusterGitTrack, as Faros would otherwise look up the secret in the same namespace, but secrets in Kubernetes are always namespace-scoped.Looking for your feedback on this approach.