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

etcdmain: Refactor etcdStart for use in embedding etcd servers #5432

Closed
wants to merge 1 commit into from

Conversation

purpleidea
Copy link
Contributor

This patch refactors out the important (and often non-public) portions
of etcdStart that need to get run for use in embedding an etcd server.
This will allow projects just as mgmt (config) to embed etcd without
duplicating code, or trailing behind the latest upstream etcd changes.

@purpleidea
Copy link
Contributor Author

This should resolve #5430

@purpleidea
Copy link
Contributor Author

If you ACK this patch, I'll also send a docs patch with some example usage. LMK.

@purpleidea purpleidea force-pushed the feat/refactor-start branch 2 times, most recently from 1fc2f83 to 8647c18 Compare May 23, 2016 08:09
@purpleidea
Copy link
Contributor Author

FWIW I think the semaphore failure is unrelated to this patch. Something is up with your CI, but travis passed.

@purpleidea
Copy link
Contributor Author

@xiang90 If someone could review, that would be great! I don't know why this would affect the integration test and cause a fail, but if you or someone could enlighten me on that, I would appreciate it thanks!

@purpleidea
Copy link
Contributor Author

The failure seems to be related to git master btw. Others see it too: eg: #5350

@jonboulle Feel like reviewing? Thanks

@xiang90
Copy link
Contributor

xiang90 commented May 25, 2016

@purpleidea Sorry for the delay. We probably do not have time to review this pull request until early next week.

@heyitsanthony
Copy link
Contributor

heyitsanthony commented May 25, 2016

@purpleidea those failures (some closed connection handling and election stuff which afaik have both been fixed) are unrelated to this one. Semaphore runs more tests than travis (integration, e2e) via INTEGRATION=1 ./test. The failure is in e2e which means it's launching a full etcd binary so I suspect it's related to your patch and not a problem with master.

@purpleidea
Copy link
Contributor Author

@heyitsanthony Thanks for the reply... Do you have any ideas why it would be related to my patch? Maybe fresh eyes would help, but it's supposed to be a straight refactor without a change in behaviour.

Thanks!

@purpleidea
Copy link
Contributor Author

@purpleidea Sorry for the delay. We probably do not have time to review this pull request until early next week.

@xiang90 With the utmost respect for you and the other maintainers of this project, I don't think that this is a good response to encourage outside contributors. IMO the priority should always be getting the PR count to zero.

@xiang90
Copy link
Contributor

xiang90 commented May 25, 2016

@purpleidea This is not how priority works in this project. We have milestones, and we prioritize things based on the milestone. Bugs and regression issues have a higher priority above all others. Our bandwidth is limited. Also there is no us or community. We are trying to be part of the community. All milestones are public.

We are growing our community. At CoreOS, we try our best to review pull requests that are even not on etcd milestones when we have time for the new members of the community. I am telling you that just want you to be patient. The reason is that this pull request involves some "big changes". We need to think care about this. This pull request is not something that I can say a "yes" or "no" in ten minutes.

@purpleidea
Copy link
Contributor Author

@xiang90 I understand, and thank you for your time. It was just my lone opinion on the matter as I am trying to move into becoming a more valuable contributor to the project, and was hoping for more facilitation in that matter. In any case, I'll patiently await some review.

Thanks!

@purpleidea purpleidea force-pushed the feat/refactor-start branch 3 times, most recently from dd67a16 to 3db1bb8 Compare June 1, 2016 00:39
This patch refactors out the important (and often non-public) portions
of etcdStart that need to get run for use in embedding an etcd server.
This will allow projects just as `mgmt` (config) to embed etcd without
duplicating code, or trailing behind the latest upstream etcd changes.
@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

@purpleidea

Sorry for the delay. If I understand the change correctly, it changes the start function to take a list of args instead of only config. Why do we want to accept args instead of config struct? Or what is the benefits of accepting a list of args?

@purpleidea
Copy link
Contributor Author

@xiang90 Sorry for being pushy earlier and thanks for looking into this.

Yes, it tries to abstract away the common things that are normally done in startEtcd and puts them into the new public helper function which is an approximate port of the existing code.

This way a user can run the equivalent of startEtcd without having to copy and keep a lot of code in sync.

I decided to pass in the few needed values as args to avoid creating yet another struct, but I'm happy to rewrite this to use a struct if you're happy with the idea.

Thanks,
James

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

@purpleidea I am wondering if we can add a helper function call defaultConf or something to generate a pre-filled conf struct. That seems to be a simpler approach to solve the copy/paste problem you have.

@purpleidea
Copy link
Contributor Author

@xiang90 I cannot find a 'defaultConf` struct in the current code base, so perhaps I misunderstand.

