Stop adopting masters with machine API #1687
Conversation
Master machine assets were introduced by openshift#491 to adopt instances generated by tf. There was never a follow up to leverage adoption for fatrther addon features. Unless there's a good reason to keep the adoption, this PR removes the machine API master awareness and assets generation
|
/hold |
openshift-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: enxebre If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @abhinavdahiya @wking |
|
Please don't do this. We're going to need the master machines in order to manage those hosts for RHHI.Next. |
|
/hold |
|
Yeah, even on AWS there are expectations we can reduce manual effort for admins in some scenarios via this. |
|
This is a no-go. Can you explain what |
|
@enxebre: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
@dhellmann
@smarterclayton
@abhinavdahiya So may be I'm just missing context on the uses cases pointed here but until those features are in place is not clear to me the benefit from adopting masters atm. |
|
There's enough risks to changing that we need a good reason to remove.
What is the good reason to remove?
…On Tue, Apr 30, 2019 at 3:59 AM Alberto García Lamela < ***@***.***> wrote:
Please don't do this. We're going to need the master machines in order to
manage those hosts for RHHI.Next.
@dhellmann <https://github.com/dhellmann>
The masters machines today are just adopted by the API. The only benefit
you get from this today, is you can delete them via cli. They are not a
set, you can not scale them in/out. Could you elaborate your user case?
Yeah, even on AWS there are expectations we can reduce manual effort for
admins in some scenarios via this.
@smarterclayton <https://github.com/smarterclayton>
Afaik all this adoption gives you atm is an easy way to delete masters via
cli/api. I'd argue that's a +1.
Can you elaborate in which scenarios can be beneficial?
Can you explain what
addon features we are not using?
Where does the adoption create problems for machine-api-? What special
things do you have to do ie master awareness?
@abhinavdahiya <https://github.com/abhinavdahiya>
All this adoption gives us today is exposing en easier way to delete
master machines. Adopting with API machines could be the foundation for
higher level addon features to have a robust API driven control plane which
could allow things like scaling capabilities for the control plane via
sets, autoscaling, stronger integration with etcd, recovering from
distaster scenarios. But afaik none of this is in place today.
So may be I'm just missing context on the uses cases pointed here but
until those features are in place is not clear to me the benefit from
adopting masters atm.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1687 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI37J4TFTKEZ6R5ERNGBX3PS734NANCNFSM4HJBCAMQ>
.
|
| return true, err | ||
| } | ||
|
|
||
| m.MachineFiles = fileList |
There was a problem hiding this comment.
This will break loading the asset from disk. The MachineFiles will be empty when the asset is loaded from disk.
It gives cluster-machine-approver a consistent way to approve server certs for kubelets on IPI control plane -> machines
So I do not agree with your resononing that control plane machines objects only serve "delete from CLI", it is an important API layer we build on IPI |
The RHHI.Next case is a baremetal hyperconverged cluster with the provisioning tool for expanding the cluster self-hosted within the cluster. The source of truth about the nodes, machines, and hosts in the cluster is custom resources in the kubernetes database, and the cluster-api provider we're building accesses that data using the kubernetes API. Because this is a self-hosted baremetal setup, there is no external component to do hardware monitoring, so we are also building that so we can report to the user about what hardware they have and whether it is healthy. In order to monitor every member of the cluster, including the masters, we need the installer to register them based on information collected from the user at the time the cluster is built. There is no other API or database for us to query later to "adopt" them. This sprint we are focusing on finishing the work to migrate the logic from our proof-of-concept scripts for doing all of this into the installer, so we will have more patches soon to show exactly how we will use the Machine objects being registered here as masters, as well as any that will be registered as workers. |
|
@dhellmann @abhinavdahiya thanks for elaborating. As far as we are clear and explicit on the message that masters machines are pets today with the risks which that carries I'm ok. |
8 participants
Master machine assets were introduced by #491 to adopt instances generated by tf. There was never a follow up to leverage adoption for fatrther addon features. Unless there's a good reason to keep the adoption, this PR removes the machine API master awareness and assets generation until there's a more complete story for it