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. |
| 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.
what's the question/what does false represent?
There was a problem hiding this comment.
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.
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.
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.
| if volume.VolumeSource.Metadata != nil { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
not sure why this is in the commit w/ my name?
There was a problem hiding this comment.
not sure why this is in the commit w/ my name?
I wondered what happened to @enj's commit.
| } | ||
| if config.CategoryExpander == nil { | ||
| config.CategoryExpander = f.CategoryExpander() | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
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.
I didn't think any defaulting was supposed to happen in conversion now
There was a problem hiding this comment.
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.
should this function be renamed so it is found by the defaulter generation?
| 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.
are you switching to go1.8 in this PR?
There was a problem hiding this comment.
nm, I see the build update commit
|
|
||
| // 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.
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
|
|
||
| // 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) { |
| "github.com/emicklei/go-restful/swagger" | ||
| "github.com/golang/glog" | ||
|
|
||
| "github.com/emicklei/go-restful-swagger12" |
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.
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.
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.
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..... |
|
|
||
| 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.
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.
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.
@juanvallejo you're in the list here: #14647
| } | ||
| 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)). |
| 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.
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.
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.
| 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.
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.
run-container is deprecated. why does it suddenly show up?
There was a problem hiding this comment.
all of these are deprecated in fact.
| } | ||
|
|
||
| t.Errorf("missing %q in oc", kubecmd.Name()) | ||
| t.Errorf("missing %q,", kubecmd.Name()) |
| 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.
added to post-rebase todos if we leave this in.
|
|
||
| // 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.
added to post-rebase todos. ugly, but ok for now.
| Expansions: map[string][]schema.GroupResource{ | ||
| "all": legacyOpenshiftUserResources, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Comparing with the upstream variant, lgtm
|
These commits look ok:
|
| return true, nil | ||
| } | ||
|
|
||
| type PersistentVolumeAttachDetachControllerConfig struct { |
There was a problem hiding this comment.
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.
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 tolerateobserved https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_gce/4329/