-
Notifications
You must be signed in to change notification settings - Fork 558
Added Windows Custom Image. Only works in dcos, but can be added to k8s #2004
Conversation
59c9a87
to
9536865
Compare
parts/agentparams.t
Outdated
@@ -52,6 +52,13 @@ | |||
}, | |||
"type": "string" | |||
}, | |||
"{{.Name}}WindowsImageSourceUrl": { | |||
"defaultValue": "", |
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.
reference the assigned value. Looks at examples from Subnet or VMSize
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.
}, | ||
"agentWindowsSourceUrl": { |
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 is this different from WindowsImageSourceUrl?
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.
For now, it isn't except its in variables() rather than parameters().
AdminUsername string `json:"adminUsername"` | ||
AdminPassword string `json:"adminPassword"` | ||
ImageVersion string `json:"imageVersion"` | ||
WindowsImageSourceURL string `json:"windowsImageSourceURL"` |
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 would recommend apply to whole cluster (all agent pools). or per agent pool, but not both. Which matches your scenario better?
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.
Whole cluster. I beleive that is doing that. Am I missing something?
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 "whole cluster" is your scenario, I think you can completely remove the per node pool change you made in parts/agentparams.t.
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've recommended some changes. Since this is only DCOS, please add validation to fail if this options was specified for Kubernetes.
@yakman2020 this looks good to go except for the cruft in |
Also, to address @anhowe's comment, please add something like this to
|
2f784ed
to
ac82598
Compare
validation added. |
ac82598
to
946a91d
Compare
946a91d
to
aa2f90f
Compare
@anhowe Added validation, removed per host custom image name. |
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.
lgtm
What this PR does / why we need it:
Allows the use of custom windows system disk images in the VLabs API deployments. This in turn allows us to use confidential daily build, or specially provisioned images. This is implemented completely for DC/OS. Someone in the k8s team will probably want to apply the changes the k8s parts.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
While this is implemented completely for DC/OS, it modifies the WindowsProfile so should be usable in any of the different code legs, such as k8s or docker swarm with fairly simple mods.
Release note: