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

Add support for pipelines controller statefulset ordinals #2361

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

jkhelil
Copy link
Member

@jkhelil jkhelil commented Sep 30, 2024

Changes

To handle HA on pipelines controller, there are two mechanisms available for scaling controllers horizontally via knative/pkg:
- Using leader election, which allows for failover, but can result in hot-spotting.(this is the default pipelines controller mode)
- Using StatefulSet ordinals, which doesn't allow for failover, but guarantees keys are evenly spread across replicas.
This PR adds support for pipelines controller statefulset ordinals

  • it exposes statefulset-ordinals: false by default on tektonconfig
  • When statefulset-ordinals set to true, it creates a new installerset pipeline-main-statefulset-xxx
  • Ensures pipelines controller is deployed as statefulsetSet and has the configuration to handle loadbalancing
  • adds e2e tests for statefulset deployment

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 30, 2024
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2024
@jkhelil jkhelil force-pushed the statefulset_ordinals branch 8 times, most recently from cb48950 to ca0d5eb Compare October 1, 2024 12:13
@jkhelil jkhelil changed the title WIP add support for pipelines controller statefulset ordinals Add support for pipelines controller statefulset ordinals Oct 1, 2024
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2024
@jkhelil jkhelil force-pushed the statefulset_ordinals branch 10 times, most recently from a9a023a to f704975 Compare October 2, 2024 09:56
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/operator/v1alpha1/tektonpipeline_defaults.go 93.3% 90.1% -3.2
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go 84.2% 86.0% 1.8
pkg/reconciler/common/transformers.go 74.4% 70.4% -4.0
pkg/reconciler/kubernetes/tektoninstallerset/client/cleanup.go 61.4% 46.6% -14.8
pkg/reconciler/kubernetes/tektoninstallerset/client/create.go 77.6% 69.0% -8.6
pkg/reconciler/kubernetes/tektonpipeline/transform.go 89.8% 89.1% -0.7

@tektoncd tektoncd deleted a comment from tekton-robot Oct 2, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 2, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 2, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
@tektoncd tektoncd deleted a comment from tekton-robot Oct 10, 2024
if p.Performance.StatefulsetOrdinals == nil {
p.Performance.StatefulsetOrdinals = ptr.Bool(false)
} else if *p.Performance.StatefulsetOrdinals {
if p.Performance.Replicas != nil && *p.Performance.Replicas > 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If p.Performance.Replicas != nil then we might have to set the buckets to 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in statefulset mode, bucket should be equal to replicas, not to 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkhelil I guess @PuneetPunamiya try to say bucket should be 1, when p.Performance.Replicas == nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry for the confusion
+1 on what @jkandasa said

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When replicas is not specified (it's nil), the default behavior of a StatefulSet is to create a single pod, in StatefulSet's Knative mode, the default behavior is to create one bucket, so there is no need to enforce that mode, it works by default.

However, when replicas is explicitly set (not nil), I found that if we don’t enforce the bucket assignment to equal to replicas, the election mode and StatefulSet mode can interfere with each other. This results in some pods being in election mode while others are in StatefulSet mode, leading to inconsistent behavior.

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2024
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/operator/v1alpha1/tektonpipeline_defaults.go 93.3% 89.9% -3.5
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go 84.2% 86.0% 1.8
pkg/reconciler/common/transformers.go 74.4% 70.4% -4.0
pkg/reconciler/kubernetes/tektoninstallerset/client/cleanup.go 61.4% 45.8% -15.6
pkg/reconciler/kubernetes/tektoninstallerset/client/create.go 77.6% 69.0% -8.6
pkg/reconciler/kubernetes/tektonpipeline/transform.go 89.8% 89.1% -0.7
test/resources/statefulset.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/operator/v1alpha1/tektonpipeline_defaults.go 93.3% 89.9% -3.5
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go 84.2% 86.0% 1.8
pkg/reconciler/common/transformers.go 74.4% 70.4% -4.0
pkg/reconciler/kubernetes/tektoninstallerset/client/cleanup.go 61.4% 45.8% -15.6
pkg/reconciler/kubernetes/tektoninstallerset/client/create.go 77.6% 69.0% -8.6
pkg/reconciler/kubernetes/tektonpipeline/transform.go 89.8% 89.1% -0.7
test/resources/statefulset.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/operator/v1alpha1/tektonpipeline_defaults.go 93.3% 89.9% -3.5
pkg/apis/operator/v1alpha1/tektonpipeline_validation.go 84.2% 86.0% 1.8
pkg/reconciler/common/transformers.go 74.4% 70.4% -4.0
pkg/reconciler/kubernetes/tektoninstallerset/client/cleanup.go 61.4% 45.8% -15.6
pkg/reconciler/kubernetes/tektoninstallerset/client/create.go 77.6% 69.0% -8.6
pkg/reconciler/kubernetes/tektonpipeline/transform.go 89.8% 89.1% -0.7
test/resources/statefulset.go Do not exist 0.0%

@jkhelil
Copy link
Member Author

jkhelil commented Oct 14, 2024

@PuneetPunamiya PTAL

@PuneetPunamiya
Copy link
Member

Thanks @jkhelil 🤙🏻
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2024
@tekton-robot tekton-robot merged commit efd8c40 into tektoncd:main Oct 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants