test: disable container-monitoring tests #2098
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rashmichandrashekar this is the consistent failure we're seeing across all Kubernetes versions: |
|
@jackfrancis : looks like the workspace is removed from the addonconfig. |
Codecov Report
@@ Coverage Diff @@
## master #2098 +/- ##
======================================
Coverage 76.6% 76.6%
======================================
Files 135 135
Lines 20606 20606
======================================
Hits 15786 15786
Misses 3897 3897
Partials 923 923 |
@jackfrancis, current implementation of creating and fetching the workspace id and keys are done as part of the aks-engine deploy command since we need to make network calls . I have spent good amount of time on the aks-engine code to figure out whether we can make network calls during the generate command. From reviewing the code, I didnt see any existing network calls made during the generate command. If you think this possible, we can update the code to support generate code path also otherwise tests related to container monitoring needs to be update to use deploy command path. |
|
@ganga1980 so the changes introduced in #2031 mean that the container-monitoring addon is no longer able to work out of the box unless you create your cluster via |
@jackfrancis, No. it works via aks-engine generate command too but the workspace id and key has to be specified the container monitoring add-on profile in definition file. For the aks-engine deploy command, it works without specifying the workspace id and keys since these will be auto populated by making network call(s). |
|
@ganga1980 this is the cluster config we maintain to test this addon: Could you remark how we can evolve that cluster config to accommodate generate? Thanks! |
@jackfrancis, Sure, let me investigate more on this and see if the generate command (same as deploy) can be simplified to fetch the workspace id and keys automatically for the container monitoring add-on. Me and @rashmichandrashekar investigating current test failures and will get the fix soon if the issue turns to be related to recent my change. |
|
@ganga1980 why not just add the workspace creation into the deployment template? By doing that, one could ensure the workspace exists and is consistent between deploy and generate? |
|
@ganga1980 I agree with your original assessment that fetching stuff over the wire should not happen via generate. So I think we just need to pre-populate the test api model w/ the correct data so we can test this scenario. |
|
Also what @devigned said makes sense. Is there a reason we can't do the workspace retrieval at runtime as the k8s layer bootstraps? |
|
|
@ganga1980 I'm pretty sure you can call list keys in the template and return the workspace key. I might still have a template laying around that does that. I'll look for it. |
@devigned, if we can achieve the fetching keys with the template then I think, we have solution. Appreciate if you can share the template so that I can play with it and see it works for all the scenarios such as new or existing workspace. |
"resources": [
{
"apiVersion": "2017-03-15-preview",
"location": "[parameters('omsWorkspaceRegion')]",
"name": "[parameters('omsWorkspaceName')]",
"type": "Microsoft.OperationalInsights/workspaces",
"comments": "Log Analytics workspace",
"properties": {
"sku": {
"name": "Standard"
}
}
}
],
"outputs": {
"workspaceId": {
"type": "string",
"value": "[reference(resourceId('Microsoft.OperationalInsights/workspaces', parameters('omsWorkspaceName'))).customerId]"
},
"workspacePrimaryKey": {
"type": "string",
"value": "[listKeys(resourceId('Microsoft.OperationalInsights/workspaces', parameters('omsWorkspaceName')), '2017-03-15-preview').primarySharedKey]"
}
}@ganga1980 I believe the above snippet should do what's needed. It'd probably be good to document it somewhere too. It wasn't super easy to figure out. |
@devigned, thanks David for sharing the template snippet. Yes, agree, we should document. Let me play with it and circle back on this. |
|
Fixed in #2109 |
Reason for Change:
Disable container-monitoring E2E tests while test signal is consistently failing.
Issue Fixed:
Requirements:
Notes: