Host plugin updates #505
Host plugin updates #505
Conversation
| @@ -282,16 +282,16 @@ def get_ext_handlers(self): | |||
| def get_ext_handler_pkgs(self, extension): | |||
| raise NotImplementedError() | |||
|
|
|||
| def get_in_vm_artifacts_profile(self): | |||
| def get_artifacts_profile(self): | |||
There was a problem hiding this comment.
Not sure if in_vm_artifacts_profile equals to artifacts_profile.
There was a problem hiding this comment.
I think there are just so many similar names which contains artifacts or profile(i.e. extension artifacts) so I am just worried that there might be another thing called 'artifact profile' different from 'in_vm_artifacts_profile'
There was a problem hiding this comment.
We can always change it, but at the moment this is the only artifact profile. It just seems unnecessary to qualify it as in_vm_.
There was a problem hiding this comment.
Discussed offline, leaving as is.
| url = URI_FORMAT_PUT_VM_STATUS.format(self.endpoint, HOST_PLUGIN_PORT) | ||
| logger.verbose("Posting VM status to host plugin") | ||
| status = textutil.b64encode(status_blob.data) | ||
| blob_type = status_blob.type if status_blob.type else config_blob_type |
There was a problem hiding this comment.
I think it might be better to use get_blob_type() here and let the function handle this fallback.
There was a problem hiding this comment.
I agree this could be improved, but I think the correct change is to remove get_blob_type since the HEAD call is not actually necessary anymore. For this PR I did not want to make more changes than necessary, but if you feel we should include that let me know.
There was a problem hiding this comment.
Discussed offline, I am going to add an event to be emitted which we can use to determine how reliable the given value is before we switch over to it.
| else: | ||
| host.manifest_uri = version.uri | ||
| logger.verbose("Manifest downloaded successfully from host plugin") | ||
| if response: |
There was a problem hiding this comment.
else we move on to the next uri, line 632.
| if not response: | ||
| logger.info("Retry fetch in {0} seconds", | ||
| LONG_WAITING_INTERVAL) | ||
| time.sleep(LONG_WAITING_INTERVAL) |
There was a problem hiding this comment.
How does the retry work here?
There was a problem hiding this comment.
fetch calls http_get which has retries built-in.
| try: | ||
| uploaded = self.status_blob.upload(ext_conf.status_upload_blob) | ||
| except (HttpError, ProtocolError) as e: | ||
| # errors have already been logged |
There was a problem hiding this comment.
If we let StatusBlob to use blobtype from extensionConfig as a fallback when it cannot retrieve one by making a request, status_blob.upload() should not throw HttpError/ProtocolError.
There was a problem hiding this comment.
Even if we did that the actual upload would subsequently fail and still throw the protocol error.
| uri, headers = host.get_artifact_request(blob) | ||
| profile = self.decode_config(self.fetch(uri, headers)) | ||
|
|
||
| if profile and not profile.isspace(): |
There was a problem hiding this comment.
use textutil.is_str_none_or_whitespace() instead?
| if not textutil.is_str_none_or_whitespace(in_vm_artifacts_profile_json): | ||
| self.__dict__.update(parse_json(in_vm_artifacts_profile_json)) | ||
| def __init__(self, artifacts_profile): | ||
| if artifacts_profile and not artifacts_profile.isspace(): |
There was a problem hiding this comment.
any reason for reverting it?
There was a problem hiding this comment.
Nope, bad merge - thanks for catching.
hglkrijger
force-pushed
the
host-plugin-artifacts
branch
from
6000733
to
fa511a1
Compare
hglkrijger
force-pushed
the
host-plugin-artifacts
branch
from
fa511a1
to
e74d0bf
Compare
|
@hglkrijger LGTM. Nice. |
hglkrijger
force-pushed
the
host-plugin-artifacts
branch
2 times, most recently
from
85d5f3a
to
f177e92
Compare
hglkrijger
force-pushed
the
host-plugin-artifacts
branch
from
f177e92
to
0c0df3c
Compare
These changes allow the agent to function normally even while outbound connectivity to the public internet is blocked, via NSG for example.
/cc @jinhyunr @brendandixon