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.

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

Openshift/azure monitoring cli #10775

Merged
merged 3 commits into from Nov 13, 2019

Conversation

@troy0820
Copy link
Contributor

troy0820 commented Oct 8, 2019

  • Import OpenShiftManagedClusterMonitorProfile
  • Create help example for openshift create to be used with --workspace_resource_id
  • Add monitor_profile to OpenShiftManagedCluster

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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

workspace_resource_id = '/' + workspace_resource_id
if workspace_resource_id.endswith('/'):
workspace_resource_id = workspace_resource_id.rstrip('/')
monitor_profile = OpenShiftManagedClusterMonitorProfile(enabled=True, workspace_resource_id=workspace_resource_id)

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Oct 11, 2019

Contributor

We should also check the provided workspace_resource_id is valid and exists. I have plan to do the same for AKS as well.

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

Will the RP be doing this server side @JackQuincy ?

This comment has been minimized.

Copy link
@JackQuincy

JackQuincy Oct 11, 2019

Contributor

Yes we are already checking the workspace is valid

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Oct 21, 2019

Contributor

@jim-minter , doing at client side would help user not running into cluster creation failure because of this. This is we learned from AKS experience.

@@ -2950,6 +2952,16 @@ def openshift_create(cmd, client, resource_group_name, name, # pylint: disable=
namespace='Microsoft.Network', type='virtualNetwork',
name=vnet_peer
)
if workspace_resource_id is not None:

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Oct 11, 2019

Contributor

In AKS, we create default Azure Log Analytics workspace if the customer passed on the "monitoring" addon and with no workspace-resource-id. Looks like in ARO case, there is no monitor addon concept and enabling monitoring governed by --workspace-resource-id parameter. is that, right? or we will be going with the same behavior as AKS i.e. creating default log analytics workspace when customer provided no workspace-resource-id parameter.

This comment has been minimized.

Copy link
@troy0820

troy0820 Oct 11, 2019

Author Contributor

ARO doesn't have a concept of add-ons at the moment. I do not believe we want to create a default log analytics workspace if they do not pass in the workspace-resource-id. We will not enable it if it's not passed in.

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Oct 21, 2019

Contributor

@troy0820, ok, thanks.

@@ -748,6 +751,8 @@
text: az openshift create -g MyResourceGroup -n MyManagedCluster --aad-client-app-id {APP_ID} --aad-client-app-secret {APP_SECRET} --aad-tenant-id {TENANT_ID} --compute-count 5
- name: Create an Openshift cluster using a custom vnet
text: az openshift create -g MyResourceGroup -n MyManagedCluster --vnet-peer "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/openshift-vnet/providers/Microsoft.Network/virtualNetworks/test"
- name: Create an Openshift cluster with Log Analytics monitoring enabled
text: az openshift create -g MyResourceGroup -n MyManagedCluster --workspace-resource-id {WORKSPACE_ID}
"""

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Oct 11, 2019

Contributor

From reviewing the code, I understand that, customer can only add the monitoring as part of cluster creation. what about the scenario to add monitoring for existing ARO cluster?

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

I understand the RP will support changing the value - @troy0820 the az client will need to as well.

This comment has been minimized.

Copy link
@troy0820

troy0820 Oct 11, 2019

Author Contributor

That is correct. I think that scenario to add monitoring to an existing cluster is valid and I am open up to feedback on how AKS does this.

This comment has been minimized.

Copy link
@JackQuincy

JackQuincy Oct 11, 2019

Contributor

I have tested on update if you add the monitor profile, or have one and enable it, it will add the agent to the cluster and logs will flow. Simarily you can change the workspace and the agents will be updated and logs will flow to the new namespace

Copy link
Contributor

ganga1980 left a comment

Reviewed and left the comments. Please have a look at it and let me know if need any additional details on these.

@@ -737,6 +737,9 @@
- name: --customer-admin-group-id
type: string
short-summary: The Object ID of an Azure Active Directory Group that memberships will get synced into the OpenShift group "osa-customer-admins". If not specified, no cluster admin access will be granted.
- name: --workspace-resource-id

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

would something like --loganalytics-workspace-resource-id be better here? I know it's a lot to type. Don't mind whether it's loganalytics or monitoring or some other name.

This comment has been minimized.

Copy link
@troy0820

troy0820 Oct 11, 2019

Author Contributor

To reduce complexity, I'm okay with the long --loganalytics-workspace-resource-id name. I can change it to that to provide a concise naming convention.

@@ -748,6 +751,8 @@
text: az openshift create -g MyResourceGroup -n MyManagedCluster --aad-client-app-id {APP_ID} --aad-client-app-secret {APP_SECRET} --aad-tenant-id {TENANT_ID} --compute-count 5
- name: Create an Openshift cluster using a custom vnet
text: az openshift create -g MyResourceGroup -n MyManagedCluster --vnet-peer "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/openshift-vnet/providers/Microsoft.Network/virtualNetworks/test"
- name: Create an Openshift cluster with Log Analytics monitoring enabled
text: az openshift create -g MyResourceGroup -n MyManagedCluster --workspace-resource-id {WORKSPACE_ID}
"""

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

I understand the RP will support changing the value - @troy0820 the az client will need to as well.

workspace_resource_id = '/' + workspace_resource_id
if workspace_resource_id.endswith('/'):
workspace_resource_id = workspace_resource_id.rstrip('/')
monitor_profile = OpenShiftManagedClusterMonitorProfile(enabled=True, workspace_resource_id=workspace_resource_id)

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

Will the RP be doing this server side @JackQuincy ?

workspace_resource_id = '/' + workspace_resource_id
if workspace_resource_id.endswith('/'):
workspace_resource_id = workspace_resource_id.rstrip('/')
monitor_profile = OpenShiftManagedClusterMonitorProfile(enabled=True, workspace_resource_id=workspace_resource_id)

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 11, 2019

@troy0820 I don't think it's quite as simple as this. You can't just graft a struct from the preview API onto the released API - Azure won't recognise it because the request API version will be old. If loganalytics is requested az will need to send a preview API request to the Azure API. It will also have to handle receiving a preview API response from the Azure API.

If loganalytics is not requested, in some ways it doesn't matter which API version is sent to the Azure API. It is possibly safer (although more work) to send a request using the released API, however. Check with MSFT and check to see what the codebase does in this regard for other preview APIs.

I think I'd expect to see:

  • if az parses server response, it can handle responses in either API version
  • az creates request using released API, then, if loganalytics is requested, it converts that to the preview API and adds the additional monitorProfile.

This comment has been minimized.

Copy link
@JackQuincy

JackQuincy Oct 11, 2019

Contributor

The way the cli is designed all az openshift calls have to have the same api version which should move to the preview version. The customer can override this value. We are running all tests on the preview version and it is passing.

This comment has been minimized.

Copy link
@jim-minter

jim-minter Oct 22, 2019

@JackQuincy I don't quite get it - does that mean the PR can't merge until the new API is GA?

This comment has been minimized.

Copy link
@JackQuincy

JackQuincy Oct 23, 2019

Contributor

No we have used preview versions in az in the past. We more have preview features. But the rest of the api should be unaffected

if not loganalytics_workspace_resource_id.startswith('/'):
loganalytics_workspace_resource_id = '/' + loganalytics_workspace_resource_id
if loganalytics_workspace_resource_id.endswith('/'):
loganalytics_workspace_resource_id = loganalytics_workspace_resource_id.rstrip('/')

This comment has been minimized.

Copy link
@JackQuincy

JackQuincy Oct 11, 2019

Contributor

@jim-minter currently the RP is not doing this logic. We will fail the call if it has a trailing '/'

This comment has been minimized.

Copy link
@ganga1980

ganga1980 Nov 8, 2019

Contributor

@JackQuincy , we are good on this since the code in this PR takes care of the scenario which you have mentioned. so can you please resolve/close this comment.

@troy0820 troy0820 force-pushed the troy0820:openshift/azure-monitoring-cli branch 3 times, most recently from de30bc2 to 7d533f2 Oct 12, 2019
@MyronFanQiu MyronFanQiu self-requested a review Oct 12, 2019
@yonzhan

This comment has been minimized.

Copy link
Contributor

yonzhan commented Oct 14, 2019

@MyronFanQiu please take a look at this PR.

@troy0820

This comment has been minimized.

Copy link
Contributor Author

troy0820 commented Oct 16, 2019

@asalkeld can you take a look?

@JackQuincy

This comment has been minimized.

Copy link
Contributor

