Openshift/azure monitoring cli #10775

troy0820 · 2019-10-08T18:30:07Z

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.

src/azure-cli/azure/cli/command_modules/acs/_help.py

ganga1980 · 2019-10-11T02:40:56Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

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

Will the RP be doing this server side @JackQuincy ?

Yes we are already checking the workspace is valid

@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.

ganga1980 · 2019-10-11T02:47:32Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

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.

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.

@troy0820, ok, thanks.

ganga1980 · 2019-10-11T02:51:42Z

src/azure-cli/azure/cli/command_modules/acs/_help.py

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?

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

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.

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

ganga1980

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

jim-minter · 2019-10-11T15:41:12Z

src/azure-cli/azure/cli/command_modules/acs/_help.py

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.

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.

jim-minter · 2019-10-11T15:42:21Z

src/azure-cli/azure/cli/command_modules/acs/_help.py

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

jim-minter · 2019-10-11T15:44:14Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

Will the RP be doing this server side @JackQuincy ?

jim-minter · 2019-10-11T15:50:30Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

@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.

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.

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

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

JackQuincy · 2019-10-11T20:17:40Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

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

@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.

yonzhan · 2019-10-14T00:44:31Z

@myronfanqiu please take a look at this PR.

troy0820 · 2019-10-16T20:48:32Z

@asalkeld can you take a look?

JackQuincy · 2019-10-16T21:02:09Z

@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?

asalkeld

looks good to me

mmyyrroonn · 2019-10-21T05:56:39Z

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

troy0820 · 2019-10-21T20:02:16Z

@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? 🙏

ehashman · 2019-10-25T15:37:31Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

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.

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

JackQuincy · 2019-11-06T22:45:57Z

LGTM

JackQuincy · 2019-11-06T22:49:23Z

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

mmyyrroonn · 2019-11-08T02:05:58Z

@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.

azure-cli/src/azure-cli/azure/cli/command_modules/vm/custom.py

Lines 992 to 1020 in 7d25cde

    
           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?

src/azure-cli/azure/cli/command_modules/acs/_help.py

troy0820

@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 · 2019-11-11T05:20:33Z

@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.

mmyyrroonn

@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 · 2019-11-12T15:18:26Z

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

profile

Add flag "--workspace-resource-id" to "az openshift create"

troy0820 · 2019-11-12T15:57:05Z

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

ganga1980 reviewed Oct 11, 2019

View reviewed changes

src/azure-cli/azure/cli/command_modules/acs/_help.py Outdated Show resolved Hide resolved

ganga1980 reviewed Oct 11, 2019

View reviewed changes

ganga1980 suggested changes Oct 11, 2019

View reviewed changes

jim-minter suggested changes Oct 11, 2019

View reviewed changes

JackQuincy reviewed Oct 11, 2019

View reviewed changes

troy0820 force-pushed the openshift/azure-monitoring-cli branch 3 times, most recently from de30bc2 to 7d533f2 Compare October 12, 2019 03:13

mmyyrroonn self-requested a review October 12, 2019 05:52

yonzhan assigned mmyyrroonn Oct 14, 2019

asalkeld previously approved these changes Oct 16, 2019

View reviewed changes

troy0820 dismissed asalkeld’s stale review via 55c89a2 October 17, 2019 20:33

ehashman reviewed Oct 25, 2019

View reviewed changes

ganga1980 approved these changes Nov 6, 2019

View reviewed changes

troy0820 force-pushed the openshift/azure-monitoring-cli branch 4 times, most recently from 8a98977 to 26eb736 Compare November 7, 2019 17:37

yonzhan added this to the S161 milestone Nov 8, 2019

yonzhan requested review from Juliehzl and mmyyrroonn November 8, 2019 01:15

olga-mir reviewed Nov 8, 2019

View reviewed changes

src/azure-cli/azure/cli/command_modules/acs/_help.py Outdated Show resolved Hide resolved

troy0820 commented Nov 8, 2019

View reviewed changes

mmyyrroonn approved these changes Nov 12, 2019

View reviewed changes

troy0820 added 3 commits November 12, 2019 10:49

[openshift]Add --loganalytics_workspace_resource_id and monitoring

76b546f

profile

[openshift]Change flag to "--workspace-resource-id"

c82bfe3

[openshift]Add monitoring profile for "az openshift create"

19f6a97

Add flag "--workspace-resource-id" to "az openshift create"

troy0820 force-pushed the openshift/azure-monitoring-cli branch from 2f77e2d to 19f6a97 Compare November 12, 2019 15:53

mmyyrroonn approved these changes Nov 13, 2019

View reviewed changes

mmyyrroonn merged commit ca2f974 into Azure:dev Nov 13, 2019

troy0820 mentioned this pull request Nov 14, 2019

Change flag workspace-resource-id to workspace-resource #11241

Merged

3 tasks

olga-mir mentioned this pull request Jan 20, 2020

[Azure Red Hat OpenShift] Add monitor subgroup to manage Log Analytics monitoring in Azure Red Hat OpensShift cluster #11904

Merged

2 tasks

Openshift/azure monitoring cli #10775

Openshift/azure monitoring cli #10775

Uh oh!

Conversation

troy0820 commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganga1980 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonzhan commented Oct 14, 2019

Uh oh!

troy0820 commented Oct 16, 2019

Uh oh!

JackQuincy commented Oct 16, 2019

Uh oh!

asalkeld left a comment

Choose a reason for hiding this comment

Uh oh!

mmyyrroonn commented Oct 21, 2019

Uh oh!

troy0820 commented Oct 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackQuincy commented Nov 6, 2019

Uh oh!

JackQuincy commented Nov 6, 2019

Uh oh!

mmyyrroonn commented Nov 8, 2019

Uh oh!

Uh oh!

troy0820 left a comment

Choose a reason for hiding this comment

Uh oh!

troy0820 commented Oct 8, 2019 •

edited

Loading