RFR: Using Ironic vendor passthru for heartbeat #66
Conversation
Conflicts: .gitignore
Conflicts: teeth_agent/agent.py teeth_agent/errors.py teeth_agent/overlord_agent_api.py
joshgachnang
changed the title
| path = '/{api_version}/agents/{mac_addr}/configuration'.format( | ||
| api_version=self.api_version, | ||
| mac_addr=mac_addr) | ||
| def get_configuration(self, mac_addrs, ipaddr, hardware_info, mode, |
There was a problem hiding this comment.
Can you rename this to something like lookup_node()?
There was a problem hiding this comment.
Also, drop the mode and version arguments (IMO it makes sense to pass those in the heartbeat, and maybe we can put the version in a User-Agent header in a different PR, but not needed here).
Also, doesn't hardware_info include the MAC addresses?
…t into JoshNang/heartbeat Conflicts: teeth_agent/errors.py
| @@ -143,6 +142,12 @@ def get_status(self): | |||
| def get_agent_mac_addr(self): | |||
| return self.hardware.get_primary_mac_address() | |||
|
|
|||
| def get_all_mac_addrs(self): | |||
There was a problem hiding this comment.
You can kill this method now
There was a problem hiding this comment.
Is there any need for get_agent_mac_addr any more? The vendor passthru takes all the MACs and finds the matching nodes and ensures they're the same node.
There was a problem hiding this comment.
No. Once this is merged, do a separate PR that kills get_agent_mac_addr and get_primary_mac_addr.
The driver won't be able to save anything during the first node lookup, so sending the URL then won't be useful.
| self.listen_address = listen_address | ||
| self.ipaddr = ipaddr | ||
| self.port = port |
There was a problem hiding this comment.
For silly reasons listen_address is actually already a tuple of (listen_host, listen_port)
| @@ -143,6 +141,9 @@ def get_status(self): | |||
| def get_agent_mac_addr(self): | |||
| return self.hardware.get_primary_mac_address() | |||
|
|
|||
| def get_node_uuid(self): | |||
| return self.node['uuid'] | |||
There was a problem hiding this comment.
Should probably check if self.node is None first, in case we manage to heartbeat before the node is looked up (or lookup fails).
There was a problem hiding this comment.
If lookup fails, it'd throw an uncaught exception. But still a worthwhile check.
There was a problem hiding this comment.
Well, yeah, that's what I want to avoid :P
There was a problem hiding this comment.
Hmm. lookup_node should probably be run in a loop with a timeout or something then, instead of the one shot or throw exception it has right now.
|
I still need to write something to deal with lookup_node throwing an exception or not being able to hit the Ironic API. Probably a loop with a timeout and/or put it inside the TeethAgentHeartbeater right before do_heartbeat(). |
| 'commands to. This is different than ' | ||
| 'listen-host because Docker will have a ' | ||
| 'different internal IP than the host IP that ' | ||
| 'Ironic will be communicating with.')) |
There was a problem hiding this comment.
IMO this shouldn't be Docker-specific. There are any number of reasons that the public IP might not match the listen IP.
There was a problem hiding this comment.
And there are lots of reasons it would be different than the listen IP as well, such as NAT.
|
Other than docs comment, LGTM |
|
|
||
| return TeethAgent(api_url=api_url, | ||
| advertise_address=(advertise_host, advertise_port), | ||
| listen_address=(listen_host, listen_port)) |
There was a problem hiding this comment.
If we're going to call these as keyword args here, can we also make them keyword args in the function definition? (or vice versa, of course, make them positional args here)
|
Other than my nitpick there, lgtm. |
Using Ironic vendor passthru for heartbeat
This will be broken until I get the py26 fixes in.