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

Parameterize CAPI maxSurge field #620

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Parameterize CAPI maxSurge field #620

merged 1 commit into from
Apr 7, 2021

Conversation

jan-est
Copy link
Contributor

@jan-est jan-est commented Apr 1, 2021

The Cluster API control plane now allows for scaling in 4293. Documentation can be found rolling-update-strategy
This PR parameterizes the required metal3_dev_env fields and adds a new environment variable MAX_SURGE_VALUE.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 1, 2021
fmuyassarov
fmuyassarov reviewed Apr 1, 2021
View reviewed changes
Copy link
Member

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

LGTM, will run the tests first

@fmuyassarov
Copy link
Member

/test-integration
/test-centos-integration

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fmuyassarov, jan-est

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021
furkatgofurov7
furkatgofurov7 reviewed Apr 1, 2021
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

LGTM

Small nit: you have to update config_example.sh and vars.md as well, but it can be tackled in a separate PR.

@fmuyassarov
Copy link
Member

LGTM

Small nit: you have to update config_example.sh and vars.md as well, but it can be tackled in a separate PR.

@jan-est can we fix it now ?

@fmuyassarov fmuyassarov mentioned this pull request Apr 1, 2021
Merged
@maelk
Copy link
Member

maelk commented Apr 1, 2021

I'd rather tackle it in this PR, since we're doing the modification, let's do everything at once.

furkatgofurov7
furkatgofurov7 reviewed Apr 6, 2021
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

I forgot to mention one more thing, btw: if we want to set MAX_SURGE_VALUE to 0 (default is 1), NUM_OF_MASTER_REPLICAS MUST be <= 3 (default is 1) according to xref. Should we note and/or take into account that case as well?

@jan-est
Copy link
Contributor Author

jan-est commented Apr 6, 2021

I forgot to mention one more thing, btw: if we want to set MAX_SURGE_VALUE to 0 (default is 1), NUM_OF_MASTER_REPLICAS MUST be <= 3 (default is 1) according to xref. Should we note and/or take into account that case as well?

Is that something we can take account in the individual test cases?

@jan-est jan-est force-pushed the add-max-surge-variables-jan branch from 0054782 to 3298388 Compare April 6, 2021 13:23
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 6, 2021
@jan-est jan-est force-pushed the add-max-surge-variables-jan branch from 3298388 to 401935b Compare April 6, 2021 13:26
kashifest
kashifest reviewed Apr 7, 2021
View reviewed changes
Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

lgtm, couple of small nits, by the way CONTROL_PLANE_MACHINE_COUNT is now replaced with NUM_OF_MASTER_REPLICAS. So do we need this CONTROL_PLANE_MACHINE_COUNT anywhere else?

config_example.sh Outdated

# This variable defines if controlplane should scale-in or scale-out during upgrade
# The field values can be 0 or 1. Default is 1. When set to 1 controlplane scale-out
# When set to 0 controlplane scale-in. In case of scale-in NUM_OF_MASTER_REPLICAS must be <=3.
Copy link
Member

Choose a reason for hiding this comment

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

Is this statement correct? In case of scale-in NUM_OF_MASTER_REPLICAS must be <=3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch NUM_OF_MASTER_REPLICAS must be 3 or greater. Fixed

vars.md
@@ -9,6 +9,8 @@ assured that they are persisted.

| Name | Option | Allowed values | Default |
| :------ | :------- | :--------------- | :-------- |
| NUM_OF_MASTER_REPLICAS | Set the controlplane replica count. ||1|
Copy link
Member

Choose a reason for hiding this comment

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

Should it be ||1| ? There are double |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be ||1| ? There are double |

Third column is empty, so I assume this is correct?

Copy link
Member

Choose a reason for hiding this comment

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

Aha ok true

@jan-est
The Cluster API control plane now allows for scaling in.
0d6792c 
This PR parameterizes the required metal3_dev_env fields
and adds a new environment variable MAX_SURGE_VALUE.
@jan-est jan-est force-pushed the add-max-surge-variables-jan branch from 401935b to 0d6792c Compare April 7, 2021 06:39
@jan-est
Copy link
Contributor Author

jan-est commented Apr 7, 2021

lgtm, couple of small nits, by the way CONTROL_PLANE_MACHINE_COUNT is now replaced with NUM_OF_MASTER_REPLICAS. So do we need this CONTROL_PLANE_MACHINE_COUNT anywhere else?

CONTROL_PLANE_MACHINE_COUNT is not used anywhere else. Originally this variable was not even initialized or defaulted anywhere in our code, which caused CAPI webhook to default replica count as 1.

@kashifest
Copy link
Member

/test-centos-integration
/test-integration
/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021
@metal3-io-bot metal3-io-bot merged commit 9886555 into metal3-io:master Apr 7, 2021
@jan-est jan-est deleted the add-max-surge-variables-jan branch August 20, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kashifest kashifest kashifest left review comments

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@fmuyassarov fmuyassarov fmuyassarov approved these changes

Assignees
No one assigned
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jan-est @fmuyassarov @metal3-io-bot @maelk @kashifest @furkatgofurov7