Add IMDS backup url #1630
Add IMDS backup url #1630
Conversation
ghost
requested review from
larohra,
narrieta,
pgombar and
vrdmr
as code owners
|
Related to #1249 |
Codecov Report
@@ Coverage Diff @@
## develop #1630 +/- ##
==========================================
Coverage ? 66.94%
==========================================
Files ? 78
Lines ? 11263
Branches ? 1575
==========================================
Hits ? 7540
Misses ? 3400
Partials ? 323
Continue to review full report at Codecov.
|
| try: | ||
| resp = self._http_get(endpoint=endpoint, resource_path=resource_path, headers=headers) | ||
| except HttpError as e: | ||
| logger.warn("Unable to connect to primary IMDS endpoint {0}", endpoint) |
There was a problem hiding this comment.
how often is get_matada invoked? this may have to be a periodic message to avoid flooding the log/serial console if the endpoint can't be reached
There was a problem hiding this comment.
Once a minute based on MonitorHandler here: https://github.com/Azure/WALinuxAgent/blob/develop/azurelinuxagent/ga/monitor.py#L114
There was a problem hiding this comment.
let's make it periodic, see https://github.com/Azure/WALinuxAgent/blob/develop/azurelinuxagent/common/event.py#L293 for an example
There was a problem hiding this comment.
There are log messages that can potentially be made periodic in monitor.py that logs with IMDS is healthy or not.
I think it would require a new feature of periodic logging that accounts for state change where IMDS health can change states (flip between healthy and unhealthy). We'd want to capture that state change in the logs, which should be rare making periodic logging meaningful and desirable most of the time.
Let me know your thoughts. Thanks!
There was a problem hiding this comment.
Yes, I have also seen sections of the code where periodic logging with state changes is needed. Currently the state flip and logging is handled explicitly in that code. Next time I touch it I'll try to refactor this functionality to the logger.
| logger.warn("Unable to connect to backup IMDS endpoint {0}", endpoint) | ||
| if not self._regex_imds_ioerror.match(str(e)): | ||
| raise e | ||
| return False, "IMDS error in /metadata/{0}: Unable to connect to endpoint".format(resource_path) |
There was a problem hiding this comment.
should we add the HttpError to this message?
There was a problem hiding this comment.
I don't feel that verbosity would help since the HttpError message is long and only provides "IOError timed out" within it and seems to be vague and not 100% accurate. There was no wait and the return was almost instant when running my tests on a VM.
I try to summarize the connection issue with a (possibly too simple) "Unable to connect to endpoint" message. I can change this as desired but I don't think the HttpError message adds value.
There was a problem hiding this comment.
If we need to debug an error here, is there anything useful we can add on top of "Unable to connect to endpoint"?
There was a problem hiding this comment.
This currently captures everything IMDS team needs for troubleshooting. Please provide an example of what you have an mind.
Any additional info that can potentially help with root cause analysis is always welcome and appreciated.
Description
Calls to metadata service can fallback to the wireserver ip address in scenarios where customer is blocking the primary metadata service ip address.
PR information
Quality of Code and Contribution Guidelines
This change is