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

Remote Node/Object References #520

Closed
davidewatson opened this issue Oct 4, 2018 · 12 comments · Fixed by #1011 or #1052
Closed

Remote Node/Object References #520

davidewatson opened this issue Oct 4, 2018 · 12 comments · Fixed by #1011 or #1052
Assignees
Labels
area/api Issues or PRs related to the APIs priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@davidewatson
Copy link
Contributor

davidewatson commented Oct 4, 2018

Problem

MachineStatus contains an optional NodeRef field:

NodeRef *corev1.ObjectReference `json:"nodeRef,omitempty"`

There are a couple of places where the ability to refer to the Node corresponding to a Machine is extremely useful if not essential. Information in Node objects can be used as surrogates for Exists and Ready conditions as well as general node health (cf. #483).

For providers which distinguish between manager and managed clusters, NodeRefs will not be set (they are local to a cluster and optional in the spec). There is a related issue about creating a dynamic link between machines/nodes. This issue is asking for a similar link which will also work in the managed cluster case.

Examples

  1. "[The MachineSet controller uses NodeRefs] to determine the ready replicas count for a MachineSet which is in turn required for the MachineDeployment controller." - @alvaroaleman

if noderefutil.IsNodeReady(node) {

  1. The deployer waits for a NodeRef on Machines to determine when a machine is ready. @chuckha

ready := m.Status.NodeRef != nil || len(m.Annotations) > 0

  1. The upgrader uses NodeRefs to find the version of kubelet running on a node. This is used to determine if an upgrade is necessary. Some providers have tooling which also uses this information to determine when an upgrade is complete.

if machine.Status.NodeRef == nil {

Possible Solution

Add a tuple (APIEndpoint, NodeName) for node objects. More generally (APIEndpoint, Namespace, ObjectName), or maybe (APIEndpoint, UUID), could be used for arbitrary remote object references.

When this has come up in the past @roberthbailey suggested that maybe we should look at the Cluster Registry to see if there is any infrastructure we can share. Looking at it briefly there is an ObjectReference type which might be useful:

https://github.com/kubernetes/cluster-registry/blob/09c490c051fbd24452921a18b366371c221a71d8/pkg/apis/clusterregistry/v1alpha1/types.go#L114

This issue is to explore whether a single solution for these use cases makes sense.

@davidewatson
Copy link
Contributor Author

@detiber @hardikdr @oneilcin

@sidharthsurana
Copy link
Contributor

One suggestion is can't we continue to have the NodeRef object be associated with the Machine Object for the remote cluster as well? After all the NodeRef as of today simply has the following:

corev1.ObjectReference {
	Kind: "Node",
	Name: node.ObjectMeta.Name,
	UID:  node.UID,
}

Now whenever any controller wants to use this NodeRef info, they simply need to refer to the Cluster this Machine belongs to, get the right kubeconfig for the cluster (either one pointing to the remote cluster, or the local cluster) and then interact with this Node object.

@davidewatson
Copy link
Contributor Author

It's a good point. The generic controllers (e.g. MachineSet) do not currently have a way to do this though.

@roberthbailey roberthbailey added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 11, 2019
@roberthbailey roberthbailey added this to the v1alpha1 milestone Jan 11, 2019
@roberthbailey
Copy link
Contributor

/assign @davidewatson

@ncdc
Copy link
Contributor

ncdc commented Mar 5, 2019

Have we decided on what we want to do here for v1alpha1?

@davidewatson
Copy link
Contributor Author

I think we should punt this from v1alpha1. We have the controllers we have for this release. The main consequence for this repo is that MachineSet controller health checks will only work when the controller is run within the same cluster it manages.

@ncdc
Copy link
Contributor

ncdc commented Mar 6, 2019

SGTM.
/milestone Next

@k8s-ci-robot k8s-ci-robot modified the milestones: v1alpha1, Next Mar 6, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 4, 2019
@detiber
Copy link
Member

detiber commented Jun 4, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 4, 2019
@vincepri
Copy link
Member

/area api

@vincepri
Copy link
Member

Regarding this issue, I'd propose to tackle this in v1alpha1 by creating a new, backward-compatible, controller that references a secret in a specific location.

  • Create a nodeRef controller in CAPA that watches Machines
  • Check <cluster-name>-kubeconfig secret is available
  • Create a client from kubeconfig
  • Check each remote node and set NodeRef if a matching Node is found

Happy to start working on it if the above sounds good!

/cc @detiber @ncdc

@detiber
Copy link
Member

detiber commented Jun 10, 2019

@vincepri sgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Issues or PRs related to the APIs priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
9 participants