-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update datasource metadata queries #1322
Update datasource metadata queries #1322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query changes looks good.
As discussed, please update the conditions on when the containers can be null for a workload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have CONTAINER_INFO_QUERY which has both container and workload details I feel it is redundant to run WORKLOAD_INFO_QUERY and also we can avoid running an extra for loop for workload -
for (DataSourceWorkload dataSourceWorkload : dataSourceNamespace.getDataSourceWorkloadHashMap().values()) { |
This can help in reducing the time taken to get the datasource details and also avoids redundancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query changes LGTM!
@shreyabiradar07 Please resolve the conflicts |
@shreyabiradar07 Can you please rebase on top of mvp_demo and test |
dfb7392
to
f31d0d3
Compare
Local monitoring demo response : https://privatebin.corp.redhat.com/?6fb5876b4ae8e63f#AoXBYDV9BTfX15vYDexamQYrnbHpWiMBkuq7BAZzaAzX Encountering the following assertion error for
Failed test logs : https://privatebin.corp.redhat.com/?cd5f4646e2d1f0e3#41XkaVvktuBJXmQDm4F11WTvfPXq4Rh7DRN164LyGUhQ |
@@ -64,7 +64,7 @@ | |||
} | |||
} | |||
}, | |||
"required": ["workload_name", "workload_type", "containers"] | |||
"required": ["workload_name", "workload_type"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is containers removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below assertion error is encountered in edge cases where there are no containers for a job workload type as "containers" is a required field:
assert [<ValidationError: "'containers' is a required property">, <ValidationError: "'containers' is a required property">, <ValidationError: ''>] == ''
Following are the few examples for edge cases observed after updating the datasource metadata queries to gather metric data for last 15 days:
"collect-profiles-28807155": {
"workload_name": "collect-profiles-28807155",
"workload_type": "job"
},
"image-pruner-28802880": {
"workload_name": "image-pruner-28802880",
"workload_type": "job"
}
Failed test logs : https://privatebin.corp.redhat.com/?cd5f4646e2d1f0e3#41XkaVvktuBJXmQDm4F11WTvfPXq4Rh7DRN164LyGUhQ
As suggested created issue #1345 to work on the enhancement of performance and queries, will work on the same in the next release |
Summarizing the steps followed to capture conditions when the containers can be null for a workload. Tried importing metadata for 3 scenarios:
but couldn't capture the case - where containers are null Next steps:
after 15m & 30m job data for both workload and container queries was present container=null was observed specifically for namespace @kusumachalasani pointed out that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR has the following changes:
Updates
NAMESPACE_QUERY
,WORKLOAD_INFO_QUERY
,CONTAINER_INFO_QUERY
dsmetadata queries to provide the average namespace, workload and container status over the past 15 days.Also includes
namespace
label inCONTAINER_INFO
query to perform the join operationon (pod, namespace)
with the pod to handle the case where same workloads are present in different namespaces.Handles an edge case - wherein for workload type
job
with no containers was earlier resulting inNullPointerException
, which is now fixed.Fixes # (1316)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Tested by running local monitoring demos on ResourceHub cluster - ./local_monitoring_demo.sh -c openshift -i quay.io/shbirada/update_dsmetadata_queries:v1
Fixed the NullPointerException seen before:
pod logs after handling the edge case: https://privatebin.corp.redhat.com/?87922ed8fdb9df55#D6FBWxRYbKHTYEsTEcts1D3aNGtmb3jgEe3eXhf5FU2F
pod logs with Exception - https://privatebin.corp.redhat.com/?527498b097f364ac#F93EzwiZG121QPcc73FKk8NU9bCTqzLMgSsbPsHuoFVS
Test Configuration
Checklist 🎯
Additional information
docker image -
quay.io/shbirada/update_dsmetadata_queries:v1