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.

Sign up for GitHub

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

Jump to bottom

thread_lock added to all PrometheusAPI calls #8553

Merged
merged 3 commits into from
Oct 19, 2023
Merged

thread_lock added to all PrometheusAPI calls #8553

merged 3 commits into from
Oct 19, 2023

Conversation

DanielOsypenko
Copy link
Contributor

@DanielOsypenko DanielOsypenko commented Sep 21, 2023 •
edited
Loading

To avoid concurrency issues when oc cmd called same time when PrometheusAPI constructor logs in via oc we need to pass threading_lock every PrometheusAPI call, otherwise unauthorized user issues and other not expected issues may happen.

Fixes #4621

@DanielOsypenko DanielOsypenko self-assigned this Sep 21, 2023
@DanielOsypenko DanielOsypenko requested review from a team as code owners September 21, 2023 17:39
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines label Sep 21, 2023
ocs-ci
ocs-ci reviewed Sep 22, 2023
View reviewed changes
Copy link

@ocs-ci ocs-ci left a comment

Choose a reason for hiding this comment

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

PR validation on existing cluster

Cluster Name: dosypenk-209
Cluster Configuration:
PR Test Suite: tier1
PR Test Path: tests/
Additional Test Params:
OCP VERSION: 4.14
OCS VERSION: 4.14
tested against branch: master

Job UNSTABLE (some or all tests failed).

fbalak
fbalak previously approved these changes Sep 22, 2023
View reviewed changes
@openshift-ci openshift-ci bot added the lgtm label Sep 22, 2023
@DanielOsypenko
Copy link
Contributor Author

DanielOsypenko commented Sep 24, 2023 •
edited
Loading

tier1 build vSphere - https://ocs4-jenkins-csb-odf-qe.apps.ocp-c1.prod.psi.redhat.com/job/qe-deploy-ocs-cluster/29691/ --- one threading lock issue with 205 tests ran (193 passed, 43 skipped, 12 failed, 6 errors)
error addressed in the next commit

@openshift-ci openshift-ci bot removed the lgtm label Sep 24, 2023
@DanielOsypenko DanielOsypenko mentioned this pull request Sep 26, 2023
Closed
ocs-ci
ocs-ci reviewed Sep 26, 2023
View reviewed changes
Copy link

@ocs-ci ocs-ci left a comment

Choose a reason for hiding this comment

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

PR validation on existing cluster

Cluster Name: dosypenk-249
Cluster Configuration:
PR Test Suite: tier1
PR Test Path: tests/manage/monitoring/test_workload_fixture.py::test_workload_rbd_cephfs_10g
Additional Test Params:
OCP VERSION: 4.14
OCS VERSION: 4.14
tested against branch: master

Job UNSTABLE (some or all tests failed).

@DanielOsypenko
Copy link
Contributor Author

thread_lock added to all PrometheusAPI calls

found that all tests using workload_fio_storageutilization where missing thread_lock. added with last commit

ocs-ci
ocs-ci reviewed Sep 26, 2023
View reviewed changes
Copy link

@ocs-ci ocs-ci left a comment

Choose a reason for hiding this comment

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

PR validation on existing cluster

Cluster Name: dosypenk-249
Cluster Configuration:
PR Test Suite: tier1
PR Test Path: tests/manage/monitoring/test_workload_fixture.py::test_workload_rbd_cephfs_10g
Additional Test Params:
OCP VERSION: 4.14
OCS VERSION: 4.14
tested against branch: master

Job PASSED.

ebondare
ebondare previously approved these changes Sep 29, 2023
View reviewed changes
@openshift-ci openshift-ci bot added the lgtm label Sep 29, 2023
@DanielOsypenko
Copy link
Contributor Author

@ebenahar @PrasadDesala @jilju @sidhant-agrawal @fbalak @paraggit @prsurve @ramkiperiy
Please check this PR.
PR may resolve some odd issues we get across regression.

fbalak
fbalak previously approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@fbalak fbalak left a comment

Choose a reason for hiding this comment

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

lgtm

@DanielOsypenko
thread_lock added to all PrometheusAPI calls
ea7f4a6 
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko
fix logical mistake
ec001f2 
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko
workload_fixture__thread_safe
1c1203d 
Signed-off-by: Daniel Osypenko <dosypenk@redhat.com>
@DanielOsypenko DanielOsypenko dismissed stale reviews from fbalak and ebondare via 1c1203d October 10, 2023 07:47
@openshift-ci openshift-ci bot removed the lgtm label Oct 10, 2023
@openshift-ci openshift-ci bot added the lgtm label Oct 10, 2023
sagihirshfeld
sagihirshfeld approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@sagihirshfeld sagihirshfeld left a comment

Choose a reason for hiding this comment

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

LGTM

yitzhak12
yitzhak12 approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@yitzhak12 yitzhak12 left a comment

Choose a reason for hiding this comment

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

Looks good

@DanielOsypenko DanielOsypenko added the Verified Mark when PR was verified and log provided label Oct 18, 2023
@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DanielOsypenko, ebenahar, ebondare, fbalak, sagihirshfeld, sidhant-agrawal, vkathole, yitzhak12

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ebenahar ebenahar merged commit 79f00fc into red-hat-storage:master Oct 19, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocs-ci ocs-ci ocs-ci left review comments

@ebenahar ebenahar ebenahar approved these changes

@yitzhak12 yitzhak12 yitzhak12 approved these changes

@sidhant-agrawal sidhant-agrawal sidhant-agrawal approved these changes

@fbalak fbalak fbalak approved these changes

@sagihirshfeld sagihirshfeld sagihirshfeld approved these changes

@ebondare ebondare ebondare approved these changes

@vkathole vkathole vkathole approved these changes

@petr-balogh petr-balogh Awaiting requested review from petr-balogh

@dahorak dahorak Awaiting requested review from dahorak

@jilju jilju Awaiting requested review from jilju

@PrasadDesala PrasadDesala Awaiting requested review from PrasadDesala

@OdedViner OdedViner Awaiting requested review from OdedViner

@Akarsha-rai Akarsha-rai Awaiting requested review from Akarsha-rai

@hnallurv hnallurv Awaiting requested review from hnallurv

Assignees

@ebenahar ebenahar

@yitzhak12 yitzhak12

@sidhant-agrawal sidhant-agrawal

@fbalak fbalak

@sagihirshfeld sagihirshfeld

@ebondare ebondare

@vkathole vkathole

@DanielOsypenko DanielOsypenko

Labels
lgtm size/L PR that changes 100-499 lines Squad/Blue Verified Mark when PR was verified and log provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mitigate unexpected behavior when 2 oc commands are executed at the same time
9 participants
@DanielOsypenko @ebenahar @yitzhak12 @sidhant-agrawal @fbalak @sagihirshfeld @ebondare @vkathole @ocs-ci