Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add check for HostGAPlugin version #2413

Merged
merged 3 commits into from
Nov 17, 2021
Merged

Add check for HostGAPlugin version #2413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5af9c2c
Add check for HostGAPlugin version
Nov 16, 2021
320e4a6
Log as info
Nov 17, 2021
fc9d50e
Merge branch 'develop' into plugin-version
narrieta Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions azurelinuxagent/common/protocol/extensions_goal_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from azurelinuxagent.common.AgentGlobals import AgentGlobals
from azurelinuxagent.common.exception import AgentError
from azurelinuxagent.common.utils import textutil
from azurelinuxagent.common.utils.flexible_version import FlexibleVersion
from azurelinuxagent.common.protocol.restapi import ExtHandlerList, VMAgentManifestList


Expand Down Expand Up @@ -108,8 +109,10 @@ def compare_attribute(attribute):
compare_attribute("activity_id")
compare_attribute("correlation_id")
compare_attribute("created_on_timestamp")
compare_attribute("status_upload_blob")
compare_attribute("status_upload_blob_type")
# The status blob was added after version 112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this check is temporary until this feature is stabilized? If so, could you please add a comment about it too. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On that note, do the latest version of HostGA contain all the attributes that we're parsing? Or will we have to have a similar check like this for other versions too in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 more question, what's the min version where the HostGA version is available? Is it .112 or greater than that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm guessing this check is temporary until this feature is stabilized? If so, could you please add a comment about it too. Thanks!
    It will stay there for a while, probably it can be removed once we get telemetry indicating that there are no old HostGAPlugins in the wild. For the moment this check is there just to avoid generating noise. No harm if this is not removed so I'll leave it as is

  • On that note, do the latest version of HostGA contain all the attributes that we're parsing? Or will we have to have a similar check like this for other versions too in the future?

We will have additional checks on the version. The purpose of this PR is mainly to set the infrastructure needed to perform those checks.

  • 1 more question, what's the min version where the HostGA version is available? Is it .112 or greater than that?

Not sure of the exact version, but Flexible version defaults to 0.0.0.0 so version 0 indicates the version field is not there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have additional checks on the version. The purpose of this PR is mainly to set the infrastructure needed to perform those checks.

I am personally not a big fan of putting checks on versions because they can be never ending. But at the same time I'm not sure of a better way to do it either so approved the PR

if from_vm_settings.host_ga_plugin_version > FlexibleVersion("1.0.8.112"):
compare_attribute("status_upload_blob")
compare_attribute("status_upload_blob_type")
compare_attribute("required_features")
compare_attribute("on_hold")

Expand Down
21 changes: 17 additions & 4 deletions azurelinuxagent/common/protocol/extensions_goal_state_from_vm_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,17 @@

from azurelinuxagent.common.exception import VmSettingsError
from azurelinuxagent.common.protocol.extensions_goal_state import ExtensionsGoalState
from azurelinuxagent.common.utils.flexible_version import FlexibleVersion
from azurelinuxagent.common.utils.textutil import format_exception


class ExtensionsGoalStateFromVmSettings(ExtensionsGoalState):
def __init__(self, etag, json_text):
super(ExtensionsGoalStateFromVmSettings, self).__init__()
self._id = etag
self._host_ga_plugin_version = None
self._schema_version = None
self._text = json_text
self._host_ga_plugin_version = FlexibleVersion()
self._schema_version = FlexibleVersion()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please help me understand what FlexibleVersion() without any parameter passed returns and what will be the default value of _host_ga_plugin_version?
Had a quick read about FlexibleVersion(version.Version) and looks like it expects a version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.version is intialized to empty and each property defaults to 0, as in

def major(self):
    return self.version[0] if len(self.version) > 0 else 0

self._activity_id = None
self._correlation_id = None
self._created_on_timestamp = None
Expand All @@ -49,6 +50,14 @@ def __init__(self, etag, json_text):
def id(self):
return self._id

@property
def host_ga_plugin_version(self):
return self._host_ga_plugin_version

@property
def schema_version(self):
return self._schema_version

@property
def activity_id(self):
return self._activity_id
Expand Down Expand Up @@ -91,8 +100,12 @@ def _parse_vm_settings(self, json_text):
# TODO: Parse all atttributes

def _parse_simple_attributes(self, vm_settings):
self._host_ga_plugin_version = vm_settings.get("hostGAPluginVersion")
self._schema_version = vm_settings.get("vmSettingsSchemaVersion")
host_ga_plugin_version = vm_settings.get("hostGAPluginVersion")
if host_ga_plugin_version is not None:
self._host_ga_plugin_version = FlexibleVersion(host_ga_plugin_version)
schema_version = vm_settings.get("vmSettingsSchemaVersion")
if schema_version is not None:
self._schema_version = FlexibleVersion(schema_version)
self._activity_id = self._string_to_id(vm_settings.get("activityId"))
self._correlation_id = self._string_to_id(vm_settings.get("correlationId"))
self._created_on_timestamp = self._ticks_to_utc_timestamp(vm_settings.get("extensionsLastModifiedTickCount"))
Expand Down
14 changes: 11 additions & 3 deletions azurelinuxagent/common/protocol/wire.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,9 +1332,17 @@ def get_header_for_cert(self):
def get_host_plugin(self):
if self._host_plugin is None:
goal_state = GoalState(self)
self._set_host_plugin(HostPluginProtocol(self.get_endpoint(),
goal_state.container_id,
goal_state.role_config_name))
self._set_host_plugin(HostPluginProtocol(self.get_endpoint(), goal_state.container_id, goal_state.role_config_name))
try:
etag, vm_settings = self._fetch_vm_settings(None)
extensions_goal_state = ExtensionsGoalStateFactory.create_from_vm_settings(etag, vm_settings)
message = "HostGAPlugin version: {0}".format(extensions_goal_state.host_ga_plugin_version)
logger.info(message)
add_event(op=WALAEventOperation.HostPlugin, message=message, is_success=True)
except Exception as exception:
message = "Failed to determine the HostGAPlugin version. Error: {0}".format(textutil.format_exception(exception))
logger.warn(message)
add_event(op=WALAEventOperation.HostPlugin, message=message, is_success=False, log_event=False)
return self._host_plugin

def get_on_hold(self):
Expand Down
2 changes: 2 additions & 0 deletions tests/data/hostgaplugin/vm_settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
{
"hostGAPluginVersion":"1.0.8.115",
"vmSettingsSchemaVersion":"0.0",
Copy link
Contributor

@kevinclark19a kevinclark19a Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to verify, these fields have always been present in real VmSettings objects, right?

Edit: Or, at least, since host ga plugin v112?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were added post 112, not sure when. We default to 0.0.0.0 if not present

"activityId": "2e7f8b5d-f637-4721-b757-cb190d49b4e9",
"correlationId": "1bef4c48-044e-4225-8f42-1d1eac1eb158",
"extensionsLastModifiedTickCount": 637693267431616449,
Expand Down
27 changes: 27 additions & 0 deletions tests/protocol/test_extensions_goal_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import re

from azurelinuxagent.common.protocol.extensions_goal_state import ExtensionsGoalState, GoalStateMismatchError
from azurelinuxagent.common.utils.flexible_version import FlexibleVersion
from tests.protocol.mocks import mockwiredata, mock_wire_protocol
from tests.tools import AgentTestCase, PropertyMock, patch

Expand All @@ -28,6 +29,32 @@ def test_property(name, value):
test_property("required_features", ['MOCK_REQUIRED_FEATURE'])
test_property("on_hold", True)

def test_compare_should_check_the_status_upload_blob_only_when_the_host_ga_plugin_version_is_greater_then_112(self):
with mock_wire_protocol(mockwiredata.DATA_FILE_VM_SETTINGS) as protocol:
from_extensions_config = protocol.client.get_extensions_goal_state()
from_vm_settings = protocol.client._extensions_goal_state_from_vm_settings

# we set a the status upload value to a dummy value to verify whether the compare() method is checking it or not
from_vm_settings._status_upload_blob = "A DUMMY VALUE FOR THE STATUS UPLOAD BLOB"

#
# 112 and below should not check the status blob
#
from_vm_settings._host_ga_plugin_version = FlexibleVersion("1.0.8.112")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth adding a check where we dont get any HostGA version at all too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't consider it interesting, since flexible version inits to 0


try:
ExtensionsGoalState.compare(from_extensions_config, from_vm_settings)
except GoalStateMismatchError as error:
self.fail("The status upload blob should not have been checked when the Host GA Plugin version is 112 or below (Got error: {0})".format(error))

#
# 113 and above should check the status blob
#
from_vm_settings._host_ga_plugin_version = FlexibleVersion("1.0.8.113")

with self.assertRaisesRegexCM(GoalStateMismatchError, r"Attribute: status_upload_blob"):
ExtensionsGoalState.compare(from_extensions_config, from_vm_settings)

def test_create_from_extensions_config_should_assume_block_when_blob_type_is_not_valid(self):
data_file = mockwiredata.DATA_FILE.copy()
data_file["ext_conf"] = "hostgaplugin/ext_conf-invalid_blob_type.xml"
Expand Down