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 redis::instance defined type #200

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

petems
Copy link
Member

@petems petems commented May 6, 2017

This will allow configuring multiple redis instances on one machine, allowing the first step towards #113

@petems petems force-pushed the adds_redis_instances_define branch 3 times, most recently from e0c496a to 1175c44 Compare July 3, 2017 21:04
@petems petems changed the title (WIP) Instance defined type Add redis::instance_config defined type Jul 3, 2017
@petems petems force-pushed the adds_redis_instances_define branch 3 times, most recently from bb43a12 to 6abb0d4 Compare July 4, 2017 00:11
@petems petems mentioned this pull request Jul 4, 2017
@petems
Copy link
Member Author

petems commented Jul 4, 2017

@jacobmw @v6 @spo0nman @bostrowski13

Working MVP of two redis instances on one server, running on two different ports:

    class { '::redis':
      default_install => false,
    }

    redis::instance {'redis1':
      port     => '7777',
      pid_file => '/var/run/redis/redis-server-redis1.pid',
    }

    redis::instance {'redis2':
      port     => '8888',
      pid_file => '/var/run/redis/redis-server-redis2.pid',
    }

@petems petems force-pushed the adds_redis_instances_define branch 7 times, most recently from d91b607 to b5d1d3f Compare July 4, 2017 19:00
@petems petems changed the title Add redis::instance_config defined type Add redis::instance defined type Jul 4, 2017
@petems petems force-pushed the adds_redis_instances_define branch 10 times, most recently from 29a3ee1 to d10933a Compare July 6, 2017 00:19
* Move the config logic into a defined type
* This will allow us to have multiple instances
of redis on one machine
* Example code:
```
# Install redis package
class { '::redis':
  default_install => false,
}

redis::instance {'redis1':
  port     => '7777',
}

redis::instance {'redis2':
  port     => '8888',
}
```
@petems petems force-pushed the adds_redis_instances_define branch from d10933a to 161b04a Compare July 6, 2017 14:12
@bostrowski13
Copy link
Contributor

this work sfor me. are other params defined within each instance like you specified the pid_file?

@petems
Copy link
Member Author

petems commented Jul 7, 2017

@bostrowski13 Yep, all of the parameters defined are the same that would be the defined the main class. In fact, you no longer need to specify the pid, you can just do:

class { '::redis':
      default_install => false,
    }

    redis::instance {'redis1':
      port     => '7777',
    }

    redis::instance {'redis2':
      port     => '8888',
    }

@petems petems merged commit 385d4f0 into voxpupuli:master Jul 7, 2017
@bostrowski13
Copy link
Contributor

that is excellent. thank you!

@spo0nman
Copy link

spo0nman commented Jul 7, 2017

@petems good to know that this code is getting used. In my earlier PR there was a bug where the data directory for redis was shared between the instances, I fixed it in my fork but never published. Look out for this bug. Good luck!

@petems
Copy link
Member Author

petems commented Jul 7, 2017

@spo0nman Ooh, interesting, that's a good point actually. I should probably add a test to make sure each instance uses it's own data folder

But yes, your original code was instrumental in getting this working so thank you very much! 😄

@petems petems deleted the adds_redis_instances_define branch July 7, 2017 12:31
@ghost
Copy link

ghost commented Jul 12, 2017

What about having separate sentinel instances? Is it possible to do this or can sentinel monitor multiple Redis instances without creating a separate sentinel instance for each.

@petems
Copy link
Member Author

petems commented Jul 13, 2017

@tdotgreenshirt From my understanding, you don't want the sentinel instance on the same machine as the redis instance because it would be a SPOF: When you lose that host you lose both.

If there is a case for multi-sentinal on one host, then it could be added as a feature?

@ghost
Copy link

ghost commented Jul 13, 2017

@petems This is similar to what we are doing:
https://i.stack.imgur.com/Citcm.png

However, if we have separate Redis instances, we also need a separate Sentinel instances.

@petems
Copy link
Member Author

petems commented Jul 13, 2017

@tdotgreenshirt Ok, can you add that as an issue and I'll tag it as an upcoming feature.

Now we have a defined instance type, and redis-sentinel is just another redis instance, this should be fairly quick to implement 😄

@bostrowski13
Copy link
Contributor

bostrowski13 commented Jul 13, 2017

thats actually very similar as to what we're after as well. be great to have that option for sentinel.

It also seems like thats a via model based on their documentation: https://redis.io/topics/sentinel
See example 2.

@ghost
Copy link

ghost commented Jul 17, 2017

Created issue for multiple Sentinel instances: #223

cegeka-jenkins pushed a commit to cegeka/puppet-redis that referenced this pull request Feb 16, 2021
* Move the config logic into a defined type
* This will allow us to have multiple instances
of redis on one machine
* Example code:
```
# Install redis package
class { '::redis':
  default_install => false,
}

redis::instance {'redis1':
  port     => '7777',
}

redis::instance {'redis2':
  port     => '8888',
}
```
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.

None yet

3 participants