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

Added Compute Instance OS Patching Properties #20284

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Added Compute Instance OS Patching Properties #20284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6bae1c
Added CI Os Patching related properties
Aug 18, 2022
10f2410
Updated descriptions as per Dennis's suggestions
Aug 19, 2022
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
5 changes: 5 additions & 0 deletions ...hineLearningServices/preview/2022-10-01-preview/examples/Compute/get/ComputeInstance.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@
"adminUserName": "azureuser",
"sshPort": 22
},
"osImageMetadata": {
"currentImageVersion": "22.06.14",
"latestImageVersion": "22.07.22",
"isLatestOsImageVersion": false
Copy link

Choose a reason for hiding this comment

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

Let's drop Os for consistency with other properties from isLatestOsImageVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't be possible as contract already changed in backend layer.

},
"customServices": [
{
"name": "rstudio-workbench",
Expand Down
23 changes: 23 additions & 0 deletions ...Microsoft.MachineLearningServices/preview/2022-10-01-preview/machineLearningServices.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3719,6 +3719,11 @@
"name"
]
},
"osImageMetadata": {
"readOnly": true,
"description": "Returns metadata about the operating system image for this compute instance.",
"$ref": "#/definitions/ImageMetadata"
},
"connectivityEndpoints": {
"readOnly": true,
"description": "Describes all connectivity endpoints available for this ComputeInstance.",
Expand Down Expand Up @@ -4851,6 +4856,24 @@
],
"type": "object"
},
"ImageMetadata": {
"type": "object",
"description": "Returns metadata about the operating system image for this compute instance.",
"properties": {
"currentImageVersion": {
"type": "string",
"description": "Specifies the current operating system image version this compute instance is running on."
},
"latestImageVersion": {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this and currentImageVersion be readOnly?

Copy link

Choose a reason for hiding this comment

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

We've got a partner request from Defender for Cloud to make this one patchable. ReadOnly should be good for now, but in case we want to make it patchable in the future, would that be possible later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

top level property "osImageMetadata" referring to ImageMetadata is itself a read only property.. Shouldn't that suffice

Copy link

Choose a reason for hiding this comment

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

Let's keep it read-only per discussion.

"type": "string",
"description": "Specifies the latest available operating system image version."
},
"isLatestOsImageVersion": {
Copy link

Choose a reason for hiding this comment

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

Let's drop Os for consistency with the other properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't be possible as contract already changed in backend layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

typically, backend implementation shouldn't be a reason to worsen our customer-facing contract definitions

Copy link
Contributor

@forteddyt forteddyt Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

also... is this just basically displaying currentImageVersion==latestImageVersion...?

Copy link

Choose a reason for hiding this comment

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

That is correct. One of the goals it that we will be able to track compliance of compute instance states using Azure Policy. Azure Policies can be defined over boolean properties, but comparing and looking up two values for current==latest seems not an option syntactically.

"type": "boolean",
Copy link
Contributor

Choose a reason for hiding this comment

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

ARM guidelines (https://armwiki.azurewebsites.net/rp_onboarding/process/api_review_best_practices.html) strongly suggest AGAINST booleans, saying:

A Boolean will forever have two valid values (true or false). A string enum type is always preferred. Also, properties should always provide better values just than True and False. For example two switches "isTypeA" and "isTypeB" should be replaced with one enum "type": [A, B, DefaultType]. Even if you still believe [True, False] are the correct values for a property, you should use a string enum with values [True, False] instead of boolean. Enums are always a more flexible and future proof option because they allow additional values to be added in the future in a non-breaking way, e.g. [True, False, Unknown].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case this variable is set as per currentVersion==latestVersion.

Copy link

Choose a reason for hiding this comment

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

Per discussion, we can make this an Enum with [True, False] to represent minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this require extensive backend change and we don't see changing this variable in future. In future we might consider adding another variable that give more extensive information.
As of now variable name together with boolean value seems good. Moreover this is just in public preview.
By the time we move to GA we would be in a better state on a need of enum or not.

We therefore plan to keep it as is.

Copy link

Choose a reason for hiding this comment

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

Please do consider this when possible, I'm going to sign off for now.

"description": "Specifies whether this compute instance is running on the latest operating system image."
}
}
},
"CustomService": {
"type": "object",
"description": "Specifies the custom service configuration",
Expand Down