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.

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

Opencost Core Refactor #2440

Merged
merged 17 commits into from Jan 17, 2024
Merged

Opencost Core Refactor #2440

merged 17 commits into from Jan 17, 2024

Conversation

mbolt35
Copy link
Contributor

@mbolt35 mbolt35 commented Jan 8, 2024

Major Changes

This refactor is focusing on a few main concerns revolving around filters, legacy filters, and unification around the current implementation. It separates an easier to consume module for external projects which reference opencost for the models and/or utilities.

Opencost Core Module

Created a new go module under the core/ directory which will now contain sharable opencost code:

  • Data Models: Any of the data model structs or support types are moved to core
  • Filters: All filter code is now part of core
  • Utility and Logging: Any utility types, logging, and other packages like env are moved to core
  • Abstraction separation from implementation - will eventually allow external implementations of opencost abstractions.

Filter Unification

  • The 2.1 Filter language will become to default standard for opencost querier services. There will be continued support for legacy http parameters for filters.
  • Moved all filter packages under core/pkg/filter -- legacy filtering support will temporarily be supported during this process under core/pkg/filter/legacy
  • More work is required here!

Require Answers

  • Is this the proper way to create a new go module in the same repository? A new go.mod was created for the module: github.com/opencost/opencost/core and the opencost go.mod uses:
replace github.com/opencost/opencost/core => ./core

to ensure that both modules are still "versioned" together. However, it's unclear if this method of replacement is the correct convention for long term maintenance.

  • When a git tag is created, will both modules leverage the same version? (that is currently the intent).

@mbolt35 mbolt35 self-assigned this Jan 8, 2024
Copy link

vercel bot commented Jan 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2024 1:51am

core/pkg/kubecost/window.go Outdated Show resolved Hide resolved
core/pkg/kubecost/window.go Outdated Show resolved Hide resolved
@Sean-Holcomb
Copy link
Contributor

@bolt given everything is moving right now and this move is bound to break people, it seems like now would be a good time to remove the name kubecost from the pkg name. What do you think?

@Sean-Holcomb
Copy link
Contributor

@bolt given everything is moving right now and this move is bound to break people, it seems like now would be a good time to remove the name kubecost from the pkg name. What do you think?

Not to say it should be this PR necessarily but flagging that both should land in the same release to prevent a double move for external users

core/pkg/kubecost/query.go Outdated Show resolved Hide resolved
core/pkg/protocol/protocol.go Show resolved Hide resolved
@mattray
Copy link
Collaborator

mattray commented Jan 16, 2024

@bolt given everything is moving right now and this move is bound to break people, it seems like now would be a good time to remove the name kubecost from the pkg name. What do you think?

#2309 is the Issue

michaelmdresser and others added 7 commits January 16, 2024 20:19
* Add ReadLocked and WriteLocked to fileutil

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Use locking r/w in FileStorage

This is necessary as we plan to use multiple processes with this library
accessing files on the same filesystem.

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Add FD-based flock()-ed read and write

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Move file lock code to separate file

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Make locks unimplemented for Windows

This should allow Windows users to still import Opencost but functionality
which requires these functions will not work. This should have minimal impact.
Refer to the following when wishing to implement for windows:
- https://github.com/golang/go/blob/master/src/cmd/go/internal/lockedfile/internal/filelock/filelock_windows.go
- maybe also https://github.com/gofrs/flock/blob/master/flock_windows.go

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Unlock after failed read and write

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Add further unit test

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

* Make r/w locked robust to FD position

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>

---------

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
… First pass at unifying filters

Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
sachin-rafay and others added 6 commits January 16, 2024 20:19
Signed-off-by: Sachin Kumar <sachin@rafay.co>

Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Sachin Kumar <sachin@rafay.co>

Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Sachin Kumar <sachin@rafay.co>

Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Sachin Kumar <sachin@rafay.co>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Niko Kovacevic <nikovacevic@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Copy link
Collaborator

@mattray mattray left a comment

Choose a reason for hiding this comment

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

Other than the 2 comments it looks like a global find and replace.

Signed-off-by: Matt Bolt <mbolt35@gmail.com>
Copy link

sonarcloud bot commented Jan 17, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

15 New issues
0 Security Hotspots
10.1% Coverage on New Code
2.5% Duplication on New Code

See analysis details on SonarCloud

@mbolt35 mbolt35 merged commit 194ebd1 into develop Jan 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants