-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
get picks for aggregator status #14513
Conversation
[test] |
Alright, the last commit is a change that is needed to wire openshift into a more "normal" aggregated API server shape so that our admission chain works after the aggregator picks we needed from upstream. It is mostly a shuffle, but it is fairly large. Luckily, our integration test flows cover this. More refactoring is necessary, but this is about as small as I could get and have it work. @soltysh @mfojtik @sttts ptal. |
for perspective, the queue is currently 26 deep and working on a pull that was tagged for merge on Monday. |
pkg/cmd/server/origin/apiserver.go
Outdated
@@ -0,0 +1,336 @@ | |||
package origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openshift-apiserver.go ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or originapi-apiserver.go
TemplateNamespaces []string | ||
} | ||
|
||
// OpenshiftNonAPIServer is only responsible for serving the APIs for Openshift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated comment?
pkg/cmd/server/origin/master.go
Outdated
// this is installing the openshift APIs into the kubeapiserver | ||
installAPIs(openshiftAPIServerConfig, kubeAPIServer.GenericAPIServer) | ||
// this sets up the openapi endpoints | ||
preparedKubeAPIServer := kubeAPIServer.GenericAPIServer.PrepareRun() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like that we call PrepareRun()
only here and not for the others. Is it the same in kube-apiserver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like that we call PrepareRun() only here and not for the others. Is it the same in kube-apiserver?
Yeah. openapi is the source of many sins
pkg/cmd/server/origin/apiserver.go
Outdated
|
||
// TODO all of our groups currently have one version, by the time we get more than one, these should be split up | ||
// into their own api servers | ||
apiGroupInfo.GroupMeta.GroupVersion = gv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep isPreferredGroupVersion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restored
pkg/cmd/server/origin/apiserver.go
Outdated
// into their own api servers | ||
apiGroupInfo.GroupMeta.GroupVersion = gv | ||
|
||
glog.Infof("Starting Origin API at %%s%s/%s/%s", api.GroupPrefix, gv.Group, gv.Version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note the %%s. This is substituted somewhere else in the old code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and all the other places with %%s
} | ||
|
||
func (c completedOpenshiftNonAPIConfig) New(delegationTarget genericapiserver.DelegationTarget, stopCh <-chan struct{}) (*OpenshiftNonAPIServer, error) { | ||
genericServer, err := c.OpenshiftNonAPIConfig.GenericConfig.SkipComplete().New("openshift-nonapiserver", delegationTarget) // completion is done in Complete, no need for a second time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonapiserver is strange somehow. openshift-non-api-routes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonapiserver is strange somehow. openshift-non-api-routes?
Not quite as much fun, but sure.
pkg/cmd/server/origin/storage.go
Outdated
//TODO/REBASE use something other than c.KubeClientsetInternal | ||
nodeConnectionInfoGetter, err := kubeletclient.NewNodeConnectionInfoGetter(c.KubeClientExternal.CoreV1().Nodes(), *c.KubeletClientConfig) | ||
if err != nil { | ||
return nil, fmt.Errorf("Unable to configure the node connection info getter: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: lower case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everywhere below as well
pkg/cmd/server/origin/storage.go
Outdated
} | ||
|
||
buildStorage, buildDetailsStorage, err := buildetcd.NewREST(c.GenericConfig.RESTOptionsGetter) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would write those if-clause in one line, if gofmt accepts that. Func gets very long with 50% error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would write those if-clause in one line, if gofmt accepts that. Func gets very long with 50% error handling.
I miss java too :)
This function is on the chopping block. I wouldn't get overly caught up in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
pkg/cmd/server/origin/master.go
Outdated
} | ||
|
||
// initMetricsRoute initializes an HTTP endpoint for metrics. | ||
func initMetricsRoute(apiContainer *restful.Container, path string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
pkg/cmd/server/origin/master.go
Outdated
|
||
storage := make(map[string]rest.Storage) | ||
for k, v := range all { | ||
if excludedV1Types.Has(k) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was empty I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
||
// c.Options.OAuthConfig.MasterPublicURL | ||
MasterPublicURL string | ||
// c.Options.OAuthConfig != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better comment? These code comments will get outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they were breadcrumbs for me. removing
_ "github.com/openshift/origin/pkg/api/install" | ||
) | ||
|
||
// TODO this function needs to be broken apart with each API group owning their own storage, probably with two method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please. 300 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please. 300 lines.
follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine with me.
Comments addressed and some more tests fixed. So glad we have tests. |
Looks good from my side. Having some more eyes on this wouldn't harm though. Quite a bit of code moved and split apart. |
Appears to be clean of conflicts with the next two in the queue. Plan to merge on green. |
This breaks the back of the problem, but then we have at least one TODO that needs doing and the generated code problem. I'd like to see this merged to get those a lot smaller. |
Re[test] |
@sttts if you could make peace with the last two commits on Monday :) |
// we're going through a "normal" API installation in the wrong server, we need to switch the admission chain | ||
// *only while we're installing these APIs*. There are tests that make sure this works and doesn't drop | ||
// plugins and we'll remove it once we're aggregating | ||
kubeAPIServer.GenericAPIServer.SetAdmission(openshiftAPIServerConfig.GenericConfig.AdmissionControl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugly, but ok.
Wondering which of these temporary hacks we can move into the if-clause below for the non-aggregating run mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering which of these temporary hacks we can move into the if-clause below for the non-aggregating run mode?
Can't. These are always needed to get the admission chain right until we're always aggregating.
:(
@deads2k wouldn't call it peace, but I am ok with those two (temporary) commits. |
…handling concerns
[merge] |
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 5 |
Evaluated for origin merge up to 78f422e |
Evaluated for origin test up to 78f422e |
flaked on #14573 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2130/) (Base Commit: c09c601) |
Green except for the flake. Blocks aggregator usage. Merging as planned. Doesn't conflict with top two pulls. |
xref #14500
Aggregator status usage didn't come until long after the API was added, so this pulls in the most important picks that are precursors to it so that the conflicts aren't crazy. The picks came in relatively clean. I've kept the fitting commits separate for ease.
@liggitt