Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Features: configurable database-hostname and allow insecure connection to database and minio #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Gitzoz
Copy link
Contributor

@Gitzoz Gitzoz commented Jun 29, 2017

Hi everyone

I made same changes so you can configure the database-hostname over a command flag, therefore it is not fixed to localhost. Also it was necessary for us to connect to a database and minio with a self signed cert and I made it so you can set that the verification of the cert ist skipped. For future use it would be better to tackle the ssl configuration again and solve it more accurate.

Best regards
Stefan

@AGFeldman
Copy link
Contributor

I don't think this will work as intended. That is, I don't think it will successfully trigger a backup when run from a remote host. Let's take a look at strata/mongo/lreplica/replica.go:

 	cmd = bson.D{{"setParameter", 1}, {"rocksdbBackup", backupPath}}
 	strata.Log("Performing command for local backup")
 	if err := session.DB("admin").Run(cmd, &result); err != nil {
 		strata.Log("Error performing local backup....")
 		return nil, err
 	}
 
 	strata.Log("Building metadata.Files")
 	// Build metadata.Files
 	files, err := ioutil.ReadDir(backupPath)

Here, we trigger a rocksdb snapshot, which creates hardlinks in the backupPath directory on the replica host. (I'm saying "replica" instead of "database" because MongoDB can be distributed, with multiple replicas in a replica set.) I think this step will/could work remotely with our session handle. But then we need to read the hard links and back up the linked files to S3/minio/azure. In the code above we start doing that with ioutil.ReadDir(backupPath), which won't work if backupPath is remote.

I think it would be pretty challenging to support remote replica in a robust and maintainable way, and I'm doubtful that it's worthwhile because, as I understand, most people can run the strata diver on the replica host.

I don't see a reason to support hardlink snapshot creation without backing up to remote storage.

Let me know if I've misunderstood or missed anything. Thanks!

@Gitzoz
Copy link
Contributor Author

Gitzoz commented Jul 6, 2017

It was not the goal to support remote replica backups!
We are running mongo-rocks and strata in a kubernetes environment and everything in docker container. Our MongoReplica setup is 1 master 2 secondary and 1 hidden secondary.
We want to backup from the hidden secondary node. The hidden secondary and the strata container mount the same folder on the host, but the hidden secondary is only available through the kubernetes network. Therefore we must override localhost with the kubernetes hostname.

We let it run in our system and it works very good.

I hope the explanation helps for understanding our problem!

Thanks in advance.

@AGFeldman
Copy link
Contributor

Sounds good, thanks!

One thing: Can you rebase your changes on latest master to make them easier to review? This PR currently contains "Fixed List in MinioStorage" and "Use fixed path in miniostorage List", which have already been added to master.

Stefan Röhrbein added 2 commits July 12, 2017 09:29
It is now possible to set a specific database hostname. The default database hostname is set to 'localhost'. Additionally removed the replica options in lrazureblobdriver.go and use replica options from lreplica_drivers/lrs3driver/lrs3driver.go, to have the same code base.
When the flags are set to true, the connection is made with a tls InsecureSkipVerify: true.
@Gitzoz
Copy link
Contributor Author

Gitzoz commented Jul 12, 2017

It should be now okay.

Copy link
Contributor

@AGFeldman AGFeldman left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

I'd like to avoid depending on lrs3driver from code that isn't specific to local replica and S3 storage. I can think about this more next weekend if you don't see a straightforward fix.

Port int `long:"port" default:"27017" description:"Backup should look for a mongod instance that is listening on this port"`
Username string `long:"username" description:"If auth is configured, specify the username with admin privileges here"`
Password string `long:"password" description:"Password for the specified user."`
DatabaseHostname string `long:"database-hostname" default:"localhost" description:"Database hostname can be override with a specific hostname in most cases localhost is sufficient"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a name like LocalHostname would be better than DatabaseHostname.

Do you think the following would description would make sense? 'localhost' or a hostname that is accessible on the local machine via e.g. kubernetes network.

By the way, another reason to avoid the name DatabaseHostname is that the MongoDB database might be distributed over multiple hosts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I have totally forgotten this PR, because I changed the company and moved to a new city. :(
I've implemented your requested changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants