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

Add service-config and expose metrics #97

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

patilsuraj767
Copy link
Contributor

fixes: #95
fixes: #96

Below are the changes made in this PR

  • Inherited config type from ipfs-cluster
  • Added mechanism to build and modify service.json inside operator.
  • Added Option to enable and expose ipfs-cluster metrics
  • upgraded go version to match ipfs-cluster project
  • upgraded kubo image to match latest

@RobotSail
Copy link
Collaborator

Let's get the ginkgo versioning fix in #94 merged into main and then we can rebase this branch and retry the build.

@cooktheryan cooktheryan requested review from cooktheryan and removed request for cooktheryan January 17, 2023 19:31
)

func GetDefaultServiceConfig() *cmdutils.ConfigHelper {
cfgHelper := cmdutils.NewConfigHelper("", "", "crdt", "badger")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use flatfs for the datastore so that should also be set here. Maybe this can be parameterized?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@coryschwartz Does IPFS Cluster maintain its own blockstore or does this value need to be in sync with what we use for the IPFS nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have copied this from ipfs-cluster project. Default value is badger.
From the code - here I can see flatfs is not a valid value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hsanjuan We would need to use flatfs here as well correct? ipfs-cluster/cmdutils doesn't seem to support flatfs as a valid value for the datastore, will we need to add support for it?

@RobotSail
Copy link
Collaborator

Suggested edit:

diff --git a/controllers/ipfs_cluster_service.go b/controllers/ipfs_cluster_service.go
index cb10c4c..7a2ab9c 100644
--- a/controllers/ipfs_cluster_service.go
+++ b/controllers/ipfs_cluster_service.go
@@ -6,7 +6,7 @@ import (
 )
 
 func GetDefaultServiceConfig() *cmdutils.ConfigHelper {
-	cfgHelper := cmdutils.NewConfigHelper("", "", "crdt", "badger")
+	cfgHelper := cmdutils.NewConfigHelper("", "", "crdt", "flatfs")
 	err := cfgHelper.Manager().Default()
 	if err != nil {
 		return nil

@RobotSail
Copy link
Collaborator

@patilsuraj767 Now that we merged #94, you can rebase this one and it should clear up the build issue as well.

@RobotSail
Copy link
Collaborator

@patilsuraj767 For some reason it's still running the old workflow in CI, you'll have to rebase to get the tests to pass.

@RobotSail
Copy link
Collaborator

@patilsuraj767 This PR looks like it's in working order according to the CI which is good. I only have a few other things that I can think of:

  1. We'll want to have documentation for how the metrics work, how to interface, etc. There's a docs folder at the root of this project where we have a read-the-docs styled documentation page. You'll want to add a section for metrics usage so that it's clear how we can extend them. This doesn't need to be super long, just as long as we know how it's used.
  2. I see that this PR updates the Go version from 1.18 to 1.19. I'm not against this if it's necessary for the docs, but we'll want to split this up into a separate PR so that we have a base commit to refer to in case of any regressions.

Once those two things, I think we should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants