Skip to content

Conversation

@rainest
Copy link
Contributor

@rainest rainest commented May 30, 2025

The existing PCS codebase contains several pieces of dead code under internal/. I decided to remove it after

From main as of fc206aa:

13:03:47-0700 baqytgul $ deadcode ./...
internal/api/api_test_setup_handler.go:37:6: unreachable func: CreateRouterAndHandler
internal/domain/power-status.go:168:6: unreachable func: PowerStatusMonitorStop
internal/domain/power-status.go:173:6: unreachable func: PowerStatusMonitorChangeInterval
internal/model/models.go:39:6: unreachable func: UUIDSliceEquals
internal/model/models.go:60:6: unreachable func: StringSliceEquals
internal/model/models.go:85:6: unreachable func: NewInvalidInputError
internal/model/models.go:90:6: unreachable func: Find
internal/model/power-status.go:76:29: unreachable func: PowerStateFilter.EnumIndex
internal/model/power-status.go:118:34: unreachable func: ManagementStateFilter.EnumIndex
internal/storage/distlock_etcd_impl.go:49:6: unreachable func: fromStorageETCD
internal/storage/distlock_mem_impl.go:51:6: unreachable func: fromStorageMEM

PowerStatusMonitorStop and PowerStatusMonitorChangeInterval: added in the initial version of power monitoring and never reachable. These are hypothetically useful, but that they've gone unused for 3y+ suggests they're YAGNI features.

Either can be handled in the cmd package instead. A PCS_POWER_SAMPLE_INTERVAL envvar is already available, and we can add a way to disable domain.PowerStatusMonitorInit() if desired.

internal/model/models.go contains generic utility functions added in the initial commit and never used. Most of these are handled by https://pkg.go.dev/golang.org/x/exp/slices. The file also contained two types that are only used by the API; these are now in the api package.

internal/model/power-status.go contained some unused utility function to convert an iota type to an int. These aren't useful, you can cast to an int if you really want to.

fromStorageETCD and fromStorageMEM have been left as-is. These exist because of poor interface design where StorageProvider and DistributedLockProvider aren't truly independent, and should be addressed as part of a larger refactor instead.

I did remove the DistributedLockProvider.InitFromStorage() functions, which were effectively dead for lack of non-test use.

@rainest rainest force-pushed the rainest/remove-cruft branch from e7f8088 to 5721be2 Compare June 11, 2025 21:10
@rainest rainest marked this pull request as ready for review June 11, 2025 21:12
@rainest
Copy link
Contributor Author

rainest commented Jun 11, 2025

@bmcdonald3 also worth considering for upstream, albeit less likely to be a concern for conflicts (insofar as this is all unused, I don't expect it to change in any CSM patches).

@yogi4 yogi4 requested a review from shunr-hpe June 17, 2025 15:11
@alexlovelltroy
Copy link
Member

@rainest Can we close this, or should we fix it up and get it merged? @shunr-hpe any feedback?

@cjh1
Copy link
Member

cjh1 commented Aug 15, 2025

We were waiting on the changes to be upstream, to avoid potential conflicts, however as @rainest points out its dead code so that is unlikely.

@shunr-hpe
Copy link
Collaborator

I'm ok with this change

@rainest rainest force-pushed the rainest/remove-cruft branch from 5721be2 to d9dbd6d Compare August 21, 2025 20:14
Remove an unused test scaffold for the API package.

This scaffold contained one function, which registered the aggregate API
router at the root path, but did not serve it. It contained nothing
specific to PCS.

Most of the API lacks tests entirely. One section (health) sets up its
own test server independently.

Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
Remove several unused utility functions from the model package.

Move several API-only types from the model package to the api package.

Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
Remove the InitFromStorage() function from the DistributedLockProvider
interface. This method was only used in tests, and was redundant. Init()
can handle all cases where InitFromStorage() would be used.

Signed-off-by: Travis Raines <571832+rainest@users.noreply.github.com>
@rainest rainest force-pushed the rainest/remove-cruft branch from d9dbd6d to ec1874c Compare August 21, 2025 20:14
@rainest
Copy link
Contributor Author

rainest commented Aug 21, 2025

Rebased to deconflict and add signoff:

  • Removed models_test.go again. main had changed it to add the integration test build flag.
  • Used main's transition_size_test.go. It made the same change to use Init() instead of InitFromStorage(), and also fixed a missing error return assign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants