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 support for minio deploy #1277
Conversation
2a14abd
to
6b471e2
Compare
@harshavardhana thanks so much for the PR, very excited to have support for minio and other s3 compatible stores. A few things I'd like to see added / changed before we merge this in:
|
answers inline
Certainly, i thought to keep it more generic and say Minio. Let me change it to minio.
I think i already did that with this patch - https://github.com/kubernetes/charts/tree/master/stable/minio
Sure would a helm chart be sufficient for you guys?
Already signed CLA as well. |
Ok, I did the following based on this PR:
After this, it appears that the cluster is healthy:
So, then I tried creating a repo and committing some data:
That all looks good. However, nothing shows up in Minio (even after a refresh): And, looking at the
Pachyderm is still using the local filesystem, not minio. So checking the manifest again (which I should have done to start) reveals, that indeed the deploy command didn't properly set the backend variable:
So, I then manually set this as:
Then, I restarted my k8s cluster, and repeated the above steps to deploy pachyderm with the new manually modified manifest. This results in:
Checking the logs we get:
It appears that it's not finding the bucket file. However, the secret is definitely in the manifest:
All in all, there seems to be two minor issues here (which I expect will be easy fixes):
|
6b471e2
to
4ae7491
Compare
I tried this not sure what ErrImagePull means.
|
is this because i need to publish these? |
Yes, normally the deploy pulls the images corresponding to your release version. However we are trying to do something custom. You will need to put the images in some registry that pachyderm can pull from. I will pushed mine up to my docker hub. Then you can replace |
@dwhitena @harshavardhana you can get |
After changing these to use y4m4/ |
|
@harshavardhana I haven't seen any problems like that with wrong IPs being assigned. Looking at the logs it seems that |
4ae7491
to
8415c63
Compare
Actually it worked fine but had do more changes.. Finishing them now there were few checks to disallow any backend other than localBackend to serve rethinkdb i just choose |
Yeah that sounds reasonable, we're likely going to need to make things a little bit more sophisticated now since people will probably want to be able to do minio with an ebs volume for amazon deploys or a PD for GCE deploys or what have you. But that's not necessary for the purposes of this PR. |
Understood.. |
d41e643
to
1d6babf
Compare
Ok @harshavardhana, we have Pachyderm running backed by Minio: After updating
The problem here was that minikube is running in a VM locally, and thus it was seeing 127.0.0.1 as the VM IP. In order to fix this, I did our minio deploy with an endpoint of 10.0.2.2:9000, where 10.0.2.2 is the IP that virtualbox uses to access the actual localhost. When I did that. Boom! Everything works great. I can commit data in and it goes right into Minio. Great work! We need to resolve the following points before or after merging this:
|
The problem i see with choosing different sets of credentials is that the CLI should be simplified and it can get really hard to specify many such options on command line. I am not sure if there is a work on this area already - to make it simple. Ideally it would be better to be in a single command so that documentation becomes easier. Sub-commands for Any other thoughts? - once we finalize i can work this out and send another PR after this. |
1d6babf
to
eac9482
Compare
After talking with @jdoliner the plan for next things are: |
func NewBlockAPIServer(dir string, cacheBytes int64, backend string) (pfsclient.BlockAPIServer, error) { | ||
log.Info("Initializing new blockAPIServer", dir, cacheBytes, backend) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to leave this in? It doesn't seem like a particularly important thing to inform the user of, you'll basically just see this once on startup. Also log messages should use lion right now to match the rest of our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no this is not necessary.
@@ -40,6 +41,7 @@ func DeployCmd(noMetrics *bool) *cobra.Command { | |||
var hostPath string | |||
var dev bool | |||
var dryRun bool | |||
var isSecure bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, I'd call this secure
, no need for the "is"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
} | ||
|
||
func (c *minioClient) Walk(name string, fn func(name string) error) error { | ||
isRecursive := true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another very small nit, I'd call this recursive
not isRecursive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
LGTM after those comments |
4f04c12
to
ee99982
Compare
Oops looks like i force pushed it and lost @dwhitena commit, Can you send it again? sorry. |
Addressed all the comments. |
This adds support for Minio and all other S3 compatible servers. This patch also uses `minio-go`. This has an added benefit i.e this can be used S3 as well transparently.
ee99982
to
17979d7
Compare
This adds support for Minio and all other S3 compatible servers.
This patch also uses
minio-go
. This has an added benefit i.ethis can be used S3 as well transparently.