linode / linode_api4-python

Official Python bindings for the Linode API
https://linode-api4.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
136 stars 74 forks source link

After network_ips_assign call instance object remains with old ip in instance1.ips.ipv4.public[0] #164

Closed alkmim closed 5 years ago

alkmim commented 5 years ago

Description

After swapping the public IP of two linode instances using LinodeClient.networking.ips_assign, the object representing the instance keeps the same IP in instance1.ips.ipv4.public[0] but updates the ipv4 list with the new IP in instance1.ipv4.

How to reproduce

import time
from linode_api4 import LinodeClient, Instance

token = <your-token>
client = LinodeClient(token)

# Creating Instances
instance1, password = client.linode.instance_create(ltype="g6-nanode-1", region="us-west", image="linode/debian10")
instance2, password = client.linode.instance_create(ltype="g6-nanode-1", region="us-west", image="linode/debian10")

# Waiting instances to be running
while True:
    status = instance1.status
    time.sleep(5)
    if status == 'running':
        break
while True:
    status = instance2.status
    time.sleep(5)
    if status == 'running':
        break

print("### Before Swapping")
print("instance1.ips.ipv4.public[0] = {}".format(instance1.ips.ipv4.public[0]))
print("instance1.ipv4 = {}".format(instance1.ipv4))

# Swapping IPs
assignments = [instance1.ips.ipv4.public[0].to(instance2), instance2.ips.ipv4.public[0].to(instance1)]
client.networking.ips_assign(region, *assignments)

# In pycharm, running in the debug mode, add a breakpoint here and open the  variable 
# instance1 in the editor. The behavior with debugging mode is different than without
# debugging mode. I added the output for both scenarios below

print("### After Swapping")
print("instance1.ips.ipv4.public[0] = {}".format(instance1.ips.ipv4.public[0]))
print("instance1.ipv4 = {}".format(instance1.ipv4))

Output (With Debugging)

### Before Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245
instance1.ipv4 = ['45.79.100.245']
### After Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245      <<< Not updated
instance1.ipv4 = ['45.33.56.174']      <<< Updated

Expected Output (With Debugging)

### Before Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245
instance1.ipv4 = ['45.79.100.245']
### After Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.33.56.174      <<< Updated
instance1.ipv4 = ['45.33.56.174']      <<< Updated

Output (Without Debugging)

### Before Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245
instance1.ipv4 = ['45.79.100.245']
### After Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245      <<< Not updated
instance1.ipv4 = ['45.79.100.245']      <<< Not updated

Expected Output (Without Debugging)

### Before Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.79.100.245
instance1.ipv4 = ['45.79.100.245']
### After Swapping
instance1.ips.ipv4.public[0] = IPAddress: 45.33.56.174      <<< Updated
instance1.ipv4 = ['45.33.56.174']      <<< Updated
Dorthu commented 5 years ago

It looks like this is happening because objects in this library cache their values when loaded, and don't update them unless they change them. Because IP Assignment doesn't go through the Instance object, it's not aware that anything's changed; the assignment objects passed into ip_assign don't even have references to the Instance objects whose IPs they're assigning.

I think the solution to this is to manually invalidate all Instance objects involved in the assignment; in the example above including this just after calling client.networking.ips_assign should do the trick:

instance1.invalidate()
instance2.invalidate()

Once invalidated, the instances will reload all of their local values from the API the next time they're accessed, which should yield the updated values.

alkmim commented 5 years ago

Thanks for the quick response and for providing a way to work around the situation.

Would this be the expected behavior (i.e. there is no issue), or the expected behavior would be to update the object without calling the invalidate()?

Dorthu commented 5 years ago

In most cases I'd want the objects to invalidate themselves when something changes. However, because the ips_assign function doesn't take the Instance objects that would need to be invalidated, I don't know how I'd do it. I considered making IPAddress.to invalidate the target Instance, but that would only work some of the time.. if you were simply transferring one IP from one Instance to another, only the Instance receiving the IP would be updated, which I think is more confusing. I'll keep thinking on it, but I'm not sure there's a good answer past manually invalidating all involved Instances.

alkmim commented 5 years ago

What about if IPAddress.to invalidates the source and the destination instance? It would invalidates the source instance, do the transfer and invalidates the target instance. Would that make sense?

I am not too familiar with the library as I am starting to use it as a user a couple days ago, so forgive me if this idea is non-sense :)

Dorthu commented 5 years ago

I actually tried that, but the IPAddress class doesn't have a reference to the Instance object that needs to be invalidated; it has a linode property, but that would return a new Instance object, meaning that if I added a self.linode.invalidate() the object stored at instance1 would still not have been updated.

alkmim commented 5 years ago

Hum. I see. that makes sense. Perhaps an update on the documentation would be a better approach? IMHO, it is better to say in the docs something like The instance object will not be updated after calling ip_assign. to invalidate the cache, call instance.invalidate than having a behavior that only the target instance will be invalidated. What do you think about this approach?

Dorthu commented 5 years ago

Opened #166 to document this behavior, which I think is a reasonable solution. Let me know if you think it makes sense, and if so I'll merge it in. Thanks for all your feedback!

alkmim commented 5 years ago

The changes are good for me. I left my approval there. Thanks for the quick response and for providing a quick and simple workaround. Feel free to close this issue.