The way you can invoke this now with this patch is like:


    listen, defers, err := etcdmain.StartEtcdHelper(
        false, false, // pick values
        nil, nil, // pick values
        nil, peerURLs, clientURLs,
        dataDir, nil,
    )
    for _, d := range defers {
        defer d()
    }
    if err != nil {
        return err
    }

    cfg := &etcdserver.ServerConfig{
        Name:               memberName,
        ClientURLs:         clientURLs,
        PeerURLs:           peerURLs,
        DataDir:            dataDir,
        InitialPeerURLsMap: initialPeerURLsMap,
        NewCluster:         newCluster,
        TickMs:        100,
        ElectionTicks: 10,
    }

    obj.server, err = etcdserver.NewServer(cfg)
    if err != nil {
        return err
    }

    obj.server.Start()

    listen(obj.server)

Which should effectively stay portable. As you mentioned, if you don't mind another struct, then it could make sense to pass that into the helper method instead of the list of params. With this approach, you still want to specify your own ServerConfig as shown here.

Let me know how you'd like me to proceed. Thanks!
(Also I'm happy to rebase anytime you like, as I've already done a few times)

@purpleidea
Copy link
Contributor Author

@xiang90 Thinking about this more, I realize this should probably be renamed to ConnectionHelper or similar, since it deals with setting up and running the tls and connection stuff. That's easy to do as desired though.

@purpleidea
Copy link
Contributor Author

...So yeah if you like the idea, I would probably rewrite it to have the StartEtcdHelper be a constructor that creates a new ConnectionHelper struct, which has a Listen() and Close() method.

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

@purpleidea Why not just use today's startEtcd(cfg) in your program?

func startEtcd(cfg *config) (<-chan struct{}, error) 

@purpleidea
Copy link
Contributor Author

@xiang90

Why not just use today's startEtcd(cfg) in your program?

Two reasons:

  1. It's private, not public.
  2. You'd have to pass in a very complicated config struct, when all we care about is the connection stuff.

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

@purpleidea Yea. So my suggestion was to make it public and have a helper function to construct the configuration. Then we do not have to change the implementation of startEtcd.

@purpleidea
Copy link
Contributor Author

On Tue, Jun 7, 2016 at 5:05 PM, Xiang Li notifications@github.com wrote:

@purpleidea https://github.com/purpleidea Yea. So my suggestion was to
make it public and have a helper function to construct the configuration.
Then we do not have to change the implementation of startEtcd.

The config that gets passed to it is highly dependent on things like flags
and other parameters which are unrelated for most people who just want to
start up and etcd server... What would you recommend? Having said that, I'm
fine using whatever public (golang) API for starting the server that you
"bless". It seems there are other projects in the same position as me, such
as openshift v3, where they re-implement (copy+paste) startEtcd.

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

The config that gets passed to it is highly dependent on things like flags
and other parameters which are unrelated for most people who just want to
start up and etcd server... What would you recommend?

Creating a clean cfg for startEtcd. Embedding that conf struct into today's config.

@purpleidea
Copy link
Contributor Author

Creating a clean cfg for startEtcd. Embedding that conf struct into today's config.

Sorry, can you elaborate please?

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

@purpleidea

I agreed that the current config struct is bad for startEtcd, just as you mentioned. The main problem is that tt contains flagset, which is unnecessary. It also contains some proxy related stuff, which is also unnecessary.

I would suggest to have another cfg struct for StartEtcd, which is clean and does not contain flag set.

@purpleidea
Copy link
Contributor Author

@xiang90 Now that I understand, I agree. While I think this would be more correct, I'm not sure that it would necessarily be more useful, since the only parameters you need to set if embedding etcd are the seven or so I mentioned for the connecting handling, since everything in config, mostly just gets copied into ServerConfig.

As a result, would you consider reviewing a reworked patch that uses your connection struct idea? I think it would solve most use cases without needing to copy values between three structs.

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

since everything in config, mostly just gets copied into ServerConfig.

After we got that clean cfg, we can pre-fill some of the common fields. So normal user can just fill in the field that they want to customize like connection related.

@purpleidea
Copy link
Contributor Author

clean cfg

I don't really know how to write this, particularly all the places it would touch and how the flag stuff works. If you're able to start this, I'm happy to fill in all the blanks to make it work.

would you consider reviewing a reworked patch that uses your connection struct idea? I think it would solve most use cases without needing to copy values between three structs.

What about this? Interested? I can cook it up now.

@xiang90
Copy link
Contributor

xiang90 commented Jun 7, 2016

What about this? Interested? I can cook it up now.

I feel this is not as clean as I would like it to be. I can work on the the clean conf idea later this week or so.

@purpleidea
Copy link
Contributor Author

I feel this is not as clean as I would like it to be.
Okay, as you wish.

I can work on the the clean conf idea later this week or so.
This is great. I really am happy to put in some leg work to make this happen sooner, since my https://github.com/purpleidea/mgmt is almost ready except requires some interface into starting the cluster and I'm using my helper patch at the moment.

Let me know how I can help!

@purpleidea
Copy link
Contributor Author

I will close this patch because of the improved version in #5584
The idea is the same, although the design is much cleaner and intuitive.
Thanks to everyone who reviewed and commented so far.

@purpleidea purpleidea closed this Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants