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

DAOS-12114 build: remove psm2 dependency #11637

Merged
merged 3 commits into from
Jun 6, 2023
Merged

DAOS-12114 build: remove psm2 dependency #11637

merged 3 commits into from
Jun 6, 2023

Conversation

soumagne
Copy link
Collaborator

@soumagne soumagne commented Mar 7, 2023

Remove also PSM2 support from CART and psm2 references in tests

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate watchers.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

daosbuild1
daosbuild1 reviewed Mar 7, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/1/execution/node/282/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/1/execution/node/311/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.4 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/1/execution/node/342/log

frostedcmos
frostedcmos previously approved these changes Mar 7, 2023
View reviewed changes
@github-actions
Copy link

github-actions bot commented Mar 7, 2023 •
edited
Loading

Bug-tracker data:
Ticket title is 'Remove psm2 support from cart'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-12114

@soumagne soumagne force-pushed the soumagne/remove_psm2 branch from bd2b4d8 to f9b6436 Compare May 17, 2023 22:15
daosbuild1
daosbuild1 reviewed May 17, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/2/execution/node/338/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/2/execution/node/334/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.4 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/2/execution/node/341/log

@soumagne soumagne force-pushed the soumagne/remove_psm2 branch from f9b6436 to 055405c Compare May 17, 2023 22:40
daosbuild1
daosbuild1 reviewed May 17, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@soumagne
DAOS-12114 build: remove psm2 dependency
4319a0e 
Remove also PSM2 support from CART and psm2 references in tests

Required-githooks: true

Signed-off-by: Jerome Soumagne <jerome.soumagne@intel.com>
@soumagne soumagne force-pushed the soumagne/remove_psm2 branch from 055405c to 4319a0e Compare May 17, 2023 22:45
daosbuild1
daosbuild1 reviewed May 17, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@soumagne soumagne requested a review from frostedcmos May 18, 2023 14:41
@soumagne soumagne marked this pull request as ready for review May 18, 2023 14:41
@soumagne soumagne requested review from a team as code owners May 18, 2023 14:41
daltonbohning
daltonbohning reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

Looks like this doc need an update:

daos/docs/dev/development.md

And maybe these?

daos/src/control/cmd/daos_server/auto_test.go
daos/src/control/cmd/dmg/auto_test.go
daos/src/control/lib/control/auto_test.go
daos/src/control/lib/control/mocks.go
daos/src/control/lib/control/mocks.go
daos/src/control/lib/hardware/libfabric/provider_test.go

frostedcmos
frostedcmos reviewed May 18, 2023
View reviewed changes
site_scons/prereq_tools/base.py
@@ -43,8 +43,6 @@
from SCons.Script import BUILD_TARGETS
from SCons.Errors import InternalError

OPTIONAL_COMPS = ['psm2']
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should keep this capability and just have it be empty? unless we think this was just 1off special handling for opx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jolivier23 @ashleypittman any preferences ? should I keep OPTIONAL_COMPS = [''] ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I'd be fine removing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok I kept it removed and I also removed the doc section about it.

frostedcmos
frostedcmos reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@frostedcmos frostedcmos left a comment

Choose a reason for hiding this comment

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

few inline comments

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/4/execution/node/1162/log

@soumagne
Copy link
Collaborator Author

@daltonbohning I think I would appreciate it if someone else from the control plane team (@kjacque please feel free to chime in) or someone else could remove the psm2 references from the go tests since those tests might need to be reworked a bit more. Maybe that can be a separate PR ?

@daltonbohning
Copy link
Contributor

@daltonbohning I think I would appreciate it if someone else from the control plane team (@kjacque please feel free to chime in) or someone else could remove the psm2 references from the go tests since those tests might need to be reworked a bit more. Maybe that can be a separate PR ?

Sure, no problem from me. I don't know the answer either :)

jolivier23
jolivier23 previously approved these changes May 19, 2023
View reviewed changes
@kjacque
Copy link
Contributor

kjacque commented May 19, 2023

I think I would appreciate it if someone else from the control plane team (@kjacque please feel free to chime in) or someone else could remove the psm2 references from the go tests since those tests might need to be reworked a bit more. Maybe that can be a separate PR ?

I don't think there's any work that's required to be done in the Go tests. In general those are unit tests, and none of them interact with the real psm2 provider or Cart.

@soumagne
DAOS-12114 build: remove doc section on optional components (psm2)
5a80380 
Required-githooks: true

Signed-off-by: Jerome Soumagne <jerome.soumagne@intel.com>
@soumagne soumagne requested a review from a team as a code owner June 2, 2023 20:40
daosbuild1
daosbuild1 reviewed Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daosbuild1
daosbuild1 reviewed Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@soumagne
Copy link
Collaborator Author

soumagne commented Jun 2, 2023

NB. I won't fix the clang format in that PR to prevent any further conflicts. We should run clang-format on the cart code in a separate PR.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11637/6/execution/node/1268/log

@soumagne soumagne requested a review from jgmoore-or June 5, 2023 15:45
@soumagne soumagne requested a review from a team June 5, 2023 16:43
@jolivier23 jolivier23 merged commit ef2f135 into master Jun 6, 2023
@jolivier23 jolivier23 deleted the soumagne/remove_psm2 branch June 6, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daosbuild1 daosbuild1 daosbuild1 left review comments

@frostedcmos frostedcmos frostedcmos approved these changes

@jgmoore-or jgmoore-or jgmoore-or approved these changes

@ashleypittman ashleypittman Awaiting requested review from ashleypittman

@daltonbohning daltonbohning Awaiting requested review from daltonbohning

@jolivier23 jolivier23 Awaiting requested review from jolivier23

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@soumagne @daosbuild1 @daltonbohning @kjacque @jolivier23 @jgmoore-or @frostedcmos