-
Notifications
You must be signed in to change notification settings - Fork 45
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
salt: Fix apiserver-proxy upstreams #2555
Conversation
The previous approached relied on the pillar target 'metalk8s:nodes:*:roles:master', which will always match all minions since they all have access to the full 'metalk8s:nodes' pillar value. Instead, we use the 'metalk8s.minions_by_role' execution method to define the right target when retrieving control plane IPs from the mine.
Hello gdemonet,My role is to assist you with the merge of this Status report is not available. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
History mismatchMerge commit #f2738cd611eefc2582cedc18473f91cee4646f4e on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
a674bdd
to
e3c97e1
Compare
History mismatchMerge commit #a674bdd085249f2fbf3e0ee9c5a0e7285f1326a4 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve |
d09888c
to
35896c9
Compare
/reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
35896c9
to
73f131f
Compare
History mismatchMerge commit #35896c9d72e7a184a7e808fecad0c771c5f5f5d6 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve |
salt/metalk8s/kubernetes/apiserver-proxy/files/apiserver-proxy.conf.j2
Outdated
Show resolved
Hide resolved
The `metalk8s.minions_by_role` method now checks for those errors and raises a `CommandExecutionError` if there are some. The apiserver-proxy configuration template now handles more cases to ensure we only generate the configuration if there are master nodes, and also ensure we override `metalk8s:nodes:_errors` when bootstrapping.
73f131f
to
4c961ab
Compare
/reset |
/approve |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
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.
LGTM
Build failedThe build for commit did not succeed in branch w/2.5/bugfix/apiserver-proxy-upstreams. The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye gdemonet. |
The previous approached relied on the pillar target
metalk8s:nodes:*:roles:master
, which will always match all minionssince they all have access to the full
metalk8s:nodes
pillar value.Instead, we use the
metalk8s.minions_by_role
execution method todefine the right target when retrieving control plane IPs from the mine.
We also change the
metalk8s.minions_by_role
method to handlepillar errors gracefully, and let clients handle an empty list as they wish
(which was already the approach taken in the apiserver-proxy
configuration template, considering an empty result implies we are
bootstrapping).