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 az_list_node variable to specify different AZs for kubelets #5413

Merged
merged 6 commits into from
Feb 18, 2020
Merged

add az_list_node variable to specify different AZs for kubelets #5413

merged 6 commits into from
Feb 18, 2020

Conversation

rptaylor
Copy link
Contributor

@rptaylor rptaylor commented Dec 5, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Since kubelet nodes can be considered ephemeral/disposable , different availability considerations may apply than for master nodes. Accordingly, on Openstack users may wish to use different AZs or different balancing across AZs for kubelet nodes than for master nodes. This PR adds a TF variable az_list_node which is exactly like az_list, but it applies only for the k8s node type. The default value is the same, so there is no change in behaviour unless users choose to modify the variable.
This provides finer control over placement of different types of nodes on Openstack.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

No

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2019
@rptaylor
Copy link
Contributor Author

rptaylor commented Dec 6, 2019

/assign @holmsten

@rptaylor
Copy link
Contributor Author

rptaylor commented Dec 7, 2019

Also fixes some deprecation warnings for TF 0.12

@huxcrux
Copy link
Contributor

huxcrux commented Dec 7, 2019

I would say that this is a user facing change? Most Openstack installation have custom AZs set meaning this might break clusters for users that doesn't read the code.

This should probably be added to the Openstack terraform documentation as well (https://github.com/kubernetes-sigs/kubespray/blob/master/contrib/terraform/openstack/README.md)

@Miouge1
Copy link
Contributor

Miouge1 commented Dec 9, 2019

Can we make this backwards compatible? Something like if az_list_node is not set then default to az_list ?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2019
@rptaylor
Copy link
Contributor Author

@Miouge1 @bl0m1
Okay, so az_list_node could be an empty list by default.

Then there could be a conditional (ternary) expression, something like

length(az_list_node) > 0 ? az_list_node : az_list

Would that be a good way?

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 11, 2019
@huxcrux
Copy link
Contributor

huxcrux commented Feb 18, 2020

Looks good :)
/assign @holmsten

@holmsten
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: holmsten, rptaylor

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 277b347 into kubernetes-sigs:master Feb 18, 2020
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Mar 7, 2020
…rnetes-sigs#5413)

* rebase and add az_list_node variable to specify different AZs for kubelets

* fix missing variable name change
@rptaylor rptaylor deleted the 20191205-az_list_node branch March 25, 2020 17:29
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants