-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Enable to sort kube_control_plane including the first node #9866
Enable to sort kube_control_plane including the first node #9866
Conversation
Hi @kerryeon. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
c92bb0d
to
e822eec
Compare
@@ -158,7 +158,7 @@ | |||
filename: "{{ kube_config_dir }}/kdd-crds.yml" | |||
state: "latest" | |||
when: | |||
- inventory_hostname == groups['kube_control_plane'][0] | |||
- inventory_hostname == first_kube_control_plane |
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.
It is not appropriate to apply these yaml files on every node here, because there may be conflicts and errors when applying them concurrently, and it also wastes time.
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
@@ -0,0 +1,83 @@ | |||
--- | |||
- name: Load ca.crt | |||
shell: set -o pipefail && cat "{{ kube_apiserver_client_cert }}" | base64 --wrap=0 |
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.
That could use slurp
rather than shell. If you have unwanted wrapping just remove it with a filter.
changed_when: false | ||
|
||
- name: Get current kubernetes cluster-info | ||
command: "{{ kubectl }} -n kube-public get configmap cluster-info -o jsonpath --template '{.data.kubeconfig}'" |
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.
Same here, why not use k8s.core.k8s_info ?
- kube-cluster-info | ||
|
||
- name: Update kubernetes cluster-info | ||
command: |
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.
Same as above.
kube_apiserver_global_endpoint: |- | ||
{% if loadbalancer_apiserver is defined -%} | ||
https://{{ apiserver_loadbalancer_domain_name }}:{{ loadbalancer_apiserver.port|default(kube_apiserver_port) }} | ||
{%- elif loadbalancer_apiserver_localhost and (loadbalancer_apiserver_port is not defined or loadbalancer_apiserver_port == kube_apiserver_port) -%} | ||
https://localhost:{{ kube_apiserver_port }} | ||
{%- else -%} | ||
https://{{ first_kube_control_plane_address }}:{{ kube_apiserver_port }} | ||
{%- endif %} | ||
kube_apiserver_endpoint: |- | ||
{% if loadbalancer_apiserver is defined -%} | ||
https://{{ apiserver_loadbalancer_domain_name }}:{{ loadbalancer_apiserver.port|default(kube_apiserver_port) }} | ||
{%- elif not is_kube_master and loadbalancer_apiserver_localhost -%} | ||
https://localhost:{{ loadbalancer_apiserver_port|default(kube_apiserver_port) }} | ||
{%- elif is_kube_master -%} | ||
https://{{ kube_apiserver_bind_address | regex_replace('0\.0\.0\.0','127.0.0.1') }}:{{ kube_apiserver_port }} | ||
{%- else -%} | ||
https://{{ first_kube_control_plane_address }}:{{ kube_apiserver_port }} | ||
{%- endif %} | ||
# kube_apiserver_global_endpoint: |- | ||
# {% if loadbalancer_apiserver is defined -%} | ||
# https://{{ apiserver_loadbalancer_domain_name }}:{{ loadbalancer_apiserver.port|default(kube_apiserver_port) }} | ||
# {%- elif loadbalancer_apiserver_localhost and (loadbalancer_apiserver_port is not defined or loadbalancer_apiserver_port == kube_apiserver_port) -%} | ||
# https://localhost:{{ kube_apiserver_port }} | ||
# {%- else -%} | ||
# https://{{ first_kube_control_plane_address }}:{{ kube_apiserver_port }} | ||
# {%- endif %} | ||
# kube_apiserver_endpoint: |- | ||
# {% if loadbalancer_apiserver is defined -%} | ||
# https://{{ apiserver_loadbalancer_domain_name }}:{{ loadbalancer_apiserver.port|default(kube_apiserver_port) }} | ||
# {%- elif not is_kube_master and loadbalancer_apiserver_localhost -%} | ||
# https://localhost:{{ loadbalancer_apiserver_port|default(kube_apiserver_port) }} | ||
# {%- elif is_kube_master -%} | ||
# https://{{ kube_apiserver_bind_address | regex_replace('0\.0\.0\.0','127.0.0.1') }}:{{ kube_apiserver_port }} | ||
# {%- else -%} | ||
# https://{{ first_kube_control_plane_address }}:{{ kube_apiserver_port }} | ||
# {%- endif %} |
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.
Why are you converting this to a fact ? Since variable are lazy that should not be necessary.
- name: create fallback_ips_parsed | ||
set_fact: | ||
fallback_ips_parsed: "{{ hostvars.localhost.fallback_ips_base | from_yaml }}" | ||
|
||
- name: set fallback_ips | ||
set_fact: | ||
fallback_ips: "{{ hostvars.localhost.fallback_ips_base | from_yaml }}" | ||
fallback_ips: "{{ fallback_ips_parsed if fallback_ips_parsed is mapping else {} }}" |
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.
It's not super clear why you need to do this, could you explain it ?
when: inventory_hostname == groups['kube_control_plane'][0] | ||
when: inventory_hostname == groups['kube_control_plane'] | first |
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.
Is that necessary to make the PR works ? It does not change the semantic right ?
If not I'd drop those, the PR is already big enough ! ^^
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kerryeon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/reopen
/remove-lifecycle rotten
|
@VannTen: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The first kubernetes control plane node has a very important role in overall kubespray workflows. However, if the first node is out of order with the others, the kubernetes cluster configuration will fail catastrophically. For example, certificate loss due to accidental ETCD and Kubernetes SSL key renewal.
Keeping the order of the first nodes is very important, but there is always the potential for human error to cause problems. So, I want to force the dynamic discovery logic to consider one of the kubernetes control plane nodes that are still functioning as the first node, so that we don't run into issues with node order in the first place.
Fortunately, I have been able to find that these attempts are already being made locally. Therefore, this PR will be a generalization of them. (#7989)
> BEFORE
> AFTER
kube_control_plane
And if control planes are changed via
cluster.yml
and etc, it will automatically update thecluster-info
to the first control plane node.> BUG FIXES
fallback_ips
. The problem is that if all nodes do not have network adapters, the value offallback_ips_base
becomes"---"
, so the value offallback_ips
, the result of yaml-parsing, becomes a str object. This bug occurs when testingmolecule_docker
.Which issue(s) this PR fixes:
Fixes #3471 (only for
kube_control_plane
)Fixes #9863
Special notes for your reviewer:
This PR has many changes for usage of
kube_control_plane | first
.So many various tests are needed, but I can't be sure that this changes are complete yet, so I want to review with others.
Does this PR introduce a user-facing change?: