Skip to content

make openshift start --write-config take a dir #1737

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2015
Merged

make openshift start --write-config take a dir #1737

merged 1 commit into from
Apr 28, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 14, 2015

This changes the --write-config parameter to take a directory (instead of boolean). It eliminates --cert-dir and always creates the certs directly in the config directory.

Since it is a single directory, the master config dir can be bundled up as a secret for use in a pod.

@liggitt review
@sdodson fyi

@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from 2e61232 to 2ccd6fa Compare April 14, 2015 19:43
@deads2k
Copy link
Contributor Author

deads2k commented Apr 14, 2015

[test]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1912/)

@sdodson
Copy link
Member

sdodson commented Apr 15, 2015

@deads2k thanks. concept/intention looks good to me.

liggitt
liggitt reviewed Apr 15, 2015
View reviewed changes
hack/test-cmd.sh
--signer-serial="${MASTER_CONFIG_DIR}/ca.serial.txt"

# create openshift config
openshift start \
Copy link
Contributor

Choose a reason for hiding this comment

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

why do this in addition to create-master-certs / create-node-config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creates the master config pointing to certs in a non-default location.

Copy link
Contributor

Choose a reason for hiding this comment

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

so are we expecting all the certs to exist already? if so, should we do --create-certs=false so we know if create-master-certs and create-node-config didn't make enough things to satisfy openshift start? same comment in other test scripts that use this setup pattern, I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running with a config file, certs are never created.

Copy link
Contributor

Choose a reason for hiding this comment

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

we're not running from config here, we're writing it... wouldn't that generate the certs if missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, just got down to where you removed certargs

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind... --create-certs is just a raw boolean now... think we should pass --create-certs=false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refresh your diff. Got that a couple commits ago.

@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from 2ccd6fa to 194ee2c Compare April 16, 2015 13:00
@deads2k
Copy link
Contributor Author

deads2k commented Apr 16, 2015

comments so far addressed.

liggitt
liggitt reviewed Apr 16, 2015
View reviewed changes
hack/test-cmd.sh
--hostnames="${SERVER_HOSTNAME_LIST}" \
--master="${MASTER_ADDR}" \
--public-master="${API_SCHEME}://${PUBLIC_MASTER_HOST}"

openshift admin create-node-config \
--listen="${KUBELET_SCHEME}://0.0.0.0:${KUBELET_PORT}" \
--node-dir="${CERT_DIR}/node-${KUBELET_HOST}" \
--node-dir="${NODE_CONFIG_DIR}/node-${KUBELET_HOST}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

NODE_CONFIG_DIR already includes node-${KUBELET_HOST}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking to see why this didn't break the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, consistently wrong.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 16, 2015

comments so far addressed.

liggitt
liggitt reviewed Apr 16, 2015
View reviewed changes
pkg/cmd/server/start/command_test.go
}

func getCleanAllInOneConfigDir() string {
cleanupMasterConfigDir()
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanupNodeConfigDirs()?

Copy link
Member

Choose a reason for hiding this comment

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

To me, @liggitt comment here makes sense, any reason why its not called?

liggitt
liggitt reviewed Apr 16, 2015
View reviewed changes
pkg/cmd/server/start/start_allinone.go
if err := nodeOptions.RunNode(); err != nil {
return err
}

if o.WriteConfigOnly {
if o.ConfigDir.Provided() {
Copy link
Contributor

Choose a reason for hiding this comment

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

use writeConfigOnly

@liggitt
Copy link
Contributor

liggitt commented Apr 16, 2015

Other things I found:

liggitt
liggitt reviewed Apr 16, 2015
View reviewed changes
pkg/cmd/server/start/start_allinone.go
flags.BoolVar(&options.WriteConfigOnly, "write-config", false, "Indicates that the command should build the configuration from command-line arguments, write it to the locations specified by --master-config and --node-config, and exit.")
flags.StringVar(&options.MasterConfigFile, "master-config", "", "Location of the master configuration file to run from, or write to (when used with --write-config). When running from configuration files, all other command-line arguments are ignored.")
flags.StringVar(&options.NodeConfigFile, "node-config", "", "Location of the node configuration file to run from, or write to (when used with --write-config). When running from configuration files, all other command-line arguments are ignored.")
flags.Var(&options.ConfigDir, "write-config", "Directory to write an intial config into. After writing, exit without starting the server.")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/intial/initial/

@liggitt
Copy link
Contributor

liggitt commented Apr 16, 2015

that's all the comments I have... once those are done and tests pass, squash, and coordinate with @sdodson on merge timing. should also probably email the devlist once it's in the merge queue

@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from 781c598 to 79bb37e Compare April 17, 2015 11:53
@liggitt
Copy link
Contributor

liggitt commented Apr 17, 2015

Also queue up a doc PR to fix up http://docs.openshift.org/latest/dev_guide/master_node_configuration.html

@liggitt
Copy link
Contributor

liggitt commented Apr 17, 2015

(and probably other places in doc that reference openshift.local.certificates, etc)

This was referenced Apr 17, 2015
Merged
Merged
@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from 79bb37e to 7e953e3 Compare April 17, 2015 18:07
@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from 7e953e3 to b2b69cc Compare April 27, 2015 17:48
@deads2k
Copy link
Contributor Author

deads2k commented Apr 27, 2015

@derekwaynecarr care to do a final check?

@derekwaynecarr
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1697/) (Image: devenv-fedora_1376)

@sdodson sdodson mentioned this pull request Apr 27, 2015
Merged
@deads2k deads2k force-pushed the deads-openshift-start-take-dir branch from b2b69cc to 40be29b Compare April 27, 2015 20:32
@deads2k
Copy link
Contributor Author

deads2k commented Apr 27, 2015

bad rebase in dns test. re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 40be29b

openshift-bot pushed a commit that referenced this pull request Apr 28, 2015
Merge pull request #1737 from deads2k/deads-openshift-start-take-dir
da05a1d 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit da05a1d into openshift:master Apr 28, 2015
@deads2k deads2k deleted the deads-openshift-start-take-dir branch April 28, 2015 15:22
@liggitt liggitt mentioned this pull request May 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@deads2k @openshift-bot @sdodson @liggitt @derekwaynecarr