Skip to content

Conversation

@olga-mir
Copy link

@olga-mir olga-mir commented Feb 18, 2020

This is a draft PR to allow early feedback.
This PR updates ARO module to latest API version 2019-10-27-preview. It depends on Azure/azure-rest-api-specs#8305 (in the process of being merged) and the consequent sdk-for-python build.
In the new version vnet-peer has been removed from "create" operation. This is due to latest changes done on Azure side while implementing new API features it is now possible to edit vnet peering after the cluster creation. I have tried deprecation approach (https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#deprecating-commands-and-arguments) but unfortunately in this circumstances we have to remove it now rather than adding a deprecation notice.

History Notes:
[ACS] BREAKING CHANGE: az openshift create: remove --vnet-peer parameter.
[ACS] az openshift create: add flags to support private cluster.
[ACS] az openshift: upgrade to 2019-10-27-preview API version.
[ACS] az openshift: add update command.


This checklist is used to make sure that common guidelines for a pull request are followed.

@yungezz yungezz added the ACS az acs/aks/openshift label Feb 21, 2020
@fengzhou-msft
Copy link
Member

Updated history notes section to conform to the required format.

Comment on lines 976 to 979
Copy link
Member

Choose a reason for hiding this comment

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

You already have the help text in _params.py. You can remove it here.

@kwoodson
Copy link
Contributor

kwoodson commented Mar 9, 2020

@olga-mir Could you please update this with your status? Thanks!

@olga-mir
Copy link
Author

olga-mir commented Mar 9, 2020

hi @kwoodson this can't be merged yet because the core sdk has not been updated with the latest API specs. On the last scrum with MSFT last Friday (6th March) I've been told that there is a conflict with AKS core sdk release/versioning and it's been taking a lot of time to resolve. I will resume working on this task when this has been sorted out

Copy link
Contributor

Choose a reason for hiding this comment

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

Per https://github.com/Azure/sdk-release-request/issues/46 this should be available in the latest release (8.3.0).

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be missing the right models, I've pinged the release manager on the ticket.

@olga-mir
Copy link
Author

waiting on this PR: Azure/azure-sdk-for-python#10332

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a wait command as there are so many support_to_wait commands ?

Copy link
Author

Choose a reason for hiding this comment

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

@arrownj yes we do have wait command:

@olga-mir
Copy link
Author

olga-mir commented Apr 1, 2020

@ehashman @kwoodson sdk has now been updated, I will resume work on this PR soon.
https://github.com/Azure/azure-sdk-for-python/pull/10464/files

@olga-mir
Copy link
Author

olga-mir commented Apr 1, 2020

@julienstroheker I have tested the update command, but in our subscription I am not able to test az openshift create

@olga-mir
Copy link
Author

olga-mir commented Apr 2, 2020

create is not working properly.

I am running
az openshift create -n olga-cli-3 -g olga-cli-3 -l eastus2 --private-cluster --management-subnet-cidr 10.0.1.0/24 --debug

The payload sent is:
{"location": "eastus2", "properties": {"openShiftVersion": "v3.11", "networkProfile": {"vnetCidr": "10.0.0.0/8", "managementSubnetCidr": "10.0.1.0/24"}, "routerProfiles": [{"name": "default"}], "masterPoolProfile": {"count": 3, "vmSize": "Standard_D4s_v3", "subnetCidr": "10.0.0.0/24", "apiProperties": {"privateApiServer": true}}, "agentPoolProfiles": [{"name": "compute", "count": 4, "vmSize": "Standard_D4s_v3", "subnetCidr": "10.0.0.0/24", "osType": "Linux", "role": "compute"}, {"name": "infra", "count": 3, "vmSize": "Standard_D4s_v3", "subnetCidr": "10.0.0.0/24", "osType": "Linux", "role": "infra"}], "authProfile": {"identityProviders": [{"name": "Azure AD", "provider": {"kind": "AADIdentityProvider", "clientId": "<id>", "secret": "<secret>", "tenantId": "<id>"}}]}}}
The payload looks correct, but the resulting cluster is not a private cluster.

@julienstroheker any thoughts?

@olga-mir
Copy link
Author

olga-mir commented Apr 3, 2020

I ran one more check and I want to provide some more detail.

I took the above payload and placed in the script that creates production cluster using rest API call directly.
I ran this PR az openshift create as described above.

Using curl the created cluster does appear to be private, while the one created with cli is not (like I observed yesterday).

In the resource group of the cluster which is created with cli, PLS, PLS NIC and lb-apiserver-internal resources are missing.

When describing both clusters using az openshift show I observed following differences:

"fqdn": "10.0.0.254" vs "osaxxxxxxxxxxxxxxxxxxxx.eastus2.cloudapp.azure.com",
"publicHostname": "10.0.0.254" vs "publicHostname": "openshift.xxxxxxxxxxxxxxxxxxxx.eastus2.azmosa.io",

Because which is produced by this PR is valid, I assume that the problem is with Azure RP.

@olga-mir
Copy link
Author

olga-mir commented Apr 3, 2020

one more thing that I've noticed is that the CLI cluster has 3 deployments on the RG, while cluster created with curl had 4. Missing deployment is pid-<uuid>

@julienstroheker
Copy link
Contributor

Your payload is sent with the wrong API Version 2019-09-30-preview, should be 2019-10-27-preview

This needs to be modified inside the _client_factory.py

You can run any command with the --debug flag in it to have more details such as payload, headers etc... this is very useful when developing.

Thanks

@olga-mir
Copy link
Author

thanks @julienstroheker indeed I've missed the api version string update (bc513c8) I thought models upgrade did the trick.

I have fixed that and tested again the created cluster is a valid cluster and update refresh-cluster also works. I have rebased and tests passed, so marking this ready for review.

@olga-mir olga-mir marked this pull request as ready for review April 21, 2020 05:55
@olga-mir
Copy link
Author

@fengzhou-msft @arrownj can either of you please take another look at this PR, it now looks good from our side.

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

LGTM. And remember to fix the PR title(2019-20-27 -> 2019-10-27).

@olga-mir olga-mir changed the title [ACS] BREAKING CHANGE: ARO remove vnet-peer, upgrade to 2019-20-27-preview API version [ACS] BREAKING CHANGE: ARO remove vnet-peer, upgrade to 2019-10-27-preview API version Apr 22, 2020
@arrownj
Copy link
Contributor

arrownj commented Apr 23, 2020

Hi @fengzhou-msft , could you help take a look at it once more? If no problem, I will merge it.

@arrownj arrownj merged commit 32e5102 into Azure:dev Apr 27, 2020
olga-mir pushed a commit to olga-mir/azure-cli that referenced this pull request May 21, 2020
arrownj pushed a commit that referenced this pull request Jun 8, 2020
* Revert "ACS: remove fields cleanup that no longer exist (#13315)"

This reverts commit cd405fa.

* Revert "[ACS] BREAKING CHANGE: ARO remove vnet-peer, upgrade to 2019-10-27-preview API version (#12240)"

This reverts commit 32e5102.

Co-authored-by: Olga Mirensky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ACS az acs/aks/openshift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants