-
Notifications
You must be signed in to change notification settings - Fork 235
feat(Tekton Multicluster ProxyAAE): onboard MulticlusterProxyAAE component #3201
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
base: main
Are you sure you want to change the base?
Conversation
|
/kind feature |
aa12d1d to
c31262f
Compare
|
As part of testing I performed the following:
No E2E tests right now due to dependencies of components even outside of tekton ecosystem like multicluster |
9164347 to
44a1b18
Compare
|
@mbpavan - Please includes tests to make sure that proxy service behaves the expected way in hub and spoke cluster. https://github.com/tektoncd/operator/tree/main/test |
#2934 (comment) - referring to this, we need mutlicluster setup done for running e2e which requites spoke, hub config and cer manager, keueue etc, so I refrained from e2e as plan is to add it in different repo also for ProxyAAE specifically currently we have a dependency of installing hub and spoke along with Kueue and cert-manager manually,For the readiness probe on multicluster-proxy-aae, we need the secret and MultiKueue to be installed. So thinling of handling E2e later. Also this kind of setup is needed generally for multicluster component and much broder, would like to have more clarity to cover those as well to cover E2e. Due to these, would like to adress E2e in another PR later |
| func (r *Reconciler) ensureDependenciesInstalled(proxy *v1alpha1.TektonMulticlusterProxyAAE) error { | ||
| if _, err := common.PipelineReady(r.pipelineInformer); err != nil { | ||
| if err.Error() == common.PipelineNotReady || err == v1alpha1.DEPENDENCY_UPGRADE_PENDING_ERR { | ||
| proxy.Status.MarkDependencyInstalling("tekton-pipelines is still installing") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/tekton-pipelines is still installing/Waiting for TektonPipeline 'pipeline' to become ready
| ) | ||
|
|
||
| // Test_ValidateTektonMulticlusterProxyAAE is a placeholder for future validation tests. | ||
| func Test_ValidateTektonMulticlusterProxyAAE(t *testing.T) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add minimal test
func Test_ValidateTektonMulticlusterProxyAAE(t *testing.T) {
tests := []struct {
name string
proxy *TektonMulticlusterProxyAAE
wantErr bool
}{
{
name: "valid proxy",
proxy: &TektonMulticlusterProxyAAE{
ObjectMeta: metav1.ObjectMeta{
Name: MultiClusterProxyAAEResourceName,
},
Spec: TektonMulticlusterProxyAAESpec{
CommonSpec: CommonSpec{
TargetNamespace: "tekton-pipelines",
},
},
},
wantErr: false,
},
}
}
|
|
||
| // IsMulticlusterProxyAAEEnabled returns true when TektonMulticlusterProxyAAE should be deployed: | ||
| // scheduler is not disabled, multi-cluster is enabled, and role is Hub. | ||
| func IsMulticlusterProxyAAEEnabled(tc *v1alpha1.TektonConfig) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Added pkg/reconciler/shared/tektonconfig/multiclusterproxyaae/multiclusterproxyaae_test.go
| logger := logging.FromContext(ctx) | ||
|
|
||
| if err := r.manifest.Filter(mf.CRDs).Delete(); err != nil { | ||
| logger.Error("Failed to delete CRDs for TektonMulticlusterProxyAAE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add which crds are failed to delete
if err := r.manifest.Filter(mf.CRDs).Delete(); err != nil {
logger.Errorw("Failed to delete CRDs for TektonMulticlusterProxyAAE", "error", err)
return err
}
44a1b18 to
e2b8285
Compare
e2b8285 to
851ad81
Compare
|
@jkhelil I have addressed your review comments, please check again |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkhelil The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
anithapriyanatarajan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbpavan I attempted to enable scheduler and set the role to "" but got an error that certificate server should be installed. Then when I edited the Tektonconfig and set the role to "spoke" expecting the error to be removed, but the proxyinstallerset was stuck with the same error. Could you please clarify the expected behavior when scheduler is enabled and role set to empty & scheduler enabled and role set to spoke in sequence?
|
|
||
| - `disabled`: set to `false` to enable the Scheduler component (default is `true`). | ||
| - `multi-cluster-disabled`: when `false`, multi-cluster features are enabled (default is `true`). | ||
| - `multi-cluster-role`: `Hub` or `Spoke`. When set to **Hub**, TektonConfig also creates and manages the [TektonMulticlusterProxyAAE](./TektonMulticlusterProxyAAE.md) component automatically (the proxy is used to communicate with spoke clusters, e.g. for [Kueue MultiKueue](https://kueue.sigs.k8s.io/docs/concepts/multikueue/)). On spoke clusters use `Spoke`; the proxy is not installed there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the default behavior if the user neither mentions Hub or Spoke?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user doesn’t mention Hub or Spoke, the intended default behavior is: multi-cluster is disabled.
Hub or Spoke only need to be set when the user explicitly enables multi-cluster (multi-cluster-disabled: false); in that case they must set one of Hub or Spoke or the CR is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a validation to explicitly error if multicluster is enabled and role value is set to empty appropriate validation error message is displayed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can take this up in a separate PR, as these are not introduced by this change and relate to general multi-cluster prerequisites and missing validation.
cc @pramodbindal
@pramodbindal can you add missing validation please and any doc changes needed for prereq in a separate PR
|
|
||
| - Scheduler is **not** disabled (`spec.scheduler.disabled` is not `true`). | ||
| - Multi-cluster is **enabled** (`spec.scheduler.multi-cluster-disabled` is `false`). | ||
| - Cluster role is **Hub** (`spec.scheduler.multi-cluster-role` is `Hub`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While attempting to install the proxy, getting this error - "config devel False Components not in ready state: Please install cert-manager (cert-manager.io/v1) First, the server could not find the requested resource
" - Does that mean this is a pre-requisite? if so please update the document on this should be installed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It is a prereq. I had added it to docs/TektonConfig.md, which was missed in #2934. This is a general prerequisite for multi-cluster.
I have also added it now to docs/TektonMulticlusterProxyAAE.md to avoid further confusion.
| logger := logging.FromContext(ctx) | ||
|
|
||
| if err := r.manifest.Filter(mf.CRDs).Delete(); err != nil { | ||
| logger.Errorw("Failed to delete CRDs for TektonMulticlusterProxyAAE", "error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Please follow consistent error logging approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is coming from other code review comment - #3201 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err logging is fine. Please use either logger.Error or logger.Errorw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh ok ok, done
khrm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to update console plugin cr with service of MulticlusterProxyAAE
|
Just a general clarification in case there’s any confusion: tektonscheduler is not part of this change. It’s covered under #2934. I’m only adding it here because tektonconfig.md was missed there. |
3f021c5 to
060a1ba
Compare
khrm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check this PR: https://github.com/tektoncd/operator/pull/2545/changes
We need to add a proxy for the proxy entry for ProxyAAE also.
060a1ba to
4b6e52c
Compare
thank you! I have added, can you check now please @khrm |
Changes
This PR adds support for the Tekton Multicluster Proxy AAE component so it can be installed and managed by the operator when running as a multi-cluster Hub (with Tekton Scheduler and Kueue/MultiKueue).
Prerequisites
Kueue and MultiKueue (or equivalent) installed when using the proxy; at least one valid MultiKueueCluster (and kubeconfig secret) for the proxy to report Ready.
Manual Testing
Install on OpenShift and Kubernetes
Upgrade on OpenShift and Kubernetes
Enable and disable on OpenShift and Kubernetes
Functional validation on Hub (OCP) and Spoke (OCP), including checks on several API endpoints
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lintbefore submitting a PRSee the contribution guide for more details.
Release Notes