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

[checkbox-ce-oem] Modify dbus warm cold boot job (BugFix) #1038

Merged
merged 11 commits into from
Aug 15, 2024
Merged

[checkbox-ce-oem] Modify dbus warm cold boot job (BugFix) #1038

merged 11 commits into from
Aug 15, 2024

Conversation

rickwu666666
Copy link
Contributor

@rickwu666666 rickwu666666 commented Mar 6, 2024 •
edited
Loading

  • Modify the command called in the job
    Since we change the command name in the test-strict-confinement snap.
  • Fix the wrong job name in test-plan
  • Change job structure
    Make the job easier to read

Description

We modify the command in the test-strict-confinement snap. Therefore, we modify the command that is called in the job.
Also, fix the wrong job ID that is included in the test plan.

Resolved issues

N/A

Documentation

N/A

Tests

Warm-boot
https://certification.canonical.com/hardware/202304-31485/submission/359313/

Cold-boot
https://certification.canonical.com/hardware/202304-31485/submission/359311/

@rickwu666666 rickwu666666 requested a review from a team as a code owner March 6, 2024 01:59
@rickwu666666 rickwu666666 changed the title Modify dbus warm cold boot job [checkbox-ce-oem] Modify dbus warm cold boot job(Bug Fix) Mar 6, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.52%. Comparing base (397306c) to head (9324e7c).
Report is 3 commits behind head on main.

❗ Current head 9324e7c differs from pull request most recent head 72eeb9f

Please upload reports for the commit 72eeb9f to get more accurate results.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
- Coverage   44.10%   40.52%   -3.58%     
==========================================
  Files         358      335      -23     
  Lines       38765    37365    -1400     
  Branches     6571     6348     -223     
==========================================
- Hits        17096    15143    -1953     
- Misses      21006    21585     +579     
+ Partials      663      637      -26
Flag Coverage Δ
contrib-provider-ce-oem 29.59% <ø> (-0.89%) ⬇️
provider-genio ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rickwu666666 rickwu666666 changed the title [checkbox-ce-oem] Modify dbus warm cold boot job(Bug Fix) [checkbox-ce-oem] Modify dbus warm cold boot job (Bug Fix) Mar 6, 2024
@rickwu666666 rickwu666666 changed the title [checkbox-ce-oem] Modify dbus warm cold boot job (Bug Fix) [checkbox-ce-oem] Modify dbus warm cold boot job (BugFix) Mar 6, 2024
kissiel
kissiel suggested changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Race condition with the background process scheduling found.

...rib/checkbox-provider-ce-oem/units/strict-confinement/powermanagement-strict-confinement.pxu Outdated Show resolved Hide resolved
LiaoU3
LiaoU3 previously approved these changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@LiaoU3 LiaoU3 left a comment

Choose a reason for hiding this comment

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

+1

@baconYao
Copy link
Contributor

Race condition with the background process scheduling found.

@rickwu666666 , I think you can reference this PR
#979

@rickwu666666 rickwu666666 force-pushed the modify_dbus_warm_cold_boot_job branch from 9324e7c to 017f551 Compare March 14, 2024 07:16
@rickwu666666
Copy link
Contributor Author

I've pushed a fix for the race condition.

stanley31huang
stanley31huang requested changes Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@stanley31huang stanley31huang left a comment

Choose a reason for hiding this comment

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

In test plan, we are going to compare the logs between the reboot test, so we need to add a job to collect logs before reboot test.

@rickwu666666 rickwu666666 force-pushed the modify_dbus_warm_cold_boot_job branch from 017f551 to e9bb247 Compare March 15, 2024 01:48
@rickwu666666
Copy link
Contributor Author

@stanley31huang , Yes you are correct. I've pushed the fix. Thanks for the review.

kissiel
kissiel suggested changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Going through an extra snap seems unnecessary. It:

  • obfuscates what is being done
  • reduces clarity and makes the whole thing less readable and debuggable
  • creates unnecessary dependencies

Also there's a serious race condition with Checkbox being created here that may lead to breakage of other jobs that may follow the one modified here.

...rib/checkbox-provider-ce-oem/units/strict-confinement/powermanagement-strict-confinement.pxu Outdated Show resolved Hide resolved
...rib/checkbox-provider-ce-oem/units/strict-confinement/powermanagement-strict-confinement.pxu Outdated Show resolved Hide resolved
@rickwu666666 rickwu666666 force-pushed the modify_dbus_warm_cold_boot_job branch from e9bb247 to d3e2ae4 Compare April 23, 2024 01:15
@rickwu666666 rickwu666666 requested a review from LiaoU3 June 7, 2024 08:29
LiaoU3
LiaoU3 previously approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@LiaoU3 LiaoU3 left a comment

Choose a reason for hiding this comment

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

+1

@rickwu666666 rickwu666666 force-pushed the modify_dbus_warm_cold_boot_job branch from d3e2ae4 to 72eeb9f Compare June 14, 2024 06:33
@rickwu666666
Copy link
Contributor Author

@stanley31huang,
I've pushed the fix based on our discussed yesterday.

LiaoU3
LiaoU3 previously approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@LiaoU3 LiaoU3 left a comment

Choose a reason for hiding this comment

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

LGTM

@rickwu666666 rickwu666666 requested review from stanley31huang and removed request for stanley31huang August 13, 2024 05:46
@stanley31huang
Copy link
Collaborator

I think we should have some extra steps to reboot the system in case we failed to power control the system via test-strict-confinement snap.
Like what we did in Baoshan project https://git.launchpad.net/~baoshan-team/baoshan/+git/checkbox-provider-baoshan/tree/units/thermal/jobs.pxu

@rickwu666666
Modify the command called in the job
f22ead1 
 Since we change the command name in test-strict-confinement snap.

Fix the wrong job name in test-plan
@rickwu666666
Change the job structure
6d8cb1c 
Make the job easier to read
@rickwu666666
Modify rtcwake argument from -m on to -m no
e37fc27 
Due to -m on is for debugging purpose
@rickwu666666
Modify the method of calling rtcwake to prevent race condition
887153a
@rickwu666666
Add init-boot-loop-data
cdf8859 
Since we need initial status of the system
@rickwu666666
Remove redundant command
2372e71
@rickwu666666
Change the wording of the job id
87c002d 
Change calling dbus logic to prevent auto job hang
@rickwu666666
Add logic to handle if cold/warm boot fail
caa1c57
@rickwu666666 rickwu666666 force-pushed the modify_dbus_warm_cold_boot_job branch from 72eeb9f to caa1c57 Compare August 14, 2024 01:00
@rickwu666666
Copy link
Contributor Author

https://certification.canonical.com/hardware/202403-33886/submission/386662/test-results/fail/
Test result of if reboot via test-strict-confinement snap failed.

stanley31huang
stanley31huang approved these changes Aug 15, 2024
View reviewed changes
Copy link
Collaborator

@stanley31huang stanley31huang left a comment

Choose a reason for hiding this comment

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

LGTM

@rickwu666666 rickwu666666 merged commit e76778e into canonical:main Aug 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kissiel kissiel kissiel requested changes

@stanley31huang stanley31huang stanley31huang approved these changes

@LiaoU3 LiaoU3 LiaoU3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rickwu666666 @baconYao @stanley31huang @kissiel @LiaoU3