Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Enable running custom images in acs-engine #2513

Merged
merged 2 commits into from
Apr 4, 2018
Merged

Enable running custom images in acs-engine #2513

merged 2 commits into from
Apr 4, 2018

Conversation

0xmichalis
Copy link
Contributor

@khenidak this is the fix we talked yesterday about enabling us to run custom images in acs-engine. Any pointers on adding an e2e test that validates generation would be appreciated.

cc @jim-minter @pweil-

@jackfrancis
Copy link
Member

@Kargakis I have rebased this locally after cloning your fork. Permission to force push? ( if you have local changes that haven't been pushed up, say no :) )

@0xmichalis
Copy link
Contributor Author

@Kargakis I have rebased this locally after cloning your fork. Permission to force push? ( if you have local changes that haven't been pushed up, say no :) )

@jackfrancis I rebased my branch if that's all you wanted. My editor messes up the satori import.. Other than that, I have no local changes - feel free to do what you need to do.

@jackfrancis
Copy link
Member

@Kargakis I'll force push w/ rebase from recent master, stand by

@ghost ghost assigned jackfrancis Mar 23, 2018
@ghost ghost added the in progress label Mar 23, 2018
@0xmichalis
Copy link
Contributor Author

I just verified that ARM does not allow data disks when an image resource ID is provided.

Cannot specify user image overrides for a disk already defined in the specified image reference.

I will update the PR shortly to exclude them.

@jim-minter @pschiffe after submitting this PR I realized it's not only going to be useful for playing around with acs-engine but will be likely how we do PR testing in OpenShift.

@seanknox
Copy link
Contributor

Thanks for this @Kargakis, this will help with a use case I need, too!

@seanknox
Copy link
Contributor

@Kargakis could you include an example of how to use this, either an API model (see examples/) and/or update documentation?

Copy link
Collaborator

@pweil- pweil- left a comment

Choose a reason for hiding this comment

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

tests and improved error lgtm. I think we're just needing an examples/ file based on current feedback.

@0xmichalis
Copy link
Contributor Author

A question to folks, the example I am adding is going to work for generate but not for deploy. Is that OK? I would guess so.

@0xmichalis
Copy link
Contributor Author

A question to folks, the example I am adding is going to work for generate but not for deploy. Is that OK? I would guess so.

Seems that some examples don't work ootb even for generate. Anyway, example added. PTAL

@jackfrancis
Copy link
Member

@Kargakis I assume we intend for these image references to be available from the public internet? Are you able to publish a known-working prototype that we can test this cluster configuration workflow against?

"ssh": {
"publicKeys": [
{
"keyData": "mykeyData"
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's keep these boilerplate non-vals empty strings in the example api model (ditto dnsPrefix above and clientId/secret below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -311,6 +311,8 @@ type MasterProfile struct {
Extensions []Extension `json:"extensions"`
Distro Distro `json:"distro,omitempty"`
KubernetesConfig *KubernetesConfig `json:"kubernetesConfig,omitempty"`
ImageName string `json:"imageName,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Did we consider delivering a struct to represent these properties instead of this flat structure? Instinctually, the advantage of that is the ability to scale out in the future without expanding the overhead of the parent struct, and easier re-use among typed API versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@jackfrancis
Copy link
Member

Added some remarks, this looks close, eager to test this soon! Thanks!

@0xmichalis
Copy link
Contributor Author

@Kargakis I assume we intend for these image references to be available from the public internet? Are you able to publish a known-working prototype that we can test this cluster configuration workflow against?

If an image is public then can't we just use the pre-existing fields? I don't think I can publish an image atm (@kwoodson is going through this process right now, he can share how long does it take). This is important for unpublished images I think and will be likely used by our CI process since we are going to be building images and deploying them via acs-engine on every PR that lands in our repo.

@kwoodson
Copy link
Contributor

If an image is public then can't we just use the pre-existing fields? I don't think I can publish an image atm (@kwoodson is going through this process right now, he can share how long does it take). This is important for unpublished images I think and will be likely used by our CI process since we are going to be building images and deploying them via acs-engine on every PR that lands in our repo.

@Kargakis @jackfrancis
The process to build an image is pretty straight forward and takes < 15 minutes. The publishing which allows separate subscriptions to try the image takes much longer. Inside of the cloud partner portal there is a form which requires information regarding the image. OS type, description, references to the vhds from a storage blob, and more. Once all of this information is set correctly, one can publish an image. The current MS process that is followed takes anywhere from 2-7 days. This involves testing the image, propagating the image to all regions, some certification process, etc.

We will be producing an origin image in the very near future. Hope this helps.

@jackfrancis
Copy link
Member

Thanks @Kargakis! Running E2E now.

@jackfrancis
Copy link
Member

This generally LGTM and back-compat tests are showing green. Is the inclusion of changes to agentPoolOnlyApi intentional at this stage?

@0xmichalis
Copy link
Contributor Author

Initially I included the changes to it for consistency. Is it used only on the hosted master deployment mode? I feel we are going to need it eventually for testing but can leave it out for now if desired.

@jackfrancis
Copy link
Member

@Kargakis Let's leave it out for now if it's not immediately operable, because its inclusion will (reasonably) engage an add'l review/validation process. Make sense to you?

@0xmichalis
Copy link
Contributor Author

@jackfrancis fine by me, done+squashed.

@jackfrancis
Copy link
Member

@Kargakis Added basic docs, feel free to validate. Will run E2E and then lgtm

@jim-minter
Copy link
Member

@jackfrancis docs look fine to me, thanks for adding these. Assuming tests pass, please merge.

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @Kargakis!

@jackfrancis jackfrancis merged commit d5ec309 into Azure:master Apr 4, 2018
@ghost ghost removed the in progress label Apr 4, 2018
@0xmichalis 0xmichalis deleted the enable-using-private-images branch April 5, 2018 07:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants