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.

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

[Federation][kubefed] Add option to disable persistence storage for etcd #40862

Merged
merged 1 commit into from
Feb 3, 2017
Merged

[Federation][kubefed] Add option to disable persistence storage for etcd #40862

merged 1 commit into from
Feb 3, 2017

Conversation

shashidharatd
Copy link

@shashidharatd shashidharatd commented Feb 2, 2017 •
edited
Loading

What this PR does / why we need it:
This is part of updates to enable deployment of federation on non-cloud environments. This pr enables disabling persistent storage for etcd via kubefed.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #40617

Special notes for your reviewer:

Release note:

[Federation] Add --etcd-persistent-storage flag to kubefed to enable/disable persistent storage for etcd

cc: @kubernetes/sig-federation-bugs @madhusudancs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 2, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 2, 2017
@shashidharatd shashidharatd changed the title Add option to disable persistence storage for etcd [Federation][kubefed] Add option to disable persistence storage for etcd Feb 2, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@madhusudancs
Copy link
Contributor

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

federation/pkg/kubefed/init/init.go, line 213 at r1 (raw file):

	// API server's state. This is where federation API server's etcd
	// stores its data.
	pvc := &api.PersistentVolumeClaim{}

Instead of this workaround, how about passing pvc directly to createAPIServer()? That way, you can pass pvc=nil when etcdPersistence is false and not pass etcdPersistence to createAPIServer() at all. Inside createAPIServer(), your check can be if pvc == nil {} instead of checking on etcdPersistence.

Comments from Reviewable

@madhusudancs madhusudancs self-assigned this Feb 2, 2017
@shashidharatd
Copy link
Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

federation/pkg/kubefed/init/init.go, line 213 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Instead of this workaround, how about passing pvc directly to createAPIServer()? That way, you can pass pvc=nil when etcdPersistence is false and not pass etcdPersistence to createAPIServer() at all. Inside createAPIServer(), your check can be if pvc == nil {} instead of checking on etcdPersistence.

Done

Comments from Reviewable

@madhusudancs
Copy link
Contributor

/lgtm

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2017
@madhusudancs
Copy link
Contributor

/approve

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 3, 2017
@madhusudancs madhusudancs added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 3, 2017
@madhusudancs
Copy link
Contributor

Glad that we have a bot reminding us about release notes :) @shashidharatd I think this deserves a release note.

@shashidharatd
Copy link
Author

Thanks @madhusudancs, Added Release Notes now. PTAL

@deads2k
Copy link
Contributor

deads2k commented Feb 3, 2017

/approve

@deads2k deads2k removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, madhusudancs, shashidharatd

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 40862, 40909)

@k8s-github-robot k8s-github-robot merged commit 49de5f5 into kubernetes:master Feb 3, 2017
marun
marun reviewed Feb 3, 2017
View reviewed changes
Copy link
Contributor

@marun marun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@madhusudancs I missed this, looks like I'm not getting notified for kubernetes/sig-federation-bugs. How do I fix that?

federation/pkg/kubefed/init/init_test.go
},
},
},
},
}
if etcdPersistence == "true" {
Copy link
Contributor

@marun marun Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think having to duplicate code in a test can be a sign of poorly-factored code, a bad test, or both. Will follow up separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please feel free to send a PR refactoring it.

@madhusudancs
Copy link
Contributor

@madhusudancs I missed this, looks like I'm not getting notified for kubernetes/sig-federation-bugs. How do I fix that?

All the pieces are not fully hooked up yet. But when that is done, you should be able to subscribe to https://groups.google.com/forum/#!forum/kubernetes-sig-federation-bugs and get the notifications. In fact, you can subscribe to the group now.

@shashidharatd shashidharatd mentioned this pull request Feb 18, 2017
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marun marun marun left review comments

@madhusudancs madhusudancs madhusudancs left review comments

Assignees

@madhusudancs madhusudancs

@nikhiljindal nikhiljindal

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Federation][kubefed] Should be possible to disable persistent storage for etcd
8 participants
@shashidharatd @k8s-reviewable @madhusudancs @deads2k @k8s-github-robot @marun @nikhiljindal @k8s-ci-robot