JackQuincy commented Oct 16, 2019

@troy0820 these changes are in west central us. We have been blocked due to dependency bugs. but you should be able to test there now. Can you provision a cluster there using a local build of your repo?

Copy link

asalkeld left a comment

looks good to me

@MyronFanQiu

This comment has been minimized.

Copy link
Member

MyronFanQiu commented Oct 21, 2019

Hello. I'm working on the CLI monitor. How about supporting both workspace_name and workspace_id by exposing --workspace to user?

@troy0820

This comment has been minimized.

Copy link
Contributor Author

troy0820 commented Oct 21, 2019

@MyronFanQiu I was trying to mirror what AKS does. I'm not sure if they use the workspace_name for workspace_resource_id in AKS. Also I was passing this in as I'm understanding the RP will handle the parsing logic for the loganalytics_workspace_resource_id

@ehashman can you TAL at this? 🙏

master_pool_profile=agent_master_pool_profile,
router_profiles=[default_router_profile])
if monitor_profile is not None:
osamc = OpenShiftManagedCluster(

This comment has been minimized.

Copy link
@ehashman

ehashman Oct 25, 2019

Member

Looking above...

from azure.mgmt.containerservice.v2019_04_30.models import OpenShiftManagedCluster

These are both referring to the v2019_04_30 model, we need to import the v2019_09_30_preview for the Azure Monitor case.

This comment has been minimized.

Copy link
@troy0820

troy0820 Oct 27, 2019

Author Contributor

😬 How did I miss that 🤦‍♂ ? Updated with an alias.

@JackQuincy

This comment has been minimized.

Copy link
Contributor

JackQuincy commented Nov 6, 2019

LGTM

@JackQuincy

This comment has been minimized.

Copy link
Contributor

JackQuincy commented Nov 6, 2019

Also update is the changes are out globally in the service.

@troy0820 troy0820 force-pushed the troy0820:openshift/azure-monitoring-cli branch 4 times, most recently from 8a98977 to 26eb736 Nov 7, 2019
@yonzhan yonzhan added this to the S161 milestone Nov 8, 2019
@yonzhan yonzhan requested review from MyronFanQiu and Juliehzl Nov 8, 2019
@MyronFanQiu

This comment has been minimized.

Copy link
Member

MyronFanQiu commented Nov 8, 2019

@troy0820 Hello. Sorry for the delay due the Ignite. I have three questions about this PR.

  • Could you create a support a workspace name in the same RG or creating a default workspace for user in the same RG? I think it will be easy for user to use. We can discuss about this one since I started to support workspace for several other service.
    def _prepare_workspace(cmd, resource_group_name, workspace):
    from msrestazure.tools import is_valid_resource_id
    from ._client_factory import cf_log_analytics
    from azure.cli.core.commands.client_factory import get_subscription_id
    from msrestazure.azure_exceptions import CloudError
    workspace_id = None
    if not is_valid_resource_id(workspace):
    workspace_name = workspace
    subscription_id = get_subscription_id(cmd.cli_ctx)
    log_client = cf_log_analytics(cmd.cli_ctx, subscription_id)
    workspace_result = None
    try:
    workspace_result = log_client.get(resource_group_name, workspace_name)
    except CloudError:
    from azure.mgmt.loganalytics.models import Workspace, Sku, SkuNameEnum
    sku = Sku(name=SkuNameEnum.per_gb2018.value)
    retention_time = 30 # default value
    location = _get_resource_group_location(cmd.cli_ctx, resource_group_name)
    workspace_instance = Workspace(location=location,
    sku=sku,
    retention_in_days=retention_time)
    workspace_result = LongRunningOperation(cmd.cli_ctx)(log_client.create_or_update(resource_group_name,
    workspace_name,
    workspace_instance))
    workspace_id = workspace_result.id
    else:
    workspace_id = workspace
    return workspace_id
  • --loganalytics-workspace-resource-id is quite long.... I know that there is another workspace id. Do we really need the word loganalytics
  • Update command doesn't support --loganalytics-workspace-resource-id. Based on your discussion, it should support. Did I miss something?
Copy link
Contributor Author

troy0820 left a comment

@MyronFanQiu : Could you create a support a workspace name in the same RG or creating a default workspace for user in the same RG? I think it will be easy for user to use. We can discuss about this one since I started to support workspace for several other service.

I believe when drafting this PR we agreed that we were not going to make a default workspace if one didn't exist. cc: @jim-minter

@MyronFanQiu: --loganalytics-workspace-resource-id is quite long.... I know that there is another workspace id. Do we really need the word loganalytics

I agree it is definitely long. I can change it to something @jim-minter suggested earlier in this thread (loganalytics or monitoring) but we went with this decision with the comments @ganga1980 mentioned as well. I think the concern is that the user would place a WorkspaceID (GUID) instead of the WorkspaceResourceID in this field and create issues for them to provision a cluster. I believe this decision was predicated on these assumptions but can be shorted to improve the user's experience.

@MyronFanQiu Update command doesn't support --loganalytics-workspace-resource-id. Based on your discussion, it should support. Did I miss something?

Currently, ARO doesn't have an update command and creating one is work subsequent to this PR. I believe we will mirror what AKS does with updating the cluster. I believe we still have to support two different api models because of the monitor profile being in 2019_preview and hide that from the cli user. Additionally, decisions around the design of the update command have not been finalized around what that will include for ARO clusters.

@amanohar

This comment has been minimized.

Copy link

amanohar commented Nov 11, 2019

@troy0820 we don't need to mirror AKS for update command. Let's discuss it before finalizing any changes.

--workspace-resource-id or -workspace-id sounds good to me.

Copy link
Member

MyronFanQiu left a comment

@troy0820 Thanks! Since you confirmed that you don't want to create a default workspace for users and currently we don't need to update the workspace-resource-id. I'm fine with this PR. BTW, could you update the azure-cli/History.rst also?

@troy0820

This comment has been minimized.

Copy link
Contributor Author

troy0820 commented Nov 12, 2019

@MyronFanQiu I can update the azure-cli/History.rst as well.

troy0820 added 3 commits Nov 7, 2019
profile
Add flag "--workspace-resource-id" to "az openshift create"
@troy0820 troy0820 force-pushed the troy0820:openshift/azure-monitoring-cli branch from 2f77e2d to 19f6a97 Nov 12, 2019
@troy0820

This comment has been minimized.

Copy link
Contributor Author

troy0820 commented Nov 12, 2019

Updated azure-cli/History.rst @Juliehzl @MyronFanQiu

@MyronFanQiu MyronFanQiu merged commit ca2f974 into Azure:dev Nov 13, 2019
20 checks passed
20 checks passed
Azure.azure-cli Build #20191112.7 succeeded
Details
Azure.azure-cli (Build Docker Image) Build Docker Image succeeded
Details
Azure.azure-cli (Build Python Wheels) Build Python Wheels succeeded
Details
Azure.azure-cli (Build Windows MSI) Build Windows MSI succeeded
Details
Azure.azure-cli (Check CLI Linter) Check CLI Linter succeeded
Details
Azure.azure-cli (Check CLI Style) Check CLI Style succeeded
Details
Azure.azure-cli (Check License, History, and DocMap) Check License, History, and DocMap succeeded
Details
Azure.azure-cli (Check Module Load Performance) Check Module Load Performance succeeded
Details
Azure.azure-cli (Extract Metadata) Extract Metadata succeeded
Details
Azure.azure-cli (Run Automation Reduced) Run Automation Reduced succeeded
Details
Azure.azure-cli (Run Automation, Profile 2018-03-01) Run Automation, Profile 2018-03-01 succeeded
Details
Azure.azure-cli (Run Automation, Python 2) Run Automation, Python 2 succeeded
Details
Azure.azure-cli (Run Automation, Python 3) Run Automation, Python 3 succeeded
Details
Azure.azure-cli (Test Docker Image) Test Docker Image succeeded
Details
Azure.azure-cli (Test Python Wheels) Test Python Wheels succeeded
Details
Azure.azure-cli (Verify src/azure-cli/requirements.*.Darwin.txt) Verify src/azure-cli/requirements.*.Darwin.txt succeeded
Details
Azure.azure-cli (Verify src/azure-cli/requirements.*.Linux.txt) Verify src/azure-cli/requirements.*.Linux.txt succeeded
Details
Azure.azure-cli (Verify src/azure-cli/requirements.*.Windows.txt) Verify src/azure-cli/requirements.*.Windows.txt succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.