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.

Sign up for GitHub

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

Jump to bottom

feat: System controller improvements #257

Merged
merged 19 commits into from
Apr 19, 2023
Merged

feat: System controller improvements #257

merged 19 commits into from
Apr 19, 2023

Conversation

raelga
Copy link
Contributor

@raelga raelga commented Apr 12, 2023

Closes #253

/kind feature
/priority important-soon
/assign

@3scale-robot 3scale-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next sprint. needs-size Indicates a PR or issue lacks a `size/foo` label and requires one. size/XL Requires about a week to complete the PR or the issue. and removed needs-size Indicates a PR or issue lacks a `size/foo` label and requires one. labels Apr 12, 2023
@raelga raelga force-pushed the feat/system-controller-improvements branch 3 times, most recently from e8210a3 to 107b3ca Compare April 13, 2023 10:31
@raelga
Copy link
Contributor Author

raelga commented Apr 13, 2023

/retitle feat: System controller improvements

@3scale-robot 3scale-robot changed the title WIP feat: System controller improvements feat: System controller improvements Apr 13, 2023
slopezz
slopezz reviewed Apr 13, 2023
View reviewed changes
Copy link
Member

@slopezz slopezz left a comment

Choose a reason for hiding this comment

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

Can you please fix the Sphinx typo on the new Console image?

api/v1alpha1/system_types.go Outdated Show resolved Hide resolved
bundle/manifests/saas-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved
config/manifests/bases/saas-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved
docs/api-reference/reference.asciidoc Outdated Show resolved Hide resolved
slopezz
slopezz reviewed Apr 13, 2023
View reviewed changes
api/v1alpha1/system_types.go
@@ -78,6 +80,10 @@ var (
corev1.ResourceMemory: resource.MustParse("2Gi"),
},
}
systemDefaultAppDeploymentStrategy defaultDeploymentRollingStrategySpec = defaultDeploymentRollingStrategySpec{
MaxUnavailable: util.IntStrPtr(intstr.FromInt(0)),
Copy link
Member

@slopezz slopezz Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This 10% strategy works without any issue even not updating the already restrictive PDB config?

I thought that if updating rolloutStrategy, its correspondant PDB needed to be updating to be more permissive, but TBH I've never tested... so I might be totally wrong and might not need to update the PDB

did you tested it in staging by increasing temporarily HPA number of replicas with lots of pods?

Copy link
Member

Choose a reason for hiding this comment

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

10% is surge, so it's 10% above the desired. PDB only cares if the number of ready replicas goes below desired-1 (we have maxUnavailable=1 configured everywhere)

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

roivaz
roivaz reviewed Apr 13, 2023
View reviewed changes
api/v1alpha1/common_types.go Outdated Show resolved Hide resolved
api/v1alpha1/system_types.go
@@ -78,6 +80,10 @@ var (
corev1.ResourceMemory: resource.MustParse("2Gi"),
},
}
systemDefaultAppDeploymentStrategy defaultDeploymentRollingStrategySpec = defaultDeploymentRollingStrategySpec{
MaxUnavailable: util.IntStrPtr(intstr.FromInt(0)),
Copy link
Member

Choose a reason for hiding this comment

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

10% is surge, so it's 10% above the desired. PDB only cares if the number of ready replicas goes below desired-1 (we have maxUnavailable=1 configured everywhere)

@raelga
feat: add int64OrDefault helper
39cecd4 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
test: test TerminationGracePeriodSeconds configuration
fcc1790 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: customize system workloads TerminationGracePeriodSeconds
0b25fd8 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
release: 0.19.1
0da21c1 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: add custom image for system console
6f75413 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
release: 0.19.2
69066c0 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: allow HPA scaling policies
fccc683 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
release: 0.19.3
3ef9cdb 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: allow custom rollout strategy on system
8de9bc7 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: add tests for the custom deployment strategy
5fe119b 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
release: v0.19.4
9cd313a 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: add twemproxy sidecar for system console
03eda96 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
test: add tests for system-console twemproxy sidecar
e12ed87 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
feat: refactor twemproxy sidecar helper
08b0d83 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
releaase: 0.19.5-alpha.1
8af0cbf 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
fix: typo in console description
19762d5 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
clean: remove protobuf tags
b1103d1 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
fix: regenerate all documentation
7e499cb 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga
release: stable 0.19.5
e102002 
Signed-off-by: Rael Garcia <rael@redhat.com>
@raelga raelga force-pushed the feat/system-controller-improvements branch from 0a7683f to e102002 Compare April 19, 2023 10:36
@slopezz
Copy link
Member

slopezz commented Apr 19, 2023

/lgtm

@3scale-robot 3scale-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023
@3scale-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 95bed7d9bfe8755e9cb1c480a9952f07fa4e0ab9

@roivaz
Copy link
Member

roivaz commented Apr 19, 2023

/lgtm

@raelga
Copy link
Contributor Author

raelga commented Apr 19, 2023

/approve

@3scale-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raelga

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

@3scale-robot 3scale-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2023
@3scale-robot 3scale-robot merged commit 0d5d982 into main Apr 19, 2023
@3scale-robot 3scale-robot deleted the feat/system-controller-improvements branch April 19, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roivaz roivaz roivaz left review comments

@slopezz slopezz slopezz left review comments

Assignees

@raelga raelga

@roivaz roivaz

@slopezz slopezz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next sprint. size/XL Requires about a week to complete the PR or the issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: system controller improvements after migration
4 participants
@raelga @slopezz @3scale-robot @roivaz