Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add IMDS backup url #1630
Add IMDS backup url #1630
Changes from 11 commits
cba2192
e417b36
c811263
1c8da0d
1215b40
5f3214c
6dbd7f1
3c3e80f
5a153e3
aaf3554
e2a6a3e
f89b1ea
35c8451
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add the HttpError to this message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.