search

kubernetes / cloud-provider-gcp

cloud-provider-gcp contains several projects used to run Kubernetes in Google Cloud
Apache License 2.0
126 stars 211 forks source link

Cloud.InstanceID() does not actually return a unique Instance ID #589

Closed antonipp closed 7 months ago

antonipp commented 1 year ago

The InstanceID function defined in gce_instances.go does not return the actual "Instance ID" as it is defined by GCP (https://cloud.google.com/compute/docs/instances/get-instance-id#api). Instead, the function returns the "Node Name" which, as described in nodename.go, is the "Name of an Instance object in the GCE API" and is the same as the hostname.

However, this name can not be used as a unique identifier for an instance. One example of where this breaks is the Managed Instance Group "Auto-heal / Auto-repair" feature: when a VM fails, the MIG will automatically re-create it with the same name but it will not actually be the same instance (the local data will be gone, the IP will change, etc).

An easy way to reproduce it:

  1. Create a MIG with one instance
  2. Describe the instance to get its id: 
    $ gcloud compute instances describe test-instance-b696f58890566cb5-t44r --format="yaml(name,id)"
id: '3325966898410103550'
name: test-instance-b696f58890566cb5-t44r
  3. Delete the instance: 
    $ gcloud compute instances delete test-instance-b696f58890566cb5-t44r
  4. Wait for the MIG to re-create the instance and describe it again: 
    $ gcloud compute instances describe test-instance-b696f58890566cb5-t44r --format="yaml(name,id)"
id: '3535995751281809694'
name: test-instance-b696f58890566cb5-t44r

    Note that the ID has changed whereas the name stayed the same!

This proved to be quite problematic in our Kubernetes setup because Kubernetes will still think that the instance is the same whereas it was changed under the hood.

So my proposal in order to be able to actually distinguish the instances would be to modify the code to something like this:

diff --git a/providers/gce/gce_instances.go b/providers/gce/gce_instances.go
index fde8ace5..398534c7 100644
--- a/providers/gce/gce_instances.go
+++ b/providers/gce/gce_instances.go
@@ -24,6 +24,7 @@ import (
        "fmt"
        "net"
        "net/http"
+       "strconv"
        "strings"
        "time"

@@ -363,16 +364,22 @@ func (g *Cloud) InstanceID(ctx context.Context, nodeName types.NodeName) (string
                // Use metadata, if possible, to fetch ID. See issue #12000
                if g.isCurrentInstance(instanceName) {
                        projectID, zone, err := getProjectAndZone()
-                       if err == nil {
-                               return projectID + "/" + zone + "/" + canonicalizeInstanceName(instanceName), nil
+                       if err != nil {
+                               return "", err
+                       }
+
+                       instanceID, err := metadata.Get("instance/id")
+                       if err != nil {
+                               return "", err
                        }
+                       return projectID + "/" + zone + "/" + instanceID, nil
                }
        }
        instance, err := g.getInstanceByName(instanceName)
        if err != nil {
                return "", err
        }
-       return g.projectID + "/" + instance.Zone + "/" + instance.Name, nil
+       return g.projectID + "/" + instance.Zone + "/" + strconv.FormatUint(instance.ID, 10), nil
 }

 // InstanceType returns the type of the specified node with the specified NodeName.

This would allow us to reliably detect when these events happen by using the actual unique instance id instead of the name which is not unique.

What do you think?

k8s-ci-robot commented 1 year ago

This issue is currently awaiting triage.

If the repository mantainers determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
antonipp commented 1 year ago

Forgot to mention but there is a workaround which is to explicitly set providerID in the Kubelet Configuration to force it to use the proper instance id.

aojea commented 1 year ago

/kind bug @bobbypage @jprzychodzen who can be a good person to triage this issue?

bowei commented 1 year ago

/assign

bowei commented 1 year ago

This might be hard to change without breaking something. Does it make more sense to expose the UID in a separate method and for your software to call that instead?

antonipp commented 1 year ago

This might be hard to change without breaking something

Hm, yeah makes sense, I just had a second look at the codebase and realized that all functions like this which use instanceByProviderID() will need to be updated too in order to account for a different ID... But the scope of changes is not that big either.

Does it make more sense to expose the UID in a separate method and for your software to call that instead?

Not sure because the software I was thinking about was the Kubelet which calls the cloudprovider.GetInstanceProviderID() function in kubelet_node_status.go and this in turns calls instances.InstanceID() here. This is a generic interface implemented by multiple Cloud Provider libraries so if we create a new function, we will need to somehow call it from there and this won't be compatible with other Cloud Provider implementations.

bobbypage commented 1 year ago

One more note -- the instance id (i.e. the UID from GCE instance) is already exposed on the node object as an annotation and here where it is set.

Does that work for your use case?

antonipp commented 1 year ago

One more note -- the instance id (i.e. the UID from GCE instance) is already exposed on the node object as an annotation and here where it is set.

Does that work for your use case?

This is something that we already rely on actually... To give a bit more context, we have a custom bash script that runs on each node before the kubelet has started and uses this annotation to check if the node that is currently booting re-uses a previously used hostname: it compares the current node UID from the IMDS with the UID in the annotation of the node object if such an object already exists.

I was hoping that we could stop relying on this custom logic and that we could include this check into the kubelet directly, for example here when the kubelet tries to register the node with the API server. Since a similar node name re-use problem happens to us in AWS, it would've been nice to write a Cloud-Provider agnostic check directly in the kubelet:

if existingNode.Spec.ProviderID != node.Spec.ProviderID {
  // Delete existing Node
  // Try again
}

This would work for AWS where the ProviderID is set to the actual UID of the instance (ex: i-0513b0ff1ebf9342d) but unfortunately doesn't work for GCP where the ProviderID is not unique :/

bobbypage commented 1 year ago

I was hoping that we could stop relying on this custom logic and that we could include this check into the kubelet directly, for example here when the kubelet tries to register the node with the API server.

We have somewhat similar logic today actually... When the kubelet attempts to register, we check if there already exists a node object in the api-server with the same name. If it does, and the instance id differs (based on looking at the annotation), we delete the old node object. See this logic in shouldDeleteNode which is called as part of the node registration process.

antonipp commented 1 year ago

Yeah, I already came across this code but I really wanted to implement a cloud-agnostic solution directly in the kubelet since we have a very similar problem in AWS... That code is too GCP-specific and is also part of a separate controller which not everybody necessarily runs.

aojea commented 1 year ago

/cc @cezarygerard

BenTheElder commented 1 year ago

Outside of GCP/AWS this field may not be set at all and the contents of this field aren't strongly formatted or guaranteed for external users AFAIK.

k8s-triage-robot commented 9 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 8 months ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 7 months ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

k8s-ci-robot commented 7 months ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes/cloud-provider-gcp/issues/589#issuecomment-2026256888): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.