-
Notifications
You must be signed in to change notification settings - Fork 4.7k
rebase to 1.7 #15100
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
rebase to 1.7 #15100
Conversation
@rootfs @liggitt @sttts @derekwaynecarr @bparees @fabianofranz @juanvallejo @danwinship You all have commits with your names in them. Please review them for correctness. I will re-ping as new commits are added for particular people. |
@@ -241,7 +241,7 @@ func (h *binaryInstantiateHandler) handle(r io.Reader) (runtime.Object, error) { | |||
if err != nil { | |||
return nil, errors.NewInternalError(fmt.Errorf("unable to connect to node, could not retrieve TLS client config: %v", err)) | |||
} | |||
upgrader := spdy.NewRoundTripper(tlsClientConfig) | |||
upgrader := spdy.NewRoundTripper(tlsClientConfig, false) |
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.
what's the question/what does false represent?
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.
what's the question/what does false represent?
FollowRedirects. You don't follow all of your upstream dependencies? I'm shocked! :)
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.
all? any! :)
anyway i guess we wouldn't anticipate a redirect on this codepath, but i'm not sure there's a good reason not to follow a redirect if there is one?
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.
anyway i guess we wouldn't anticipate a redirect on this codepath, but i'm not sure there's a good reason not to follow a redirect if there is one?
It requires peeking and guessing at what the content is. I think I'd default to off.
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.
ok
if volume.VolumeSource.Metadata != nil { | ||
return true | ||
} | ||
} |
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.
not sure why this is in the commit w/ my name?
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.
not sure why this is in the commit w/ my name?
I wondered what happened to @enj's commit.
@@ -502,6 +502,9 @@ func CompleteAppConfig(config *newcmd.AppConfig, f *clientcmd.Factory, c *cobra. | |||
if config.ClientMapper == nil { | |||
config.ClientMapper = resource.ClientMapperFunc(f.ClientForMapping) | |||
} | |||
if config.CategoryExpander == nil { | |||
config.CategoryExpander = f.CategoryExpander() | |||
} |
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.
not sure what this is but if it's needed for the resourcebuilder now, it seems reasonable enough to me.
I think |
Ok. I'll update for that then. I just chose you based on package name. |
Alright, I've gotten down to just #15104 and #15101 stopping progress on compiling the main process, blocking e2e, test-integration.sh, and test-cmd.sh.
|
mainServicesHandler config.ServiceConfigHandler | ||
unidlingLoadBalancer EndpointsConfigHandler | ||
mainEndpointsHandler EndpointsConfigHandler | ||
mainServicesHandler ServiceConfigHandler |
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.
It looks like the upstream types were renamed to EndpointsHandler
and ServiceHandler
. This is going to need a bunch of work though because the interfaces have grown and HybridProxier needs to implement the new functions. I guess you already filed #15101 for this.
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.
It looks like the upstream types were renamed to EndpointsHandler and ServiceHandler. This is going to need a bunch of work though because the interfaces have grown and HybridProxier needs to implement the new functions. I guess you already filed #15101 for this.
Yeah. Sounds like I may as well revert that commit and let a real fix come. The guts changed a lot.
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.
The hybrid proxy is gonna suck. They split things up so you no longer get the full endpoint set, you get add/remove/change/etc events. And that makes it harder for the hybrid proxy.
if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found { | ||
defaulting.(func(*RollingDeploymentStrategyParams))(in) | ||
} | ||
SetDefaults_RollingDeploymentStrategyParams(in) |
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.
I didn't think any defaulting was supposed to happen in conversion now
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.
liggitt 11 hours ago Member
I didn't think any defaulting was supposed to happen in conversion now
I'm uncertain about changing behavior. This is prexisting.
pkg/cmd/server/api/v1/conversions.go
Outdated
if obj.MasterClients.OpenShiftLoopbackClientConnectionOverrides.Burst <= 0 { | ||
obj.MasterClients.OpenShiftLoopbackClientConnectionOverrides.Burst = 300 | ||
} | ||
setDefault_ClientConnectionOverrides(obj.MasterClients.OpenShiftLoopbackClientConnectionOverrides) |
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.
should this function be renamed so it is found by the defaulter generation?
@@ -67,7 +66,7 @@ func (l *ldapClientConfig) Connect() (ldap.Client, error) { | |||
// Ensure tlsConfig specifies the server we're connecting to | |||
if tlsConfig != nil && !tlsConfig.InsecureSkipVerify && len(tlsConfig.ServerName) == 0 { | |||
// Add to a copy of the tlsConfig to avoid mutating the original | |||
c := utilnet.CloneTLSConfig(tlsConfig) | |||
c := tlsConfig.Clone() |
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.
are you switching to go1.8 in this PR?
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.
nm, I see the build update commit
@@ -67,12 +67,12 @@ func (strategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) | |||
} | |||
|
|||
// GetAttrs returns labels and fields of a given object for filtering purposes | |||
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) { | |||
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) { |
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.
pls document the bool.
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.
pls document the bool.
My post-rebase list has us rethinking our lives after manually updating 50 or identical methods to match the kube ones (also done manually by clayton). Yay initializers
@@ -83,12 +83,12 @@ func (strategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) | |||
} | |||
|
|||
// GetAttrs returns labels and fields of a given object for filtering purposes | |||
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) { | |||
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) { |
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.
godoc for the bool
"github.com/golang/glog" | ||
|
||
"github.com/emicklei/go-restful-swagger12" |
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: no new line above
pkg/cmd/server/start/admission.go
Outdated
_ "github.com/openshift/origin/pkg/quota/admission/runonceduration" | ||
_ "github.com/openshift/origin/pkg/scheduler/admission/podnodeconstraints" | ||
_ "github.com/openshift/origin/pkg/security/admission" | ||
_ "k8s.io/apiserver/pkg/admission/plugin/namespace/lifecycle" |
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.
Do we call NewAdmissionOptions to register the ns lifecycle plugin? There is also RegisterAllAdmissionPlugins in k8s.io/apiserver.
Now with working compilation. @danwinship @dcbw ptal at @DirectXMan12 ptal at @bparees new commit for you to review: |
sccAdmission := sccadmission.NewConstraint() | ||
sccAdmission.SetSecurityInformers(ctx.SecurityInformers) | ||
sccAdmission.SetInternalKubeClientSet(ctx.ClientBuilder.KubeInternalClientOrDie(bootstrappolicy.InfraBuildControllerServiceAccountName)) | ||
if err := sccAdmission.Validate(); 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.
looks like this resolves #14909 ?
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.
wait nevermind.
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.
looks like this resolves #14909 ?
No, this just makes the hack fail closer to compile time instead of a random failure at runtime.
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.
ok back to my original statement. looks like it resolves #14909.
lgtm.
Now with a running server! Also, don't tell @smarterclayton that I just had to bump that 10 second timeout because my all in one didn't start..... |
@@ -139,7 +139,7 @@ func (o *BuildHookOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, ar | |||
o.Cmd = cmd | |||
|
|||
mapper, typer := f.Object() | |||
o.Builder = resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.ClientForMapping), kapi.Codecs.UniversalDecoder()). | |||
o.Builder = resource.NewBuilder(mapper, f.CategoryExpander(), typer, resource.ClientMapperFunc(f.ClientForMapping), kapi.Codecs.UniversalDecoder()). |
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.
hm, I believe the NewBuilder
func added to the factory_builder should be in kube 1.7.
Any reason why we are still using resource.NewBuilder
?
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.
Any reason why we are still using resource.NewBuilder?
It's a rebase and smaller changes are easier. Not all the methods have access to the factory.
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.
@juanvallejo you're in the list here: #14647
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.
you're in the list here: #14647
ack
@@ -25,7 +25,7 @@ func ImportObjects(f *clientcmd.Factory, ns, location string) error { | |||
return err | |||
} | |||
glog.V(8).Infof("Importing data:\n%s\n", string(data)) | |||
r := resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true)). | |||
r := resource.NewBuilder(mapper, f.CategoryExpander(), typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true)). |
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.
Same question as 7a9f25d#r126534856
return nil, err | ||
} | ||
// Resolve cmd flags to add any user overrides | ||
if err := cmdflags.Resolve(options.ProxyArguments, proxyOptions.AddFlags); len(err) > 0 { |
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 did this get moved to the start of the function? The goal is that options.ProxyArguments is supposed to override the defaults that get set here. In particular, you're supposed to be able to add:
proxyArguments:
proxy-mode: ["userspace"]
to node-config.yaml to override the default value of proxyconfig.Mode
that gets set below.
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.
Eventually we want to enter with the config, not the options. Not all the overrides being expressed will have args. I'll see if I can bring the remaining ones back into options.
@@ -476,8 +476,6 @@ func (c *NodeConfig) RunProxy() { | |||
endpointsConfig.RegisterEventHandler(endpointsHandler) | |||
go endpointsConfig.Run(utilwait.NeverStop) | |||
|
|||
recorder.Eventf(c.ProxyConfig.NodeRef, kapi.EventTypeNormal, "Starting", "Starting kube-proxy.") |
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.
It doesn't look like anything would have ever modified c.ProxyConfig.NodeRef after it was created in the old code. So you could just move its creation to here and do
nodeRef := &kclientv1.ObjectReference{
Kind: "Node",
Name: hostname,
UID: ktypes.UID(hostname),
Namespace: "",
}
recorder.Eventf(nodeRef, kapi.EventTypeNormal, "Starting", "Starting kube-proxy.")
(based on the current NodeRef definition in kubernetes/cmd/kube-proxy/app/server.go)
@deads2k revert |
Evaluated for origin test up to fe8b4e7 |
"plugin", | ||
"resize", | ||
"rollingupdate", | ||
"run-container", |
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.
run-container is deprecated. why does it suddenly show 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.
all of these are deprecated in fact.
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.
post-rebase todo exists.
@@ -59,7 +68,7 @@ kubectlLoop: | |||
continue | |||
} | |||
|
|||
t.Errorf("missing %q in oc", kubecmd.Name()) | |||
t.Errorf("missing %q,", kubecmd.Name()) |
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: comma?
return fmt.Errorf("invalid --registry-url flag: %v", err) | ||
} | ||
// golang validation tighten and our code actually expects this to be scheme-less | ||
// TODO figure out how to validate |
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.
added to post-rebase todos if we leave this in.
@@ -719,6 +719,13 @@ func startControllers(options configapi.MasterConfig, allocationController origi | |||
return err | |||
} | |||
|
|||
// the service account passed for the recyclable volume plugins needs to exist. We want to do this via the init function, but its a kube init function | |||
// for the rebase, create that service account here | |||
// TODO make this a lot cleaner |
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.
added to post-rebase todos. ugly, but ok for now.
Expansions: map[string][]schema.GroupResource{ | ||
"all": legacyOpenshiftUserResources, | ||
}, | ||
} |
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.
Comparing with the upstream variant, lgtm
These commits look ok:
|
return true, nil | ||
} | ||
|
||
type PersistentVolumeAttachDetachControllerConfig struct { |
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.
got replaced by the "normal" one from upstream: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/controllermanager.go#L332-L333
persistentvolumecontroller.ControllerParameters{ | ||
KubeClient: ctx.ClientBuilder.ClientOrDie("persistent-volume-binder"), | ||
SyncPeriod: ctx.Options.PVClaimBinderSyncPeriod.Duration, | ||
AlphaProvisioner: alphaProvisioner, |
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.
lgtm comparing with upstream's kubernetes/kubernetes#44090
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3193/) (Base Commit: 22888b2) (PR Branch Commit: fe8b4e7) |
@stevekuznetsov can you help me get my e2e disablement working? I want to disable these:
|
Alright, I think that's fully reviewed. I've rebased and the pull against master is here: #15234 |
Just the stateful set errors. fully reviewed. I'm moving #15234 |
Pull to rebase to 1.7.
Failing e2es
Extended.[k8s.io] Services should be able to create a functioning NodePort service
- sjennings thought this wouldn't work without a cloud provider. - I've disabled for now, added to post rebase list.Extended.[k8s.io] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] should adopt matching orphans and release non-matching pods
- this passes, but cleanup fails, so it takes 10 minutesExtended.[k8s.io] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] should not deadlock when a pod's predecessor fails
- this passes, but cleanup fails, so it takes 10 minutesExtended.[k8s.io] SchedulerPriorities [Serial] Pod should be schedule to node that don't match the PodAntiAffinity terms
- I've disabled for now, added to post rebase list.Flake
Extended.[k8s.io] SchedulerPriorities [Serial] Pod should perfer to scheduled to nodes pod can tolerate
observed https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_gce/4329/