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

use a more 'normal' storage factory #20741

Closed
wants to merge 2 commits into from
Closed

use a more 'normal' storage factory #20741

wants to merge 2 commits into from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 23, 2018

This attempts to remove our special storage factory and collapse onto the same one that upstream uses. It currently fails tests and needs at least one more, so WIP.

@enj this is why I'm worried about that CR test.

/assign @sttts

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 23, 2018
@deads2k deads2k force-pushed the server-21-descope-storage branch 2 times, most recently from c1173bf to bd762db Compare August 24, 2018 12:19
sttts
sttts reviewed Aug 24, 2018
View reviewed changes
pkg/cmd/openshift-apiserver/openshiftapiserver/configprocessing/etcd.go Outdated
"k8s.io/apiserver/pkg/storage/storagebackend"
)

func GetEtcdOptions(startingFlags map[string][]string, etcdConnectionInfo configapi.EtcdConnectionInfo, storagePrefix string, defaultWatchCacheSizes map[schema.GroupResource]int) (*options.EtcdOptions, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc. startignFlags are api server flags?

sttts
sttts reviewed Aug 24, 2018
View reviewed changes
pkg/cmd/openshift-apiserver/openshiftapiserver/configprocessing/etcd.go Outdated
storageConfig.CAFile = etcdConnectionInfo.CA

etcdOptions := options.NewEtcdOptions(storageConfig)
etcdOptions.DefaultStorageMediaType = "application/json" // TODO(post-1.6.1-rebase): enable protobuf with etcd3 as upstream
Copy link
Contributor

Choose a reason for hiding this comment

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

no proto?

sttts
sttts reviewed Aug 24, 2018
View reviewed changes
pkg/cmd/server/origin/rest/storage_options.go
// storage versions: no overrides anymore
map[schema.GroupResource]schema.GroupVersion{},
// quorum resources:
map[schema.GroupResource]struct{}{
Copy link
Contributor

Choose a reason for hiding this comment

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

this special quorum handling is gone, isn't it? So we default to quorum=true?

sttts
sttts reviewed Aug 24, 2018
View reviewed changes
pkg/cmd/util/flags/flags.go Outdated
return apply(args, fs, false)
}

func ResolveIgnoreMissing(args map[string][]string, fn func(*pflag.FlagSet)) []error {
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2018
@deads2k deads2k force-pushed the server-21-descope-storage branch from bd762db to a3e4a17 Compare August 27, 2018 13:47
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Aug 27, 2018

/hold

holding to make sure we don't accidentally merge a json backed storage impl.

@enj Any idea why the proto deserialization doesn't work? I'm looking and I can't see where you generated the proto files for your type, so how did that ever work? reflective conversion?

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2018
@deads2k deads2k force-pushed the server-21-descope-storage branch from a3e4a17 to 6feba2f Compare August 27, 2018 14:51
@deads2k deads2k changed the title [WIP] use a more 'normal' storage factory use a more 'normal' storage factory Aug 27, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 27, 2018
@deads2k deads2k force-pushed the server-21-descope-storage branch from 6feba2f to c8ac2d5 Compare August 27, 2018 14:52
@deads2k
Copy link
Contributor Author

deads2k commented Aug 27, 2018

/hold cancel

This is ready for review/merge now.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2018
@deads2k deads2k force-pushed the server-21-descope-storage branch from c8ac2d5 to d205dd3 Compare August 27, 2018 15:20
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deads2k
deads2k commented Aug 27, 2018
View reviewed changes
pkg/oc/clusterup/coreinstall/kubeapiserver/config.go
@@ -80,8 +80,6 @@ func (opt KubeAPIServerStartConfig) MakeMasterConfig(dockerClient dockerhelper.I
return "", err
}

masterconfig.KubernetesMasterConfig.APIServerArguments["feature-gates"] = []string{"CustomResourceSubresources=true"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is now on by default.

@deads2k deads2k mentioned this pull request Aug 28, 2018
Merged
sttts
sttts reviewed Aug 29, 2018
View reviewed changes
pkg/util/restoptions/configgetter.go

// perform watch cache heuristic like upstream
if apiserverOptions.Etcd.EnableWatchCache {
glog.V(2).Infof("Initializing cache sizes based on %dMB limit", apiserverOptions.GenericServerRunOptions.TargetRAMMB)
Copy link
Contributor

Choose a reason for hiding this comment

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

the log message is gone. Intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the log message is gone. Intentionally?

Yeah. If we need to expose it, we need to expose it a different way. Metric probably.

@sttts
Copy link
Contributor

sttts commented Aug 29, 2018

Have tried to follow the path of all options. Looks good so far. Just a question about the log output. Otherwise lgtm.

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2018
@openshift-ci-robot
Copy link

@deads2k: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/gcp d205dd3 link /test gcp
ci/openshift-jenkins/extended_conformance_install d205dd3 link /test extended_conformance_install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@deads2k deads2k closed this Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@mfojtik mfojtik Awaiting requested review from mfojtik

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.
4 participants
@deads2k @openshift-ci-robot @sttts @openshift-bot