Declerative targeting of packages to clusters #3542
Conversation
| } | ||
|
|
||
| type ClusterTargetSelector struct { | ||
| ClusterType ClusterType `json:"clusterType,omitempty"` |
There was a problem hiding this comment.
Maybe this should be a GVK or GK? We can see how it goes though...
There was a problem hiding this comment.
Yeah, we could make this a GK/GVK. That will help with discovery for clusters that are represented as resources in the local cluster, but we still need logic to know how to work with those clusters, i.e. connecting to them. Also, there might be situations where we want to discover clusters through some other mean that listing resources, although we could argue that all APIs must reflect clusters as resources.
| ) | ||
|
|
||
| type PackageSelector struct { | ||
| Namespace string `json:"namespace,omitempty"` |
There was a problem hiding this comment.
I guess we need to go cross-namespace on either packages or targets, and I agree that packages is less dangerous!
There was a problem hiding this comment.
I think we need to allow for cross-namespace. I added this partly as a way to allow for restricting the controller to target a subset of packages, given that package revisions do not yet support labels.
| } | ||
|
|
||
| if apierrors.IsNotFound(err) { | ||
| klog.Infof("Creating rootsyncset %s", rootSyncSetName) |
There was a problem hiding this comment.
I wonder if we can express this as creating a set of "child objects", and then use applylib or similar. Should be less code and take care of pruning. Not a blocker though!
There was a problem hiding this comment.
Yeah, I did consider that approach, but in the end decided on the more standard controller approach.
| } | ||
|
|
||
| newReconcile := time.Now().Add(30 * time.Second) | ||
| r.NextScheduledReconcile[req.NamespacedName] = newReconcile |
There was a problem hiding this comment.
Do we need to cache this information? I've always seen it just returning RequeueAfter...
There was a problem hiding this comment.
Not sure I follow. Are you saying there is a way to query the workqueue to check when the next reconcile is scheduled for? This solution is just temporary until we have support for watches on packagerevisions and I've figured out how to set up watches correctly with labelselectors. That is the next steps here.
|
A few questions / thoughts, but looks good. Merge and iterate :-) |
|
I agree with the merge and iterate approach, but I think we should come up with an approach where all the "very alpha" controllers doesn't run by default for users trying out Porch. We had some users reporting issues due to our renaming of some of the CRDs in the latest release. We could disable them in the regular porch release and just ask users who want to try them out to build from source. Or even better, publish porch as a kpt package an allow users to opt-in to these controllers if they want to. |
I also think controllers with GCP-specific functionality need to be separated from the rest of the controllers and optionally deployed. Preferably in a single controller-manager. Alternatively they can be part of KCC, especially if they depend on KCC CRDs. |
mortent
force-pushed
the
DeclarativeClusterTargeting
branch
2 times, most recently
from
f589bd5
to
05ede1e
Compare
mortent
force-pushed
the
DeclarativeClusterTargeting
branch
2 times, most recently
from
12c1dd7
to
c4dfd89
Compare
mortent
force-pushed
the
DeclarativeClusterTargeting
branch
3 times, most recently
from
c3debe8
to
17068fd
Compare
mortent
force-pushed
the
DeclarativeClusterTargeting
branch
2 times, most recently
from
31ff51e
to
0cd7023
Compare
mortent
changed the title
| } | ||
|
|
||
| func (r *RootSyncRolloutReconciler) findRolloutsForPackageRevision(pkgRev client.Object) []reconcile.Request { | ||
| // There is not support for reverse lookup by label: https://github.com/kubernetes/kubernetes/issues/1348 |
There was a problem hiding this comment.
I think controller-runtime has some ability to build our own indexes, but I agree this is a pain.
There was a problem hiding this comment.
Yeah, there is support for building indexes based on fields in controller-runtime, but I couldn't find support for indexes on labels.
mortent
force-pushed
the
DeclarativeClusterTargeting
branch
from
0cd7023
to
3a32040
Compare
This adds the RootSyncRollout controller that provides declarative cluster targeting of packages built upon the RootSyncDeployment controller.